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

Update #17

Merged
merged 3 commits into from
Mar 30, 2018
Merged

Update #17

merged 3 commits into from
Mar 30, 2018

Conversation

jugglinmike
Copy link
Contributor

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:

Relocate tests for BindingIdentifier of strict fns

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.

Research script:

#!/bin/bash

test_files='
pass/050a006ae573e260.js
pass/2c0f785914da9d0b.js
pass/574ea84fc61bdc31.js
pass/6c4fe38464c16309.js
pass/8643da76fe7e95c7.js
pass/e0c3d30b6fe96812.js'

for test_file in $test_files; do
  echo $test_file:
  git log --format='%h %cd %s' -- $test_file
done

grep -E -e $(echo $test_files | sed 's/ /|/g') -e '^##' licenses.md

(
  cd ../acorn
  git checkout $(git log -n 1 --format='%h' --since 2017-04-26 master)
  grep -E 'function\s+static' test/tests.js
)

And the output:

pass/050a006ae573e260.js:
73c001c Wed Apr 26 15:41:55 2017 -0700 Switch names to slugs
pass/2c0f785914da9d0b.js:
73c001c Wed Apr 26 15:41:55 2017 -0700 Switch names to slugs
pass/574ea84fc61bdc31.js:
73c001c Wed Apr 26 15:41:55 2017 -0700 Switch names to slugs
pass/6c4fe38464c16309.js:
73c001c Wed Apr 26 15:41:55 2017 -0700 Switch names to slugs
pass/8643da76fe7e95c7.js:
73c001c Wed Apr 26 15:41:55 2017 -0700 Switch names to slugs
pass/e0c3d30b6fe96812.js:
73c001c Wed Apr 26 15:41:55 2017 -0700 Switch names to slugs
## Acorn
* pass/050a006ae573e260.js
* pass/2c0f785914da9d0b.js
* pass/574ea84fc61bdc31.js
* pass/6c4fe38464c16309.js
* pass/8643da76fe7e95c7.js
* pass/e0c3d30b6fe96812.js
## Esmangle
## Esprima
## Shift
## UglifyJS2
testFail("function static() { \"use strict\"; }",
testFail("\"use strict\"; function static() { }",

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
@bakkot
Copy link
Collaborator

bakkot commented Mar 30, 2018

Thanks for the patch!

It seems likely that they were intentionally re-classified in order to match the specification at the time they were imported

Yup. I used upstream tests as a source, but did my own classification.

Have you accepted novel test material in this project?

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).

@bakkot bakkot merged commit cd1d1da into tc39:master Mar 30, 2018
@bakkot
Copy link
Collaborator

bakkot commented Mar 30, 2018

Thanks!

@jugglinmike
Copy link
Contributor Author

My pleasure!

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

Successfully merging this pull request may close these issues.

2 participants