dart2js cps: Do not propagate impure expressions across null receiver.

If the receiver is null, the arguments are not evaluated.

Example:

Dart:
    var x = null;
    var y = bar();
    x.foo(y);

JS before:
    null.foo$1(bar());

JS after:
    var y = bar();
    null.foo$1(y);

If the receiver is known not be null, we still propagate into the
arguments, and "pure" expressions can always propagate even if the
receiver might be null.

InvokeMethod now has a field isReceiverNotNull, which carries a bit of
static type information. Another field like this is InvokeStatic.isPure.

I intentionally chose to pass along the *least* amount of information
that is sufficient for what we need in the tree optimizations.

That CPS type propagation was not very good at proving when things are
not null, so a couple of things missing from it have been filled in.

BUG=
R=kmillikin@google.com

Review URL: https://codereview.chromium.org//1159643005

pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart

@@ -269,6 +269,11 @@ class InvokeMethod extends Expression implements Invoke {
   final Reference<Continuation> continuation;
   final SourceInformation sourceInformation;
 
+  /// If true, it is known that the receiver cannot be `null`.
+  ///
+  /// This field is `null` until initialized by optimization phases.
+  bool receiverIsNotNull;
+
   InvokeMethod(Primitive receiver,
                this.selector,
                List<Primitive> arguments,

pkg/compiler/lib/src/cps_ir/type_propagation.dart

@@ -28,17 +28,22 @@ abstract class TypeSystem<T> {
   T get stringType;
   T get listType;
   T get mapType;
+  T get nonNullType;
 
   T getReturnType(FunctionElement element);
   T getSelectorReturnType(Selector selector);
   T getParameterType(ParameterElement element);
+  T getFieldType(FieldElement element);
   T join(T a, T b);
+  T exact(ClassElement element);
   T getTypeOf(ConstantValue constant);
 
   bool areDisjoint(T leftType, T rightType);
 
   /// True if all values satisfying [type] are booleans (null is not a boolean).
   bool isDefinitelyBool(T type);
+
+  bool isDefinitelyNotNull(T type);
 }
 
 class UnitTypeSystem implements TypeSystem<String> {
@@ -52,15 +57,20 @@ class UnitTypeSystem implements TypeSystem<String> {
   get mapType => UNIT;
   get stringType => UNIT;
   get typeType => UNIT;
+  get nonNullType => UNIT;
 
   getParameterType(_) => UNIT;
   getReturnType(_) => UNIT;
   getSelectorReturnType(_) => UNIT;
+  getFieldType(_) => UNIT;
   join(a, b) => UNIT;
+  exact(_) => UNIT;
   getTypeOf(_) => UNIT;
 
   bool isDefinitelyBool(_) => false;
 
+  bool isDefinitelyNotNull(_) => false;
+
   bool areDisjoint(String leftType, String rightType) {
     return false;
   }
@@ -78,6 +88,7 @@ class TypeMaskSystem implements TypeSystem<TypeMask> {
   TypeMask get stringType => inferrer.stringType;
   TypeMask get listType => inferrer.listType;
   TypeMask get mapType => inferrer.mapType;
+  TypeMask get nonNullType => inferrer.nonNullType;
 
   // TODO(karlklose): remove compiler here.
   TypeMaskSystem(dart2js.Compiler compiler)
@@ -96,6 +107,10 @@ class TypeMaskSystem implements TypeSystem<TypeMask> {
     return inferrer.getGuaranteedTypeOfSelector(selector);
   }
 
+  TypeMask getFieldType(FieldElement field) {
+    return inferrer.getGuaranteedTypeOfElement(field);
+  }
+
   @override
   TypeMask join(TypeMask a, TypeMask b) {
     return a.union(b, classWorld);
@@ -106,10 +121,20 @@ class TypeMaskSystem implements TypeSystem<TypeMask> {
     return computeTypeMask(inferrer.compiler, constant);
   }
 
+  TypeMask exact(ClassElement element) {
+    // The class world does not know about classes created by
+    // closure conversion, so just treat those as a subtypes of Function.
+    // TODO(asgerf): Maybe closure conversion should create a new ClassWorld?
+    if (element.isClosure) return functionType;
+    return new TypeMask.exact(element, classWorld);
+  }
+
   bool isDefinitelyBool(TypeMask t) {
     return t.containsOnlyBool(classWorld) && !t.isNullable;
   }
 
+  bool isDefinitelyNotNull(TypeMask t) => !t.isNullable;
+
   @override
   bool areDisjoint(TypeMask leftType, TypeMask rightType) {
     TypeMask intersection = leftType.intersection(rightType, classWorld);
@@ -286,6 +311,8 @@ class _TransformingVisitor<T> extends RecursiveVisitor {
     });
 
     if (letPrim == null) {
+      _AbstractValue<T> receiver = getValue(node.receiver.definition);
+      node.receiverIsNotNull = receiver.isDefinitelyNotNull(typeSystem);
       super.visitInvokeMethod(node);
     } else {
       visitLetPrim(letPrim);
@@ -465,7 +492,8 @@ class _TypePropagationVisitor<T> implements Visitor {
 
   void visitFunctionDefinition(FunctionDefinition node) {
     if (node.thisParameter != null) {
-      setValue(node.thisParameter, nonConstant());
+      // TODO(asgerf): Use a more precise type for 'this'.
+      setValue(node.thisParameter, nonConstant(typeSystem.nonNullType));
     }
     node.parameters.forEach(visit);
     setReachable(node.body);
@@ -608,7 +636,7 @@ class _TypePropagationVisitor<T> implements Visitor {
 
     assert(cont.parameters.length == 1);
     Parameter returnValue = cont.parameters[0];
-    setValue(returnValue, nonConstant());
+    setValue(returnValue, nonConstant(typeSystem.getReturnType(node.target)));
   }
 
   void visitConcatenateStrings(ConcatenateStrings node) {
@@ -806,7 +834,11 @@ class _TypePropagationVisitor<T> implements Visitor {
   }
 
   void visitGetStatic(GetStatic node) {
-    setValue(node, nonConstant());
+    if (node.element.isFunction) {
+      setValue(node, nonConstant(typeSystem.functionType));
+    } else {
+      setValue(node, nonConstant(typeSystem.getFieldType(node.element)));
+    }
   }
 
   void visitSetStatic(SetStatic node) {
@@ -819,7 +851,7 @@ class _TypePropagationVisitor<T> implements Visitor {
 
     assert(cont.parameters.length == 1);
     Parameter returnValue = cont.parameters[0];
-    setValue(returnValue, nonConstant());
+    setValue(returnValue, nonConstant(typeSystem.getFieldType(node.element)));
   }
 
   void visitIsTrue(IsTrue node) {
@@ -858,12 +890,12 @@ class _TypePropagationVisitor<T> implements Visitor {
     setReachable(node.input.definition);
     _AbstractValue<T> value = getValue(node.input.definition);
     if (!value.isNothing) {
-      setValue(node, nonConstant());
+      setValue(node, nonConstant(typeSystem.nonNullType));
     }
   }
 
   void visitGetField(GetField node) {
-    setValue(node, nonConstant());
+    setValue(node, nonConstant(typeSystem.getFieldType(node.field)));
   }
 
   void visitSetField(SetField node) {
@@ -871,11 +903,11 @@ class _TypePropagationVisitor<T> implements Visitor {
   }
 
   void visitCreateBox(CreateBox node) {
-    setValue(node, nonConstant());
+    setValue(node, nonConstant(typeSystem.nonNullType));
   }
 
   void visitCreateInstance(CreateInstance node) {
-    setValue(node, nonConstant());
+    setValue(node, nonConstant(typeSystem.exact(node.classElement)));
   }
 
   void visitReifyRuntimeType(ReifyRuntimeType node) {
@@ -896,7 +928,8 @@ class _TypePropagationVisitor<T> implements Visitor {
   }
 
   void visitCreateInvocationMirror(CreateInvocationMirror node) {
-    setValue(node, nonConstant());
+    // TODO(asgerf): Expose [Invocation] type.
+    setValue(node, nonConstant(typeSystem.nonNullType));
   }
 }
 
@@ -977,6 +1010,13 @@ class _AbstractValue<T> {
     if (kind == NOTHING) return true;
     return typeSystem.isDefinitelyBool(type);
   }
+
+  /// True if null is not a member of this value.
+  bool isDefinitelyNotNull(TypeSystem<T> typeSystem) {
+    if (kind == NOTHING) return true;
+    if (kind == CONSTANT) return !constant.isNull;
+    return typeSystem.isDefinitelyNotNull(type);
+  }
 }
 
 class ConstantExpressionCreator

pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart

@@ -347,8 +347,17 @@ class StatementRewriter extends Transformer implements Pass {
   }
 
   Expression visitInvokeMethod(InvokeMethod node) {
-    _rewriteList(node.arguments);
-    node.receiver = visitExpression(node.receiver);
+    if (node.receiverIsNotNull) {
+      _rewriteList(node.arguments);
+      node.receiver = visitExpression(node.receiver);
+    } else {
+      // Impure expressions cannot be propagated across the method lookup,
+      // because it throws when the receiver is null.
+      inEmptyEnvironment(() {
+        _rewriteList(node.arguments);
+      });
+      node.receiver = visitExpression(node.receiver);
+    }
     return node;
   }
 

pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart

@@ -350,9 +350,10 @@ class Builder implements cps_ir.Visitor<Node> {
   }
 
   Statement visitInvokeMethod(cps_ir.InvokeMethod node) {
-    Expression invoke = new InvokeMethod(getVariableUse(node.receiver),
-                                         node.selector,
-                                         translateArguments(node.arguments));
+    InvokeMethod invoke = new InvokeMethod(getVariableUse(node.receiver),
+                                           node.selector,
+                                           translateArguments(node.arguments));
+    invoke.receiverIsNotNull = node.receiverIsNotNull;
     return continueWithExpression(node.continuation, invoke);
   }
 

pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart

@@ -192,14 +192,17 @@ class InvokeStatic extends Expression implements Invoke {
 /**
  * A call to a method, operator, getter, setter or index getter/setter.
  *
- * In contrast to the CPS-based IR, the receiver and arguments can be
- * arbitrary expressions.
+ * If [receiver] is `null`, an error is thrown before the arguments are
+ * evaluated. This corresponds to the JS evaluation order.
  */
 class InvokeMethod extends Expression implements Invoke {
   Expression receiver;
   final Selector selector;
   final List<Expression> arguments;
 
+  /// If true, it is known that the receiver cannot be `null`.
+  bool receiverIsNotNull = false;
+
   InvokeMethod(this.receiver, this.selector, this.arguments) {
     assert(receiver != null);
   }
@@ -211,6 +214,9 @@ class InvokeMethod extends Expression implements Invoke {
 }
 
 /// Invoke [target] on [receiver], bypassing ordinary dispatch semantics.
+///
+/// Since the [receiver] is not used for method lookup, it may be `null`
+/// without an error being thrown.
 class InvokeMethodDirectly extends Expression implements Invoke {
   Expression receiver;
   final Element target;

tests/co19/co19-dart2js.status

@@ -9638,7 +9638,6 @@ Language/12_Expressions/13_Property_Extraction_A03_t01: RuntimeError # Please tr
 Language/12_Expressions/13_Property_Extraction_A03_t02: RuntimeError # Please triage this failure.
 Language/12_Expressions/13_Property_Extraction_A03_t03: RuntimeError # Please triage this failure.
 Language/12_Expressions/15_Method_Invocation/4_Super_Invocation_A01_t01: RuntimeError # Please triage this failure.
-Language/12_Expressions/15_Method_Invocation/4_Super_Invocation_A02_t04: RuntimeError # Please triage this failure.
 Language/12_Expressions/22_Equality_A03_t01: RuntimeError # Please triage this failure.
 Language/12_Expressions/30_Identifier_Reference_A09_t03: Crash # (i=0): For-loop variable captured in loop header
 Language/12_Expressions/33_Type_Cast_A01_t01: Crash # Instance of 'TypeOperator': type casts not implemented.