Use destructuring assignments for named parameters (#180)

lib/src/js/builder.dart

Index: lib/src/js/builder.dart
diff --git a/lib/src/js/builder.dart b/lib/src/js/builder.dart
index 93d5c67711641631f5b5a4fbdcf9e1d675ca7c59..60bf7f9984e8d8e2e437b2e5343f8b03a8f07c8c 100644
--- a/lib/src/js/builder.dart
+++ b/lib/src/js/builder.dart
@@ -811,6 +811,22 @@ class MiniJsParser {
     return expression;
   }
 
+  InterpolatedIdentifier parseInterpolatedIdentifier() {
+    var id = new InterpolatedIdentifier(parseHash());
+    interpolatedValues.add(id);
+    return id;
+  }
+
+  Identifier parseIdentifier() {
+    if (acceptCategory(HASH)) {
+      return parseInterpolatedIdentifier();
+    } else {
+      var id = new Identifier(lastToken);
+      expectCategory(ALPHA);
+      return id;
+    }
+  }
+
   /**
    * CoverParenthesizedExpressionAndArrowParameterList[Yield] :
    *     ( Expression )
@@ -1136,45 +1152,95 @@ class MiniJsParser {
   }
 
   /** Parse a variable declaration list, with `var` or `let` [keyword] */
-  VariableDeclarationList parseVariableDeclarationList(String keyword) {
-    // Supports one form for interpolated variable declaration:
-    //    let # = ...
-    if (acceptCategory(HASH)) {
-      var name = new InterpolatedIdentifier(parseHash());
-      interpolatedValues.add(name);
+  VariableDeclarationList parseVariableDeclarationList(String keyword) =>
+      finishVariableDeclarationList(keyword);

[Jennifer Messerly, 2015/12/01 02:10:27]

why introduce this redirect? If they aren't different, the two methods should be
merged. I'm fine with either name.

[ochafik, 2015/12/02 20:05:46]

Done.

 
-      Expression initializer = acceptString("=") ? parseAssignment() : null;
-      return new VariableDeclarationList(keyword,
-          [new VariableInitialization(name, initializer)]);
-    }
-
-    String firstVariable = lastToken;
-    expectCategory(ALPHA);
-    return finishVariableDeclarationList(keyword, firstVariable);
-  }
-
-  VariableDeclarationList finishVariableDeclarationList(
-      String keyword, String firstVariable) {
+  VariableDeclarationList finishVariableDeclarationList(String keyword) {
     var initialization = [];
 
-    void declare(String variable) {
+    do {
+      var declarator;
+      switch (lastCategory) {

[Jennifer Messerly, 2015/12/01 02:10:27]

this pattern occurs in a couple of places, pull it out into a method?

[ochafik, 2015/12/02 20:05:46]

+1, meet parseVariableBinding()

+        case ALPHA:
+        case HASH:
+          declarator = parseIdentifier();
+          break;
+        case LBRACE:
+        case LSQUARE:
+          declarator = parseDestructuring();
+          break;
+        default:
+          throw new StateError('Unexpected token $lastToken: ${categoryToString(lastCategory)}');

[Jennifer Messerly, 2015/12/01 02:10:27]

long line. FYI, to prevent large churn here we exclude js_ast from dartfmt, so
it must be formatted manually for now.

[ochafik, 2015/12/02 20:05:46]

Done.

+      }
       Expression initializer = null;
       if (acceptString("=")) {
         initializer = parseAssignment();
       }
-      var declaration = new Identifier(variable);
-      initialization.add(new VariableInitialization(declaration, initializer));
-    }
+      initialization.add(new VariableInitialization(declarator, initializer));
+    } while (acceptCategory(COMMA));
 
-    declare(firstVariable);
-    while (acceptCategory(COMMA)) {
-      String variable = lastToken;
-      expectCategory(ALPHA);
-      declare(variable);
-    }
     return new VariableDeclarationList(keyword, initialization);
   }
 
+  Destructuring parseDestructuring() {

[Jennifer Messerly, 2015/12/01 02:10:27]

It'd be worth adding a link to where this is defined in the ES6 spec:
http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-binding-pat...
http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-assignment

Also good to comment that we only support restructuring in variable
initialization at this time, in particular it's not supported in
LeftHandSideExpression (although I have another comment on this below)

Probably worth adding a TODO that this is not a complete parsing implementation
for destructuring.

[ochafik, 2015/12/02 20:05:46]

Done.

+    if (acceptCategory(LBRACE)) {
+      return parseObjectDestructuring();
+    } else {
+      expectCategory(LSQUARE);
+      return parseArrayDestructuring();
+    }
+  }
+
+  ArrayDestructuring parseArrayDestructuring() {
+    var variables = <DestructuredVariable>[];
+    do {
+      var name;
+      var structure;
+      var defaultValue;
+
+      switch (lastCategory) {
+        case ALPHA:
+        case HASH:
+          name = parseIdentifier();
+          break;
+        case LBRACE:
+        case LSQUARE:
+          structure = parseDestructuring();
+          break;
+        default:
+          throw new StateError('Unexpected token $lastToken: ${categoryToString(lastCategory)}');
+      }
+      if (acceptString("=")) {
+        defaultValue = parseExpression();
+      }
+      variables.add(new DestructuredVariable(
+          name: name, structure: structure, defaultValue: defaultValue));
+    } while (acceptCategory(COMMA));
+
+    expectCategory(RSQUARE);
+    return new ArrayDestructuring(variables);
+  }
+
+  ObjectDestructuring parseObjectDestructuring() {
+    var variables = <DestructuredVariable>[];
+    do {
+      var name = parseIdentifier();
+      var structure;
+      var defaultValue;
+
+      if (acceptCategory(COLON)) {

[Jennifer Messerly, 2015/12/01 02:10:27]

Hmmm, normally we'd do parseAssignment, like the old variable declaration code
did. parseAssignment will parse the left-hand-side, which will include any
nested destructurings.

The parser code here can be a little finicky, but in this case I think
implementing destructuring support in parseLeftHandSide might actually simplify
some of the logic, by allowing more code reuse, and it would also support more
use cases. WDYT?

[ochafik, 2015/12/02 20:05:46]

Hehe, tried that, but... the grammar is a bit sensitive (`{x: []}` can be parsed
as a object literal *or* a ObjectBindingPattern depending on its location, e.g.
`({x: []} = y)` vs `let y = {x: []}`).

Keeping things unchanged for now, and can actually even remove the parsing if
you prefer, since I'm not using it anymore from js_codegen (I'm building the
ObjectBindingPattern manually).

+        structure = parseDestructuring();
+      } else if (acceptString("=")) {
+        defaultValue = parseExpression();
+      }
+      variables.add(new DestructuredVariable(
+          name: name, structure: structure, defaultValue: defaultValue));
+    } while (acceptCategory(COMMA));
+
+    expectCategory(RBRACE);
+    return new ObjectDestructuring(variables);
+  }
+
   Expression parseVarDeclarationOrExpression() {
     var keyword = acceptVarOrLet();
     if (keyword != null) {
@@ -1394,7 +1460,7 @@ class MiniJsParser {
             iterableExpression,
             body);
       }
-      var declarations = finishVariableDeclarationList(keyword, identifier);
+      var declarations = finishVariableDeclarationList(keyword);
       expectCategory(SEMICOLON);
       return finishFor(declarations);
     }
@@ -1498,15 +1564,7 @@ class MiniJsParser {
   }
 
   ClassExpression parseClass() {
-    Identifier name;
-    if (acceptCategory(HASH)) {
-      var interpolatedName = new InterpolatedIdentifier(parseHash());
-      interpolatedValues.add(interpolatedName);
-      name = interpolatedName;
-    } else {
-      name = new Identifier(lastToken);
-      expectCategory(ALPHA);
-    }
+    Identifier name = parseIdentifier();
     Expression heritage = null;
     if (acceptString('extends')) {
       heritage = parseLeftHandSide();