Skip to content

Commit 4ffea12

Browse files
authored
Only generate dynamic calls when class has generic params (#1388)
Currently, a dynamic call is generated when a field is a function (previously added [back in 2018](#498)). Without the dynamic call, [this test](https://github.com/google/built_value.dart/blob/f20857b873aa6c77395b75aa18a4c03b80f18017/end_to_end_test/test/generics_test.dart#L71-L81) fails. The error message is: ``` 00:00 +9 -1: GenericFunction can be compared [E] │ type '(int) => Null' is not a subtype of type '(dynamic) => dynamic' │ package:end_to_end_test/generics.g.dart 1726:58 _$GenericFunction.== │ test/generics_test.dart 77:63 main.<fn>.<fn> ``` In this commit, only use dynamic calls when the enclosing class has generic parameters. This should eliminate a significant number of dynamic call generation in cases that don't need it. Ideally we would only generate the dynamic call when the *field* is a function and uses type parameters from the enclosing class, but my understanding is that is much more complex and requires deeper integration with the analyzer. The *.g.dart files were regenerated by locally modifying the pubspec of end_to_end_test to use dependency_overrides to point to the local repo (not sure if this is the right approach). Motivation: Dynamic modules do not support dynamic calls and the current generation of dynamic calls makes using built_value in dynamic modules problematic. See b/481568738 for details. Tested: Running the tests, and also doing an internal global presubmit with this change.
1 parent f20857b commit 4ffea12

5 files changed

Lines changed: 64 additions & 39 deletions

File tree

built_value_generator/lib/src/value_source_class.dart

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1354,7 +1354,10 @@ abstract class ValueSourceClass
13541354
result.writeln('bool operator==(Object other) {');
13551355
result.writeln(' if (identical(other, this)) return true;');
13561356

1357-
if (comparedFunctionFields.isNotEmpty) {
1357+
var needsDynamic =
1358+
comparedFunctionFields.isNotEmpty && genericParameters.isNotEmpty;
1359+
1360+
if (needsDynamic) {
13581361
result.writeln(' final dynamic _\$dynamicOther = other;');
13591362
}
13601363
result.writeln(' return other is $name${forBuilder ? 'Builder' : ''}');
@@ -1364,7 +1367,7 @@ abstract class ValueSourceClass
13641367
comparedFields.map((field) {
13651368
var nameOrThisDotName =
13661369
field.name == 'other' ? 'this.other' : field.name;
1367-
return field.isFunctionType
1370+
return needsDynamic && field.isFunctionType
13681371
? '$nameOrThisDotName == _\$dynamicOther.${field.name}'
13691372
: '$nameOrThisDotName == other.${field.name}';
13701373
}).join('&&'),

built_value_generator/test/built_value_generator_test.dart

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -880,6 +880,37 @@ abstract class NestedValue implements Built<NestedValue, NestedValueBuilder> {
880880
'1. Make builder field nestedValue have `nestedBuilder: false` in order to use `comparableBuilders: true`.'));
881881
});
882882

883+
test('uses dynamic for equality check on function fields with generics',
884+
() async {
885+
expect(
886+
await generate('''library value;
887+
import 'package:built_value/built_value.dart';
888+
part 'value.g.dart';
889+
abstract class Value<T> implements Built<Value<T>, ValueBuilder<T>> {
890+
Value._();
891+
factory Value([void Function(ValueBuilder<T>) updates]) = _\$Value<T>;
892+
void Function(T) get callback;
893+
}
894+
'''),
895+
contains(r'final dynamic _$dynamicOther = other;'));
896+
});
897+
898+
test(
899+
'does not use dynamic for equality check on function fields without generics',
900+
() async {
901+
expect(
902+
await generate('''library value;
903+
import 'package:built_value/built_value.dart';
904+
part 'value.g.dart';
905+
abstract class Value implements Built<Value, ValueBuilder> {
906+
Value._();
907+
factory Value([void Function(ValueBuilder) updates]) = _\$Value;
908+
void Function(int) get callback;
909+
}
910+
'''),
911+
isNot(contains(r'dynamicOther')));
912+
});
913+
883914
test('cleans generated class names for private classes', () async {
884915
expect(
885916
await generate('''library value;

end_to_end_test/lib/mixins.g.dart

Lines changed: 1 addition & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

end_to_end_test/lib/records.g.dart

Lines changed: 15 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

end_to_end_test/lib/values.g.dart

Lines changed: 12 additions & 17 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)