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

pkg/analyzer/lib/src/task/strong/checker.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.
 
 // TODO(jmesserly): this was ported from package:dev_compiler, and needs to be
 // refactored to fit into analyzer.
 library analyzer.src.task.strong.checker;
 
+import 'dart:collection';
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/standard_resolution_map.dart';
 import 'package:analyzer/dart/ast/token.dart' show TokenType;
 import 'package:analyzer/dart/ast/token.dart';
 import 'package:analyzer/dart/ast/visitor.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/source/error_processor.dart' show ErrorProcessor;
 import 'package:analyzer/src/dart/element/element.dart';
+import 'package:analyzer/src/dart/element/member.dart';
 import 'package:analyzer/src/dart/element/type.dart';
 import 'package:analyzer/src/error/codes.dart' show StrongModeCode;
 import 'package:analyzer/src/generated/engine.dart' show AnalysisOptionsImpl;
 import 'package:analyzer/src/generated/resolver.dart' show TypeProvider;
 import 'package:analyzer/src/generated/type_system.dart';
 import 'package:analyzer/src/summary/idl.dart';
 
 import 'ast_properties.dart';
 
 /// Given an [expression] and a corresponding [typeSystem] and [typeProvider],
@@ skipping 15 matching lines @@
     return typeSystem.functionTypeToConcreteType(type);
   }
   return type;
 }
 
 bool hasStrictArrow(Expression expression) {
   var element = _getKnownElement(expression);
   return element is FunctionElement || element is MethodElement;
 }
 
+InterfaceType _getCovariantUpperBound(TypeSystem rules, ClassElement iface) {
+  var upperBound = rules.instantiateToBounds(iface.type) as InterfaceType;
+  var typeArgs = upperBound.typeArguments;
+  // TODO(jmesserly): remove this. It is a workaround for fuzzy arrows.
+  // To prevent extra checks due to fuzzy arrows, we need to instantiate with
+  // `Object` rather than `dynamic`. Consider a case like:
+  //
+  //     class C<T> {
+  //       void forEach(f(T t)) {}
+  //     }
+  //
+  // If we try `(dynamic) ~> void <: (T) ~> void` with fuzzy arrows, we will
+  // treat `dynamic` as `bottom` and get `(bottom) -> void <: (T) -> void`
+  // which indicates that a check is required on the parameter `f`. This check
+  // is not sufficient when `T` is `dynamic`, however, because calling a
+  // function with a fuzzy arrow type is not safe and requires a dynamic call.
+  // See: https://github.com/dart-lang/sdk/issues/29295
+  //
+  // For all other values of T, the check is unnecessary: it is sound to pass
+  // a function that accepts any Object.
+  if (typeArgs.any((t) => t.isDynamic)) {
+    var newTypeArgs = typeArgs
+        .map((t) => t.isDynamic ? rules.typeProvider.objectType : t)
+        .toList();
+    upperBound = iface.type.instantiate(newTypeArgs);
+  }
+  return upperBound;
+}
+
 DartType _elementType(Element e) {
   if (e == null) {
     // Malformed code - just return dynamic.
     return DynamicTypeImpl.instance;
   }
   return (e as dynamic).type;
 }
 
 Element _getKnownElement(Expression expression) {
   if (expression is ParenthesizedExpression) {
@@ skipping 32 matching lines @@
     field = setter.variable;
   } else {
     return null;
   }
   if (field.isSynthetic) return null;
   return field;
 }
 
 /// Looks up the declaration that matches [member] in [type] and returns it's
 /// declared type.
-FunctionType _getMemberType(InterfaceType type, ExecutableElement member) =>
-    _memberTypeGetter(member)(type);
-
-_MemberTypeGetter _memberTypeGetter(ExecutableElement member) {
+FunctionType _getMemberType(InterfaceType type, ExecutableElement member) {
   String memberName = member.name;
   final isGetter = member is PropertyAccessorElement && member.isGetter;
   final isSetter = member is PropertyAccessorElement && member.isSetter;
 
-  FunctionType f(InterfaceType type) {
-    ExecutableElement baseMethod;
-
-    if (member.isPrivate) {
-      var subtypeLibrary = member.library;
-      var baseLibrary = type.element.library;
-      if (baseLibrary != subtypeLibrary) {
-        return null;
-      }
+  ExecutableElement baseMethod;
+  if (member.isPrivate) {
+    var subtypeLibrary = member.library;
+    var baseLibrary = type.element.library;
+    if (baseLibrary != subtypeLibrary) {
+      return null;
     }
-
-    try {
-      if (isGetter) {
-        assert(!isSetter);
-        // Look for getter or field.
-        baseMethod = type.getGetter(memberName);
-      } else if (isSetter) {
-        baseMethod = type.getSetter(memberName);
-      } else {
-        baseMethod = type.getMethod(memberName);
-      }
-    } catch (e) {
-      // TODO(sigmund): remove this try-catch block (see issue #48).
-    }
-    if (baseMethod == null || baseMethod.isStatic) return null;
-    return baseMethod.type;
   }
 
-  return f;
+  try {
+    if (isGetter) {
+      assert(!isSetter);
+      // Look for getter or field.
+      baseMethod = type.getGetter(memberName);
+    } else if (isSetter) {
+      baseMethod = type.getSetter(memberName);
+    } else {
+      baseMethod = type.getMethod(memberName);
+    }
+  } catch (e) {
+    // TODO(sigmund): remove this try-catch block (see issue #48).

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

Looks like this can be removed now?

https://github.com/dart-archive/dev_compiler/issues/48

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

Done.

+  }
+  if (baseMethod == null || baseMethod.isStatic) return null;
+  return baseMethod.type;
 }
 
-typedef FunctionType _MemberTypeGetter(InterfaceType type);
-
 /// Checks the body of functions and properties.
 class CodeChecker extends RecursiveAstVisitor {
   final StrongTypeSystemImpl rules;
   final TypeProvider typeProvider;
   final AnalysisErrorListener reporter;
   final AnalysisOptionsImpl _options;
   _OverrideChecker _overrideChecker;
 
   bool _failure = false;
   bool _hasImplicitCasts;
+  ClassElement _currentClass;
+  HashSet<ExecutableElement> _covariantPrivateMembers;
 
   CodeChecker(TypeProvider typeProvider, StrongTypeSystemImpl rules,
       AnalysisErrorListener reporter, this._options)
       : typeProvider = typeProvider,
         rules = rules,
         reporter = reporter {
     _overrideChecker = new _OverrideChecker(this);
   }
 
   bool get failure => _failure;
@@ skipping 27 matching lines @@
       _checkImplicitCast(expr, type);
     }
   }
 
   /// Analyzer checks boolean conversions, but we need to check too, because
   /// it uses the default assignability rules that allow `dynamic` and `Object`
   /// to be assigned to bool with no message.
   void checkBoolean(Expression expr) =>
       checkAssignment(expr, typeProvider.boolType);
 
-  void checkFunctionApplication(InvocationExpression node) {
+  void _checkFunctionApplication(InvocationExpression node) {
     var ft = _getTypeAsCaller(node);
 
     if (_isDynamicCall(node, ft)) {
       // If f is Function and this is a method invocation, we should have
       // gotten an analyzer error, so no need to issue another error.
       _recordDynamicInvoke(node, node.function);
     } else {
+      _checkFunctionApplicationForCovariance(node);
       checkArgumentList(node.argumentList, ft);
     }
   }
 
   DartType getType(TypeAnnotation type) {
     return type?.type ?? DynamicTypeImpl.instance;
   }
 
   void reset() {
     _failure = false;
@@ skipping 64 matching lines @@
         default:
           assert(false);
       }
     }
     node.visitChildren(this);
   }
 
   @override
   void visitClassDeclaration(ClassDeclaration node) {
     _overrideChecker.check(node);
+    var savedClass = _currentClass;

[vsm, 2017/06/26 22:14:20]

Can these ever be nested?  If not, maybe

assert(_currentClass == null);

?

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

Done.

+    _currentClass = node.element;
     super.visitClassDeclaration(node);
+    _currentClass = savedClass;
   }
 
   @override
   void visitClassTypeAlias(ClassTypeAlias node) {
     _overrideChecker.check(node);
     super.visitClassTypeAlias(node);
   }
 
   @override
   void visitComment(Comment node) {
     // skip, no need to do typechecking inside comments (they may contain
     // comment references which would require resolution).
   }
 
   @override
   void visitCompilationUnit(CompilationUnit node) {
     _hasImplicitCasts = false;
+    _covariantPrivateMembers = new HashSet();
     node.visitChildren(this);
     setHasImplicitCasts(node, _hasImplicitCasts);
+    setCovariantPrivateMembers(node, _covariantPrivateMembers);
   }
 
   @override
   void visitConditionalExpression(ConditionalExpression node) {
     checkBoolean(node.condition);
     node.visitChildren(this);
   }
 
   /// Check constructor declaration to ensure correct super call placement.
   @override
@@ skipping 105 matching lines @@
   @override
   void visitForStatement(ForStatement node) {
     if (node.condition != null) {
       checkBoolean(node.condition);
     }
     node.visitChildren(this);
   }
 
   @override
   void visitFunctionExpressionInvocation(FunctionExpressionInvocation node) {
-    checkFunctionApplication(node);
+    _checkFunctionApplication(node);
     node.visitChildren(this);
   }
 
   @override
   void visitIfStatement(IfStatement node) {
     checkBoolean(node.condition);
     node.visitChildren(this);
   }
 
   @override
@@ skipping 106 matching lines @@
       //
       // ... from case like:
       //
       //     SomeType s;
       //     s.someDynamicField(...); // static get, followed by dynamic call.
       //
       // The first case is handled here, the second case is handled below when
       // we call [checkFunctionApplication].
       setIsDynamicInvoke(node.methodName, true);
     } else {
-      checkFunctionApplication(node);
+      _checkFunctionApplication(node);
+      _trackPrivateMemberCovariance(target, node.methodName.staticElement);
     }
     node.visitChildren(this);
   }
 
   @override
   void visitPostfixExpression(PostfixExpression node) {
     _checkUnary(node.operand, node.operator, node.staticElement);
     node.visitChildren(this);
   }
 
@@ skipping 30 matching lines @@
     node.visitChildren(this);
   }
 
   @override
   void visitReturnStatement(ReturnStatement node) {
     _checkReturnOrYield(node.expression, node);
     node.visitChildren(this);
   }
 
   @override
+  void visitSimpleIdentifier(SimpleIdentifier node) {
+    _trackPrivateMemberCovariance(null, node.staticElement);
+  }
+
+  @override
   void visitSuperConstructorInvocation(SuperConstructorInvocation node) {
     var element = node.staticElement;
     if (element != null) {
       var type = resolutionMap.staticElementForConstructorReference(node).type;
       checkArgumentList(node.argumentList, type);
     }
     node.visitChildren(this);
   }
 
   @override
@@ skipping 28 matching lines @@
           variableElement.kind == ElementKind.FIELD) {
         _validateTopLevelInitializer(variableElement.name, node.initializer);
       }
     }
     return super.visitVariableDeclaration(node);
   }
 
   @override
   void visitVariableDeclarationList(VariableDeclarationList node) {
     TypeAnnotation type = node.type;
-    if (type == null) {
-      // No checks are needed when the type is var. Although internally the
-      // typing rules may have inferred a more precise type for the variable
-      // based on the initializer.
-    } else {
-      for (VariableDeclaration variable in node.variables) {
-        var initializer = variable.initializer;
-        if (initializer != null) {
+
+    for (VariableDeclaration variable in node.variables) {
+      var initializer = variable.initializer;
+      if (initializer != null) {
+        if (type != null) {
           checkAssignment(initializer, type.type);
+        } else {
+          // Most checks are not needed when the type is var, however, we still
+          // need to check for unsafe function types from the initialize due
+          // to covariance.
+          var toType = _getDefiniteType(initializer);
+          if (toType is FunctionType) {
+            _checkImplicitCastForCovariance(initializer, toType);
+          }
         }
       }
     }
+
     node.visitChildren(this);
   }
 
   @override
   void visitWhileStatement(WhileStatement node) {
     checkBoolean(node.condition);
     node.visitChildren(this);
   }
 
   @override
@@ skipping 33 matching lines @@
       // back to it. So these two implicit casts are equivalent:
       //
       //     y = /*implicit cast*/(y + 42);
       //     /*implicit assignment cast*/y += 42;
       //
       _checkImplicitCast(expr.leftHandSide, lhsType,
           from: returnType, opAssign: true);
     }
   }
 
-  void _checkFieldAccess(AstNode node, AstNode target, SimpleIdentifier field) {
-    if (field.staticElement == null &&
-        !typeProvider.isObjectMember(field.name)) {
+  void _checkFieldAccess(
+      AstNode node, Expression target, SimpleIdentifier field) {
+    var element = field.staticElement;
+    _trackPrivateMemberCovariance(target, element);
+    if (element == null && !typeProvider.isObjectMember(field.name)) {
       _recordDynamicInvoke(node, target);
     }
     node.visitChildren(this);
   }
 
   /// Checks if an implicit cast of [expr] from [from] type to [to] type is
   /// needed, and if so records it.
   ///
   /// If [from] is omitted, uses the static type of [expr].
   ///
   /// If [expr] does not require an implicit cast because it is not related to
   /// [to] or is already a subtype of it, does nothing.
   void _checkImplicitCast(Expression expr, DartType to,
       {DartType from, bool opAssign: false}) {
     from ??= _getDefiniteType(expr);
 
     if (_needsImplicitCast(expr, to, from: from) == true) {
       _recordImplicitCast(expr, to, from: from, opAssign: opAssign);
+    } else if (to is FunctionType) {
+      _checkImplicitCastForCovariance(expr, to);
     }
   }
 
   /// Checks if the assignment is valid with respect to non-nullable types.
   /// Returns `false` if a nullable expression is assigned to a variable of
   /// non-nullable type and `true` otherwise.
   bool _checkNonNullAssignment(
       Expression expression, DartType to, DartType from) {
     if (rules.isNonNullableType(to) && rules.isNullableType(from)) {
       _recordMessage(
@@ skipping 56 matching lines @@
         //     /*implicit assignment cast*/y++;
         //
         _checkImplicitCast(operand, lhsType, from: returnType, opAssign: true);
       }
     }
   }
 
   DartType _getDefiniteType(Expression expr) =>
       getDefiniteType(expr, rules, typeProvider);
 
+  /// This gets the return type that is safe to rely on, given that we

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

What does "This gets the return type" mean here?

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

I've improved this comment, let me know if it helps

+  /// might be making a call on an (unsafe) covariant interface type.
+  ///
+  /// For example:
+  ///
+  ///     typedef F<T>(T t);
+  ///     class C<T> {
+  ///       F<T> f;
+  ///       C(this.f);
+  ///     }
+  ///     main() {
+  ///       C<Object> c = new C<int>((int x) => x + 42));
+  ///       F<Object> f = c.f; // need an implicit cast here.
+  ///       f('hello');
+  ///     }
+  ///
+  void _checkImplicitCastForCovariance(Expression node, FunctionType to,

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

Some high level comments here.  

My original thinking was that this check would always be done, regardless of the
target type.  That is, even if you immediately cast to Object, there's an
implied read check that will fail.  This seems more predictable, but less
forgiving, and is symmetric with the covariant parameter checks, which check
even if you don't use the argument (or use it only at a safe type).

I'm not 100% set on that approach though, it's not unreasonable to take the
target type into account.  It does seem more developer friendly.  Taking this
approach is much more delicate though.  In particular, I think you need to
either do this as part of downwards inference, or else essentially reproduce the
downwards context logic.  Otherwise I think you'll run into problems with things
like conditional expressions and the ?? operator which don't have an implied
cast immediately on the expression.  That is, I'm worried about things like:

///     main() {
///       C<Object> c = new C<int>((int x) => x + 42));
///       F<Object> f = (b) ? (x) => x : c.f;  // Need check on c.f here
///       (f ?? c.f)("hello") // Need check on c.f here

I don't think these will get caught here?

I worry that there may be other examples as well.  Can we characterize all the
syntactic possibilities?  Maybe better would be to record subsumption points
during downwards inference and use that to drive this "lazy" cast insertion?

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

Did a big refactor to simplify this. We always add the check now. Also this test
case was added.

+      {DartType target}) {
+    while (node is NamedExpression) {
+      node = (node as NamedExpression).expression;
+    }
+    while (node is ParenthesizedExpression) {
+      node = (node as ParenthesizedExpression).expression;
+    }
+
+    Element member;
+    bool isTearOff = false;
+    bool isMethodInvokeTarget = false;
+    if (node is PropertyAccess) {
+      target = node.realTarget.staticType;
+      member = node.propertyName.staticElement;
+      isTearOff = member is MethodElement;
+      if (target == null) return;
+    } else if (node is PrefixedIdentifier) {
+      target = node.prefix.staticType;
+      member = node.staticElement;
+      isTearOff = member is MethodElement;
+      if (target == null) return;
+    } else if (node is SimpleIdentifier) {
+      member = node.staticElement;
+      var parent = node.parent;
+      isMethodInvokeTarget =
+          parent is MethodInvocation && parent.methodName == node;
+      isTearOff = member is MethodElement;
+    } else if (node is MethodInvocation) {
+      target = node.realTarget?.staticType;
+      member = node.methodName.staticElement;
+    } else if (node is FunctionExpressionInvocation) {
+      member = node.staticElement;
+    }
+
+    // If we're calling an instance method or getter, then we
+    // want to check the result type.
+    //
+    // We intentionally ignore method tear-offs, because those methods have

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

This comment confused me, until I read the code below.  We're *not* ignoring
method tear-offs, it's just that we're only concerned with their return types.

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

(this is gone in the new code)

+    // covariance checks for their parameters inside the method.
+    if (member is ExecutableElement && _isInstanceMember(member)) {
+      if (target == null) {
+        if (!_inCurrentClass(member)) return;
+        // Use implicit `this` as the target.
+        target = _currentClass?.type;
+      }
+
+      if (target is InterfaceType && target.typeArguments.isNotEmpty) {

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

Not sure that this is sufficient?  Consider:

class D extends C<int> {
  F<int> f; // override f
  D(this.f);
}

main() {
   C<Object> c = new D((int x) => x + 42)); 
   // etc.
}

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

in this case, `c` has type `C<Object>` and so it will get checked.

+        // Get the lower bound of the declared return type (e.g. `F<Null>`) and
+        // see if it can be assigned to the expected type (e.g. `F<Object>`).

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

Clever... :)

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

Acknowledged.

+        //
+        // That way we can tell if any lower `T` will work or not.
+        var classType = target.element.type;
+        var classLowerBound = classType.instantiate(new List.filled(
+            classType.typeParameters.length, typeProvider.nullType));
+        var memberLowerBound = _lookUpMember(classLowerBound, member).type;
+
+        // If this is a tear-off, we can ignore the parameters (they're already
+        // checked) but we still need to check the return type.
+        var expectedType = isTearOff ? to.returnType : to;
+        if (!rules.isSubtypeOf(memberLowerBound.returnType, expectedType)) {
+          // We can't put an `as` expression inside of a MethodInvocation node,
+          // so we use the special cast annotation to indicate this.
+          if (isMethodInvokeTarget) {
+            setImplicitOperationCast(node, to);
+          } else {
+            setImplicitCast(node, to);
+          }
+          _hasImplicitCasts = true;
+        }
+      }
+    }
+  }
+
+  /// Similar to [_checkImplicitCastForCovariance] but make sure the function
+  /// type we're applying arguments to is valid.
+  ///
+  /// For example:
+  ///
+  ///     typedef F<T>(T t);
+  ///     class C<T> {
+  ///       F<T> f;
+  ///       C(this.f);
+  ///     }
+  ///     main() {
+  ///       C<Object> c = new C<int>((int x) => x + 42));
+  ///       Object obj = c.f; // no cast.
+  ///       c.f('hello');     // implicit cast
+  ///     }
+  void _checkFunctionApplicationForCovariance(InvocationExpression node) {
+    DartType target;
+    Element member;
+    if (node is MethodInvocation) {
+      target = node.realTarget?.staticType;
+      member = node.methodName.staticElement;
+    } else if (node is FunctionExpressionInvocation) {
+      member = node.staticElement;
+    }
+    // Caller side checks will take care of methods.
+    if (member is MethodElement) return;
+
+    var type = _getDefiniteType(node.function);
+    if (type is FunctionType) {
+      _checkImplicitCastForCovariance(node.function, type, target: target);
+    }
+  }
+
+  /// We can eliminate covariance checks on private members if they are only
+  /// accessed through something with a known generic type, such as `this`.
+  ///
+  /// For these expressions, we will know the generic parameters exactly:
+  ///
+  /// - this
+  /// - super
+  /// - non-factory instance creation
+  ///
+  /// For example:
+  ///
+  ///     class C<T> {
+  ///       T _t;
+  ///     }
+  ///     class D<T> extends C<T> {
+  ///        method<S extends T>(T t, C<T> c) {
+  ///          // implicit cast: t as T;
+  ///          // implicit cast: c as C<T>;
+  ///
+  ///          // These do not need further checks. The type parameter `T` for
+  ///          // `this` must be the same as our `T`
+  ///          this._t = t;
+  ///          super._t = t;
+  ///          new C<T>()._t = t; // non-factory
+  ///
+  ///          // This needs further checks. The type of `c` could be `C<S>` for
+  ///          // some `S <: T`.
+  ///          c._t = t;
+  ///          // factory statically returns `C<T>`, dynamically returns `C<S>`.
+  ///          new F<T, S>()._t = t;
+  ///        }
+  ///     }
+  ///     class F<T, S extends T> extends C<T> {
+  ///       factory F() => new C<S>();
+  ///     }
+  ///
+  void _trackPrivateMemberCovariance(Expression target, Element e) {
+    if (e is ExecutableElement && e.isPrivate && _isInstanceMember(e)) {
+      if (target == null) return; // implicit this or invalid code.
+      if (target is ThisExpression || target is SuperExpression) return;
+      if (target is InstanceCreationExpression &&
+          target.staticElement?.isFactory == false) {
+        return;
+      }
+      if (e is PropertyAccessorElement && e.isGetter) return;
+
+      var type = target.staticType;
+      if (type is InterfaceType && type.typeArguments.isNotEmpty) {
+        _covariantPrivateMembers
+            .add(e is Member ? (e as Member).baseElement : e);
+      }
+    }
+  }
+
+  bool _isInstanceMember(ExecutableElement e) =>
+      !e.isStatic &&
+      (e is MethodElement ||
+          e is PropertyAccessorElement && e.variable is FieldElement);
+
+  bool _inCurrentClass(ExecutableElement e) {
+    if (_currentClass == null) return false;
+    var match = _lookUpMember(_currentClass.type, e);
+    return e.enclosingElement == match.enclosingElement;
+  }
+
+  ExecutableElement _lookUpMember(InterfaceType type, ExecutableElement e) {
+    var name = e.name;
+    var library = e.library;
+    return e is PropertyAccessorElement
+        ? (e.isGetter
+            ? type.lookUpInheritedGetter(name, library: library)
+            : type.lookUpInheritedSetter(name, library: library))
+        : type.lookUpInheritedMethod(name, library: library);
+  }
+
   /// Gets the expected return type of the given function [body], either from
   /// a normal return/yield, or from a yield*.
   DartType _getExpectedReturnType(FunctionBody body, {bool yieldStar: false}) {
     FunctionType functionType;
     var parent = body.parent;
     if (parent is Declaration) {
       functionType = _elementType(parent.element);
     } else {
       assert(parent is FunctionExpression);
       functionType =
@@ skipping 192 matching lines @@
       errorCode = StrongModeCode.DYNAMIC_CAST;
     } else if (parent is VariableDeclaration && parent.initializer == expr) {
       errorCode = StrongModeCode.ASSIGNMENT_CAST;
     } else {
       errorCode = opAssign
           ? StrongModeCode.DOWN_CAST_IMPLICIT_ASSIGN
           : StrongModeCode.DOWN_CAST_IMPLICIT;
     }
     _recordMessage(expr, errorCode, [from, to]);
     if (opAssign) {
-      setImplicitAssignmentCast(expr, to);
+      setImplicitOperationCast(expr, to);
     } else {
       setImplicitCast(expr, to);
     }
     _hasImplicitCasts = true;
   }
 
   void _recordMessage(AstNode node, ErrorCode errorCode, List arguments) {
     // Compute the right severity taking the analysis options into account.
     // We construct a dummy error to make the common case where we end up
     // ignoring the strong mode message cheaper.
@@ skipping 190 matching lines @@
 
   void check(Declaration node) {
     var element =
         resolutionMap.elementDeclaredByDeclaration(node) as ClassElement;
     if (element.type.isObject) {
       return;
     }
     _checkSuperOverrides(node, element);
     _checkMixinApplicationOverrides(node, element);
     _checkAllInterfaceOverrides(node, element);
+    _checkForCovariantGenerics(node, element);
+  }
+
+  void _checkForCovariantGenerics(Declaration node, ClassElement element) {

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

These functions really need some documentation at the head explaining what
they're doing.  Something like:

Collect up all of the generic interfaces implemented by this class, and check
each concrete member of this class against each generic super-interface to
determine what covariance checks are needed.   Attaches these checks as
meta-data on XXX and YYY.

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

Done.

+    // Find all generic interfaces that are implemented, if any.
+    //
+    // Because we're going to instantiate these to their bounds, we don't have
+    // to track type parameters.
+    var allCovariant = _findCovariantSupertypes(element.type);
+    if (allCovariant.isEmpty) return;
+
+    // For each most-derived concrete member of the class, (which may be from a
+    // superclasses or mixin), check it against all generic superinterfaces that
+    // it was not already checked against.
+    var seenConcreteMembers = new HashSet<String>();
+    var members = _getConcreteMembers(element.type, seenConcreteMembers);
+
+    // For members on this class, check them against all interfaces.
+    var checks = _findCovariantChecks(members, allCovariant);
+    setClassCovariantParameters(node, checks);
+
+    checks = _findSuperclassCovariantChecks(
+        element, allCovariant, seenConcreteMembers);
+    setSuperclassCovariantParameters(node, checks);
+  }
+
+  Set<Element> _findSuperclassCovariantChecks(ClassElement element,

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

Ok, I need some in person guidance on this code, I'm too lost.  :)

This also really needs some documentation comments for future readers who don't
have the benefit of sitting down at the whiteboard with you.

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

I've added a lot of comments -- do they help?

+      Set<ClassElement> allCovariant, HashSet<String> seenConcreteMembers) {
+    var visited = new HashSet<ClassElement>()..add(element);
+    var superChecks = new Set<Element>();
+    var existingChecks = new HashSet<Element>();
+
+    void visitImmediateSuper(InterfaceType type) {
+      // For members of mixins/supertypes, check them against new interfaces,
+      // and also record any existing checks they already had.
+      var oldCovariant = _findCovariantSupertypes(type);
+      var newCovariant = allCovariant.difference(oldCovariant);
+      if (newCovariant.isEmpty) return;
+
+      void visitSuper(InterfaceType type) {
+        var element = type.element;
+        if (visited.add(element)) {
+          var members = _getConcreteMembers(type, seenConcreteMembers);
+          _findCovariantChecks(members, newCovariant, superChecks);
+          _findCovariantChecks(members, oldCovariant, existingChecks);
+          element.mixins.reversed.forEach(visitSuper);
+          var s = element.supertype;
+          if (s != null) visitSuper(s);
+        }
+      }
+
+      visitSuper(type);
+    }
+
+    element.mixins.reversed.forEach(visitImmediateSuper);
+    var s = element.supertype;
+    if (s != null) visitImmediateSuper(s);
+
+    superChecks.removeAll(existingChecks);
+    return superChecks;
+  }
+
+  /// Gets all concrete instance members declared on this type, skipping already
+  /// [seenConcreteMembers] and adding any found ones to it.
+  static List<ExecutableElement> _getConcreteMembers(
+      InterfaceType type, HashSet<String> seenConcreteMembers) {
+    var members = <ExecutableElement>[];
+    for (var declaredMembers in [type.accessors, type.methods]) {
+      for (var member in declaredMembers) {
+        // We only visit each most derived concrete member.
+        // To avoid visiting an overridden superclass member, we skip members
+        // we've seen, and visit starting from the class, then mixins in
+        // reverse order, then superclasses.
+        if (!member.isStatic &&
+            !member.isAbstract &&
+            seenConcreteMembers.add(member.name)) {
+          members.add(member);
+        }
+      }
+    }
+    return members;
+  }
+
+  Set<Element> _findCovariantChecks(Iterable<ExecutableElement> members,
+      Iterable<ClassElement> covariantInterfaces,
+      [Set<Element> covariantChecks]) {
+    covariantChecks ??= new Set();
+    if (members.isEmpty) return covariantChecks;
+
+    for (var iface in covariantInterfaces) {
+      var upperBound = _getCovariantUpperBound(rules, iface);
+
+      for (var m in members) {
+        var superMemberType = _getMemberType(upperBound, m);
+        if (superMemberType == null) continue;
+
+        void addCheck(Element e) {
+          covariantChecks.add(e is Member ? e.baseElement : e);
+        }
+
+        rules.visitCovariantParameters(
+            m.type, superMemberType, addCheck, addCheck);
+      }
+    }
+    return covariantChecks;
+  }
+
+  static Set<ClassElement> _findCovariantSupertypes(InterfaceType type) {
+    var visited = new HashSet<ClassElement>();
+    var genericSupertypes = new Set<ClassElement>();
+
+    void visitTypeAndSupertypes(InterfaceType type) {
+      var element = type.element;
+      if (visited.add(element)) {
+        if (element.typeParameters.isNotEmpty) {
+          genericSupertypes.add(element);
+        }
+        var supertype = element.supertype;
+        if (supertype != null) visitTypeAndSupertypes(supertype);
+        element.mixins.forEach(visitTypeAndSupertypes);
+        element.interfaces.forEach(visitTypeAndSupertypes);
+      }
+    }
+
+    visitTypeAndSupertypes(type);
+
+    return genericSupertypes;
   }
 
   /// Checks that implementations correctly override all reachable interfaces.
   /// In particular, we need to check these overrides for the definitions in
   /// the class itself and each its superclasses. If a superclass is not
   /// abstract, then we can skip its transitive interfaces. For example, in:
   ///
   ///     B extends C implements G
   ///     A extends B with E, F implements H, I
   ///
@@ skipping 98 matching lines @@
   /// Check that individual methods and fields in [subType] correctly override
   /// the declarations in [baseType].
   ///
   /// The [errorLocation] node indicates where errors are reported, see
   /// [_checkSingleOverride] for more details.
   ///
   /// The set [seen] is used to avoid reporting overrides more than once. It
   /// is used when invoking this function multiple times when checking several
   /// types in a class hierarchy. Errors are reported only the first time an
   /// invalid override involving a specific member is encountered.
-  _checkIndividualOverridesFromType(
+  void _checkIndividualOverridesFromType(
       InterfaceType subType,
       InterfaceType baseType,
       AstNode errorLocation,
       Set<String> seen,
       bool isSubclass) {
     void checkHelper(ExecutableElement e) {
       if (e.isStatic) return;
       if (seen.contains(e.name)) return;
       if (_checkSingleOverride(e, baseType, null, errorLocation, isSubclass)) {
         seen.add(e.name);
@@ skipping 114 matching lines @@
   ///                              ^
   ///
   /// When checking for overrides from a type and it's super types, [node] is
   /// the AST node that defines [element]. This is used to determine whether the
   /// type of the element could be inferred from the types in the super classes.
   bool _checkSingleOverride(ExecutableElement element, InterfaceType type,
       AstNode node, AstNode errorLocation, bool isSubclass) {
     assert(!element.isStatic);
 
     FunctionType subType = _elementType(element);
-    // TODO(vsm): Test for generic
     FunctionType baseType = _getMemberType(type, element);
     if (baseType == null) return false;
 
     if (isSubclass && element is PropertyAccessorElement) {
       // Disallow any overriding if the base class defines this member
       // as a field.  We effectively treat fields as final / non-virtual,
       // unless they are explicitly marked as @virtual
       var field = _getMemberField(type, element);
       if (field != null && !field.isVirtual) {
         _checker._recordMessage(
             errorLocation, StrongModeCode.INVALID_FIELD_OVERRIDE, [
           element.enclosingElement.name,
           element.name,
           subType,
           type,
           baseType
         ]);
       }
     }
+
     if (!rules.isOverrideSubtypeOf(subType, baseType)) {
       ErrorCode errorCode;
       var parent = errorLocation?.parent;
       if (errorLocation is ExtendsClause ||
           parent is ClassTypeAlias && parent.superclass == errorLocation) {
         errorCode = StrongModeCode.INVALID_METHOD_OVERRIDE_FROM_BASE;
       } else if (parent is WithClause) {
         errorCode = StrongModeCode.INVALID_METHOD_OVERRIDE_FROM_MIXIN;
       } else {
         errorCode = StrongModeCode.INVALID_METHOD_OVERRIDE;
@@ skipping 65 matching lines @@
   }
 
   /// If node is a [ClassDeclaration] returns its members, otherwise if node is
   /// a [ClassTypeAlias] this returns an empty list.
   WithClause _withClause(Declaration node) {
     return node is ClassDeclaration
         ? node.withClause
         : (node as ClassTypeAlias).withClause;
   }
 }