Use default params when --destructure-named-params + fix renaming of reserved destructured params

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.
 
 import 'dart:collection' show HashSet, HashMap, SplayTreeSet;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/ast.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/constant.dart';
 import 'package:analyzer/src/generated/element.dart';
@@ skipping 945 matching lines @@
       var params =
           _emitFormalParameterList(node.parameters, allowDestructuring: false);
 
       var fun = js.call('function(#) { return $newKeyword #(#); }',
           [params, _visit(redirect), params]) as JS.Fun;
       return annotate(
           new JS.Method(name, fun, isStatic: true)..sourceInformation = node,
           node.element);
     }
 
+    // For const constructors we need to ensure default values are
+    // available for use by top-level constant initializers.
+    if (node.constKeyword != null)

[Jennifer Messerly, 2016/02/09 00:33:07]

nit: use curly here.

alternate idea: save node.element.enclosingElement into a local var.

[ochafik, 2016/02/09 01:51:19]

Done (extracted the ClassDeclaration to a local var to match the other instance
of this pattern).

+      _loader.startTopLevel(node.element.enclosingElement);
+    var params = _emitFormalParameterList(node.parameters);
+    if (node.constKeyword != null)
+      _loader.finishTopLevel(node.element.enclosingElement);
+
     // 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);
       body.add(_visit(node.body));
-      var fun = new JS.Fun(
-          _visit(node.parameters) as List<JS.Parameter>, new JS.Block(body));
+      var fun = new JS.Fun(params, new JS.Block(body));
       return annotate(
           new JS.Method(name, fun, isStatic: true)..sourceInformation = node,
           node.element);
     }
 
     // Code generation for Object's constructor.
     JS.Block body;
     if (isObject &&
         node.body is EmptyFunctionBody &&
         node.constKeyword != null &&
@@ skipping 18 matching lines @@
         return result === void 0 ? this : result;
       }''') as JS.Block;
     } else {
       body = _emitConstructorBody(node, fields);
     }
 
     // We generate constructors as initializer methods in the class;
     // this allows use of `super` for instance methods/properties.
     // It also avoids V8 restrictions on `super` in default constructors.
     return annotate(
-        new JS.Method(name,
-            new JS.Fun(_visit(node.parameters) as List<JS.Parameter>, body))
-          ..sourceInformation = node,
+        new JS.Method(name, new JS.Fun(params, body))..sourceInformation = node,
         node.element);
   }
 
   JS.Expression _constructorName(ConstructorElement ctor) {
     var name = ctor.name;
     if (name != '') {
       return _emitMemberName(name, isStatic: true);
     }
 
     // Factory default constructors use `new` as their name, for readability
     // Other default constructors use the class name, as they aren't called
     // from call sites, but rather from Object's constructor.
     // TODO(jmesserly): revisit in the context of Dart metaclasses, and cleaning
     // up constructors to integrate more closely with ES6.
     return _propertyName(ctor.isFactory ? 'new' : ctor.enclosingElement.name);
   }
 
   JS.Block _emitConstructorBody(
       ConstructorDeclaration node, List<FieldDeclaration> fields) {
     var body = <JS.Statement>[];
 
     // Generate optional/named argument value assignment. These can not have
     // side effects, and may be used by the constructor's initializers, so it's
     // nice to do them first.
-    // Also for const constructors we need to ensure default values are
-    // available for use by top-level constant initializers.
     ClassDeclaration cls = node.parent;
     if (node.constKeyword != null) _loader.startTopLevel(cls.element);
     var init = _emitArgumentInitializers(node, constructor: true);
     if (node.constKeyword != null) _loader.finishTopLevel(cls.element);
     if (init != null) body.add(init);
 
     // Redirecting constructors: these are not allowed to have initializers,
     // and the redirecting ctor invocation runs before field initializers.
     var redirectCall = node.initializers.firstWhere(
         (i) => i is RedirectingConstructorInvocation,
@@ skipping 163 matching lines @@
     if (node is ConstructorDeclaration != constructor) return null;
 
     var parameters = _parametersOf(node);
     if (parameters == null) return null;
 
     var body = <JS.Statement>[];
     for (var param in parameters.parameters) {
       var jsParam = _visit(param.identifier);
 
       if (param.kind == ParameterKind.NAMED) {
-        if (!_isDestructurableNamedParam(param)) {
+        if (!options.destructureNamedParams) {
           // 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) {
+      } else if (param.kind == ParameterKind.POSITIONAL &&
+          !options.destructureNamedParams) {
         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 && _hasUnsoundTypeParameter(paramType)) {
         body.add(js
             .statement('dart.as(#, #);', [jsParam, _emitTypeName(paramType)]));
@@ skipping 786 matching lines @@
   @override
   JS.Property visitNamedExpression(NamedExpression node) {
     assert(node.parent is ArgumentList);
     return new JS.Property(
         _propertyName(node.name.label.name), _visit(node.expression));
   }
 
   bool _isNamedParam(FormalParameter param) =>
       param.kind == ParameterKind.NAMED;
 
-  /// We cannot destructure named params that clash with JS reserved names:
-  /// see discussion in https://github.com/dart-lang/dev_compiler/issues/392.
-  bool _isDestructurableNamedParam(FormalParameter param) =>
-      _isNamedParam(param) &&
-      !invalidVariableName(param.identifier.name) &&
-      options.destructureNamedParams;
-
   @override
   List<JS.Parameter> visitFormalParameterList(FormalParameterList node) =>
       _emitFormalParameterList(node);
 
   List<JS.Parameter> _emitFormalParameterList(FormalParameterList node,
       {bool allowDestructuring: true}) {
     var result = <JS.Parameter>[];
 
     var namedVars = <JS.DestructuredVariable>[];
-    var destructure = allowDestructuring &&
-        node.parameters.where(_isNamedParam).every(_isDestructurableNamedParam);
+    var destructure = allowDestructuring && options.destructureNamedParams;
     var hasNamedArgsConflictingWithObjectProperties = false;
     var needsOpts = false;
 
     for (FormalParameter param in node.parameters) {
       if (param.kind == ParameterKind.NAMED) {
         if (destructure) {
           if (_jsObjectProperties.contains(param.identifier.name)) {
             hasNamedArgsConflictingWithObjectProperties = true;
           }
-          namedVars.add(new JS.DestructuredVariable(
-              name: _visit(param.identifier),
-              defaultValue: _defaultParamValue(param)));
+          if (invalidVariableName(param.identifier.name)) {

[Jennifer Messerly, 2016/02/09 00:33:07]

JS should handle this. It already understands which names are invalid. See
"propertyNameOut" in "printer.dart". The nice thing about fixing this in JS
Printer is, every user of js_ast will benefit. :) And we don't have to think
about it every time we want to emit these nodes.

[ochafik, 2016/02/09 01:51:19]

So, the logic here is a bit inconvenient: if the name is reserved, then we
output `{['function']: func = null}`, otherwise we output `{x = null}` (which is
equivalent to `{x: x = null}` or `{['x']: x = null}`, but more readable).

Passing a LiteralString (which is what propertyNameOut would need down the line)
will not get us the `function -> func` rewrite for the identifier (unless we
call localNamer.getName, but then we have to build an Identifier, which would
have eluded the var visitors).

Passing an Identifier only would get us the rewrite, but we would lose the
original name in Printer (as _RenameVisitor happens before).

So... we have to pass both a LiteralString and an Identifier. But we want to be
able to simplify `{['x']: x}` to `{x}` (and `{['x']}` wouldn't be valid), so if
we leave this to the printer we'll have to twist the logic of propertyNameOut a
bit too much, IMHO :-S (it has to tell its caller whether it emitted a computed
property key, etc).

+            namedVars.add(new JS.DestructuredVariable(
+                name: js.string(param.identifier.name),
+                structure:
+                    new JS.SimpleBindingPattern(_visit(param.identifier)),
+                defaultValue: _defaultParamValue(param)));
+          } else {
+            namedVars.add(new JS.DestructuredVariable(
+                name: _visit(param.identifier),

[Jennifer Messerly, 2016/02/09 00:33:07]

(this might be moot with my comment above, but...)

it looks like you could restructure this to share more code? in particular

JS.Expression name;
JS.SimpleBindingPattern structure = null;
String paramName = param.identifier.name;
if (invalidVariableName(paramName)) {
  name = js.string(paramName)
  structure = new JS.SimpleBindingPattern(_visit(param.identifier));
} else {
  name = _visit(param.identifier);
}
namedVars.add(new JS.DestructuredVariable(
    name: name,
    structure: structure,
    defaultValue: _defaultParamValue(param));

[ochafik, 2016/02/09 01:51:20]

Done.

+                defaultValue: _defaultParamValue(param)));
+          }
         } else {
           needsOpts = true;
         }
       } else {
-        result.add(_visit(param));
+        var jsParam = _visit(param);
+        result.add(
+            param is DefaultFormalParameter && options.destructureNamedParams
+                ? new JS.DestructuredVariable(
+                    name: jsParam, defaultValue: _defaultParamValue(param))

[Jennifer Messerly, 2016/02/09 00:33:07]

BTW, are "name" and "defaultValue" always supplied? If so maybe they should be
required params.

[ochafik, 2016/02/09 01:51:20]

Good question! They're not compulsory, although in practice we pass (nullable)
defaultValue params in all 3 instantiations.
(example of unnamed + no default would be first param of `function f({x}) {}`)

Would seem a bit awkward to pass defaultValue as required param, before the
(more important, and appearing before in lexical order) name & pattern, though.

+                : jsParam);
       }
     }
 
     if (needsOpts) {
       result.add(_namedArgTemp);
     } else if (namedVars.isNotEmpty) {
       // Note: `var {valueOf} = {}` extracts `Object.prototype.valueOf`, so
       // in case there are conflicting names we create an object without
       // any prototype.
       var defaultOpts = hasNamedArgsConflictingWithObjectProperties
@@ skipping 1520 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);
 }