Skip to content
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

Missing coverage: computed properties as shorthand props #1202

Closed
mroch opened this issue Aug 29, 2017 · 3 comments
Closed

Missing coverage: computed properties as shorthand props #1202

mroch opened this issue Aug 29, 2017 · 3 comments

Comments

@mroch
Copy link
Contributor

mroch commented Aug 29, 2017

The Flow parser incorrectly allowed object literals like {0} and {[x]} because it parsed the property's key and treated it as a shorthand definition when it didn't find a value.

The first case is covered already, the latter seems not to be. On one hand, it's not reasonable to test every bogus thing someone could write, but I think this one is worth testing because it's so similar to other property keys.

({x})    // covered by language/expressions/object/prop-def-id-valid.js
({0})    // covered, indirectly, by while/if/do-while tests
({true}) // covered by language/reserved-words/ident-reference-false.js
({[x]})  // not covered
facebook-github-bot pushed a commit to facebook/flow that referenced this issue Aug 30, 2017
Summary:
`({0})` is not valid. the PropertyDefinition is not a valid
IdentifierReference because it's a number literal, nor is it a valid
PropertyName because there is no value.

see also tc39/test262#1202

Reviewed By: gabelevi

Differential Revision: D5732586

fbshipit-source-id: a6bd2817ccba9463692fa94e64e2bdc1a64b735b
@leobalter
Copy link
Member

While I might agree with verifying {0} or {[x]}, but I'm not totally fond of it. I understand it might cover a regression in a specific implementation, but I'm afraid I don't have anything specific to cover in the spec for those.

This might scale to the uncertainty of every possibility we can use to match. {0} and {[x]} are just starters.

This doesn't mean I'm against this, I'll try my best to make a fit for these cases into Test262, somehow I want to add them to this test suite.

@mroch
Copy link
Contributor Author

mroch commented Sep 12, 2017

yeah, agreed that it's not reasonable to test random invalid stuff. i think the only justification here is that it's a valid computed property key when it's followed by a value, but not alone. things that are neither valid keys nor valid shorthand seem definitely out of scope. I won't be upset if you just close this, though.

@leobalter
Copy link
Member

I think I can do this.

Starting from https://tc39.github.io/ecma262/#prod-ObjectLiteral:

PropertyDefinition:
  IdentifierReference
  ...
  PropertyName : AssignmentExpression

we need to distinguish PropertyName - that requires being followed by an AssignmentExpression - from an restricted IdentifierReference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants