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

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 a4e3c60ce577433dc4d86e18af80e6ae9988bb8c..c763cbf381ef12eb727e0f9b3d96170e47adc0be 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -963,14 +963,21 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
           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);
@@ -1009,9 +1016,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     // 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);
   }
 
@@ -1036,8 +1041,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     // 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);
@@ -1221,7 +1224,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
       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, "'");
@@ -1237,7 +1240,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
             _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)]));
       }
@@ -2044,13 +2048,6 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
   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);
@@ -2060,8 +2057,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
     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;
 
@@ -2071,14 +2067,27 @@ class JSCodegenVisitor extends GeneralizingAstVisitor with ClosureAnnotator {
           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);
       }
     }