Use destructuring assignments for named parameters (#180)

Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter clashes with reserved JS names (see discussion in https://github.com/dart-lang/dev_compiler/issues/392)
- When a parameter clashes with a Object.prototype property, using a clean default opts value (Object.create(null)).
- Passing opts through in aliased constructors, both for speed/concision and correctness (since default param value semantic is weird there)

BUG=
R=jmesserly@google.com

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

[ochafik, 2015-12-01 00:41:26]

Description was changed from

==========
Fix aliased constructors: pass-through the opts object to preserve weird default
values behaviour


Destructure named args in param list


Use destructuring assignments for named parameters (#180)

Before:
let onMatch = opts && 'onMatch' in opts ? opts.onMatch : null;
let onNonMatch = opts && 'onNonMatch' in opts ? opts.onNonMatch :
null;
After:
let {onMatch = null, onNonMatch = null} = opts || {};

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

to

==========
Use destructuring assignments for named parameters (#180)

Before:
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : b_default;
    ...

After:
  function f(a, {b = b_default}) {
    ...

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

[ochafik, 2015-12-01 00:43:53]

Description was changed from

==========
Use destructuring assignments for named parameters (#180)

Before:
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : b_default;
    ...

After:
  function f(a, {b = b_default}) {
    ...

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

to

==========
Use destructuring assignments for named parameters (#180)

Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note: still reverting to old code when any parameter is named 'super' (can't
alias it) or 'constructor' (or any other Object.prototype member: destructuring
would fetch it from the default {} opts param value even if not specified).

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

[ochafik, 2015-12-01 00:46:26]

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

[ochafik, 2015-12-01 00:46:27]

Sorry, looks like I created a new issue :-S
Anyway, not 100% happy with the extra Nodes I've created (suggestions welcome)
but happy with the output code :-)

[ochafik, 2015-12-01 01:00:40]

Description was changed from

==========
Use destructuring assignments for named parameters (#180)

Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note: still reverting to old code when any parameter is named 'super' (can't
alias it) or 'constructor' (or any other Object.prototype member: destructuring
would fetch it from the default {} opts param value even if not specified).

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

to

==========
Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter is named 'super' (can't alias
it) or 'constructor' (or any other Object.prototype member: destructuring would
fetch it from the default {} opts param value even if not specified).
- Passing opts through in aliased constructors, both for speed/concision and
correctness (since default param value semantic is weird there)

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

[Jennifer Messerly, 2015-12-01 01:06:37]

haven't looked at the code yet but...

> Note: still reverting to old code when any parameter is named 'super' (can't
alias it)

super isn't a legal parameter name in Dart either? (quick test and VM and
Analyzer both treat it as an error)

> or 'constructor' (or any other Object.prototype member: destructuring
> would fetch it from the default {} opts param value even if not specified).

Object.create(null) should give you a clean prototype in those cases.

[Jennifer Messerly, 2015-12-01 02:10:28]

A few comments. But this is looking really nice!

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:930: node.parameters, allowDestructuring:
false);
yeah, FYI, this makes total sense.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1191: Map<FormalParameter, JS.Identifier>
paramNames =
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.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1921: static final Set<String>
_jsObjectProperties = new Set<String>()..addAll([
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.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1939: bool
_isDestructurableNamedParam(FormalParameter param) {
I think we can allow destructing always and just use Object.create(null) when
needed

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1958: List<JS.Parameter>
_visitFormalParameterList(
for better or worse, we have an "_emit" naming convention for our own methods
(different from the visit* inherited ones).

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1964: var canDestructure = allowDestructuring &&
_willDestructureNamedParams(node);
long line

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart
File lib/src/js/builder.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1156: finishVariableDeclarationList(keyword);
why introduce this redirect? If they aren't different, the two methods should be
merged. I'm fine with either name.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1163: switch (lastCategory) {
this pattern occurs in a couple of places, pull it out into a method?

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1173: throw new StateError('Unexpected token $lastToken:
${categoryToString(lastCategory)}');
long line. FYI, to prevent large churn here we exclude js_ast from dartfmt, so
it must be formatted manually for now.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1185: Destructuring parseDestructuring() {
It'd be worth adding a link to where this is defined in the ES6 spec:
http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-binding-pat...
http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-assignment

Also good to comment that we only support restructuring in variable
initialization at this time, in particular it's not supported in
LeftHandSideExpression (although I have another comment on this below)

Probably worth adding a TODO that this is not a complete parsing implementation
for destructuring.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1231: if (acceptCategory(COLON)) {
Hmmm, normally we'd do parseAssignment, like the old variable declaration code
did. parseAssignment will parse the left-hand-side, which will include any
nested destructurings.

The parser code here can be a little finicky, but in this case I think
implementing destructuring support in parseLeftHandSide might actually simplify
some of the logic, by allowing more code reuse, and it would also support more
use cases. WDYT?

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/nodes.dart#n...
lib/src/js/nodes.dart:763: abstract class Declarator extends Expression {
The ES6 spec calls this a "Binding", as in BindingIdentifier and BindingPattern
and LexicalBinding:
http://www.ecma-international.org/ecma-262/6.0/#sec-declarations-and-the-vari...,
so maybe "VariableBinding"?

Not sure it's better, but would allow reusing some spec terminology. That said,
the existing AST nodes don't reuse terminology (...otherwise
"VariableInitialization" would be called "LexicalBinding")

You could add a link to the spec too for easy look up. 

(yeah, I kind of love spec links :). But they're great in compilers to quickly
double check why something is implemented the way it is.)

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/precedence.dart
File lib/src/js/precedence.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/precedence.d...
lib/src/js/precedence.dart:37: const DESTRUCTURING = PRIMARY + 1;
hmm, not sure this really makes sense as a precedence level. Did it solve an
actual problem with printing or just added because most existing nodes have it?

My gut sense is it doesn't really make sense here, as it's not really an
expression, but a form of assignment/variable initialization.

Since you'd need something to fill in, I'd just leave the getters in the AST
nodes as returning PRIMARY. That will ensure no parens get added ever. & I'd put
a comment to that effect.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/template.dart
File lib/src/js/template.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/template.dar...
lib/src/js/template.dart:235: return error('Interpolated value #$nameOrPosition
($item: ${item?.runtimeType}) is not an Identifier'
long line.

also, printing the item & runtime type isn't a bad idea for debugging, but if
we're going to do it here, why not in all the other places?

I'd say keep it consistent one way or the other. (e.g. add a helper if
item/runtimeType is desired, and call it from all of the interpolation
places...)

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/template.dar...
lib/src/js/template.dart:853: return new ObjectDestructuring(makeVars.map((m) =>
m(arguments)).toList());
long line

[ochafik, 2015-12-02 19:48:21]

Description was changed from

==========
Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter is named 'super' (can't alias
it) or 'constructor' (or any other Object.prototype member: destructuring would
fetch it from the default {} opts param value even if not specified).
- Passing opts through in aliased constructors, both for speed/concision and
correctness (since default param value semantic is weird there)

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

to

==========
Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter clashes with reserved JS names
(see discussion in https://github.com/dart-lang/dev_compiler/issues/392)
- When a parameter clashes with a Object.prototype property, using a clean
default opts value (Object.create(null)).
- Passing opts through in aliased constructors, both for speed/concision and
correctness (since default param value semantic is weird there)

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

[ochafik, 2015-12-02 20:05:47]

Thanks a lot John!

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:930: node.parameters, allowDestructuring:
false);
On 2015/12/01 02:10:27, John Messerly wrote:
> yeah, FYI, this makes total sense.

Acknowledged.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1191: Map<FormalParameter, JS.Identifier>
paramNames =
On 2015/12/01 02:10:27, John Messerly wrote:
> 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.

Indeed, removed :-)

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1921: static final Set<String>
_jsObjectProperties = new Set<String>()..addAll([
On 2015/12/01 02:10:27, John Messerly wrote:
> 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.

Done.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1939: bool
_isDestructurableNamedParam(FormalParameter param) {
On 2015/12/01 02:10:27, John Messerly wrote:
> I think we can allow destructing always and just use Object.create(null) when
> needed

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

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1958: List<JS.Parameter>
_visitFormalParameterList(
On 2015/12/01 02:10:27, John Messerly wrote:
> for better or worse, we have an "_emit" naming convention for our own methods
> (different from the visit* inherited ones).

Done.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1964: var canDestructure = allowDestructuring &&
_willDestructureNamedParams(node);
On 2015/12/01 02:10:27, John Messerly wrote:
> long line

Acknowledged.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart
File lib/src/js/builder.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1156: finishVariableDeclarationList(keyword);
On 2015/12/01 02:10:27, John Messerly wrote:
> why introduce this redirect? If they aren't different, the two methods should
be
> merged. I'm fine with either name.

Done.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1163: switch (lastCategory) {
On 2015/12/01 02:10:27, John Messerly wrote:
> this pattern occurs in a couple of places, pull it out into a method?

+1, meet parseVariableBinding()

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1173: throw new StateError('Unexpected token $lastToken:
${categoryToString(lastCategory)}');
On 2015/12/01 02:10:27, John Messerly wrote:
> long line. FYI, to prevent large churn here we exclude js_ast from dartfmt, so
> it must be formatted manually for now.

Done.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1185: Destructuring parseDestructuring() {
On 2015/12/01 02:10:27, John Messerly wrote:
> It'd be worth adding a link to where this is defined in the ES6 spec:
>
http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-binding-pat...
> http://www.ecma-international.org/ecma-262/6.0/#sec-destructuring-assignment
> 
> Also good to comment that we only support restructuring in variable
> initialization at this time, in particular it's not supported in
> LeftHandSideExpression (although I have another comment on this below)
> 
> Probably worth adding a TODO that this is not a complete parsing
implementation
> for destructuring.

Done.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/builder.dart...
lib/src/js/builder.dart:1231: if (acceptCategory(COLON)) {
On 2015/12/01 02:10:27, John Messerly wrote:
> Hmmm, normally we'd do parseAssignment, like the old variable declaration code
> did. parseAssignment will parse the left-hand-side, which will include any
> nested destructurings.
> 
> The parser code here can be a little finicky, but in this case I think
> implementing destructuring support in parseLeftHandSide might actually
simplify
> some of the logic, by allowing more code reuse, and it would also support more
> use cases. WDYT?

Hehe, tried that, but... the grammar is a bit sensitive (`{x: []}` can be parsed
as a object literal *or* a ObjectBindingPattern depending on its location, e.g.
`({x: []} = y)` vs `let y = {x: []}`).

Keeping things unchanged for now, and can actually even remove the parsing if
you prefer, since I'm not using it anymore from js_codegen (I'm building the
ObjectBindingPattern manually).

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/nodes.dart#n...
lib/src/js/nodes.dart:763: abstract class Declarator extends Expression {
On 2015/12/01 02:10:27, John Messerly wrote:
> The ES6 spec calls this a "Binding", as in BindingIdentifier and
BindingPattern
> and LexicalBinding:
>
http://www.ecma-international.org/ecma-262/6.0/#sec-declarations-and-the-vari...,
> so maybe "VariableBinding"?
> 
> Not sure it's better, but would allow reusing some spec terminology. That
said,
> the existing AST nodes don't reuse terminology (...otherwise
> "VariableInitialization" would be called "LexicalBinding")
> 
> You could add a link to the spec too for easy look up. 
> 
> (yeah, I kind of love spec links :). But they're great in compilers to quickly
> double check why something is implemented the way it is.)

Done (thank you so much for all these links, I feel ashamed of being too lazy to
put them around at the first place :-S)

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/precedence.dart
File lib/src/js/precedence.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/precedence.d...
lib/src/js/precedence.dart:37: const DESTRUCTURING = PRIMARY + 1;
On 2015/12/01 02:10:27, John Messerly wrote:
> hmm, not sure this really makes sense as a precedence level. Did it solve an
> actual problem with printing or just added because most existing nodes have
it?
> 
> My gut sense is it doesn't really make sense here, as it's not really an
> expression, but a form of assignment/variable initialization.
> 
> Since you'd need something to fill in, I'd just leave the getters in the AST
> nodes as returning PRIMARY. That will ensure no parens get added ever. & I'd
put
> a comment to that effect.

Done.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/template.dart
File lib/src/js/template.dart (right):

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/template.dar...
lib/src/js/template.dart:235: return error('Interpolated value #$nameOrPosition
($item: ${item?.runtimeType}) is not an Identifier'
On 2015/12/01 02:10:27, John Messerly wrote:
> long line.
> 
> also, printing the item & runtime type isn't a bad idea for debugging, but if
> we're going to do it here, why not in all the other places?
> 
> I'd say keep it consistent one way or the other. (e.g. add a helper if
> item/runtimeType is desired, and call it from all of the interpolation
> places...)
> 

Done.

https://codereview.chromium.org/1484263002/diff/20001/lib/src/js/template.dar...
lib/src/js/template.dart:853: return new ObjectDestructuring(makeVars.map((m) =>
m(arguments)).toList());
On 2015/12/01 02:10:28, John Messerly wrote:
> long line

Done.

[Jennifer Messerly, 2015-12-02 20:41:09]

LGTM!

https://codereview.chromium.org/1484263002/diff/110001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1484263002/diff/110001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1998: {bool allowDestructuring}) {
default to true?

https://codereview.chromium.org/1484263002/diff/110001/lib/src/js/template.dart
File lib/src/js/template.dart (right):

https://codereview.chromium.org/1484263002/diff/110001/lib/src/js/template.da...
lib/src/js/template.dart:847: return new ArrayBindingPattern(makeVars.map((m) =>
m(arguments)).toList());
long line

https://codereview.chromium.org/1484263002/diff/110001/test/codegen/methods.dart
File test/codegen/methods.dart (right):

https://codereview.chromium.org/1484263002/diff/110001/test/codegen/methods.d...
test/codegen/methods.dart:23: clashWithJsReservedName({function}) => function;
nice test case :)

[ochafik, 2015-12-02 21:56:08]

Thanks!

https://codereview.chromium.org/1484263002/diff/110001/lib/src/codegen/js_cod...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1484263002/diff/110001/lib/src/codegen/js_cod...
lib/src/codegen/js_codegen.dart:1998: {bool allowDestructuring}) {
On 2015/12/02 20:41:09, John Messerly wrote:
> default to true?

Done.

https://codereview.chromium.org/1484263002/diff/110001/lib/src/js/template.dart
File lib/src/js/template.dart (right):

https://codereview.chromium.org/1484263002/diff/110001/lib/src/js/template.da...
lib/src/js/template.dart:847: return new ArrayBindingPattern(makeVars.map((m) =>
m(arguments)).toList());
On 2015/12/02 20:41:09, John Messerly wrote:
> long line

Done.

[ochafik, 2015-12-02 21:56:39]

Description was changed from

==========
Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter clashes with reserved JS names
(see discussion in https://github.com/dart-lang/dev_compiler/issues/392)
- When a parameter clashes with a Object.prototype property, using a clean
default opts value (Object.create(null)).
- Passing opts through in aliased constructors, both for speed/concision and
correctness (since default param value semantic is weird there)

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

to

==========
Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter clashes with reserved JS names
(see discussion in https://github.com/dart-lang/dev_compiler/issues/392)
- When a parameter clashes with a Object.prototype property, using a clean
default opts value (Object.create(null)).
- Passing opts through in aliased constructors, both for speed/concision and
correctness (since default param value semantic is weird there)

BUG=
R=jmesserly@google.com

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

[ochafik, 2015-12-02 21:56:40]

Committed patchset #8 (id:130001) manually as
c9d909ced98dea2aefe20322a59d18fbab2d93b2 (presubmit successful).