Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Implement const/let pattern declarations (ES6 destructuring) #1967

Closed
wants to merge 5 commits into from
Closed

Implement const/let pattern declarations (ES6 destructuring) #1967

wants to merge 5 commits into from

Conversation

dsgkirkby
Copy link
Contributor

This should resolve #415.

Details

Relevant part of the spec here: https://www.ecma-international.org/ecma-262/6.0/#sec-let-and-const-declarations-runtime-semantics-evaluation

For // 3. Let env be the running execution context’s LexicalEnvironment., I assumed that the env variable passed to the function already fulfills and it's not necessary to retrieve it here.

I've omitted the ReturnIfAbrupt(Value) steps from the spec, as this is also done for the rest of the implementation in this file.

Testing

test262 has one additional test pass as a result of this change, though somewhat surprisingly it's for for.
I've verified in the command line REPL that destructuring assignment works as expected in the basic case.

@NTillmann
Copy link
Contributor

Looks great!

Please fix the Flow error...
In scripts/test262-runner.js, please update this line to reflect increased test passes:

if (!args.filterString && (numPassedES5 < 11738 || numPassedES6 < 3981 || numTimeouts > 0)) {

Also, take a closer look at that file... There are various pattern matching heuristics in place to filter out entire chunks of the test262 test suite that didn't use to work, including lots of destructuring tests. Maybe some of those can be enabled now?

@dsgkirkby
Copy link
Contributor Author

I tried removing the filter for destructuring-binding (results for ES6 tests only):

All tests

Master: 5427 / 6116 (689 failed - 92.9% pass rate)
Branch: 5684 / 6116 (432 failed - 91.6% pass rate)

Added tests only

Master: 1168 / 1468 (300 failed - 79.6% pass rate)
Branch: 1389 / 1468 (79 failed - 97.1% pass rate)

I haven't dug into exactly what's failing but I think that's probably good enough to enable these tests?

@dsgkirkby
Copy link
Contributor Author

Update:

  • It seems that circleci runs less tests than locally so I've lowered the expected passes to match how many pass there
  • The added failures are due to lexical environments being leaked, which looks like it might be an issue with generators - those failures are happening on master for some generator-related tests

Copy link
Contributor

@NTillmann NTillmann left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot for doing this.

Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

Implement Destructuring
3 participants