Skip to content

Commit

Permalink
feat: use covariant in operator== override for improved type safe…
Browse files Browse the repository at this point in the history 
…ty (#180)
  • Loading branch information
@felangel
felangel authored Oct 16, 2024
1 parent 204a1f3 commit 6ddd3f0
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 121 deletions.
32 changes: 16 additions & 16 deletions benchmarks/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ Benchmarks used to measure the performance of equality comparisons using `packag

```
EmptyEquatable
total runs: 7 741 659
total runs: 7 984 597
total time: 2.0000 s
average run: 0 μs
runs/second: Infinity
Expand All @@ -22,7 +22,7 @@ EmptyEquatable
time per unit: 0.0000 μs
PrimitiveEquatable
total runs: 1 347 013
total runs: 1 349 110
total time: 2.0000 s
average run: 1 μs
runs/second: 1 000 000
Expand All @@ -31,7 +31,7 @@ PrimitiveEquatable
time per unit: 0.0100 μs
CollectionEquatable (static, small)
total runs: 54 740
total runs: 54 582
total time: 2.0000 s
average run: 36 μs
runs/second: 27 778
Expand All @@ -40,25 +40,25 @@ CollectionEquatable (static, small)
time per unit: 0.3600 μs
CollectionEquatable (static, medium)
total runs: 45 852
total runs: 46 839
total time: 2.0000 s
average run: 43 μs
runs/second: 23 256
average run: 42 μs
runs/second: 23 810
units: 100
units/second: 2 325 581
time per unit: 0.4300 μs
units/second: 2 380 952
time per unit: 0.4200 μs
CollectionEquatable (static, large)
total runs: 20 328
total runs: 20 867
total time: 2.0001 s
average run: 98 μs
runs/second: 10 204
average run: 95 μs
runs/second: 10 526
units: 100
units/second: 1 020 408
time per unit: 0.9800 μs
units/second: 1 052 632
time per unit: 0.9500 μs
CollectionEquatable (dynamic, small)
total runs: 623 140
total runs: 629 974
total time: 2.0000 s
average run: 3 μs
runs/second: 333 333
Expand All @@ -67,7 +67,7 @@ CollectionEquatable (dynamic, small)
time per unit: 0.0300 μs
CollectionEquatable (dynamic, medium)
total runs: 618 821
total runs: 628 191
total time: 2.0000 s
average run: 3 μs
runs/second: 333 333
Expand All @@ -76,7 +76,7 @@ CollectionEquatable (dynamic, medium)
time per unit: 0.0300 μs
CollectionEquatable (dynamic, large)
total runs: 627 611
total runs: 632 540
total time: 2.0000 s
average run: 3 μs
runs/second: 333 333
Expand Down
8 changes: 3 additions & 5 deletions lib/src/equatable.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,11 @@ macro class Equatable implements ClassDeclarationsMacro, ClassDefinitionMacro {
ClassDeclaration clazz,
MemberDeclarationBuilder builder,
) async {
final (object, boolean) = await (
builder.codeFrom(_dartCore, 'Object'),
builder.codeFrom(_dartCore, 'bool'),
).wait;
final boolean = await builder.codeFrom(_dartCore, 'bool');

return builder.declareInType(
DeclarationCode.fromParts(
['external ', boolean, ' operator==(', object, ' other);'],
['external ', boolean, ' operator==(', ' covariant ${clazz.identifier.name}', ' other);'],

This comment has been minimized.

Sign in to view

Copy link
@Zekfad

Zekfad Oct 17, 2024

While this seems good on surface, this will create a silent runtime error when object comes from (or to) external library in form of Object/dynamic or more commonly List<Object> (for example cache).
Such error would be hard to debug, I think its more reasonable that object is not equal to unrelated one than it's possible that == will randomly throw an error.

This comment has been minimized.

Sign in to view

Copy link
@felangel

felangel Oct 17, 2024

Author Owner

Can you give an example? I thought the case you described would be a compiler error rather than a runtime error.

This comment has been minimized.

Sign in to view

Copy link
@Zekfad

Zekfad Oct 17, 2024

Here's quick example:

class A {
  @override
  bool operator==(covariant A other) => false;
}

void main() {
  var a = <Object>[A()];
  print(a[0] == A()); // ok
  print(a[0] == Object()); // runtime error (TypeError)
}

Here's quote from the docs:

The covariant keyword
Some (rarely used) coding patterns rely on tightening a type by overriding a parameter's type with a subtype, which is invalid. In this case, you can use the covariant keyword to tell the analyzer that you are doing this intentionally. This removes the static error and instead checks for an invalid argument type at runtime.
),
);
}
Expand Down
100 changes: 0 additions & 100 deletions test/equatable_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import 'package:test/test.dart';

import 'custom_list.dart';

class NonEquatable {}

@Equatable()
class EmptyEquatable {
const EmptyEquatable();
Expand All @@ -29,13 +27,6 @@ class MultipartEquatable<T extends Object> {
final T d2;
}

@Equatable()
class OtherEquatable {
const OtherEquatable(this.data);

final String data;
}

enum Color { blonde, black, brown }

@Equatable()
Expand Down Expand Up @@ -97,12 +88,6 @@ void main() {
expect(instanceA == instanceB, true);
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = EmptyEquatable();
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});
});

group('Simple Equatable (string)', () {
Expand All @@ -124,18 +109,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = SimpleEquatable('simple');
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when compared to different equatable', () {
final instanceA = SimpleEquatable('simple');
final instanceB = OtherEquatable('simple');
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = SimpleEquatable('simple');
final instanceB = SimpleEquatable('Simple');
Expand All @@ -162,12 +135,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = SimpleEquatable(0);
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = SimpleEquatable(0);
final instanceB = SimpleEquatable(1);
Expand All @@ -194,12 +161,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = SimpleEquatable(true);
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = SimpleEquatable(true);
final instanceB = SimpleEquatable(false);
Expand Down Expand Up @@ -227,12 +188,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = SimpleEquatable({'a': 1, 'b': 2, 'c': 3});
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = SimpleEquatable({'a': 1, 'b': 2, 'c': 3});
final instanceB = SimpleEquatable({'a': 1, 'b': 2, 'c': 4});
Expand Down Expand Up @@ -308,18 +263,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = SimpleEquatable(
{
SimpleEquatable<String>('a'): 1,
SimpleEquatable<String>('b'): 2,
SimpleEquatable<String>('c'): 3,
},
);
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = SimpleEquatable(
{
Expand Down Expand Up @@ -383,17 +326,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = SimpleEquatable(
EquatableData(
key: 'foo',
value: 'bar',
),
);
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = SimpleEquatable(
EquatableData(
Expand Down Expand Up @@ -438,12 +370,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = MultipartEquatable('s1', 's2');
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = MultipartEquatable('s1', 's2');
final instanceB = MultipartEquatable('s2', 's1');
Expand Down Expand Up @@ -499,17 +425,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = ComplexEquatable(
name: 'Joe',
age: 40,
hairColor: Color.black,
children: ['Bob'],
);
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = ComplexEquatable(
name: 'Joe',
Expand Down Expand Up @@ -623,21 +538,6 @@ void main() {
expect(instanceA.hashCode == instanceB.hashCode, true);
});

test('should return false when compared to non-equatable', () {
final instanceA = Credentials.fromJson(
json.decode(
'''
{
"username":"Admin",
"password":"admin"
}
''',
) as Map<String, dynamic>,
);
final instanceB = NonEquatable();
expect(instanceA == instanceB, false);
});

test('should return false when values are different', () {
final instanceA = Credentials.fromJson(
json.decode(
Expand Down

0 comments on commit 6ddd3f0

Please sign in to comment.