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

Validate fn "name" when enforcing strict mode #381

Open
wants to merge 1 commit into
base: es2016
Choose a base branch
from

Conversation

jugglinmike
Copy link

Commit message:

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

Would you folks like me to also add tests directly into this project?

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
@jugglinmike
Copy link
Author

The project's linting rules disallow re-assignment of function parameters, and this patch doesn't satisfy that restriction. However, there are a number of previously-existing infractions, so I'm not sure if the linting rules are still being enforced.

@bakkot
Copy link
Member

bakkot commented Mar 30, 2018

The build is failing because you'll need to add yourself to the contributors file here (sorry for the hassle).

I think this will probably need to wait until the 2019 branch, separately, since this was a normative change which didn't occur until after the 2018 spec was cut. (We track yearly releases, rather than trying to match the current spec at any given time.)

@jugglinmike
Copy link
Author

The build is failing because you'll need to add yourself to the contributors
file here (sorry for the hassle).

No worries. I'm no stranger to frustrating CLA overhead, myself. Signature offered here: shapesecurity/CLA#28

I think this will probably need to wait until the 2019 branch, separately,
since this was a normative change which didn't occur until after the 2018
spec was cut. (We track yearly releases, rather than trying to match the
current spec at any given time.)

That's fine by me, as long as I know the patch is otherwise acceptable, I'll leave the merging in your hands.

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.

None yet

2 participants