When accounting for direct implementors, skip over private ones (#2354)

Skip over private implementors when listing implementors

Author: srawlins

lib/src/model/class.dart

@@ -166,14 +166,35 @@ class Class extends Container
     return '${package.baseHref}$filePath';
   }
 
-  /// Returns all the implementors of this class.
+  /// Returns all the "immediate" public implementors of this class.
+  ///
+  /// If this class has a private implementor, then that is counted as a proxy
+  /// for any public implementors of that private class.
   Iterable<Class> get publicImplementors {
-    return model_utils.filterNonPublic(
-        model_utils.findCanonicalFor(packageGraph.implementors[href] ?? []));
+    var result = <Class>{};
+    var seen = <Class>{};
+
+    // Recursively adds [implementor] if public, or the impelentors of
+    // [implementor] if not.
+    void addToResult(Class implementor) {
+      if (seen.contains(implementor)) return;
+      seen.add(implementor);
+      if (implementor.isPublic) {
+        result.add(implementor);
+      } else {
+        model_utils
+            .findCanonicalFor(packageGraph.implementors[implementor] ?? [])
+            .forEach(addToResult);
+      }
+    }
+
+    model_utils
+        .findCanonicalFor(packageGraph.implementors[this] ?? [])
+        .forEach(addToResult);
+    return result;
   }
 
-  /*lazy final*/
-  List<Method> _inheritedMethods;
+  /*lazy final*/ List<Method> _inheritedMethods;
 
   Iterable<Method> get inheritedMethods {
     if (_inheritedMethods == null) {
@@ -201,8 +222,7 @@ class Class extends Container
 
   bool get hasPublicInheritedMethods => publicInheritedMethods.isNotEmpty;
 
-  /*lazy final*/
-  List<Operator> _inheritedOperators;
+  /*lazy final*/ List<Operator> _inheritedOperators;
 
   Iterable<Operator> get inheritedOperators {
     if (_inheritedOperators == null) {

lib/src/model/package_graph.dart

@@ -151,7 +151,7 @@ class PackageGraph {
   /// is true.
   bool allExtensionsAdded = false;
 
-  Map<String, List<Class>> get implementors {
+  Map<Class, List<Class>> get implementors {
     assert(allImplementorsAdded);
     return _implementors;
   }
@@ -201,8 +201,8 @@ class PackageGraph {
   final Map<Tuple2<Element, Library>, Set<ModelElement>>
       allInheritableElements = {};
 
-  /// Map of Class.href to a list of classes implementing that class
-  final Map<String, List<Class>> _implementors = {};
+  /// A mapping of the list of classes which implement each class.
+  final Map<Class, List<Class>> _implementors = {};
 
   /// A list of extensions that exist in the package graph.
   final List<Extension> _extensions = [];
@@ -576,26 +576,26 @@ class PackageGraph {
     return hrefMap;
   }
 
-  void _addToImplementors(Class c) {
+  void _addToImplementors(Class class_) {
     assert(!allImplementorsAdded);
-    _implementors.putIfAbsent(c.href, () => []);
-    void _checkAndAddClass(Class key, Class implClass) {
-      _implementors.putIfAbsent(key.href, () => []);
-      var list = _implementors[key.href];
+    _implementors.putIfAbsent(class_, () => []);
+    void checkAndAddClass(Class key) {
+      _implementors.putIfAbsent(key, () => []);
+      var list = _implementors[key];
 
-      if (!list.any((l) => l.element == c.element)) {
-        list.add(implClass);
+      if (!list.any((l) => l.element == class_.element)) {
+        list.add(class_);
       }
     }
 
-    for (var type in c.mixins) {
-      _checkAndAddClass(type.element, c);
+    for (var type in class_.mixins) {
+      checkAndAddClass(type.element);
     }
-    if (c.supertype != null) {
-      _checkAndAddClass(c.supertype.element, c);
+    if (class_.supertype != null) {
+      checkAndAddClass(class_.supertype.element);
     }
-    for (var type in c.interfaces) {
-      _checkAndAddClass(type.element, c);
+    for (var type in class_.interfaces) {
+      checkAndAddClass(type.element);
     }
   }
 

test/end2end/model_test.dart

@@ -1328,12 +1328,12 @@ void main() {
       expect(GenericMixin.characterLocation, isNotNull);
     });
 
-    test(('Verify mixin member is available in findRefElementCache'), () {
+    test('Verify mixin member is available in findRefElementCache', () {
       expect(packageGraph.findRefElementCache['GenericMixin.mixinMember'],
           isNotEmpty);
     });
 
-    test(('Verify inheritance/mixin structure and type inference'), () {
+    test('Verify inheritance/mixin structure and type inference', () {
       expect(
           TypeInferenceMixedIn.mixins
               .map<String>((DefinedElementType t) => t.element.name),
@@ -1356,7 +1356,7 @@ void main() {
           orderedEquals(['int']));
     });
 
-    test(('Verify non-overridden members have right canonical classes'), () {
+    test('Verify non-overridden members have right canonical classes', () {
       var member = TypeInferenceMixedIn.instanceFields
           .firstWhere((f) => f.name == 'member');
       var modifierMember = TypeInferenceMixedIn.instanceFields
@@ -1368,7 +1368,7 @@ void main() {
       expect(mixinMember.canonicalEnclosingContainer, equals(GenericMixin));
     });
 
-    test(('Verify overrides & documentation inheritance work as intended'), () {
+    test('Verify overrides & documentation inheritance work as intended', () {
       expect(overrideByEverything.canonicalEnclosingContainer,
           equals(TypeInferenceMixedIn));
       expect(overrideByGenericMixin.canonicalEnclosingContainer,
@@ -1398,8 +1398,7 @@ void main() {
               .getter));
     });
 
-    test(('Verify that documentation for mixin applications contains links'),
-        () {
+    test('Verify that documentation for mixin applications contains links', () {
       expect(
           overrideByModifierClass.oneLineDoc,
           contains(
@@ -3582,6 +3581,18 @@ String topLevelFunction(int param1, bool param2, Cool coolBeans,
           .toList();
     });
 
+    test('private classes do not break the implementor chain', () {
+      var Super1 = fakeLibrary.classes.firstWhere((c) => c.name == 'Super1');
+      var publicImplementors = Super1.publicImplementors.map((i) => i.name);
+      expect(publicImplementors, hasLength(3));
+      // A direct implementor.
+      expect(publicImplementors, contains('Super4'));
+      // An implementor through _Super2.
+      expect(publicImplementors, contains('Super3'));
+      // An implementor through _Super5 and _Super2.
+      expect(publicImplementors, contains('Super6'));
+    });
+
     test('the first class is Apple', () {
       expect(apple.name, equals('Apple'));
     });

testing/test_package/lib/fake.dart

@@ -1160,3 +1160,15 @@ extension ExtensionOnNull on Null {
 extension ExtensionOnTypeParameter<T> on T {
   T aFunctionReturningT(T other) => other;
 }
+
+class Super1 {}
+
+class _Super2 implements Super1 {}
+
+class Super3 implements _Super2 {}
+
+class Super4 implements Super1 {}
+
+class _Super5 implements _Super2 {}
+
+class Super6 implements _Super5 {}