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

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

 // Copyright (c) 2015, 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 analyzer.src.generated.type_system;
 
 import 'dart:collection';
 import 'dart:math' as math;
 
 import 'package:analyzer/dart/ast/ast.dart' show AstNode;
@@ skipping 485 matching lines @@
     return type is FunctionType ||
         !nonnullableTypes.contains(_getTypeFullyQualifiedName(type));
   }
 
   /// Check that [f1] is a subtype of [f2] for an override.
   ///
   /// This is different from the normal function subtyping in two ways:
   /// - 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
   bool isSubtypeOf(DartType leftType, DartType rightType) {
     return _isSubtypeOf(leftType, rightType, null);
   }
 
   /// Given a [type] T that may have an unknown type `?`, returns a type
   /// R such that R <: T for any type substituted for `?`.
   ///
@@ skipping 259 matching lines @@
     return InterfaceTypeImpl.computeLeastUpperBound(type1, type2) ??
         typeProvider.dynamicType;
   }
 
   /// Check that [f1] is a subtype of [f2].
   ///
   /// This will always assume function types use fuzzy arrows, in other words
   /// 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(
       InterfaceType i1, InterfaceType i2, Set<TypeImpl> visitedTypes) {
     if (identical(i1, i2)) {
       return true;
     }
 
     // Guard recursive calls
     _GuardedSubtypeChecker<InterfaceType> guardedInterfaceSubtype = _guard(
@@ skipping 1362 matching lines @@
     // which is a super type of the function type.
     if (t1 is InterfaceType) {
       t1 = _typeSystem.getCallMethodDefiniteType(t1);
       if (t1 == null) return;
     }
 
     if (t1 is FunctionType && t2 is FunctionType) {
       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;
           });
     }
   }
 
   static String _formatConstraints(Iterable<_TypeConstraint> constraints) {
     List<List<String>> lineParts =
         new Set<_TypeConstraintOrigin>.from(constraints.map((c) => c.origin))
             .map((o) => o.formatError())
             .toList();
@@ skipping 177 matching lines @@
   /// Combining these constraints results in a lower bound of `num`.
   ///
   /// In general, we choose the lower bound as our inferred type, so we can
   /// offer the most constrained (strongest) result type.
   final DartType lowerBound;
 
   _TypeRange({DartType lower, DartType upper})
       : lowerBound = lower ?? UnknownInferredType.instance,
         upperBound = upper ?? UnknownInferredType.instance;
 }