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

Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `function f({['arguments']: arguments$ = 1} = {}) {}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/ff88e54754e4a5c61cc93fe4d8eb2cee7b30319c

[ochafik, 2016-02-06 16:47:28]

Description was changed from

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

BUG=
==========

to

==========
Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `f({['arguments']: arguments$ = 1}) {}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
==========

[ochafik, 2016-02-06 16:48:11]

Description was changed from

==========
Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `f({['arguments']: arguments$ = 1}) {}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
==========

to

==========
Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `function f({['arguments']: arguments$ = 1})
{}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
==========

[ochafik, 2016-02-06 16:57:15]

ochafik@google.com changed reviewers:
+ jmesserly@google.com, vsm@google.com

[ochafik, 2016-02-06 16:57:16]

Hey guys, I found a way to destructure all named arguments (using computed
properties for those that are reserved identifiers), and realized we weren't
using default params yet (but fencing this behind the same flag for now).

[ochafik, 2016-02-08 15:46:50]

Description was changed from

==========
Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `function f({['arguments']: arguments$ = 1})
{}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
==========

to

==========
Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `function f({['arguments']: arguments$ = 1} =
{}) {}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
==========

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

One comment about factoring, otherwise LGTM.

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:968: if (node.constKeyword != null)
nit: use curly here.

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

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2070: if
(invalidVariableName(param.identifier.name)) {
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.

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2078: name: _visit(param.identifier),
(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));

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2089: name: jsParam, defaultValue:
_defaultParamValue(param))
BTW, are "name" and "defaultValue" always supplied? If so maybe they should be
required params.

https://codereview.chromium.org/1677863002/diff/1/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1677863002/diff/1/lib/src/js/printer.dart#new...
lib/src/js/printer.dart:623: visitDestructuredVariable(DestructuredVariable
node) {
this code looks a lot like propertyNameOut. Can they be unified cleanly?

if not, it may still be worth handling the invalid name issue on this side of
the world.

(Generally Printer is very easy to work with, unlike our Dart->JS transformer
where there's a lot of Dart->JS semantic knowledge we're trying to keep in
mind.)

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

Thanks John!

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:968: if (node.constKeyword != null)
On 2016/02/09 00:33:07, John Messerly wrote:
> nit: use curly here.
> 
> alternate idea: save node.element.enclosingElement into a local var.

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

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2070: if
(invalidVariableName(param.identifier.name)) {
On 2016/02/09 00:33:07, John Messerly wrote:
> 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.

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).

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2078: name: _visit(param.identifier),
On 2016/02/09 00:33:07, John Messerly wrote:
> (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));
> 

Done.

https://codereview.chromium.org/1677863002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:2089: name: jsParam, defaultValue:
_defaultParamValue(param))
On 2016/02/09 00:33:07, John Messerly wrote:
> BTW, are "name" and "defaultValue" always supplied? If so maybe they should be
> required params.

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.

https://codereview.chromium.org/1677863002/diff/1/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1677863002/diff/1/lib/src/js/printer.dart#new...
lib/src/js/printer.dart:623: visitDestructuredVariable(DestructuredVariable
node) {
On 2016/02/09 00:33:07, John Messerly wrote:
> this code looks a lot like propertyNameOut. Can they be unified cleanly?
> 
> if not, it may still be worth handling the invalid name issue on this side of
> the world.
> 
> (Generally Printer is very easy to work with, unlike our Dart->JS transformer
> where there's a lot of Dart->JS semantic knowledge we're trying to keep in
> mind.)

I agree in principle, but see the other comment: I think we need both a
LiteralString (for the matching pattern, with a computed / non computed printing
logic that differs from Printer's semantics of isValidJavaScriptId) and an
identifier. I think this + the different printing logic (add a ": identifier"
only if we printed the name in computed property style) makes it hard to unify.

[Jennifer Messerly, 2016-02-09 16:16:26]

Yeah makes sense. Thanks for the explanation. :)

[ochafik, 2016-02-09 16:46:38]

Description was changed from

==========
Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `function f({['arguments']: arguments$ = 1} =
{}) {}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
==========

to

==========
Improve --destructure-named-params:
- Destructure named params that clash with JS reserved names:
  `f({arguments = 1}) {}` becomes `function f({['arguments']: arguments$ = 1} =
{}) {}`
- Use ES6 default params for positional & optional parameters
- Added simple tests

BUG=
R=jmesserly@google.com

Committed:
https://github.com/dart-lang/dev_compiler/commit/ff88e54754e4a5c61cc93fe4d8eb...
==========

[ochafik, 2016-02-09 16:46:40]

Committed patchset #3 (id:40001) manually as
ff88e54754e4a5c61cc93fe4d8eb2cee7b30319c (presubmit successful).