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

Remove tests that allow ...[ and ...{ in object destructuring #1050

Closed
rwaldron opened this issue May 23, 2017 · 17 comments
Closed

Remove tests that allow ...[ and ...{ in object destructuring #1050

rwaldron opened this issue May 23, 2017 · 17 comments
Assignees

Comments

@rwaldron
Copy link
Contributor

per resolution of 16.i.h https://github.com/tc39/agendas/blob/master/2017/05.md

I'll work on this during my next cycle

@rwaldron rwaldron self-assigned this May 23, 2017
@leobalter
Copy link
Member

leobalter commented May 23, 2017

by object destructuring, this means rest operators (not spread) directly in object patterns for destructuring will only be valid with name identifiers.

let [{a, ...[]}] = [{/*...*/}]; // invalid
let {a, ...[]} = {/*...*/}; // invalid
let {a, ...[b]} = {/*...*/}; // invalid
let {a, ...{}} = {/*...*/}; // invalid
let {a, ...{b}} = {/*...*/}; // invalid
let {a, ...b} = {/*...*/}; // valid
let {...b} = {/*...*/}; // valid

This needs to wait the respective proposal to be fixed and match the references.

@rwaldron
Copy link
Contributor Author

let {...[]} = {/*...*/}; // valid

I don't think that's correct. We need confirmation

This needs to wait the respective proposal to be fixed and match the references.

Everything I've reviewed so far has esid: pending

@leobalter
Copy link
Member

I don't think that's correct. We need confirmation

right you are, my copy-pasta fault.

Everything I've reviewed so far has esid: pending

pending means only we don't have the reference on what the link would be in the specs yet. Still, we require a minimum spec text to reference each case. Of course, providing this in the info tag helps a lot.

@leobalter
Copy link
Member

A few examples of test cases requiring a new review:

src/dstr-binding/obj-ptrn-rest-nested-obj.case
src/dstr-binding/obj-ptrn-rest-obj-nested-rest.case
src/dstr-binding/obj-ptrn-rest-obj-own-property.case

@rwaldron
Copy link
Contributor Author

pending means only we don't have the reference on what the link would be in the specs yet

I know, but all the cases I've seen said "esid: pending", so there is no reason to...

This needs to wait the respective proposal to be fixed and match the references.

...

A few examples of test cases requiring a new review:
src/dstr-binding/obj-ptrn-rest-nested-obj.case
src/dstr-binding/obj-ptrn-rest-obj-nested-rest.case
src/dstr-binding/obj-ptrn-rest-obj-own-property.case

Yes, already removed in a branch.

@rwaldron
Copy link
Contributor Author

I'll also include new negative syntax error tests.

rwaldron added a commit to rwaldron/test262 that referenced this issue Jun 23, 2017
leobalter added a commit that referenced this issue Jun 28, 2017
Remove tests that allow `...{` in object destructuring. Ref gh-1050
@gsathya
Copy link
Member

gsathya commented Jul 11, 2017

FYI, these are the obsolete tests that I had to disable in V8:
https://chromium-review.googlesource.com/c/550559/13/test/test262/test262.status

@rwaldron
Copy link
Contributor Author

@gsathya thanks, you should update to latest test262 to get these changes: 29e69dd

@littledan
Copy link
Member

Sorry, this is my fault on the late test262 update; I'll report any issues here if there are stray tests.

@rwaldron
Copy link
Contributor Author

@littledan no problem!

@gsathya
Copy link
Member

gsathya commented Jul 13, 2017

Thanks. If all the obsolete tests have been deleted, can this issue be closed now?

@littledan
Copy link
Member

Have any tests been added that check that the old syntax is no longer allowed? I just upgraded V8 to the new version of test262, and Sathya's patch to use the new semantics hasn't landed yet, but the test262 tests all pass.

@gsathya
Copy link
Member

gsathya commented Jul 13, 2017

Even with the latest roll, I had to skip a bunch of tests:
https://chromium-review.googlesource.com/c/550559/16/test/test262/test262.status

@littledan
Copy link
Member

Note that the spec is not yet complete here. There is an open PR at tc39/proposal-object-rest-spread#51 which seems to have consensus among implementers at least.

Andarist added a commit to Andarist/babylon that referenced this issue Aug 13, 2017
Andarist added a commit to Andarist/babylon that referenced this issue Aug 24, 2017
Andarist added a commit to Andarist/babylon that referenced this issue Aug 24, 2017
Andarist added a commit to Andarist/babylon that referenced this issue Aug 24, 2017
Andarist added a commit to Andarist/babylon that referenced this issue Aug 24, 2017
@gsathya
Copy link
Member

gsathya commented Sep 11, 2017

@leobalter Is this fixed now with #1187 ?

@leobalter
Copy link
Member

yes, I believe we're all set on this. Thanks for the reminder.

@adrianheine
Copy link
Contributor

@rwaldron Did you include those negative tests? I can't seem to find them.

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

5 participants