fixes #516, nested cascade code generation

our let* was not hygenic, due to incorrect use of named substitution

[email protected]

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

Author: John Messerly

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

@@ -81,7 +81,8 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   final _privateNames =
       new HashMap<LibraryElement, HashMap<String, JS.TemporaryId>>();
-  final _temps = new HashMap<Element, JS.TemporaryId>();
+  final _initializingFormalTemps =
+      new HashMap<ParameterElement, JS.TemporaryId>();
 
   final _dartxVar = new JS.Identifier('dartx');
   final _runtimeLibVar = new JS.Identifier('dart');
@@ -1912,12 +1913,9 @@ class CodeGenerator extends GeneralizingAstVisitor
       return _emitParameter(element);
     }
 
+    // If this is one of our compiler's temporary variables, return its JS form.
     if (element is TemporaryVariableElement) {
-      if (name[0] == '#') {
-        return new JS.InterpolatedExpression(name.substring(1));
-      } else {
-        return _getTemp(element, name);
-      }
+      return element.jsVariable;
     }
 
     return new JS.Identifier(name);
@@ -1931,16 +1929,14 @@ class CodeGenerator extends GeneralizingAstVisitor
       /// Rename private names so they don't shadow the private field symbol.
       /// The renamer would handle this, but it would prefer to rename the
       /// temporary used for the private symbol. Instead rename the parameter.
-      return _getTemp(element, '${element.name.substring(1)}');
+      return _initializingFormalTemps.putIfAbsent(
+          element, () => new JS.TemporaryId(element.name.substring(1)));
     }
 
     var type = declaration ? emitTypeRef(element.type) : null;
     return new JS.Identifier(element.name, type: type);
   }
 
-  JS.TemporaryId _getTemp(Element key, String name) =>
-      _temps.putIfAbsent(key, () => new JS.TemporaryId(name));
-
   List<Annotation> _parameterMetadata(FormalParameter p) =>
       (p is NormalFormalParameter)
           ? p.metadata
@@ -2086,7 +2082,7 @@ class CodeGenerator extends GeneralizingAstVisitor
 
       // Handle the left hand side, to ensure each of its subexpressions are
       // evaluated only once.
-      var vars = <String, JS.Expression>{};
+      var vars = <JS.MetaLetVariable, JS.Expression>{};
       var x = _bindLeftHandSide(vars, left, context: left);
       // Capture the result of evaluating the left hand side in a temp.
       var t = _bindValue(vars, 't', x, context: x);
@@ -2097,7 +2093,7 @@ class CodeGenerator extends GeneralizingAstVisitor
 
     // Desugar `x += y` as `x = x + y`, ensuring that if `x` has subexpressions
     // (for example, x is IndexExpression) we evaluate those once.
-    var vars = <String, JS.Expression>{};
+    var vars = <JS.MetaLetVariable, JS.Expression>{};
     var lhs = _bindLeftHandSide(vars, left, context: context);
     var inc = AstBuilder.binaryExpression(lhs, op, right);
     inc.staticElement = element;
@@ -2146,7 +2142,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     //
     // However with MetaLet, we get clean code in statement or void context,
     // or when one of the expressions is stateless, which seems common.
-    var vars = <String, JS.Expression>{};
+    var vars = <JS.MetaLetVariable, JS.Expression>{};
     var left = _bindValue(vars, 'l', node.target);
     var body = js.call('# == null ? null : #',
         [_visit(left), _emitSet(_stripNullAwareOp(node, left), right)]);
@@ -2722,7 +2718,7 @@ class CodeGenerator extends GeneralizingAstVisitor
       // This should be a hint or warning for dead code.
       if (!isNullable(left)) return _visit(left);
 
-      var vars = <String, JS.Expression>{};
+      var vars = <JS.MetaLetVariable, JS.Expression>{};
       // Desugar `l ?? r` as `l != null ? l : r`
       var l = _visit(_bindValue(vars, 'l', left, context: left));
       return new JS.MetaLet(vars, [
@@ -2771,7 +2767,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   bool _isNull(Expression expr) => expr is NullLiteral;
 
   SimpleIdentifier _createTemporary(String name, DartType type,
-      {bool nullable: true}) {
+      {bool nullable: true, JS.Expression variable}) {
     // We use an invalid source location to signal that this is a temporary.
     // See [_isTemporary].
     // TODO(jmesserly): alternatives are
@@ -2781,7 +2777,10 @@ class CodeGenerator extends GeneralizingAstVisitor
     // * create a new subtype of LocalVariableElementImpl to mark a temp.
     var id =
         new SimpleIdentifier(new StringToken(TokenType.IDENTIFIER, name, -1));
-    id.staticElement = new TemporaryVariableElement.forNode(id);
+
+    variable ??= new JS.TemporaryId(name);
+
+    id.staticElement = new TemporaryVariableElement.forNode(id, variable);
     id.staticType = type;
     DynamicInvoke.set(id, type.isDynamic);
     addTemporaryVariable(id.staticElement, nullable: nullable);
@@ -2811,7 +2810,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// unless [expr] is a SimpleIdentifier, in which case a temporary is not
   /// needed.
   Expression _bindLeftHandSide(
-      Map<String, JS.Expression> scope, Expression expr,
+      Map<JS.MetaLetVariable, JS.Expression> scope, Expression expr,
       {Expression context}) {
     Expression result;
     if (expr is IndexExpression) {
@@ -2855,14 +2854,15 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// variables), then the resulting code will be simplified automatically.
   ///
   /// [scope] will be mutated to contain the new temporary's initialization.
-  Expression _bindValue(
-      Map<String, JS.Expression> scope, String name, Expression expr,
+  Expression _bindValue(Map<JS.MetaLetVariable, JS.Expression> scope,
+      String name, Expression expr,
       {Expression context}) {
     // No need to do anything for stateless expressions.
     if (isStateless(_currentFunction, expr, context)) return expr;
 
-    var t = _createTemporary('#$name', getStaticType(expr));
-    scope[name] = _visit(expr);
+    var variable = new JS.MetaLetVariable(name);
+    var t = _createTemporary(name, getStaticType(expr), variable: variable);
+    scope[variable] = _visit(expr);
     return t;
   }
 
@@ -2900,7 +2900,7 @@ class CodeGenerator extends GeneralizingAstVisitor
 
     // Handle the left hand side, to ensure each of its subexpressions are
     // evaluated only once.
-    var vars = <String, JS.Expression>{};
+    var vars = <JS.MetaLetVariable, JS.Expression>{};
     var left = _bindLeftHandSide(vars, expr, context: expr);
 
     // Desugar `x++` as `(x1 = x0 + 1, x0)` where `x0` is the original value
@@ -2927,7 +2927,7 @@ class CodeGenerator extends GeneralizingAstVisitor
         return js.call('$op#', _visit(expr));
       } else if (op.lexeme == '++' || op.lexeme == '--') {
         // We need a null check, so the increment must be expanded out.
-        var vars = <String, JS.Expression>{};
+        var vars = <JS.MetaLetVariable, JS.Expression>{};
         var x = _bindLeftHandSide(vars, expr, context: expr);
 
         var one = AstBuilder.integerLiteral(1)..staticType = types.intType;
@@ -2960,7 +2960,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   JS.Node visitCascadeExpression(CascadeExpression node) {
     var savedCascadeTemp = _cascadeTarget;
 
-    var vars = <String, JS.Expression>{};
+    var vars = <JS.MetaLetVariable, JS.Expression>{};
     _cascadeTarget = _bindValue(vars, '_', node.target, context: node);
     var sections = _visitList(node.cascadeSections) as List<JS.Expression>;
     sections.add(_visit(_cascadeTarget));
@@ -3779,7 +3779,9 @@ JS.LiteralString _propertyName(String name) => js.string(name, "'");
 /// variable. These objects use instance equality, and should be shared
 /// everywhere in the tree where they are treated as the same variable.
 class TemporaryVariableElement extends LocalVariableElementImpl {
-  TemporaryVariableElement.forNode(Identifier name) : super.forNode(name);
+  final JS.Expression jsVariable;
+  TemporaryVariableElement.forNode(Identifier name, this.jsVariable)
+      : super.forNode(name);
 
   int get hashCode => identityHashCode(this);
   bool operator ==(Object other) => identical(this, other);

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

@@ -36,7 +36,7 @@ class MetaLet extends Expression {
   /// If the expression does not end up using `x` more than once, or if those
   /// expressions can be treated as [stateless] (e.g. they are non-mutated
   /// variables), then the resulting code will be simplified automatically.
-  final Map<String, Expression> variables;
+  final Map<MetaLetVariable, Expression> variables;
 
   /// A list of expressions in the body.
   /// The last value should represent the returned value.
@@ -171,49 +171,40 @@ class MetaLet extends Expression {
   }
 
   Block _finishStatement(List<Statement> statements) {
-    var params = <TemporaryId>[];
-    var values = <Expression>[];
-    var block = _build(params, values, new Block(statements));
-    if (params.isEmpty) return block;
-
-    var vars = [];
-    for (int i = 0; i < params.length; i++) {
-      vars.add(new VariableInitialization(params[i], values[i]));
-    }
-
-    return new Block(<Statement>[
-      new VariableDeclarationList('let', vars).toStatement(),
-      block
-    ]);
-  }
-
-  Node _build(List<TemporaryId> params, List<Expression> values, Node node) {
     // Visit the tree and count how many times each temp was used.
     var counter = new _VariableUseCounter();
+    var node = new Block(statements);
     node.accept(counter);
     // Also count the init expressions.
     for (var init in variables.values) init.accept(counter);
 
-    var substitutions = {};
-    _substitute(node) => new Template(null, node).safeCreate(substitutions);
-
-    variables.forEach((name, init) {
+    var initializers = <VariableInitialization>[];
+    var substitutions = <MetaLetVariable, Expression>{};
+    variables.forEach((variable, init) {
       // Since this is let*, subsequent variables can refer to previous ones,
       // so we need to substitute here.
-      init = _substitute(init);
-      int n = counter.counts[name];
-      if (n == null || n < 2) {
-        substitutions[name] = _substitute(init);
+      init = _substitute(init, substitutions);
+      int n = counter.counts[variable];
+      if (n == 1) {
+        // Replace interpolated exprs with their value, if it only occurs once.
+        substitutions[variable] = init;
       } else {
-        params.add(substitutions[name] = new TemporaryId(name));
-        values.add(init);
+        // Otherwise replace it with a temp, which will be assigned once.
+        var temp = new TemporaryId(variable.displayName);
+        substitutions[variable] = temp;
+        initializers.add(new VariableInitialization(temp, init));
       }
     });
 
-    // Interpolate the body:
-    // Replace interpolated exprs with their value, if it only occurs once.
-    // Otherwise replace it with a temp, which will be assigned once.
-    return _substitute(node);
+    // Interpolate the body.
+    node = _substitute(node, substitutions);
+    if (initializers.isNotEmpty) {
+      node = new Block([
+        new VariableDeclarationList('let', initializers).toStatement(),
+        node
+      ]);
+    }
+    return node;
   }
 
   /// If we finish with an assignment to an identifier, try to simplify the
@@ -231,11 +222,10 @@ class MetaLet extends Expression {
   ///
   MetaLet _simplifyAssignment(Identifier left, {bool isDeclaration: false}) {
     // See if the result value is a let* temporary variable.
-    if (body.last is! InterpolatedExpression) return null;
+    if (body.last is! MetaLetVariable) return null;
 
-    InterpolatedExpression last = body.last;
-    String name = last.nameOrPosition;
-    if (!variables.containsKey(name)) return null;
+    MetaLetVariable result = body.last;
+    if (!variables.containsKey(result)) return null;
 
     // Variables declared can't be used inside their initializer, so make
     // sure we don't transform an assignment into an initializer.
@@ -252,8 +242,8 @@ class MetaLet extends Expression {
       if (finder.found) return null;
     }
 
-    var vars = new Map<String, Expression>.from(variables);
-    var value = vars.remove(name);
+    var vars = new Map<MetaLetVariable, Expression>.from(variables);
+    var value = vars.remove(result);
     Expression assign;
     if (isDeclaration) {
       // Technically, putting one of these in a comma expression is not
@@ -266,18 +256,59 @@ class MetaLet extends Expression {
     }
 
     var newBody = new Expression.binary([assign]..addAll(body), ',');
-    Binary comma = new Template(null, newBody).safeCreate({name: left});
-    return new MetaLet(vars, comma.commaToExpressionList(),
+    newBody = _substitute(newBody, {result: left});
+    return new MetaLet(vars, newBody.commaToExpressionList(),
         statelessResult: statelessResult);
   }
 }
 
+/// Similar to [Template.instantiate] but works with free variables.
+Node _substitute(Node tree, Map<MetaLetVariable, Expression> substitutions) {
+  var generator = new InstantiatorGeneratorVisitor(/*forceCopy:*/ false);
+  var instantiator = generator.compile(tree);
+  var nodes = new List<MetaLetVariable>.from(generator
+      .analysis.containsInterpolatedNode
+      .where((n) => n is MetaLetVariable));
+  if (nodes.isEmpty) return tree;
+
+  return instantiator(new Map.fromIterable(nodes,
+      key: (v) => (v as MetaLetVariable).nameOrPosition,
+      value: (v) => substitutions[v] ?? v));
+}
+
+/// A temporary variable used in a [MetaLet].
+///
+/// Each instance of this class represents a fresh variable. The same object
+/// should be used everywhere to refer to the same variable. Different variables
+/// with the same name are different, and will be renamed later on, if needed.
+///
+/// These variables will be replaced when the `let*` is complete, depending on
+/// how often they occur and whether they can be optimized away. See [MetaLet]
+/// for more information.
+///
+/// This class should never reach our final JS code.
+class MetaLetVariable extends InterpolatedExpression {
+  /// The suggested display name of this variable.
+  ///
+  /// This name should not be used
+  final String displayName;
+
+  /// Compute fresh IDs to avoid
+  static int _uniqueId = 0;
+
+  MetaLetVariable(String displayName)
+      : displayName = displayName,
+        super(displayName + '@${++_uniqueId}');
+}
+
 class _VariableUseCounter extends BaseVisitor {
-  final counts = <String, int>{};
+  final counts = <MetaLetVariable, int>{};
   @override
   visitInterpolatedExpression(InterpolatedExpression node) {
-    int n = counts[node.nameOrPosition];
-    counts[node.nameOrPosition] = n == null ? 1 : n + 1;
+    if (node is MetaLetVariable) {
+      int n = counts[node];
+      counts[node] = n == null ? 1 : n + 1;
+    }
   }
 }
 

pkg/dev_compiler/lib/src/js_ast/template.dart

@@ -121,15 +121,6 @@ class Template {
     }
     return instantiator(arguments);
   }
-
-  /// Like [instantiate] but works with free variables.
-  Node safeCreate(Map arguments) {
-    if (holeNames.isEmpty) return ast;
-    return instantiator(new Map.fromIterable(holeNames, value: (name) {
-      var a = arguments[name];
-      return a != null ? a : new InterpolatedExpression(name);
-    }));
-  }
 }
 
 /**