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

Normative: Expand text included in "function code" #1091

Closed
wants to merge 1 commit into from

Conversation

jugglinmike
Copy link
Contributor

Previously reported at https://bugs.ecmascript.org/show_bug.cgi?id=4243 This patch is at attempt to implement the solution agreed upon there. (I think this issue was simply overshadowed by a related discussion on non-simple parameter lists, which ultimately lead to the restriction introduced in ES2016.)

The current specification text allows both of the following forms:

function static() { 'use strict'; }
(function static() { 'use strict'; });

Existing implementations violate that text by producing parsing errors for both.

With this patch applied, only the FunctionExpression will be disallowed, so implementations will continue to be in violation of the spec.

@ljharb ljharb added the normative change Affects behavior required to correctly evaluate some ECMAScript source text label Feb 4, 2018
@bterlson
Copy link
Member

bterlson commented Feb 5, 2018

@ljharb @bmeck either of you want to take a stab at reviewing this? I don't want to pull this for 2018, so it will sit here at least until I create the 2018 branch later this week.

@bmeck bmeck self-requested a review February 5, 2018 22:13
@bmeck
Copy link
Member

bmeck commented Feb 5, 2018

I'll get it done in next few days.

@bakkot
Copy link
Contributor

bakkot commented Feb 5, 2018

This looks good to me, but there's a closely related issue (#632) which this doesn't resolve. Actually, it maybe exacerbates that issue: if this is intended to resolve the bugzilla bug by making function eval() { 'use strict'; } unambiguously not an error, that would imply function f(a, a) { 'use strict'; } would also unambiguously not be an error. That seems wrong.

Possibly we should fix #632 by some other means, but I'd be OK rewording the first two early errors here as part of this PR.

There's also some discussion of this problem with @RReverser at tc39/test262-parser-tests#8 (comment).

@bakkot
Copy link
Contributor

bakkot commented Feb 5, 2018

By the way, this is probably needs-consensus: if I recall correctly, some people (maybe @ajklein?) were in favor of function static() { 'use strict'; } being an error, since it already is in all engines.

@bmeck
Copy link
Member

bmeck commented Feb 5, 2018

@jugglinmike is there a reason this isn't being applied to FunctionDeclarations as well. The main note I see is https://bugs.ecmascript.org/show_bug.cgi?id=4243#c8 which I'm not quite sure I agree on some things being related to different scopes and differing in behavior based upon if they are used as an expression vs a declaration. In particular, the early error would not be related to the hoisted binding that gets applied to a sloppy outer scope, so I'm not sure the two concepts should be related.

@jugglinmike
Copy link
Contributor Author

if this is intended to resolve the bugzilla bug by making function eval() { 'use strict'; } unambiguously not an error, that would imply function f(a, a) { 'use strict'; } would also unambiguously not be an error. That seems
wrong.

@bakkot That was my intent. It doesn't seem wrong to me because as per this
patch, I consider the "function code" contour to be different for expressions
and declarations.

But to @bmek's point, it sounds like that is a decision from the BugZilla
discussion that we're revisiting here. If that's right, then we should decide
if it's better to have a simple definition of "function code" or one that more
closely aligns with the runtime semantics.

@bakkot
Copy link
Contributor

bakkot commented Feb 6, 2018

It doesn't seem wrong to me because as per this patch, I consider the "function code" contour to be different for expressions and declarations.

@jugglinmike, the thing I'm pointing at here is that the same wording prohibits function eval() { 'use strict'; } as prohibits function f(a, a) { 'use strict'; } (note the duplicate parameter), and I think we definitely do want to keep the second one of these as an error regardless of how we come down on the function name issue.

@jugglinmike
Copy link
Contributor Author

@bakkot I believe the proposed modifications for each of these issues makes them orthogonal:

It seems as though gh-632 can be resolved with a change dedicated to that issue (which I would be happy to write).

Copy link

@waldemarhorwat waldemarhorwat left a comment

Choose a reason for hiding this comment

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

This could use clarification. I have no idea what this note is trying to say or why it's here.

@jugglinmike
Copy link
Contributor Author

I raised this at the March 2019 TC39 meeting (slides available here), and the committee elected to change the specification to match current implementation status--that is, to unconditionally include the binding identifier in the definition of "function code". This more restrictive interpretation matches the majority of implementations today, but requires a different change to the specification. I'll follow up with a modified proposal within the next few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants