Skip to content

Update - #17

Merged
bakkot merged 3 commits into
tc39:masterfrom
jugglinmike:strict-fn-name
Mar 30, 2018
Merged

Update#17
bakkot merged 3 commits into
tc39:masterfrom
jugglinmike:strict-fn-name

Conversation

@jugglinmike

Copy link
Copy Markdown
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

bakkot commented Mar 30, 2018

Copy link
Copy Markdown
Member

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

bakkot commented Mar 30, 2018

Copy link
Copy Markdown
Member

Thanks!

@jugglinmike

Copy link
Copy Markdown
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