Improve couldApplyTo and add tests (#2062)

* intermediate -- tests pass

* First version of extension tree complete

* it works

* It actually works, with tests that are debugged

* Cleanups

* Add test case for implements

* Remove extension tree and correct couldApplyTo once more.

* Remove debugging code

Author: jcollins-g

lib/src/element_type.dart

@@ -254,6 +254,11 @@ class TypeParameterElementType extends DefinedElementType {
     }
     return _nameWithGenerics;
   }
+
+  @override
+  ClassElement get _boundClassElement => interfaceType.element;
+  @override
+  InterfaceType get interfaceType => (type as TypeParameterType).bound;
 }
 
 /// An [ElementType] associated with an [Element].
@@ -311,6 +316,26 @@ abstract class DefinedElementType extends ElementType {
     }
     return _typeArguments;
   }
+
+  /// By default, the bound is the type of the declared class.
+  ClassElement get _boundClassElement => (element.element as ClassElement);
+  Class get boundClass =>
+      ModelElement.fromElement(_boundClassElement, packageGraph);
+  InterfaceType get interfaceType => type;
+
+  InterfaceType _instantiatedType;
+
+  /// Return this type, instantiated to bounds if it isn't already.
+  DartType get instantiatedType {
+    if (_instantiatedType == null) {
+      if (!interfaceType.typeArguments.every((t) => t is InterfaceType)) {
+        _instantiatedType = packageGraph.typeSystem.instantiateToBounds(interfaceType);
+      } else {
+        _instantiatedType = interfaceType;
+      }
+    }
+    return _instantiatedType;
+  }
 }
 
 /// Any callable ElementType will mix-in this class, whether anonymous or not.
@@ -446,8 +471,8 @@ class CallableGenericTypeAliasElementType extends ParameterizedElementType
   @override
   ElementType get returnType {
     if (_returnType == null) {
-      _returnType = ElementType.from(
-          type.returnType, library, packageGraph, this);
+      _returnType =
+          ElementType.from(type.returnType, library, packageGraph, this);
     }
     return _returnType;
   }

lib/src/markdown_processor.dart

@@ -206,11 +206,13 @@ MatchingLinkResult _getMatchingLinkElement(
   if (refModelElement == null && element is ModelElement) {
     Container preferredClass = _getPreferredClass(element);
     if (preferredClass is Extension) {
-      element.warn(PackageWarning.notImplemented, message: 'Comment reference resolution inside extension methods is not yet implemented');
+      element.warn(PackageWarning.notImplemented,
+          message:
+              'Comment reference resolution inside extension methods is not yet implemented');
     } else {
-      refModelElement =
-          _MarkdownCommentReference(codeRef, element, commentRefs, preferredClass)
-              .computeReferredElement();
+      refModelElement = _MarkdownCommentReference(
+              codeRef, element, commentRefs, preferredClass)
+          .computeReferredElement();
     }
   }
 
@@ -649,8 +651,8 @@ class _MarkdownCommentReference {
       Inheritable overriddenElement =
           (element as Inheritable).overriddenElement;
       while (overriddenElement != null) {
-        tryClasses.add(
-            (element as Inheritable).overriddenElement.enclosingElement);
+        tryClasses
+            .add((element as Inheritable).overriddenElement.enclosingElement);
         overriddenElement = overriddenElement.overriddenElement;
       }
     }
@@ -669,7 +671,7 @@ class _MarkdownCommentReference {
 
     if (results.isEmpty && realClass != null) {
       for (Class superClass
-          in realClass.publicSuperChain.map((et) => et.element as Class)) {
+          in realClass.publicSuperChain.map((et) => et.element)) {
         if (!tryClasses.contains(superClass)) {
           _getResultsForClass(superClass);
         }
@@ -719,12 +721,12 @@ class _MarkdownCommentReference {
         // TODO(jcollins-g): get rid of reimplementation of identifier resolution
         //                   or integrate into ModelElement in a simpler way.
         List<Class> superChain = [tryClass];
-        superChain.addAll(tryClass.interfaces.map((t) => t.element as Class));
+        superChain.addAll(tryClass.interfaces.map((t) => t.element));
         // This seems duplicitous with our caller, but the preferredClass
         // hint matters with findCanonicalModelElementFor.
         // TODO(jcollins-g): This makes our caller ~O(n^2) vs length of superChain.
         //                   Fortunately superChains are short, but optimize this if it matters.
-        superChain.addAll(tryClass.superChain.map((t) => t.element as Class));
+        superChain.addAll(tryClass.superChain.map((t) => t.element));
         for (final c in superChain) {
           _getResultsForSuperChainElement(c, tryClass);
           if (results.isNotEmpty) break;
@@ -778,7 +780,9 @@ String _linkDocReference(String codeRef, Warnable warnable,
       // current element.
       warnable.warn(PackageWarning.unresolvedDocReference,
           message: codeRef,
-          referredFrom: warnable.documentationIsLocal ? null : warnable.documentationFrom);
+          referredFrom: warnable.documentationIsLocal
+              ? null
+              : warnable.documentationFrom);
     }
     return '<code>${htmlEscape.convert(codeRef)}</code>';
   }

lib/src/model.dart

@@ -6,7 +6,7 @@
 library dartdoc.models;
 
 import 'dart:async';
-import 'dart:collection' show UnmodifiableListView;
+import 'dart:collection' show HashSet, UnmodifiableListView;
 import 'dart:convert';
 import 'dart:io';
 
@@ -22,7 +22,6 @@ import 'package:analyzer/dart/ast/ast.dart'
         InstanceCreationExpression;
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
-import 'package:analyzer/dart/element/type_system.dart';
 import 'package:analyzer/dart/element/visitor.dart';
 import 'package:analyzer/file_system/file_system.dart' as file_system;
 import 'package:analyzer/file_system/physical_file_system.dart';
@@ -36,7 +35,6 @@ import 'package:analyzer/src/dart/element/element.dart';
 import 'package:analyzer/src/dart/element/inheritance_manager3.dart';
 import 'package:analyzer/src/dart/element/member.dart'
     show ExecutableMember, Member, ParameterMember;
-import 'package:analyzer/src/dart/element/type.dart' show InterfaceTypeImpl;
 import 'package:analyzer/src/dart/sdk/sdk.dart';
 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/java_io.dart';
@@ -805,7 +803,8 @@ class Class extends Container
       _potentiallyApplicableExtensions = utils
           .filterNonDocumented(packageGraph.extensions)
           .where((e) => e.couldApplyTo(this))
-          .toList(growable: false);
+          .toList(growable: false)
+            ..sort(byName);
     }
     return _potentiallyApplicableExtensions;
   }
@@ -1344,26 +1343,44 @@ class Extension extends Container
         ElementType.from(_extension.extendedType, library, packageGraph);
   }
 
-  /// Returns [true] if there is an instantiation of [c] to which this extension
-  /// could be applied.
-  bool couldApplyTo(Class c) =>
-      _couldApplyTo(extendedType.type, c.element, packageGraph.typeSystem);
-
-  static bool _couldApplyTo(
-      DartType extendedType, ClassElement element, Dart2TypeSystem typeSystem) {
-    InterfaceTypeImpl classInstantiated =
-        typeSystem.instantiateToBounds(element.thisType);
-    classInstantiated = element.instantiate(
-        typeArguments: classInstantiated.typeArguments.map((a) {
-          if (a.isDynamic) {
-            return typeSystem.typeProvider.neverType;
-          }
-          return a;
-        }).toList(),
-        nullabilitySuffix: classInstantiated.nullabilitySuffix);
-
-    return (classInstantiated.element == extendedType.element) ||
-        typeSystem.isSubtypeOf(classInstantiated, extendedType);
+  bool couldApplyTo(Class c) => _couldApplyTo(c.modelType);
+
+  /// Return true if this extension could apply to [t].
+  bool _couldApplyTo(DefinedElementType t) {
+    return t.instantiatedType == extendedType.instantiatedType ||
+        (t.instantiatedType.element == extendedType.instantiatedType.element &&
+            isSubtypeOf(t)) ||
+        isBoundSupertypeTo(t);
+  }
+
+  /// The instantiated to bounds [extendedType] of this extension is a subtype of
+  /// [t].
+  bool isSubtypeOf(DefinedElementType t) => packageGraph.typeSystem
+      .isSubtypeOf(extendedType.instantiatedType, t.instantiatedType);
+
+  bool isBoundSupertypeTo(DefinedElementType t) =>
+      _isBoundSupertypeTo(t.type, HashSet());
+
+  /// Returns true if at least one supertype (including via mixins and
+  /// interfaces) is equivalent to or a subtype of [extendedType] when
+  /// instantiated to bounds.
+  bool _isBoundSupertypeTo(
+      InterfaceType superType, HashSet<InterfaceType> visited) {
+    ClassElement superClass = superType?.element;
+    if (visited.contains(superType)) return false;
+    visited.add(superType);
+    if (superClass == extendedType.type.element &&
+        (superType == extendedType.instantiatedType ||
+            packageGraph.typeSystem
+                .isSubtypeOf(superType, extendedType.instantiatedType))) {
+      return true;
+    }
+    List<InterfaceType> supertypes = [];
+    ClassElementImpl.collectAllSupertypes(supertypes, superType, null);
+    for (InterfaceType toVisit in supertypes) {
+      if (_isBoundSupertypeTo(toVisit, visited)) return true;
+    }
+    return false;
   }
 
   @override
@@ -1418,7 +1435,7 @@ class Extension extends Container
   }
 
   @override
-  DefinedElementType get modelType => super.modelType;
+  ParameterizedElementType get modelType => super.modelType;
 
   List<ModelElement> _allModelElements;
 
@@ -5062,13 +5079,11 @@ class PackageGraph {
       package._libraries.sort((a, b) => compareNatural(a.name, b.name));
       package._libraries.forEach((library) {
         library.allClasses.forEach(_addToImplementors);
-        // TODO(jcollins-g): Use a better data structure.
         _extensions.addAll(library.extensions);
       });
     });
     _implementors.values.forEach((l) => l.sort());
     allImplementorsAdded = true;
-    _extensions.sort(byName);
     allExtensionsAdded = true;
 
     // We should have found all special classes by now.
@@ -5179,8 +5194,7 @@ class PackageGraph {
   final Map<String, List<Class>> _implementors = Map();
 
   /// A list of extensions that exist in the package graph.
-  // TODO(jcollins-g): Consider implementing a smarter structure for this.
-  final List<Extension> _extensions = List();
+  final List<Extension> _extensions = [];
 
   /// PackageMeta for the default package.
   final PackageMeta packageMeta;
@@ -5212,7 +5226,7 @@ class PackageGraph {
   /// TODO(brianwilkerson) Replace the driver with the session.
   final AnalysisDriver driver;
   final AnalysisSession session;
-  final TypeSystem typeSystem;
+  final Dart2TypeSystem typeSystem;
   final DartSdk sdk;
 
   Map<Source, SdkLibrary> _sdkLibrarySources;

test/model_test.dart

@@ -2199,6 +2199,43 @@ void main() {
           orderedEquals([uphill]));
     });
 
+    test('applicableExtensions include those from implements & mixins', () {
+      Extension extensionCheckLeft,
+          extensionCheckRight,
+          extensionCheckCenter,
+          extensionCheckImplementor2,
+          onNewSchool,
+          onOldSchool;
+      Class implementor, implementor2, school;
+      Extension getExtension(String name) =>
+          fakeLibrary.extensions.firstWhere((e) => e.name == name);
+      Class getClass(String name) =>
+          fakeLibrary.classes.firstWhere((e) => e.name == name);
+      extensionCheckLeft = getExtension('ExtensionCheckLeft');
+      extensionCheckRight = getExtension('ExtensionCheckRight');
+      extensionCheckCenter = getExtension('ExtensionCheckCenter');
+      extensionCheckImplementor2 = getExtension('ExtensionCheckImplementor2');
+      onNewSchool = getExtension('OnNewSchool');
+      onOldSchool = getExtension('OnOldSchool');
+
+      implementor = getClass('Implementor');
+      implementor2 = getClass('Implementor2');
+      school = getClass('School');
+
+      expect(
+          implementor.potentiallyApplicableExtensions,
+          orderedEquals([
+            extensionCheckCenter,
+            extensionCheckImplementor2,
+            extensionCheckLeft,
+            extensionCheckRight
+          ]));
+      expect(implementor2.potentiallyApplicableExtensions,
+          orderedEquals([extensionCheckImplementor2, extensionCheckLeft]));
+      expect(school.potentiallyApplicableExtensions,
+          orderedEquals([onNewSchool, onOldSchool]));
+    });
+
     test('type parameters and bounds work with applicableExtensions', () {
       expect(
           superMegaTron.potentiallyApplicableExtensions, orderedEquals([leg]));

testing/test_package/lib/fake.dart

@@ -1018,6 +1018,49 @@ class AnotherExtended<T extends BaseTest> extends BaseTest {}
 
 class BigAnotherExtended extends AnotherExtended<SubclassBaseTest> {}
 
+extension ExtensionCheckLeft on Implemented1 {
+  int get left => 3;
+}
+
+extension ExtensionCheckRight on Implemented2 {
+  int get right => 4;
+}
+
+extension ExtensionCheckCenter on BaseExtended {
+  bool get center => true;
+}
+
+class BaseExtended {}
+
+class Implemented1 {}
+
+class Implemented2 {}
+
+class Implementor extends BaseExtended
+    implements Implemented1, Implemented2, Implementor2 {}
+
+extension ExtensionCheckImplementor2 on Implementor2 {
+  int get test => 1;
+}
+
+class Implementor2 implements Implemented1 {}
+
+class OldSchoolMixin {
+  aThing() {}
+}
+
+mixin NewSchoolMixin {}
+
+extension OnNewSchool on NewSchoolMixin {
+  String get bar => 'hello';
+}
+
+extension OnOldSchool on OldSchoolMixin {
+  int get foo => 4;
+}
+
+class School with OldSchoolMixin, NewSchoolMixin {}
+
 //
 //
 //