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 55694c65d1669e8138a171ff5c32bd14d5658c7b..65fe5d307e0d4bb0978ae6d69dfb658e9fb5c4fa 100644
--- a/pkg/dev_compiler/lib/src/compiler/code_generator.dart
+++ b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
@@ -9,7 +9,7 @@ import 'dart:math' show min, max;
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/standard_ast_factory.dart';
-import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
+import 'package:analyzer/dart/ast/token.dart' show TokenType;
 import 'package:analyzer/dart/ast/standard_resolution_map.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/type.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;
@@ -190,6 +189,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,
@@ -280,6 +281,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);
 
@@ -691,11 +696,12 @@ class CodeGenerator extends Object
     Expression fromExpr = node.expression;
     var from = getStaticType(fromExpr);
     var to = node.type.type;
-
     JS.Expression jsFrom = _visit(fromExpr);
 
+    bool isImplicit = CoercionReifier.isImplicitCast(node);
+
     // Skip the cast if it's not needed.
-    if (rules.isSubtypeOf(from, to)) return jsFrom;
+    if (!isImplicit && rules.isSubtypeOf(from, to)) return jsFrom;

[vsm, 2017/06/23 15:59:07]

I'd like to understand this better.

If *from* is a subtype of *to*, this should be a no-op.  If it's not a no-op,
then *from* is not a sound type for fromExpr.

I'm trying to think through an example:

foo<T extends S>(T x) { ... x as R ... }

Sound type of x: S (I think?  I.e., are we statically guaranteeing that foo must
be passed at least an S?)
Written type of x: T (enforced by runtime check at beginning)
Requested type: R (have we enforced T by this point?  S?)

[Jennifer Messerly, 2017/06/23 18:36:21]

sure! this code was here already though? happy to help explain
Exactly right! Maybe it helps to look through the covariance tests. Or see this
example:
https://github.com/dart-lang/sdk/issues/27259#issuecomment-305005599

You filed a bug about this issue too at some point, as did Erik. a simple
version is:

typedef F<T>(T t);
class C<T> {
  F<T> f;
}
main() {
  C<Object> c = new C<int>()..f = (int x) => x.isNegative;
  
  // type of `g` is F<Object>. Static type of `c.f` is also F<Object>
  // however, because of covariance, c.f actually contains a F<int>.
  var g = c.f; 
  g('hi');
}


... I've also got many examples in CodeChecker.

Caller side checks on unsound return types is a pragmatic choice. If we did them
on callee side, they'd almost always fail (you couldn't access `c.f` even from a
C<int> interface). So instead we allow you to use it from a safe interface.
`C<int>.f` is safe, `C<Object>.f` is probably not safe (tho the cast can succeed
sometimes)
I'm not sure I understand your example. Function types in Dart are sound. The
problem is when you access a member on a covariant *class type*. The static
types on class members lie. They are not safe. Ever. Hence runtime checks,
usually on parameters. But we also must guard on return values, when they have
parameters themselves. Does that make sense?

 
     // All Dart number types map to a JS double.
     if (_isNumberInJS(from) && _isNumberInJS(to)) {
@@ -710,11 +716,7 @@ class CodeGenerator extends Object
     }
 
     var type = _emitType(to);
-    if (CoercionReifier.isImplicitCast(node)) {
-      return js.call('#._check(#)', [type, jsFrom]);
-    } else {
-      return js.call('#.as(#)', [type, jsFrom]);
-    }
+    return js.call(isImplicit ? '#._check(#)' : '#.as(#)', [type, jsFrom]);
   }
 
   @override
@@ -808,9 +810,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);
@@ -894,8 +903,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),
@@ -1431,9 +1444,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;
@@ -1574,9 +1666,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;
@@ -2280,6 +2382,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);
@@ -2306,37 +2410,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);
+      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);
@@ -2606,12 +2689,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(<JS.Statement>[
-        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) {
@@ -3157,7 +3249,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)]);
   }
@@ -3481,8 +3573,11 @@ class CodeGenerator extends Object
       return _callHelper('#(#, #)', [name, jsTarget, args]);
     }
     jsTarget = _emitTargetAccess(jsTarget, memberName, element);
+    var castTo = getImplicitOperationCast(node.methodName);
+    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);
   }
 
@@ -3501,9 +3596,12 @@ 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);
     }
+
     var fn = _visit(function);
     var args = _emitArgumentList(node.argumentList);
     if (isDynamicInvoke(function)) {
@@ -4683,12 +4781,6 @@ class CodeGenerator extends Object
     ]);
   }
 
-  static Token _getOperator(Expression node) {
-    if (node is PropertyAccess) return node.operator;
-    if (node is MethodInvocation) return node.operator;
-    return null;
-  }
-
   // TODO(jmesserly): this is dropping source location.
   Expression _stripNullAwareOp(Expression node, Expression newTarget) {
     if (node is PropertyAccess) {