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

Allow duplicate __proto__ keys in patterns, simple case (#6705) #10532

Merged
merged 4 commits into from Oct 14, 2019

Conversation

alejo90
Copy link
Contributor

@alejo90 alejo90 commented Oct 9, 2019

Q                       A
Fixed Issues? Fixes #6705
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

This is a fix for an error being thrown when there is a duplicate __proto__ in a destructuring pattern. It only fixes the simple case when the pattern is immediately followed by a =. I renamed the checkPropClash function to checkDuplicatedProto, as advised in the issue. Instead of relying exclusively on a boolean flag to determine whether __proto__ had appeared before or not, I store the amount of times it has appeared and the position of the first duplicated occurrence. In parseObj, I check for the existence of the first duplicated occurrence property I stored before and raise an error if it's there and the next token is not a =.
Please let me know if there are any concerns or improvements I can make. I'm thinking about the more complicated case as well but I'll need a little more time with the code before I can tackle it.
Thanks @nicolo-ribaudo for the hints on how to get started.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Oct 9, 2019

We also need to update the test262 whitelist. Could you run make bootstrap-test262 and make test-test262-update-whitelist?

@nicolo-ribaudo nicolo-ribaudo added pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories labels Oct 9, 2019
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR we have fixed the simple case of #6705, I suggest we submit a new issue to cover the following cases:

// argument destructuring
({ __proto__: x, __proto__: y }) => {}
// forX destructuring
for ({ __proto__: x, __proto__: y } of []);
// nested destructuring
({ a: { __proto__: x, __proto__: y } } = { a: {} }) 

@nicolo-ribaudo
Copy link
Member

@JLHwung I have repoened the issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should allow duplicate __proto__ keys in patterns
4 participants