fix #27259, implement covariance checking for strong mode and DDC

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

Index: pkg/analyzer/lib/src/generated/type_system.dart
diff --git a/pkg/analyzer/lib/src/generated/type_system.dart b/pkg/analyzer/lib/src/generated/type_system.dart
index ff4eaf445fa7c4461ed3dd5aa2f59323e164d557..a7b7858b4159362fe08959c7e63b3a70fa147a28 100644
--- a/pkg/analyzer/lib/src/generated/type_system.dart
+++ b/pkg/analyzer/lib/src/generated/type_system.dart
@@ -503,13 +503,35 @@ class StrongTypeSystemImpl extends TypeSystem {
   /// - we know the function types are strict arrows,
   /// - it allows opt-in covariant parameters.
   bool isOverrideSubtypeOf(FunctionType f1, FunctionType f2) {
-    return FunctionTypeImpl.relate(
-        f1,
-        f2,
-        (t1, t2, t1Covariant, _) =>
-            isSubtypeOf(t2, t1) || t1Covariant && isSubtypeOf(t1, t2),
-        instantiateToBounds,
-        returnRelation: isSubtypeOf);
+    return FunctionTypeImpl.relate(f1, f2, isSubtypeOf, instantiateToBounds,
+        parameterRelation: _isOverrideSubtypeOfParameter);
+  }
+
+  bool _isOverrideSubtypeOfParameter(ParameterElement p1, ParameterElement p2) {
+    return isSubtypeOf(p2.type, p1.type) ||
+        p1.isCovariant && isSubtypeOf(p1.type, p2.type);
+  }
+
+  /// Given the [f1] of a member override and the [f2] of the

[Leaf, 2017/06/28 18:12:29]

"Given the type [f1]... and the type [f2]"?

[Jennifer Messerly, 2017/07/05 20:11:20]

fixed in the moved/refactored code

+  /// base member, calls [visit] if it encounters any parameter from [f1] that
+  /// needs a covariance check.

[Leaf, 2017/06/28 18:12:29]

Clarify is this "covariant generic covariance check", or "covariant override
check" or both?

[Jennifer Messerly, 2017/07/05 20:11:20]

fixed in the moved/refactored code

+  void visitCovariantParameters(

[Leaf, 2017/06/28 18:12:29]

Ok, after reading the code in checker.dart, I think I understand what this code
is doing (except the type formal part), but I think it either should be moved or
renamed.  

Ignoring type formals, this code visits every parameter which is not a valid
override, where an override can be valid because it is a contra-variant change,
or because it is marked covariant.

You happen to use this to implement covariance checking by instantiating the
superclass type to its bound, and then checking whether the subclass type is a
valid override of the most general possible superclass type.  If it is, then no
amount of covariant subsumption can break soundness, but if it isn't, then we
need a covariance check.

The fact that you use this for covariance checking has nothing to do with this
method though (I think).  That is, the covariance checking is a combination of:
1) Doing something to the superclass type (instantiate to bounds)
2) Doing override checking (this function).

So it feels to me that either this is something general that should be in
type_system.dart and shouldn't talk about covariance at all, or else it should
just be moved into the same location as the rest of the covariance checking.  

At the least, the documentation here should explain the connection to
covariance.

[Jennifer Messerly, 2017/07/05 20:11:20]

moved!

+      FunctionType f1,
+      FunctionType f2,
+      visitTypeFormal(TypeParameterElement p),
+      visitParameter(ParameterElement p)) {
+    var fresh = FunctionTypeImpl.relateTypeFormals(f1, f2, (b2, b1, p2, p1) {
+      if (!isSubtypeOf(b2, b1)) visitTypeFormal(p1);

[Leaf, 2017/06/28 18:12:29]

Not sure I understand this.

[Jennifer Messerly, 2017/07/05 20:11:20]

I've added a large example and doc comment to the refactored method in
checked.dart. Let me know if it helps clarify?

+      return true;
+    });
+    if (fresh != null) {
+      f1 = f1.instantiate(fresh);
+      f2 = f2.instantiate(fresh);
+    }
+    FunctionTypeImpl.relateParameters(f1.parameters, f2.parameters, (p1, p2) {
+      if (!_isOverrideSubtypeOfParameter(p1, p2)) visitParameter(p1);
+      return true;
+    });
   }
 
   @override
@@ -789,13 +811,10 @@ class StrongTypeSystemImpl extends TypeSystem {
   /// that dynamic parameters of f1 and f2 are treated as bottom.
   bool _isFunctionSubtypeOf(
       FunctionType f1, FunctionType f2, Set<TypeImpl> visitedTypes) {
-    return FunctionTypeImpl.relate(
-        f1,
-        f2,
-        (t1, t2, _, __) =>
-            _isSubtypeOf(t2, t1, visitedTypes, dynamicIsBottom: true),
-        instantiateToBounds,
-        returnRelation: isSubtypeOf);
+    return FunctionTypeImpl.relate(f1, f2, isSubtypeOf, instantiateToBounds,
+        parameterRelation: (p1, p2) => _isSubtypeOf(
+            p2.type, p1.type, visitedTypes,
+            dynamicIsBottom: true));
   }
 
   bool _isInterfaceSubtypeOf(
@@ -2178,14 +2197,16 @@ class _GenericInferrer {
       FunctionTypeImpl.relate(
           t1,
           t2,
-          (t1, t2, _, __) {
-            _matchSubtypeOf(t2, t1, null, origin,
-                covariant: !covariant, dynamicIsBottom: true);
+          (t1, t2) {
+            // TODO(jmesserly): should we flip covariance when we're relating
+            // type formal bounds? They're more like parameters.
+            matchSubtype(t1, t2);
             return true;
           },
           _typeSystem.instantiateToBounds,
-          returnRelation: (t1, t2) {
-            matchSubtype(t1, t2);
+          parameterRelation: (p1, p2) {
+            _matchSubtypeOf(p2.type, p1.type, null, origin,
+                covariant: !covariant, dynamicIsBottom: true);
             return true;
           });
     }