Use destructuring assignments for named parameters (#180)

lib/src/codegen/js_codegen.dart

Index: lib/src/codegen/js_codegen.dart
diff --git a/lib/src/codegen/js_codegen.dart b/lib/src/codegen/js_codegen.dart
index 0dd62fd6d66a560036728d90a15671685a87c050..723102570e699d9dd93854eae83186da7a8f3c06 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -926,9 +926,11 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       // Pass along all arguments verbatim, and let the callee handle them.
       // TODO(jmesserly): we'll need something different once we have
       // rest/spread support, but this should work for now.
-      var params = _visit(node.parameters);
+      var params = _visitFormalParameterList(
+          node.parameters, allowDestructuring: false);

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

yeah, FYI, this makes total sense.

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

Acknowledged.

+
       var fun = js.call('function(#) { return $newKeyword #(#); }',
-          [params, _visit(redirect), params,]) as JS.Fun;
+          [params, _visit(redirect), params]) as JS.Fun;
       return annotate(
           new JS.Method(name, fun, isStatic: true)..sourceInformation = node,
           node.element);
@@ -1185,25 +1187,31 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     if (parameters == null) return null;
 
     var body = <JS.Statement>[];
+
+    Map<FormalParameter, JS.Identifier> paramNames =

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

is this change still needed? the map doesn't seem used except once, to fetch the
result of _visit(param.identifier). Seems like we could keep the old code below
& revert this.

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

Indeed, removed :-)

+        new Map.fromIterable(parameters.parameters,
+            value: (param) => _visit(param.identifier));
+
     for (var param in parameters.parameters) {
-      var jsParam = _visit(param.identifier);
+      var jsParam = paramNames[param];
 
       if (param.kind == ParameterKind.NAMED) {
-        // Parameters will be passed using their real names, not the (possibly
-        // renamed) local variable.
-        var paramName = js.string(param.identifier.name, "'");
-
-        // TODO(ochafik): Fix `'prop' in obj` to please Closure's renaming
-        // (either test if `obj.prop !== void 0`, or use JSCompiler_renameProperty).
-        body.add(js.statement('let # = # && # in # ? #.# : #;', [
-          jsParam,
-          _namedArgTemp,
-          paramName,
-          _namedArgTemp,
-          _namedArgTemp,
-          paramName,
-          _defaultParamValue(param),
-        ]));
+        if (!_isDestructurableNamedParam(param)) {
+          // Parameters will be passed using their real names, not the (possibly
+          // renamed) local variable.
+          var paramName = js.string(param.identifier.name, "'");
+
+          // TODO(ochafik): Fix `'prop' in obj` to please Closure's renaming.
+          body.add(js.statement('let # = # && # in # ? #.# : #;', [
+            jsParam,
+            _namedArgTemp,
+            paramName,
+            _namedArgTemp,
+            _namedArgTemp,
+            paramName,
+            _defaultParamValue(param),
+          ]));
+        }
       } else if (param.kind == ParameterKind.POSITIONAL) {
         body.add(js.statement('if (# === void 0) # = #;',
             [jsParam, jsParam, _defaultParamValue(param)]));
@@ -1354,17 +1362,22 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     if (parent is FunctionDeclaration) {
       return _emitFunctionBody(params, node.body);
     } else {
-      String code;
+      // Chrome Canary does not accept default values with destructuring in
+      // arrow functions yet (e.g. `({a} = {}) => 1`) but happily accepts them
+      // with regular functions (e.g. `function({a} = {}) { return 1 }`).
+      // Note that Firefox accepts both syntaxes just fine.
+      // TODO(ochafik): Simplify this code when Chrome Canary catches up.
+      var canUseArrowFun = !node.parameters.parameters.any(_isNamedParam);
+
+      String code = canUseArrowFun ? '(#) => #' : 'function(#) { return # }';
       JS.Node jsBody;
       var body = node.body;
       if (body.isGenerator || body.isAsynchronous) {
-        code = '(#) => #';
         jsBody = _emitGeneratorFunctionBody(params, body);
       } else if (body is ExpressionFunctionBody) {
-        code = '(#) => #';
         jsBody = _visit(body.expression);
       } else {
-        code = '(#) => { #; }';
+        code = canUseArrowFun ? '(#) => { #; }' : 'function(#) { #; }';
         jsBody = _visit(body);
       }
       var clos = js.call(code, [params, jsBody]);
@@ -1905,15 +1918,71 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
         _propertyName(node.name.label.name), _visit(node.expression));
   }
 
+  static final Set<String> _jsObjectProperties = new Set<String>()..addAll([

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

it'd be good to cite where this list comes from.

http://www.ecma-international.org/ecma-262/6.0/#sec-properties-of-the-object-...
http://www.ecma-international.org/ecma-262/6.0/#sec-additional-properties-of-...
https://javascript.spec.whatwg.org/#object

Between those, that seems to cover the list.

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

Done.

+      "constructor",
+      "toString",
+      "toLocaleString",
+      "valueOf",
+      "hasOwnProperty",
+      "isPrototypeOf",
+      "propertyIsEnumerable",
+      "__defineGetter__",
+      "__lookupGetter__",
+      "__defineSetter__",
+      "__lookupSetter__",
+      "__proto__"
+  ]);
+
+  bool _isNamedParam(FormalParameter param) =>
+      param.kind == ParameterKind.NAMED;
+
+  bool _isDestructurableNamedParam(FormalParameter param) {

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

I think we can allow destructing always and just use Object.create(null) when
needed

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

Oh sweet, thanks! Done. (removes lots of nonsense!)

+    var name = param.identifier.name;
+    return _isNamedParam(param) &&
+        !_jsObjectProperties.contains(name) &&
+        name != 'super';
+  }
+
+  _namedParams(FormalParameterList list) =>
+      list.parameters.where(_isNamedParam);
+
+  bool _willDestructureNamedParams(FormalParameterList list) {
+    var named = _namedParams(list).toList(growable: false);
+    return named.isNotEmpty && named.every(_isDestructurableNamedParam);
+  }
+
   @override
-  List<JS.Parameter> visitFormalParameterList(FormalParameterList node) {
+  List<JS.Parameter> visitFormalParameterList(FormalParameterList node) =>
+      _visitFormalParameterList(node, allowDestructuring: true);
+
+  List<JS.Parameter> _visitFormalParameterList(

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

for better or worse, we have an "_emit" naming convention for our own methods
(different from the visit* inherited ones).

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

Done.

+      FormalParameterList node, {bool allowDestructuring}) {
+
     var result = <JS.Parameter>[];
+
+    var namedVars = <JS.DestructuredVariable>[];
+    var canDestructure = allowDestructuring && _willDestructureNamedParams(node);

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

long line

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

Acknowledged.

+    var needsOpts = false;
+
     for (FormalParameter param in node.parameters) {
       if (param.kind == ParameterKind.NAMED) {
-        result.add(_namedArgTemp);
-        break;
+        if (canDestructure) {
+          namedVars.add(new JS.DestructuredVariable(
+              name: _visit(param.identifier),
+              defaultValue: _defaultParamValue(param)));
+        } else {
+          needsOpts = true;
+        }
+      } else {
+        result.add(_visit(param));
       }
-      result.add(_visit(param));
+    }
+    if (canDestructure) {
+      result.add(new JS.DestructuredVariable(
+          structure: new JS.ObjectDestructuring(namedVars),
+          defaultValue: js.call('{}')));
+    } else if (needsOpts) {
+      result.add(_namedArgTemp);
     }
     return result;
   }