New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In variable declaration, in object destructuring, shorthand property with default should have true shorthand field #1459

Closed
zjmiller opened this Issue Jan 29, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@zjmiller
Contributor

zjmiller commented Jan 29, 2016

Test case:

> require('esprima').parse('var {a = b} = c;').body[0].declarations[0].id.properties[0]

Actual output:

{ type: 'Property',
  key: { type: 'Identifier', name: 'a' },
  computed: false,
  value: 
   { type: 'AssignmentPattern',
     left: { type: 'Identifier', name: 'a' },
     right: { type: 'Identifier', name: 'b' } },
  kind: 'init',
  method: false,
  shorthand: false }

Expected:

{ type: 'Property',
  key: { type: 'Identifier', name: 'a' },
  computed: false,
  value: 
   { type: 'AssignmentPattern',
     left: { type: 'Identifier', name: 'a' },
     right: { type: 'Identifier', name: 'b' } },
  kind: 'init',
  method: false,
  shorthand: true }
@ariya

This comment has been minimized.

Show comment
Hide comment
@ariya

ariya Jan 30, 2016

Contributor

Do you mean "...should have true shorthand field" instead?

Contributor

ariya commented Jan 30, 2016

Do you mean "...should have true shorthand field" instead?

@zjmiller

This comment has been minimized.

Show comment
Hide comment
@zjmiller

zjmiller Jan 30, 2016

Contributor

Right of course. I'll edit it.

Contributor

zjmiller commented Jan 30, 2016

Right of course. I'll edit it.

@zjmiller zjmiller changed the title from In variable declaration, in object destructuring, shorthand property with default should have false shorthand field to In variable declaration, in object destructuring, shorthand property with default should have true shorthand field Jan 30, 2016

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jan 30, 2016

Contributor

The shorthand value doesn't mean anything for this node, right? Can't it be either value?

Contributor

michaelficarra commented Jan 30, 2016

The shorthand value doesn't mean anything for this node, right? Can't it be either value?

@ariya

This comment has been minimized.

Show comment
Hide comment
@ariya

ariya Jan 30, 2016

Contributor

It should be consistent with the case without AssignmentPattern. Compare to this (correct):

> require('esprima').parse('var {x} = y;').body[0].declarations[0].id.properties[0]
{ type: 'Property',
  key: { type: 'Identifier', name: 'x' },
  computed: false,
  value: { type: 'Identifier', name: 'x' },
  kind: 'init',
  method: false,
  shorthand: true }
Contributor

ariya commented Jan 30, 2016

It should be consistent with the case without AssignmentPattern. Compare to this (correct):

> require('esprima').parse('var {x} = y;').body[0].declarations[0].id.properties[0]
{ type: 'Property',
  key: { type: 'Identifier', name: 'x' },
  computed: false,
  value: { type: 'Identifier', name: 'x' },
  kind: 'init',
  method: false,
  shorthand: true }
@zjmiller

This comment has been minimized.

Show comment
Hide comment
@zjmiller

zjmiller Jan 30, 2016

Contributor

The shorthand value is the only thing distinguishing var {a = b} = c; from var {a: a = b} = c;. These behave the same, but I think it makes sense for a parser to focus on the syntax and pretty much ignore behavior. Also, there's something to be said for uniformity. Esprima, in assignment expressions, gives the property in {a = b} a shorthand value of true. Acorn, in both assignment expressions and variable declarations, gives the property in {a = b} a shorthand value of true. Given this, it's odd for Esprima, in variable declarations, to give the property in {a = b} a shorthand value of false.

This kind of inconsistency can make life harder for downstream projects. For example, escodegen, when generating a property, looks at the shorthand value to determine whether to write just the key field or the key field, a colon, and the value field. So, giving var {a = b} = c; to Esprima, and giving the AST to escodgen, results in the string var {a: a = b} = c;. The false shorthand field means escodgen adds unnecessary characters here (and doesn't do so when given the Acorn AST).

Contributor

zjmiller commented Jan 30, 2016

The shorthand value is the only thing distinguishing var {a = b} = c; from var {a: a = b} = c;. These behave the same, but I think it makes sense for a parser to focus on the syntax and pretty much ignore behavior. Also, there's something to be said for uniformity. Esprima, in assignment expressions, gives the property in {a = b} a shorthand value of true. Acorn, in both assignment expressions and variable declarations, gives the property in {a = b} a shorthand value of true. Given this, it's odd for Esprima, in variable declarations, to give the property in {a = b} a shorthand value of false.

This kind of inconsistency can make life harder for downstream projects. For example, escodegen, when generating a property, looks at the shorthand value to determine whether to write just the key field or the key field, a colon, and the value field. So, giving var {a = b} = c; to Esprima, and giving the AST to escodgen, results in the string var {a: a = b} = c;. The false shorthand field means escodgen adds unnecessary characters here (and doesn't do so when given the Acorn AST).

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jan 30, 2016

Contributor

Makes sense. 👍

Contributor

michaelficarra commented Jan 30, 2016

Makes sense. 👍

@ariya

This comment has been minimized.

Show comment
Hide comment
@ariya

ariya Jan 30, 2016

Contributor

Yup, I agree. I'm all for consistency 👍

Contributor

ariya commented Jan 30, 2016

Yup, I agree. I'm all for consistency 👍

@ariya ariya closed this in 37ec714 Jan 31, 2016

ariya added a commit to ariya/esprima that referenced this issue Feb 3, 2016

Fix shorthand object destructuring defaults in variable declarations
In a variable declaration, in an object destructuring pattern, a shorthand property with a default value was being incorrectly parsed as having a false shorthand field. For example, the shorthand property in `var {a = b} = c;` was being incorrectly parsed as having a false shorthand field.

Fixes #1459
Closes gh-1458
@ariya

This comment has been minimized.

Show comment
Hide comment
@ariya

ariya Feb 3, 2016

Contributor

For completeness: I've backported this and the new Esprima 2.7.2 also contains this fix.

Contributor

ariya commented Feb 3, 2016

For completeness: I've backported this and the new Esprima 2.7.2 also contains this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment