dart2js cps: Do not propagate expressions into foreign code.

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

Index: pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
diff --git a/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart b/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
index 0639b9127411b8569aed4bc216e8b1798bf627d6..1da49af039387e30978d8090a34f6f356b69ca4d 100644
--- a/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
+++ b/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
@@ -278,6 +278,11 @@ class StatementRewriter extends Transformer implements Pass {
     // it means we are looking at the first use.
     assert(unseenUses[node.variable] < node.variable.readCount);
     assert(unseenUses[node.variable] >= 0);
+
+    // We cannot reliably find the first dynamic use of a variable that is
+    // accessed from a JS function in a foreign code fragment.
+    if (node.variable.isCaptured) return node;
+
     bool isFirstUse = unseenUses[node.variable] == 0;
 
     // Propagate constant to use site.
@@ -1139,15 +1144,25 @@ class StatementRewriter extends Transformer implements Pass {
     return polarity ? node.thenStatement : node.elseStatement;
   }
 
+  void handleForeignCode(ForeignCode node) {
+    // Arguments will get inserted in a JS code template.  The arguments will
+    // not always be evaluated (e.g. if the template is '# && #').
+    // TODO(asgerf): We could analyze the JS AST to see if arguments are
+    //               definitely evaluated left-to-right.

[sra1, 2015/11/12 02:30:08]

We need to do this. The cost on code quality of not doing so is too high (adds
nearly 6k to swarm).

      get$length: function(receiver) {
        return this._html$_element.classList.length;
      },
-->
      get$length: function(receiver) {
        var v0 = this._html$_element;
        v0 = v0.classList;
        return v0.length;
      },

What would be the most convenient form of the information?

[asgerf, 2015/11/12 12:09:46]

I think we can encode the whole thing with an integer N, meaning the first N
arguments will definitely be evaluated exactly once, in left-to-right order,
without intermediate side-effects or heap dependencies, before any of the
remaining arguments are evaluated.

So for example, if the template is "foo(#,#) && bar(#,#)", N=2 because the first
two arguments are always evaluated.

The requirement about side effects and heap dependencies might be a problem. For
example, if the template was "#.length > #", and the second argument is x.pop(),
we must evaluate the pop() before getting the length, so it can't propagate. N=1
is the only safe answer there.

We could assume that the side-effect spec string of the whole template also
summarizes the side-effects in-between the arguments. So if the entire
expression has no side effects/dependencies, we can assume there are also no
side effects/dependencies separating the arguments. It won't change N=1 in the
previous example, but it saves us from doing an essentially impossible
side-effect analysis on the JS AST.

+    inEmptyEnvironment(() {
+      _rewriteList(node.arguments);
+    });
+  }
+
   @override
   Expression visitForeignExpression(ForeignExpression node) {
-    _rewriteList(node.arguments);
+    handleForeignCode(node);
     return node;
   }
 
   @override
   Statement visitForeignStatement(ForeignStatement node) {
-    _rewriteList(node.arguments);
+    handleForeignCode(node);
     return node;
   }