Update - #17
Merged
Merged
Conversation
The modified test material was introduced via commit 73c001c on 2017-04-26. According to this project's `licenses.md` file, it originated from the Acorn project. At that that date, the tests containing the relevant productions were labeled as negative syntax tests. However, the tests were introduced to this project as positive syntax tests. Although the new interpretation was technically correct in terms of the ECMA262 specificatoin at that time, TC39 reached consensus in March 2018 to classify these productions as syntax errors. Re-locate the relevant tests to align with the decision. Note that due to the discrepency described above, no change to the source project is necessary.
These tests reflect the consensus reached by TC39 in March 2018.
jugglinmike
added a commit
to jugglinmike/shift-parser-js
that referenced
this pull request
Mar 30, 2018
TC39 reached consensus in March 2018 to extend the definition of the term "function code" to include the BindingIdentifier informally referred to as the function's "name" in those productions which include it [1]. This has the effect of restricting the set of valid identifiers for functions whose bodies include a "use strict" directive. Because the project's test suite does not include tests for this case, no change to existing test material is necessary. However, the `test262-parser-tests` project *does* require modification, so this patch should not be applied until that project has been updated accordingly [2]. [1] tc39/ecma262#1158 [2] tc39/test262-parser-tests#17
Member
|
Thanks for the patch!
Yup. I used upstream tests as a source, but did my own classification.
No, but we can start doing so. I'll push a commit to your branch updating the licenses file (which needs to be updated in light of these tests being moved anyway). |
Member
|
Thanks! |
Contributor
Author
|
My pleasure! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi @bakkot,
We elected to change the specification on the interpretation of function names this month, and I've recently submitted a spec patch to do just that. I wanted to follow up by reflecting that decision in this project.
It turns out that we had coverage for the old expected behavior. This made my job easy: I just moved the files. The problem is that these tests originate from the Acorn project. I thought this would mean that Acorn also needed to be updated (in contrast to my prior research), but it turns out that the relevant tests in Acorn already classify these tests as valid syntax. It seems likely that they were intentionally re-classified in order to match the specification at the time they were imported, but I wanted to draw your attention to this in order to be sure (and to explain why no change in Acorn seems to be necessary).
(I've included the script I used to research this below.)
I've also added a few more tests to improve coverage. Have you accepted novel test material in this project? Or should I be landing the new tests in one of the upstream projects first?
Commit message:
Research script:
And the output: