Use destructuring assignments for named parameters (#180)

lib/src/codegen/js_codegen.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library dev_compiler.src.codegen.js_codegen;
 
 import 'dart:collection' show HashSet, HashMap, SplayTreeSet;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/ast.dart' hide ConstantEvaluator;
@@ skipping 908 matching lines @@
 
     var name = _constructorName(node.element);
 
     // Wacky factory redirecting constructors: factory Foo.q(x, y) = Bar.baz;
     var redirect = node.redirectedConstructor;
     if (redirect != null) {
       var newKeyword = redirect.staticElement.isFactory ? '' : 'new';
       // 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);
     }
 
     // Factory constructors are essentially static methods.
     if (node.factoryKeyword != null) {
       var body = <JS.Statement>[];
       var init = _emitArgumentInitializers(node, constructor: true);
       if (init != null) body.add(init);
@@ skipping 236 matching lines @@
   /// as generic type checks needed due to our covariance.
   JS.Statement _emitArgumentInitializers(node, {bool constructor: false}) {
     // Constructor argument initializers are emitted earlier in the code, rather
     // than always when we visit the function body, so we control it explicitly.
     if (node is ConstructorDeclaration != constructor) return null;
 
     var parameters = _parametersOf(node);
     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, "'");
+        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
-        // (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),
-        ]));
+          // 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)]));
       }
 
       // TODO(jmesserly): various problems here, see:
       // https://github.com/dart-lang/dev_compiler/issues/161
       var paramType = param.element.type;
       if (!constructor && _hasTypeParameter(paramType)) {
         body.add(js.statement(
@@ skipping 130 matching lines @@
   @override
   JS.Expression visitFunctionExpression(FunctionExpression node) {
     var params = _visit(node.parameters) as List<JS.Parameter>;
     if (params == null) params = <JS.Parameter>[];
 
     var parent = node.parent;
     var inStmt = parent.parent is FunctionDeclarationStatement;
     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]);
       if (!inStmt) {
         var type = getStaticType(node);
         return _emitFunctionTagged(clos, type,
             topLevel: _executesAtTopLevel(node));
       }
       return clos;
     }
@@ skipping 520 matching lines @@
     return args;
   }
 
   @override
   JS.Property visitNamedExpression(NamedExpression node) {
     assert(node.parent is ArgumentList);
     return new JS.Property(
         _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;
   }
 
   @override
   JS.Statement visitExpressionStatement(ExpressionStatement node) =>
       _visit(node.expression).toStatement();
 
   @override
   JS.EmptyStatement visitEmptyStatement(EmptyStatement node) =>
@@ skipping 1420 matching lines @@
 
 /// A special kind of element created by the compiler, signifying a temporary
 /// 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);
 
   int get hashCode => identityHashCode(this);
   bool operator ==(Object other) => identical(this, other);
 }