fix #25183, add support to ErrorVerifier for generic methods

pkg/analyzer/lib/src/generated/error_verifier.dart

Index: pkg/analyzer/lib/src/generated/error_verifier.dart
diff --git a/pkg/analyzer/lib/src/generated/error_verifier.dart b/pkg/analyzer/lib/src/generated/error_verifier.dart
index 094b3e4b3ae5bf21a91ec67e384981f5dd6dd2dd..951b6688fac0b9351b1362206d66ede3d5befb36 100644
--- a/pkg/analyzer/lib/src/generated/error_verifier.dart
+++ b/pkg/analyzer/lib/src/generated/error_verifier.dart
@@ -1347,6 +1347,76 @@ class ErrorVerifier extends RecursiveAstVisitor<Object> {
     if (overridingFT == null || overriddenFT == null) {
       return false;
     }
+
+    // Handle generic function type parameters.
+    // TODO(jmesserly): generic methods should work without strong mode,
+    // however "instantiateToBounds" is currently on StrongTypeSystemImpl.

[Leaf, 2015/12/09 22:02:29]

The non-strong mode semantics might be different, so I suspect that
instantiateToBounds will end up with two different implementations (I think the
non-strong mode POR is that it instantiates with dynamic).  So it might make
sense to put it on the interface and have two different versions.  On the other
hand, I think in non-strong mode you may be able to implement this simply by
leaving the variables un-instantiated (since you're checking assignability
instead of subtyping).  I don't think this works here because we do subtyping
with proper variance.

[Jennifer Messerly, 2015/12/09 22:37:41]

Done. Fixed this TODO by implementing the right logic in the spec type system

+    TypeSystem ts = _typeSystem;
+    if (ts is StrongTypeSystemImpl) {
+      // TODO(jmesserly): this duplicates some code in isSubtypeOf and most of
+      // _isGenericFunctionSubtypeOf. Ideally, we'd let TypeSystem produce
+      // an error message once it's ready to "return false".

[Leaf, 2015/12/09 22:02:29]

Yeah, this seems like it would work out well.  Maybe just by passing in an
optional callback to call before failing which yields the error.

[Jennifer Messerly, 2015/12/09 22:37:41]

Sounds good. I don't want to try that restructure yet but as we fold in
CodeChecker I think it would be a great time.

+      if (!overridingFT.boundTypeParameters.isEmpty) {
+        if (overriddenFT.boundTypeParameters.isEmpty) {
+          overriddenFT = ts.instantiateToBounds(overriddenFT);
+        } else {
+          List<TypeParameterElement> params1 = overridingFT.boundTypeParameters;
+          List<TypeParameterElement> params2 = overriddenFT.boundTypeParameters;
+          int count = params1.length;
+          if (params2.length != count) {
+            _errorReporter.reportErrorForNode(
+                StaticWarningCode.INVALID_METHOD_OVERRIDE_TYPE_PARAMETERS,
+                errorNameTarget, [
+              count,
+              params2.length,
+              overriddenExecutable.enclosingElement.displayName
+            ]);
+            return true;
+          }
+          // We build up a substitution matching up the type parameters
+          // from the two types, {variablesFresh/variables1} and
+          // {variablesFresh/variables2}
+          List<DartType> variables1 = new List<DartType>();
+          List<DartType> variables2 = new List<DartType>();
+          List<DartType> variablesFresh = new List<DartType>();
+          for (int i = 0; i < count; i++) {
+            TypeParameterElement p1 = params1[i];
+            TypeParameterElement p2 = params2[i];
+            TypeParameterElementImpl pFresh =
+                new TypeParameterElementImpl(p2.name, -1);
+
+            DartType variable1 = p1.type;
+            DartType variable2 = p2.type;
+            DartType variableFresh = new TypeParameterTypeImpl(pFresh);
+
+            variables1.add(variable1);
+            variables2.add(variable2);
+            variablesFresh.add(variableFresh);
+            DartType bound1 = p1.bound ?? DynamicTypeImpl.instance;
+            DartType bound2 = p2.bound ?? DynamicTypeImpl.instance;
+            bound1 = bound1.substitute2(variablesFresh, variables1);
+            bound2 = bound2.substitute2(variablesFresh, variables2);
+            pFresh.bound = bound2;
+            if (!ts.isSubtypeOf(bound2, bound1)) {
+              _errorReporter.reportErrorForNode(
+                  StaticWarningCode.INVALID_METHOD_OVERRIDE_TYPE_PARAMETER_BOUND,
+                  errorNameTarget, [
+                p1.displayName,
+                p1.bound,
+                p2.displayName,
+                p2.bound,
+                overriddenExecutable.enclosingElement.displayName
+              ]);
+              return true;
+            }
+          }
+          // Proceed with the rest of the checks, using instantiated types.

[Leaf, 2015/12/09 22:02:29]

The error messages that we get from the rest of the checks may be less than
ideal in the case that the programmer used a different name for a type parameter
in the overriding method.  Maybe add a TODO to consider how to present this
nicely?

[Jennifer Messerly, 2015/12/09 22:37:41]

Eeeep good catch. I've switch it to use the "overridingFT" name, in other words,
p1.name. Added test too!


With that fixed, I *think* this will be okay, since the errors are in terms of
the method you're looking at. The phrasing of Analyzer's existing error messages
kind of takes that into account. For example:

abstract class B<T> {
  Iterable<T> method();
}

abstract class D<S> extends B<S> {
  String method();
}
// [warning] The return type 'String' is not assignable to 'Iterable<S>' as
required by the method it is overriding from 'B'
(/Users/jmesserly/scratch/test.dart, line 6, col 10)

... it's in terms of your generic parameter 'S' even though if you go over to
'B' you'll see 'T'. So it tells you what you can put in there to make it work.

+          overridingFT = overridingFT.instantiate(variablesFresh);
+          overriddenFT = overriddenFT.instantiate(variablesFresh);
+        }
+      }
+    }
+
     DartType overridingFTReturnType = overridingFT.returnType;
     DartType overriddenFTReturnType = overriddenFT.returnType;
     List<DartType> overridingNormalPT = overridingFT.normalParameterTypes;