fix #25183, add support to ErrorVerifier for generic methods

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

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library engine.resolver.error_verifier;
 
 import 'dart:collection';
 import "dart:math" as math;
 
 import 'package:analyzer/src/generated/static_type_analyzer.dart';
@@ skipping 1329 matching lines @@
     String executableElementName = executableElement.name;
     FunctionType overridingFT = executableElement.type;
     FunctionType overriddenFT = overriddenExecutable.type;
     InterfaceType enclosingType = _enclosingClass.type;
     overriddenFT = _inheritanceManager
         .substituteTypeArgumentsInMemberFromInheritance(
             overriddenFT, executableElementName, enclosingType);
     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;
     List<DartType> overriddenNormalPT = overriddenFT.normalParameterTypes;
     List<DartType> overridingPositionalPT = overridingFT.optionalParameterTypes;
     List<DartType> overriddenPositionalPT = overriddenFT.optionalParameterTypes;
     Map<String, DartType> overridingNamedPT = overridingFT.namedParameterTypes;
     Map<String, DartType> overriddenNamedPT = overriddenFT.namedParameterTypes;
     // CTEC.INVALID_OVERRIDE_REQUIRED, CTEC.INVALID_OVERRIDE_POSITIONAL and
     // CTEC.INVALID_OVERRIDE_NAMED
@@ skipping 4750 matching lines @@
     toCheck.add(type.element);
     // type arguments
     if (type is InterfaceType) {
       InterfaceType interfaceType = type;
       for (DartType typeArgument in interfaceType.typeArguments) {
         _addTypeToCheck(typeArgument);
       }
     }
   }
 }