Merge pull request #250 from davidmorgan/allow-concrete-in-mixins

davidmorgan · web-flow · commit 0ad1a5aec4e8 · 2017-10-27T15:16:06.000+02:00
Stop generating implementation for fields already implemented in a mixin.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+## 4.3.1
+
+- Fix generation when a field comes from an interface but is also implemented
+  by a mixin.
+
 ## 4.3.0
 
 - Support serializing Int64.
diff --git a/benchmark/pubspec.yaml b/benchmark/pubspec.yaml
@@ -12,12 +12,16 @@ environment:
 dependencies:
   browser: any
   built_collection: ^1.0.0
-  built_value: ^4.3.0
+# built_value: ^4.3.0
+  built_value:
+    path: ../built_value
 
 dev_dependencies:
   build: ^0.10.0
   build_runner: ^0.4.0
-  built_value_generator: ^4.3.0
+# built_value_generator: ^4.3.0
+  built_value_generator:
+    path: ../built_value_generator
   source_gen: ^0.7.0
   quiver: '>=0.21.0 <0.26.0'
   test: any
diff --git a/built_value_generator/lib/src/fields.dart b/built_value_generator/lib/src/fields.dart
@@ -7,7 +7,8 @@ import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:built_collection/built_collection.dart';
 
-/// Gets fields, including from interfaces.
+/// Gets fields, including from interfaces. Fields from interfaces are only
+/// returned if they are not also implemented by a mixin.
 ///
 /// If a field is overriden then just the closest (overriding) field is
 /// returned.
@@ -21,11 +22,17 @@ BuiltList<FieldElement> collectFields(ClassElement element) {
     ..addAll(element.mixins)
     ..forEach((interface) => fields.addAll(collectFields(interface.element)));
 
-// Overridden fields have multiple declarations, so deduplicate by adding
-// to a set that compares on field name.
+  // Overridden fields have multiple declarations, so deduplicate by adding
+  // to a set that compares on field name.
   final fieldSet = new LinkedHashSet<FieldElement>(
       equals: (a, b) => a.displayName == b.displayName,
       hashCode: (a) => a.displayName.hashCode);
   fieldSet.addAll(fields);
-  return new BuiltList<FieldElement>(fieldSet);
+
+  // Filter out interface fields that are implemented in a mixin.
+  return new BuiltList<FieldElement>.build((b) => b
+    ..addAll(fieldSet)
+    ..where((field) =>
+        field.enclosingElement == element ||
+        element.lookUpGetter(field.name, element.library).isAbstract));
 }
diff --git a/built_value_generator/pubspec.yaml b/built_value_generator/pubspec.yaml
@@ -14,7 +14,9 @@ dependencies:
   analyzer: '>=0.29.0 <0.31.0'
   build: ^0.10.0
   built_collection: ^1.0.0
-  built_value: ^4.3.0
+# built_value: ^4.3.0
+  built_value:
+    path: ../built_value
   source_gen: ^0.7.0
   quiver: '>=0.21.0 <0.26.0'
 
diff --git a/built_value_test/pubspec.yaml b/built_value_test/pubspec.yaml
@@ -11,7 +11,9 @@ environment:
   sdk: '>=1.8.0 <2.0.0'
 
 dependencies:
-  built_value: ^4.3.0
+# built_value: ^4.3.0
+  built_value:
+    path: ../built_value
   built_collection: ^1.0.0
   collection: ^1.0.0
   quiver: '>=0.21.0 <0.26.0'
@@ -20,5 +22,7 @@ dependencies:
 dev_dependencies:
   build: ^0.10.0
   build_runner: ^0.4.0
-  built_value_generator: ^4.3.0
+# built_value_generator: ^4.3.0
+  built_value_generator:
+    path: ../built_value_generator
   source_gen: ^0.7.0
diff --git a/chat_example/pubspec.yaml b/chat_example/pubspec.yaml
@@ -12,7 +12,9 @@ environment:
 dependencies:
   browser: ^0.10.0
   built_collection: ^1.0.0
-  built_value: ^4.3.0
+# built_value: ^4.3.0
+  built_value:
+    path: ../built_value
   shelf: ^0.6.0
   shelf_proxy: ^0.1.0
   shelf_web_socket: ^0.2.1
@@ -21,6 +23,8 @@ dependencies:
 dev_dependencies:
   build: ^0.10.0
   build_runner: ^0.4.0
-  built_value_generator: ^4.3.0
+# built_value_generator: ^4.3.0
+  built_value_generator:
+    path: ../built_value_generator
   source_gen: ^0.7.0
   test: any
diff --git a/end_to_end_test/lib/polymorphism.dart b/end_to_end_test/lib/polymorphism.dart
@@ -65,6 +65,29 @@ abstract class Cage implements Built<Cage, CageBuilder> {
   Cage._();
 }
 
+// It's possible to implement getters via mixins. Such getters are treated
+// exactly as concrete getters in the immediate class -- they are ignored.
+abstract class FourLeggedWalker {
+  int get legs => 4;
+}
+
+abstract class HasLegs {
+  int get legs;
+}
+
+// "Legs" field comes from "FourLeggedWalker" mixin and does not lead to a
+// generated field.
+abstract class StandardCat extends Object
+    with Walker, FourLeggedWalker
+    implements HasLegs, Built<StandardCat, StandardCatBuilder> {
+  static Serializer<StandardCat> get serializer => _$standardCatSerializer;
+
+  bool get tail;
+
+  factory StandardCat([updates(StandardCatBuilder b)]) = _$StandardCat;
+  StandardCat._();
+}
+
 @BuiltValue(instantiable: false)
 abstract class HasField<T> implements Built<HasField<T>, HasFieldBuilder<T>> {
   T get field;
diff --git a/end_to_end_test/lib/polymorphism.g.dart b/end_to_end_test/lib/polymorphism.g.dart
diff --git a/end_to_end_test/lib/serializers.dart b/end_to_end_test/lib/serializers.dart
@@ -36,6 +36,7 @@ part 'serializers.g.dart';
   Robot,
   SecondTestEnum,
   SimpleValue,
+  StandardCat,
   StandardJsonValue,
   TestEnum,
   ValueUsingImportAs,
diff --git a/end_to_end_test/lib/serializers.g.dart b/end_to_end_test/lib/serializers.g.dart
diff --git a/end_to_end_test/pubspec.yaml b/end_to_end_test/pubspec.yaml
@@ -11,12 +11,16 @@ environment:
 
 dependencies:
   built_collection: ^1.0.0
-  built_value: ^4.3.0
+# built_value: ^4.3.0
+  built_value:
+    path: ../built_value
 
 dev_dependencies:
   build: ^0.10.0
   build_runner: ^0.4.0
-  built_value_generator: ^4.3.0
+# built_value_generator: ^4.3.0
+  built_value_generator:
+    path: ../built_value_generator
   fixnum: ^0.10.0
   source_gen: ^0.7.0
   quiver: '>=0.21.0 <0.26.0'
diff --git a/end_to_end_test/test/polymorphism_serializer_test.dart b/end_to_end_test/test/polymorphism_serializer_test.dart
@@ -50,6 +50,23 @@ void main() {
     });
   });
 
+  group('StandardCat', () {
+    final data = new StandardCat((b) => b..tail = true);
+    final serialized = [
+      'StandardCat',
+      'tail',
+      true,
+    ];
+
+    test('can be serialized', () {
+      expect(serializers.serialize(data), serialized);
+    });
+
+    test('can be deserialized', () {
+      expect(serializers.deserialize(serialized), data);
+    });
+  });
+
   group('HasField', () {
     final data = new BuiltList<HasField<dynamic>>([
       new HasString((b) => b..field = 'hello'),
diff --git a/end_to_end_test/test/polymorphism_test.dart b/end_to_end_test/test/polymorphism_test.dart
@@ -105,6 +105,12 @@ void main() {
     });
   });
 
+  group('StandardCat', () {
+    test('uses legs field from mixin', () {
+      expect(new StandardCat((b) => b..tail = true).legs, 4);
+    });
+  });
+
   group('HasField', () {
     test('can be used as an interface, including builder', () {
       final hasFields = <HasField<dynamic>>[
diff --git a/example/pubspec.yaml b/example/pubspec.yaml
@@ -12,12 +12,16 @@ environment:
 dependencies:
   meta: ^1.0.4
   built_collection: ^1.0.0
-  built_value: ^4.3.0
+# built_value: ^4.3.0
+  built_value:
+    path: ../built_value
 
 dev_dependencies:
   build: ^0.10.0
   build_runner: ^0.4.0
-  built_value_generator: ^4.3.0
+# built_value_generator: ^4.3.0
+  built_value_generator:
+    path: ../built_value_generator
   source_gen: ^0.7.0
   quiver: '>=0.21.0 <0.26.0'
   test: any
