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

Duplicate parameters of functions whose bodies are strict #632

Closed
bakkot opened this issue Jul 6, 2016 · 15 comments · Fixed by #1137
Closed

Duplicate parameters of functions whose bodies are strict #632

bakkot opened this issue Jul 6, 2016 · 15 comments · Fixed by #1137

Comments

@bakkot
Copy link
Contributor

bakkot commented Jul 6, 2016

It looks to me that function declarations whose bodies are strict but which are not within strict code are permitted to have duplicated parameter names, and I suspect this is a mistake.

We have the following partial definition for strict mode code:

Code is interpreted as strict mode code in the following situations: [...]

Function code is strict mode code if the associated FunctionDeclaration [...] is contained in strict mode code or if the code that produces the value of the function's [[ECMAScriptCode]] internal slot begins with a Directive Prologue that contains a Use Strict Directive.

Function code is source text that is parsed to supply the value of the [[ECMAScriptCode]] and [[FormalParameters]] internal slots (see 9.2) of an ECMAScript function object.

We also have these Early Errors for FunctionDeclarations:

FunctionDeclaration : function BindingIdentifier ( FormalParameters ) { FunctionBody }
If the source code matching this production is strict mode code, the Early Error rules for StrictFormalParameters : FormalParameters are applied.

StrictFormalParameters : FormalParameters
It is a Syntax Error if BoundNames of FormalParameters contains any duplicate elements.

As I read this, "the source code matching this production [i.e. FunctionDeclaration : function BindingIdentifier ( FormalParameters ) { FunctionBody }]" is not strict mode code just because its body contains a Use Strict Directive; only the FormalParameters and FunctionBody parts are. As such, the the Early Error rules for StrictFormalParameters : FormalParameters are not applied.

Hence, a Script like function foo(a, a){'use strict'} would not have any Early Errors.

This seems wrong, and implementations do indeed enforce this Early Error. Either way, I think it would be good to have a NOTE here clarifying whether the strict-mode Early Errors for parameters and function names are intended to apply to function declarations (and expressions) which contain a Use Strict Directive but are not themselves inside of strict mode code.

See related issue previously. (Also, when are we going to get the Bugzilla cert fixed?)

@bakkot bakkot changed the title Strict mode, function code, and parameters Duplicate parameters of functions whose bodies are strict Jul 6, 2016
@getify
Copy link
Contributor

getify commented Jul 6, 2016

IANASA (I am not a spec author), but...

Function code is strict mode code if ... the code that produces the value of the function's [[ECMAScriptCode]] internal slot begins with a Directive Prologue that contains a Use Strict Directive.

I read that to say that the entire function ("function code") is considered strict if its body is strict (regardless of surrounding code). Therefore, I would assume the strict mode early error would apply to the param list of such a function.

Defining "function code" as "the entire function" is backed up by:

Function code is source text that is parsed to supply the value of the [[ECMAScriptCode]] and [[FormalParameters]] internal slots (see 9.2) of an ECMAScript function object.

IOW, "function code" is not just the body, but also its parameter list.

@bakkot
Copy link
Contributor Author

bakkot commented Jul 6, 2016

@getify, that's approximately true, and the formal parameters are indeed strict mode code since they are a part of the function code.

However, the conditions for this Early Error to be applied are not that the FormalParameters are strict code, but rather

If the source code matching this production is strict mode code

where "this production" is the FunctionDeclaration production, which is not strict code as I read it.

Note that the FunctionDeclaration production includes a BindingIdentifier which is not "source text that is parsed to supply the value of the [[ECMAScriptCode]] and [[FormalParameters]] internal slots", and as such is not "function code" and therefore not strict mode code. As such, the full FunctionDeclaration production can't be either. I believe this is backed up by the discussion on the Bugzilla bug I linked.

@allenwb
Copy link
Member

allenwb commented Jul 6, 2016

@bakkot The intent of that spec. language (I wrote it) is that function foo(a, a){'use strict'} produces an early error.

As @getify points out, the formal parameters are port of the "function code". Perhaps a clarify note would help.

Also see https://github.com/tc39/tc39-notes/blob/master/es7/2015-07/july-29.md#conclusionresolution for a change that was made in ES2016

@bakkot
Copy link
Contributor Author

bakkot commented Jul 6, 2016

@allenwb, good to know that this is indeed intended to be an error. It still looks to me like this is not what the spec implies, for the reason I give above; namely, the condition for the error to be applied is not that the parameters are strict mode code but rather that the entire FunctionDeclaration production is strict mode code (which, from what I can tell, it is not), despite the unfortunate naming of StrictFormalParameters.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 6, 2016

FWIW, I agree with @bakkot's reading.

@allenwb
Copy link
Member

allenwb commented Jul 6, 2016

The intended reading is the same as @getify 's. Since there is confusion about this, the spec. language should be improved. I probably used "source code matching" instead of "function code" out of concern for possible phasing issues.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 6, 2016

The intended reading is the same as @getify 's.

(Except that @getify didn't supply a reading for the early error in question.)

I probably used "source code matching" instead of "function code" out of concern for possible phasing issues.

I don't think we'd need to reverse that choice. To achieve the intended effect for this early error, we could replace:

    the source code matching this production is strict mode code

with:

    the source code matching /FormalParameters/ is strict mode code

Note that there are three other early errors that use the problematic phrasing.

@allenwb
Copy link
Member

allenwb commented Jul 6, 2016

the source code matching /FormalParameters/ is strict mode code

That seems equally confusing since is may be the presence of a use strict directive in the FunctionBody that causes the FormalParameters to be strict mode code.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 6, 2016

I don't see why it'd be confusing: the wording clearly requires the reader to understand the circumstances that would cause FormalParameters to be strict mode code, and the link will send them straight to 10.2.1, where they should discover that one such circumstance is the presence of a USD in the FunctionBody. (And if you think that discovery wouldn't be obvious enough, then isn't that 10.2.1's problem?)

But we could equivalently say

the source code matching /FunctionBody/ is strict mode code

if you think that'd be less confusing.

@nenionoda
Copy link

Have I understood it correctly then, that FunctionDeclaration (especially including its BindingIdentifier) would be strict mode code if its FunctionBody is strict ( i.e., starts with a use strict directive)?

@nenionoda
Copy link

nenionoda commented Jul 7, 2016

I do know the FormalParameters will be "strict"ly treated in presence of a use strict directive in FunctionBody. Below merely refers to my previous comment:

I'm becoming almost certain that I'm wrong, i.e., I'm almost believing that "the source text matching" a FunctionDeclaration (as a whole) will not be strict mode code if its corresponding FunctionBody contains a use strict. I'm especially asking this because I found this in the spec:

FunctionDeclaration:functionBindingIdentifier(FormalParameters){FunctionBody}
FunctionDeclaration:function(FormalParameters){FunctionBody}
FunctionExpression:functionBindingIdentifier(FormalParameters){FunctionBody}
...
If the source code matching this production is strict mode code[<- when would it happen?], it is a Syntax Error if BindingIdentifier is the IdentifierName eval or the IdentifierName arguments.

I used to think this means that below would not be valid:

function eval() { "use strict;" }

Then again though, I was told that a use strict directive only applies to the "function code", which is made up of the parameters and the function body only -- the BindingIdentifier has not been mentioned there.

This made me wonder for some time about what other cases would make "the source text matching" a FunctionDeclaration be strict mode code, and among the very few scenarios I found was when the FunctionDeclaration is directly contained in the body of a strict function, or is in a module, i.e., it is being parsed as a top level source element, with a source of type "module"; summing up:

function eval() { 
   "use strict"; // I used to think this would be an error; looks like I've been wrong

}
function eval() {
  // ceratinly **not** an error

}
function Strict() {
    "use strict";
    function eval() {
         // will be an error, 'eval' is the 'BindingIdentifier' of this function,
         // while it is surrounded by strict mode code
    }
}

To add to the confusion, take a look at these two, both from the spec:

Function code is strict mode code if the associated FunctionDeclaration, FunctionExpression,
GeneratorDeclaration, GeneratorExpression, MethodDefinition, or ArrowFunction is contained in strict mode code or if the code that produces the value of the function’s [[ECMAScriptCode]] internal slot begins with a Directive Prologue that contains a Use Strict Directive.

but

Function code is source text that is parsed to supply the value of the [[ECMAScriptCode]] and
[[FormalParameters]] internal slots (see 9.2) of an ECMAScript function object. The function code of a particular ECMAScript function does not include any source text that is parsed as the function code of a nested FunctionDeclaration, FunctionExpression, GeneratorDeclaration, GeneratorExpression, MethodDefinition, ArrowFunction, ClassDeclaration, or ClassExpression.

Reading it again, The function code of a particular ECMAScript function does not include any source text that is parsed as the function code of a nested FunctionDeclaration, FunctionExpression, GeneratorDeclaration, GeneratorExpression, MethodDefinition, ArrowFunction, ClassDeclaration, or ClassExpression.

In my humble opinion, the former quotation implies this must be an error:

function Strict() {
   'use strict';
   function HowShouldTheParamsBeTreatedNow(a,a) {} // should it be an error?
}

The latter though, in my humble opinion, implies quite the contrary.

I hope I've been wrong believing I've been wrong.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 7, 2016

To resolve the disagreement, I think you need to reconsider the conclusions you draw from the latter quotation. It's true that the parameter-list of the inner function is part of its function code, and not part of the function code of the outer function. But that doesn't prevent the strictness of the outer function from descending to the inner.

@jmdyck
Copy link
Collaborator

jmdyck commented Jul 7, 2016

Have I understood it correctly then, that FunctionDeclaration (especially including its BindingIdentifier) would be strict mode code if its FunctionBody is strict ( i.e., starts with a use strict directive)?

That's not what the spec says, but I gather that's how some implementations behave. It might help to read https://bugs.ecmascript.org/show_bug.cgi?id=4243

@nenionoda
Copy link

@jmdyck Thanks a lot! I must stand corrected then :)

@jugglinmike
Copy link
Contributor

@bakkot It seems like your reading and @jmdyck's proposed modification are in-line with both implementations and the intent of the original author (for what it's worth: I prefer the second phrasing). If that's right, then we already have consensus and can move forward with the patch to the spec. Would either you or @jmdyck like to file a pull request? I'd be happy to do the honors if that would be helpful--let me know.

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 a pull request may close this issue.

6 participants