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

pkg/dev_compiler/lib/src/compiler/code_generator.dart

Index: pkg/dev_compiler/lib/src/compiler/code_generator.dart
diff --git a/pkg/dev_compiler/lib/src/compiler/code_generator.dart b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
index 6f2c2d1cd8dedcb5c0079afd775497b6c1cd194a..8a2d2ebc6fed04130739eed75c73b35d7b7036c2 100644
--- a/pkg/dev_compiler/lib/src/compiler/code_generator.dart
+++ b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
@@ -31,8 +31,7 @@ import 'package:analyzer/src/summary/summarize_ast.dart'
 import 'package:analyzer/src/summary/summarize_elements.dart'
     show PackageBundleAssembler;
 import 'package:analyzer/src/summary/summary_sdk.dart';
-import 'package:analyzer/src/task/strong/ast_properties.dart'
-    show isDynamicInvoke, setIsDynamicInvoke, getImplicitAssignmentCast;
+import 'package:analyzer/src/task/strong/ast_properties.dart';
 import 'package:path/path.dart' show isWithin, relative, separator;
 
 import '../closure/closure_annotator.dart' show ClosureAnnotator;
@@ -195,6 +194,8 @@ class CodeGenerator extends Object
   /// unit.
   final virtualFields = new VirtualFieldModel();
 
+  final _usedCovariantPrivateMembers = new HashSet<ExecutableElement>();
+
   CodeGenerator(
       AnalysisContext c, this.summaryData, this.options, this._extensionTypes)
       : context = c,
@@ -289,6 +290,10 @@ class CodeGenerator extends Object
       throw new StateError('Can only call emitModule once.');
     }
 
+    for (var unit in compilationUnits) {
+      _usedCovariantPrivateMembers.addAll(getCovariantPrivateMembers(unit));
+    }
+
     // Transform the AST to make coercions explicit.
     compilationUnits = CoercionReifier.reify(compilationUnits);
 
@@ -699,17 +704,38 @@ class CodeGenerator extends Object
     Expression fromExpr = node.expression;
     var from = getStaticType(fromExpr);
     var to = node.type.type;
-
     JS.Expression jsFrom = _visit(fromExpr);
 
-    // Skip the cast if it's not needed.
-    if (rules.isSubtypeOf(from, to)) return jsFrom;
+    // If the check was put here by static analysis to ensure soundness, we
+    // can't skip it. This happens because of unsound covariant generics:
+    //
+    //      typedef F<T>(T t);
+    //      class C<T> {
+    //        F<T> f;
+    //        add(T t) {
+    //          // required check `t as T`
+    //        }
+    //      }
+    //      main() {
+    //        C<Object> c = new C<int>()..f = (int x) => x.isEven;
+    //        c.f('hi'); // required check `c.f as F<Object>`
+    //        c.add('hi);
+    //      }
+    //
+    // NOTE: due to implementation details, we do not currently reify the the
+    // `C<T>.add` check in CoercionReifier, so it does not reach this point;
+    // rather we check for it explicitly when emitting methods and fields.
+    // However we do reify the `c.f` check, so we must not eliminate it.
+    var isRequiredForSoundness = CoercionReifier.isRequiredForSoundness(node);
+    if (!isRequiredForSoundness && rules.isSubtypeOf(from, to)) return jsFrom;

[vsm, 2017/07/06 15:43:02]

thanks!  The rename is nice too.

 
     // All Dart number types map to a JS double.
     if (typeRep.isNumber(from) && typeRep.isNumber(to)) {
       // Make sure to check when converting to int.
       if (from != types.intType && to == types.intType) {
         // TODO(jmesserly): fuse this with notNull check.
+        // TODO(jmesserly): this does not correctly distinguish user casts from
+        // required-for-soundness casts.
         return _callHelper('asInt(#)', jsFrom);
       }
 
@@ -717,14 +743,8 @@ class CodeGenerator extends Object
       return jsFrom;
     }
 
-    var type = _emitType(to,
-        nameType: options.nameTypeTests || options.hoistTypeTests,
-        hoistType: options.hoistTypeTests);
-    if (CoercionReifier.isImplicitCast(node)) {
-      return js.call('#._check(#)', [type, jsFrom]);
-    } else {
-      return js.call('#.as(#)', [type, jsFrom]);
-    }
+    var code = isRequiredForSoundness ? '#._check(#)' : '#.as(#)';
+    return js.call(code, [_emitType(to), jsFrom]);
   }
 
   @override
@@ -740,9 +760,7 @@ class CodeGenerator extends Object
     } else {
       // Always go through a runtime helper, because implicit interfaces.
 
-      var castType = _emitType(type,
-          nameType: options.nameTypeTests || options.hoistTypeTests,
-          hoistType: options.hoistTypeTests);
+      var castType = _emitType(type);
 
       result = js.call('#.is(#)', [castType, lhs]);
     }
@@ -820,9 +838,16 @@ class CodeGenerator extends Object
     // The resulting 'class' is a mixable class in this case.
     bool isMixinAlias = supertype.isObject && classElem.mixins.length == 1;
 
+    // TODO(jmesserly): what do we do if the mixin alias has implied superclass
+    // covariance checks (due to new interfaces)? We can't add them without
+    // messing up the inheritance chain and breaking the ability of the mixin
+    // alias to be mixed in elsewhere. We're going to need something special,
+    // like adding these checks when we copy in the methods.
+    var jsMethods = <JS.Method>[];
+    _emitSuperclassCovarianceChecks(node, jsMethods);
     var classExpr = isMixinAlias
         ? _emitClassHeritage(classElem)
-        : _emitClassExpression(classElem, []);
+        : _emitClassExpression(classElem, jsMethods);
     var className = isGeneric
         ? new JS.Identifier(classElem.name)
         : _emitTopLevelName(classElem);
@@ -906,8 +931,12 @@ class CodeGenerator extends Object
     }
 
     var savedClassProperties = _classProperties;
-    _classProperties =
-        new ClassPropertyModel.build(_extensionTypes, virtualFields, classElem);
+    _classProperties = new ClassPropertyModel.build(
+        _extensionTypes,
+        virtualFields,
+        classElem,
+        getClassCovariantParameters(node),
+        _usedCovariantPrivateMembers);
 
     var jsCtors = _defineConstructors(classElem, className, fields, ctors);
     var classExpr = _emitClassExpression(classElem, _emitClassMethods(node),
@@ -1443,9 +1472,88 @@ class CodeGenerator extends Object
     // Add all of the super helper methods
     jsMethods.addAll(_superHelpers.values);
 
+    _emitSuperclassCovarianceChecks(node, jsMethods);
     return jsMethods.where((m) => m != null).toList(growable: false);
   }
 
+  void _emitSuperclassCovarianceChecks(
+      Declaration node, List<JS.Method> methods) {
+    var covariantParams = getSuperclassCovariantParameters(node);
+    if (covariantParams == null) return;
+
+    for (var member in covariantParams.map((p) => p.enclosingElement).toSet()) {
+      var name = _declareMemberName(member);
+      if (member is PropertyAccessorElement) {
+        var param = member.parameters[0];
+        assert(covariantParams.contains(param));
+        methods.add(new JS.Method(
+            name,
+            js.call('function(x) { return super.#(#._check(x)); }',
+                [name, _emitType(param.type)]),
+            isSetter: true));
+        methods.add(new JS.Method(
+            name, js.call('function() { return super.#; }', [name]),
+            isGetter: true));
+      } else if (member is MethodElement) {
+        var type = member.type;
+
+        var body = <JS.Statement>[];
+        var typeFormals = _emitTypeFormals(type.typeFormals);
+        if (type.typeFormals.any(covariantParams.contains)) {
+          body.add(js.statement(
+              '#.checkBounds([#]);', [_emitType(type), typeFormals]));
+        }
+
+        var jsParams = <JS.Parameter>[];
+        bool foundNamedParams = false;
+        for (var param in member.parameters) {
+          JS.Parameter jsParam;
+          if (param.kind == ParameterKind.NAMED) {
+            foundNamedParams = true;
+            if (covariantParams.contains(param)) {
+              var name = _propertyName(param.name);
+              body.add(js.statement('if (# in #) #._check(#.#);', [
+                name,
+                namedArgumentTemp,
+                _emitType(param.type),
+                namedArgumentTemp,
+                name
+              ]));
+            }
+          } else {
+            jsParam = _emitParameter(param);
+            jsParams.add(jsParam);
+            if (covariantParams.contains(param)) {
+              if (param.kind == ParameterKind.POSITIONAL) {
+                body.add(js.statement('if (# !== void 0) #._check(#);',
+                    [jsParam, _emitType(param.type), jsParam]));
+              } else {
+                body.add(js.statement(
+                    '#._check(#);', [_emitType(param.type), jsParam]));
+              }
+            }
+          }
+        }
+
+        if (foundNamedParams) jsParams.add(namedArgumentTemp);
+
+        if (typeFormals.isEmpty) {
+          body.add(js.statement('return super.#(#);', [name, jsParams]));
+        } else {
+          body.add(js.statement(
+              'return super.#(#)(#);', [name, typeFormals, jsParams]));
+        }
+        var fn = new JS.Fun(jsParams, new JS.Block(body),
+            typeParams: typeFormals, returnType: emitTypeRef(type.returnType));
+        methods.add(new JS.Method(name, _makeGenericFunction(fn)));
+      } else {
+        throw new StateError(
+            'unable to generate a covariant check for element: `$member` '
+            '(${member.runtimeType})');
+      }
+    }
+  }
+
   /// Emits a Dart factory constructor to a JS static method.
   JS.Method _emitFactoryConstructor(ConstructorDeclaration node) {
     var element = node.element;
@@ -1586,9 +1694,19 @@ class CodeGenerator extends Object
           ? [new JS.Super(), name]
           : [new JS.This(), virtualField];
 
-      result.add(new JS.Method(
-          name, js.call('function(value) { #[#] = value; }', args),
-          isSetter: true));
+      String jsCode;
+      var setter = element.setter;
+      var covariantParams = _classProperties.covariantParameters;
+      if (setter != null &&
+          covariantParams != null &&
+          covariantParams.contains(setter.parameters[0])) {
+        args.add(_emitType(setter.parameters[0].type));
+        jsCode = 'function(value) { #[#] = #._check(value); }';
+      } else {
+        jsCode = 'function(value) { #[#] = value; }';
+      }
+
+      result.add(new JS.Method(name, js.call(jsCode, args), isSetter: true));
     }
 
     return result;
@@ -1950,8 +2068,7 @@ class CodeGenerator extends Object
 
       var type = _emitAnnotatedFunctionType(reifiedType, node.metadata,
           parameters: node.parameters?.parameters,
-          nameType: options.hoistSignatureTypes,
-          hoistType: options.hoistSignatureTypes,
+          nameType: false,
           definite: true);
 
       if (needsSignature) {
@@ -1992,8 +2109,7 @@ class CodeGenerator extends Object
         var memberName = _constructorName(element);
         var type = _emitAnnotatedFunctionType(element.type, node.metadata,
             parameters: node.parameters.parameters,
-            nameType: options.hoistSignatureTypes,
-            hoistType: options.hoistSignatureTypes,
+            nameType: false,
             definite: true);
         var property = new JS.Property(memberName, type);
         tCtors.add(property);
@@ -2294,6 +2410,8 @@ class CodeGenerator extends Object
     var parameters = _parametersOf(node);
     if (parameters == null) return null;
 
+    var covariantParams = _classProperties?.covariantParameters;
+
     var body = <JS.Statement>[];
     for (var param in parameters.parameters) {
       var jsParam = _emitSimpleIdentifier(param.identifier);
@@ -2320,39 +2438,16 @@ class CodeGenerator extends Object
         }
       }
 
-      // TODO(jmesserly): various problems here, see:
-      // https://github.com/dart-lang/sdk/issues/27259
-      var paramType =
-          resolutionMap.elementDeclaredByFormalParameter(param).type;
-      if (node is MethodDeclaration &&
-          (resolutionMap.elementDeclaredByFormalParameter(param).isCovariant ||
-              _unsoundCovariant(paramType, true))) {
-        var castType = _emitType(paramType,
-            nameType: options.nameTypeTests || options.hoistTypeTests,
-            hoistType: options.hoistTypeTests);
+      var paramElement = resolutionMap.elementDeclaredByFormalParameter(param);
+      if (paramElement.isCovariant ||
+          covariantParams != null && covariantParams.contains(paramElement)) {
+        var castType = _emitType(paramElement.type);
         body.add(js.statement('#._check(#);', [castType, jsParam]));
       }
     }
     return body.isEmpty ? null : _statement(body);
   }
 
-  /// Given a type [t], return whether or not t is unsoundly covariant.
-  /// If [contravariant] is true, then t appears in a contravariant
-  /// position.
-  bool _unsoundCovariant(DartType t, bool contravariant) {
-    if (t is TypeParameterType) {
-      return contravariant && t.element.enclosingElement is ClassElement;
-    }
-    if (t is FunctionType) {
-      if (_unsoundCovariant(t.returnType, contravariant)) return true;
-      return t.parameters.any((p) => _unsoundCovariant(p.type, !contravariant));
-    }
-    if (t is ParameterizedType) {
-      return t.typeArguments.any((t) => _unsoundCovariant(t, contravariant));
-    }
-    return false;
-  }
-
   JS.Expression _defaultParamValue(FormalParameter param) {
     if (param is DefaultFormalParameter && param.defaultValue != null) {
       return _visit(param.defaultValue);
@@ -2623,10 +2718,21 @@ class CodeGenerator extends Object
         : new JS.Block(
             [_emitGeneratorFunctionBody(element, parameters, body).toReturn()]);
     var typeFormals = _emitTypeFormals(type.typeFormals);
+
     var returnType = emitTypeRef(type.returnType);
     if (type.typeFormals.isNotEmpty) {
-      code = new JS.Block(
-          [new JS.Block(_typeTable.discharge(type.typeFormals)), code]);
+      var block = <JS.Statement>[
+        new JS.Block(_typeTable.discharge(type.typeFormals))
+      ];
+
+      var covariantParams = _classProperties?.covariantParameters;
+      if (covariantParams != null &&
+          type.typeFormals.any(covariantParams.contains)) {
+        block.add(js.statement('#.checkBounds(#);',
+            [_emitType(type), new JS.ArrayInitializer(typeFormals)]));
+      }
+
+      code = new JS.Block(block..add(code));
     }
 
     if (element.isOperator && element.name == '[]=' && formals.isNotEmpty) {
@@ -2893,9 +2999,9 @@ class CodeGenerator extends Object
   }
 
   JS.Expression _emitAnnotatedType(DartType type, List<Annotation> metadata,
-      {bool nameType: true, bool hoistType: true}) {
+      {bool nameType: true}) {
     metadata ??= [];
-    var typeName = _emitType(type, nameType: nameType, hoistType: hoistType);
+    var typeName = _emitType(type, nameType: nameType);
     return _emitAnnotatedResult(typeName, metadata);
   }
 
@@ -2911,7 +3017,7 @@ class CodeGenerator extends Object
 
   JS.ArrayInitializer _emitTypeNames(
       List<DartType> types, List<FormalParameter> parameters,
-      {bool nameType: true, bool hoistType: true}) {
+      {bool nameType: true}) {
     var result = <JS.Expression>[];
     for (int i = 0; i < types.length; ++i) {
       var metadata = parameters != null
@@ -2938,16 +3044,13 @@ class CodeGenerator extends Object
       {List<FormalParameter> parameters,
       bool lowerTypedef: false,
       bool nameType: true,
-      bool hoistType: true,
       definite: false}) {
     var parameterTypes = type.normalParameterTypes;
     var optionalTypes = type.optionalParameterTypes;
     var namedTypes = type.namedParameterTypes;
-    var rt =
-        _emitType(type.returnType, nameType: nameType, hoistType: hoistType);
+    var rt = _emitType(type.returnType, nameType: nameType);
 
-    var ra = _emitTypeNames(parameterTypes, parameters,
-        nameType: nameType, hoistType: hoistType);
+    var ra = _emitTypeNames(parameterTypes, parameters, nameType: nameType);
 
     List<JS.Expression> typeParts;
     if (namedTypes.isNotEmpty) {
@@ -2959,7 +3062,7 @@ class CodeGenerator extends Object
       assert(namedTypes.isEmpty);
       var oa = _emitTypeNames(
           optionalTypes, parameters?.sublist(parameterTypes.length),
-          nameType: nameType, hoistType: hoistType);
+          nameType: nameType);
       typeParts = [rt, ra, oa];
     } else {
       typeParts = [rt, ra];
@@ -2992,8 +3095,7 @@ class CodeGenerator extends Object
     }
     fullType = _callHelper(helperCall, [typeParts]);
     if (!nameType) return fullType;
-    return _typeTable.nameType(type, fullType,
-        hoistType: hoistType, definite: definite);
+    return _typeTable.nameType(type, fullType, definite: definite);
   }
 
   JS.Expression _emitAnnotatedFunctionType(
@@ -3001,13 +3103,11 @@ class CodeGenerator extends Object
       {List<FormalParameter> parameters,
       bool lowerTypedef: false,
       bool nameType: true,
-      bool hoistType: true,
       bool definite: false}) {
     var result = _emitFunctionType(type,
         parameters: parameters,
         lowerTypedef: lowerTypedef,
         nameType: nameType,
-        hoistType: hoistType,
         definite: definite);
     return _emitAnnotatedResult(result, metadata);
   }
@@ -3016,10 +3116,8 @@ class CodeGenerator extends Object
   ///
   /// If [nameType] is true, then the type will be named.  In addition,
   /// if [hoistType] is true, then the named type will be hoisted.
-  JS.Expression _emitConstructorAccess(DartType type,
-      {bool nameType: true, bool hoistType: true}) {
-    return _emitJSInterop(type.element) ??
-        _emitType(type, nameType: nameType, hoistType: hoistType);
+  JS.Expression _emitConstructorAccess(DartType type, {bool nameType: true}) {
+    return _emitJSInterop(type.element) ?? _emitType(type, nameType: nameType);
   }
 
   /// Emits an expression that lets you access statics on a [type] from code.
@@ -3063,7 +3161,6 @@ class CodeGenerator extends Object
       {bool lowerTypedef: false,
       bool lowerGeneric: false,
       bool nameType: true,
-      bool hoistType: true,
       ClassElement subClass,
       JS.Expression className}) {
     // The void and dynamic types are not defined in core.
@@ -3113,7 +3210,7 @@ class CodeGenerator extends Object
       // go through use similar logic as generic classes. This makes them
       // different from universal function types.
       return _emitFunctionType(type as FunctionType,
-          lowerTypedef: lowerTypedef, nameType: nameType, hoistType: hoistType);
+          lowerTypedef: lowerTypedef, nameType: nameType);
     }
 
     if (type is TypeParameterType) {
@@ -3128,19 +3225,14 @@ class CodeGenerator extends Object
       Iterable jsArgs = null;
       if (args.any((a) => !a.isDynamic)) {
         jsArgs = args.map((x) => _emitType(x,
-            nameType: nameType,
-            hoistType: hoistType,
-            subClass: subClass,
-            className: className));
+            nameType: nameType, subClass: subClass, className: className));
       } else if (lowerGeneric || element == subClass) {
         jsArgs = [];
       }
       if (jsArgs != null) {
         var genericName = _emitTopLevelName(element, suffix: '\$');
         var typeRep = js.call('#(#)', [genericName, jsArgs]);
-        return nameType
-            ? _typeTable.nameType(type, typeRep, hoistType: hoistType)
-            : typeRep;
+        return nameType ? _typeTable.nameType(type, typeRep) : typeRep;
       }
     }
 
@@ -3198,7 +3290,7 @@ class CodeGenerator extends Object
       ..staticElement = element
       ..staticType = getStaticType(lhs);
 
-    var castTo = getImplicitAssignmentCast(left);
+    var castTo = getImplicitOperationCast(left);
     if (castTo != null) inc = CoercionReifier.castExpression(inc, castTo);
     return new JS.MetaLet(vars, [_emitSet(lhs, inc)]);
   }
@@ -3521,8 +3613,11 @@ class CodeGenerator extends Object
       return _callHelper('#(#, #)', [name, jsTarget, args]);
     }
     jsTarget = _emitTargetAccess(jsTarget, memberName, element);
+    var castTo = getImplicitOperationCast(node);
+    if (castTo != null) {
+      jsTarget = js.call('#._check(#)', [_emitType(castTo), jsTarget]);
+    }
     if (typeArgs != null) jsTarget = new JS.Call(jsTarget, typeArgs);
-
     return new JS.Call(jsTarget, args);
   }
 
@@ -3592,8 +3687,10 @@ class CodeGenerator extends Object
   /// an expression.
   JS.Expression _emitFunctionCall(InvocationExpression node,
       [Expression function]) {
-    if (function == null) {
-      function = node.function;
+    function ??= node.function;
+    var castTo = getImplicitOperationCast(function);
+    if (castTo != null) {
+      function = CoercionReifier.castExpression(function, castTo);
     }
     if (_isCoreIdentical(function)) {
       return _emitCoreIdenticalCall(node.argumentList.arguments);
@@ -5138,9 +5235,7 @@ class CodeGenerator extends Object
 
     // TODO(jmesserly): this is inconsistent with [visitIsExpression], which
     // has special case for typeof.
-    var castType = _emitType(clause.exceptionType.type,
-        nameType: options.nameTypeTests || options.hoistTypeTests,
-        hoistType: options.hoistTypeTests);
+    var castType = _emitType(clause.exceptionType.type);
 
     return new JS.If(js.call('#.is(#)', [castType, _visit(_catchParameter)]),
         then, otherwise);
@@ -5396,7 +5491,7 @@ class CodeGenerator extends Object
       if (op == '&&') return shortCircuit('# && #');
       if (op == '||') return shortCircuit('# || #');
     }
-    if (node is AsExpression && CoercionReifier.isImplicitCast(node)) {
+    if (node is AsExpression && CoercionReifier.isRequiredForSoundness(node)) {
       assert(node.staticType == types.boolType);
       return _callHelper('dtest(#)', _visit(node.expression));
     }