fix part of #25200, reject non-generic function subtype of generic function

pkg/analyzer/lib/src/dart/element/type.dart

Index: pkg/analyzer/lib/src/dart/element/type.dart
diff --git a/pkg/analyzer/lib/src/dart/element/type.dart b/pkg/analyzer/lib/src/dart/element/type.dart
index 1b9c290665b16c8ee63d14e80b244efee4f6cf64..076d551523c04d785402ed714f1f36d32d4c88ab 100644
--- a/pkg/analyzer/lib/src/dart/element/type.dart
+++ b/pkg/analyzer/lib/src/dart/element/type.dart
@@ -534,28 +534,7 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
     // To test this, we instantiate both types with the same (unique) type
     // variables, and see if the result is equal.
     if (typeFormals.isNotEmpty) {
-      List<DartType> instantiateTypeArgs = new List<DartType>();
-      List<DartType> variablesThis = new List<DartType>();
-      List<DartType> variablesOther = new List<DartType>();
-      for (int i = 0; i < typeFormals.length; i++) {
-        TypeParameterElement pThis = typeFormals[i];
-        TypeParameterElement pOther = otherType.typeFormals[i];
-        TypeParameterTypeImpl pFresh = new TypeParameterTypeImpl(
-            new TypeParameterElementImpl(pThis.name, -1));
-        instantiateTypeArgs.add(pFresh);
-        variablesThis.add(pThis.type);
-        variablesOther.add(pOther.type);
-        // Check that the bounds are equal after equating the previous
-        // bound variables.
-        if (pThis.bound?.substitute2(instantiateTypeArgs, variablesThis) !=
-            pOther.bound?.substitute2(instantiateTypeArgs, variablesOther)) {
-          return false;
-        }
-      }
-      // After instantiation, they will no longer have typeFormals,
-      // so we will continue below.
-      return this.instantiate(instantiateTypeArgs) ==
-          otherType.instantiate(instantiateTypeArgs);
+      return structuralCompareGeneric(this, otherType, (t, s) => t == s);
     }
 
     return returnType == otherType.returnType &&
@@ -722,8 +701,11 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
       return relation;
     }
 

[Leaf, 2016/02/09 21:29:25]

Why does this ignore the generic case?  If just not handled yet, then maybe
TODO?

[Jennifer Messerly, 2016/02/10 22:42:07]

Good catch. Fixed!

-    return structuralCompare(this, type,
-        (TypeImpl t, TypeImpl s) => t.isMoreSpecificThan(s, withDynamic));
+    return structuralCompare(
+        this,
+        type,
+        (DartType t, DartType s) =>

[Leaf, 2016/02/09 21:29:25]

Non-actionable comment, but the TypeImpl/Type distinction doesn't seem to be
very coherent in the code right now.  

[Jennifer Messerly, 2016/02/10 22:42:07]

Yeah, definitely. I'm trying to get things strong mode clean (in another branch)
and there are definitely issues around unsafe *Impl downcasts.

+            (t as TypeImpl).isMoreSpecificThan(s, withDynamic));
   }
 
   @override
@@ -732,9 +714,78 @@ class FunctionTypeImpl extends TypeImpl implements FunctionType {
     if (relation != null) {
       return relation;
     }
+    // This type cast is safe, because _trivialFunctionRelation checks for it.

[Leaf, 2016/02/09 21:29:25]

It feels like this code could become part of structuralCompareGeneric.  It would
presumably need to be parameterized over the instantiation to bounds though,
which might start to get heavy.  It almost feels like structuralCompareXXX
should be shared methods on the TypeSystem class.  I think we don't have access
to an appropriate type system instance here though, unfortunately.  So maybe
this is also a non-actionable comment, but food for thought?

[Jennifer Messerly, 2016/02/10 22:42:07]

Great idea. Done.

+    FunctionType f2 = type as FunctionType;
+    if (typeFormals.isNotEmpty) {
+      if (f2.typeFormals.isEmpty) {
+        List<DartType> dynamicTypeArgs = new List<DartType>.filled(

[Leaf, 2016/02/09 21:29:25]

Would be nice to use instantiateToBounds here, but I guess that doesn't work out
with the way that the current separation between TypeSystem methods and *Type
methods works.

[Jennifer Messerly, 2016/02/10 22:42:07]

Added instantiateToBounds parameter.

I do wonder if this code (now called "FunctionTypeImpl.relate" as in "relate two
function types") should be in TypeSystem, though.

+            typeFormals.length, DynamicTypeImpl.instance);
+        return instantiate(dynamicTypeArgs).isSubtypeOf(f2);
+      } else {
+        return structuralCompareGeneric(
+            this, f2, (DartType t, DartType s) => t.isSubtypeOf(s));
+      }
+    } else if (f2.typeFormals.isNotEmpty) {
+      return false;
+    }
 
     return structuralCompare(

[Leaf, 2016/02/09 21:29:25]

See comment below on the structuralCompare/structuralCompareGeneric split.  I
think this code gets clearer if there's just a little logic to decide whether to
instantiate to bounds or return false in the mismatch case, and otherwise just
calls directly to structuralCompare.

[Jennifer Messerly, 2016/02/10 22:42:07]

Fixed :)

-        this, type, (TypeImpl t, TypeImpl s) => t.isAssignableTo(s));
+        this, f2, (DartType t, DartType s) => t.isAssignableTo(s));
+  }
+
+  /**
+   * Check a [functionRelation] (typically isSubtypeOf) between [f1] and [f2]
+   * where f1 and f2 are known to be generic function types (both have type
+   * formals).
+   *

[Bob Nystrom, 2016/02/09 01:25:18]

Writing nit: It's easy to get into the habit of future tense, but present is
usually better.

http://www.writer-on-line.com/content/view/319/66/~Articles/Technical-Writing...

[Jennifer Messerly, 2016/02/10 22:42:07]

Acknowledged.

+   * This will first check that f1 and f2 have the same number of type formals.
+   * It will also check that their bounds (`<T extends LowerBound>`) are
+   * compatible using [relation]. If the formals are not compatible, this will
+   * return `false`.
+   *
+   * If [functionRelation] is omitted, this will use [relation] for both.
+   */
+  static bool structuralCompareGeneric(

[Leaf, 2016/02/09 21:29:25]

I'm not very keen on the current factoring of this into structuralCompare and
structuralCompareGeneric (and some of the resultant consequences).  It seems to
be almost always the case that there is some twisty conditional logic to see
whether or not to call structuralCompare, or to call structuralCompareGeneric
with a callback that will essentially end up calling structuralCompare.  This
gives the small optimization benefit of in some cases not checking for the
presence of formals twice, at quite a bit of readability expense.  I think this
code would be much clearer if the top level function was just structuralCompare,
with named parameters for the various sub-component comparison callbacks as
needed.  This could then be factored out to structuralCompareTypeFormals and
structuralCompareParametersAndReturn or somesuch if desired and/or if there
really is some code that truly needs to be able to call directly into only one
of these as well. 

[Jennifer Messerly, 2016/02/10 22:42:07]

All factoring is fixed :D. At least as much as possible without moving code over
to TypeSystem.

+      FunctionType f1, FunctionType f2, bool relation(DartType t, DartType s),
+      [bool functionRelation(FunctionType t, FunctionType s)]) {
+    functionRelation ??= relation;
+
+    List<TypeParameterElement> params1 = f1.typeFormals;
+    List<TypeParameterElement> params2 = f2.typeFormals;
+    int count = params1.length;
+    if (params2.length != count) {
+      return false;
+    }
+    // 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>();

[Bob Nystrom, 2016/02/09 01:25:18]

<DartType>[]

[Jennifer Messerly, 2016/02/10 22:42:07]

Done.

+    for (int i = 0; i < count; i++) {
+      TypeParameterElement p1 = params1[i];
+      TypeParameterElement p2 = params2[i];
+      TypeParameterElementImpl pFresh =
+          new TypeParameterElementImpl(p2.name, -1);

[Bob Nystrom, 2016/02/09 01:25:18]

What do you think about either making the offset optional or adding a
.synthetic() named constructor?

Should its isSynthetic property return true?

[Jennifer Messerly, 2016/02/10 22:42:07]

Done.

+
+      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 (!relation(bound2, bound1)) {
+        return false;
+      }
+    }
+    return functionRelation(
+        f1.instantiate(variablesFresh), f2.instantiate(variablesFresh));
   }
 
   /**