Normative: Extend definition of "function code"

[jugglinmike]

Commit message:

In the March 2018 meeting of TC39, the committee elected to extend the
definition of "function code" to include the BindingIdentifier of the
productions where a BindingIdentifier is present. This decision was motivated
by a desire to match implementation reality.

This alteration does not require a change to Test262 because like the
majority of modern implementations, Test262 already enforces the new
behavior [1].

[1] tc39/test262#1404

I don't know the best way to specify this, though I have a feeling that what I'm proposing here isn't it. The phrase "any BindingIdentifier present in the grammar production" is somewhat vague, but including a specific list of productions would be pretty verbose, especially considering the existing list of productions in this paragraph.

We could formalize it a bit by using an abstract operation. It's tempting to use HasName, but that's only valid for expressions. We'd need to either extend HasName (maybe refactor as "FunctionName" which may return null) or define a new operation (and then write something like, "If HasName returns true for the production, the BindingIdentifier is also included.").

But that's a lot of change for such a minor detail, especially considering that most implementers have taken it for granted.

I think @bterlson and @bakkot are the most likely people to care about this, but I'm happy to take input from anyone!

[jugglinmike]

For completeness, here's some prior discussion:

• Original issue report - https://bugs.ecmascript.org/show_bug.cgi?id=4243
• Prior proposal - Normative: Expand text included in "function code" #1091

And here are some patches to ensure implementations honor the new language:

• Update to test262-parser-tests - Update test262-parser-tests#17
• Update to shapesecurity/shift-parser-js - Validate fn "name" when enforcing strict mode shapesecurity/shift-parser-js#381

[bakkot]

I don't know the best way to specify this, though I have a feeling that what I'm proposing here isn't it. The phrase "any BindingIdentifier present in the grammar production" is somewhat vague, but including a specific list of productions would be pretty verbose, especially considering the existing list of productions in this paragraph.

Agreed that this is a bit ambiguous: there's no obvious referent for "the grammar production".

My preference would be to do something like #1137, except to make the clauses which say "source code matching |BindingIdentifier|" instead read "source code matching |FunctionBody|".

[jugglinmike]

Ah, with an approach like that, I can now appreciate why you considered this issue so closely tied to gh-632.

The text would have be a little more involved, though. It's not enough to say, "[...] the Early Error rules for BindingIdentifier are applied," because (unlike the rules for UniqueFormalParameters) the rules we're interested in are only relevant for strict mode code. We'd need to also stipulate that the rules be interpreted as if the code was strict mode code.

At that point, though, it starts to feel like we're effectively extending the definition of "strict mode code" but using an informal/indirect mechanism. I'm more inclined to extend the definition directly, even though I'm still not sure how best to do that. What do you think?

[jugglinmike]

I'm still curious to hear what you think, @bakkot :)

[bakkot]

Sorry for completely failing to follow up on this for two months! Belated thoughts:

What do you think?

Yeah, I'd missed the subtlety that it is not enough to apply the rules, because the rules themselves switch on the strictness of the BindingIdentifier. I agree that rules out my original suggestion.

What do you think about changing the definition of function code to something like

Function code is source text that is provided as a FunctionDeclaration, FunctionExpression, [...] or [...]. 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, [...] or [...].

(The "provided as a GrammarProduction" wording is already used in the definition of "module code".) That's more explicit, and I think it works just as well: of the 39 uses of "function code" in the spec,

• 5 are its definition
• 2 are defining "strict mode code" in terms of "function code"
• 25 are of the form "If the function code for [sometimes 'for this'] AsyncArrowProduction [or other function production] is strict mode code, let strict be true. Otherwise, let strict be false."
• 7 are in the non-normative introduction to B.3.3.

---

I think the wording in B.3.3 might need to be adjusted a little if we went with my suggestion here, if we wanted to be precise (because it would then no longer ever be the case that "[a FunctionDeclaration] occurs within the function code of an enclosing function", strictly speaking). Though, that's only the case because of the "does not include..." in the definition, which I don't understand the purpose of to begin with: the term "function code" is only used to determine what code is strict mode code, and strictness of a given function implies strictness of all contained functions, so why bother excluding the contained functions from the "function code" of the outer one?

[ljharb]

If it's excluded, how does that impact Function toString?

[bakkot]

It doesn't. Function.prototype.toString is not defined in terms of "function code".

Now that you mention it, though, I've gone and looked at the Function.prototype.toString revision, and I see it adds a definition for "the source text matched by a grammar production", so perhaps that would be the right phrasing instead of "provided as a".

[jmdyck]

If the body, the parameters, and now the binding identifier of a function-ish declaration are included in the "function code" of the function, and thus either all of them or none of them are strict mode code, then it seems like it would simplify things to say that the declaration itself was or wasn't strict mode code. (We couldn't do that as long as BindingIdentifier was the holdout, but now we can.)

That is, instead of defining what constitutes "function code", and then saying when function code is strict, just say when declarations are strict. Change if the function code for |declaration| is strict mode code to if |declaration| is strict mode code.

(Even better would be to say Let _strict_ be IsStrict(|declaration|), where we define IsStrict() more fully than it is now, and drop most of 10.2, but that's probably orthogonal.)

[bakkot]

@jmdyck, that would be a conflation of types, in the current specification. "strict mode code" is currently a kind of source text, whereas |declaration| is a parse node. Making if |declaration| is strict mode code work would require changing that and hence require a much larger refactoring, touching everywhere strict mode is referenced, including all of the strict early errors (e.g.) which currently read something like "It is a Syntax Error if the code matched by this production is contained in strict mode code and [...]".

I think that's repairable by instead saying if the source text matched by |declaration| is strict mode code, though.

[jmdyck]

in the current specification. "strict mode code" is currently a kind of source text, whereas |declaration| is a parse node

Right, so what I'm suggesting is not a conflation of types, but rather a change of type. (I suppose, rather than if |declaration| is strict mode code, I should have suggested the wording if |declaration| is strict.)

(Speaking of conflation, note that the current spec is inconsistent on whether 'function code' belongs to a function object or a function-ish declaration. So that should go away.)

touching everywhere strict mode is referenced

Yup, pretty much.

if the source text matched by |declaration| is strict mode code

Yeah, you could say it that way.

But consider the case of a non-strict |outerDecl| containing strict |innerDecl|s. When you ask "is the source text matched by |outerDecl| strict mode code?", the somewhat puzzled answer might be "well, some of it is". (This doesn't happen in the current spec, because function code is non-overlapping.) Of course, you can mitigate that by inserting an "all" ("is all the source text matched by |outerDecl| strict mode code?") or by saying that "all" is implied. (In contrast, the question "is |outerDecl| strict?" seems (to me, anyhow) less prone to that puzzlement, and IsStrict(|outerDecl|) even less so.)

[jugglinmike]

@bakkot Thanks for getting back to me--now it's my turn to apologize for a delay. Sorry!

If I understand your latest suggestion, then this patch would replace:

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.

With:

Function code is the source text matched by a |FunctionDeclaration|,
|FunctionExpression|, |GeneratorDeclaration|, |GeneratorExpression|,
|AsyncFunctionDeclaration|, |AsyncFunctionExpression|,
|AsyncGeneratorDeclaration|, |AsyncGeneratorExpression|, |MethodDefinition|,
|ArrowFunction|, |AsyncArrowFunction|, |ClassDeclaration|, or
|ClassExpression|.

If that's right, then my only concern would be about function code specified via the Function constructor. Wouldn't we need to account for that, too? If so, would it be sufficient (or desirable) to tack an additional clause to this new definition?

[ljharb]

@jugglinmike all the function constructors, I’d expect.

[jmdyck]

@jugglinmike:

Function code is the source text matched by a ... |ClassDeclaration|, or
|ClassExpression|.

These last two aren't included in the current definition of 'function code', and I don't think it makes sense to add them, given how the term is currently used.

That said, a different term that includes those two nonterminals could be useful. In old bug 4146, I suggested region-defining nonterminals (which also included Script and Module) as part of DRY-ing 10.2. (Note that the definition of 'function code' there includes the BindingIdentifier. At the time, that was a bug in my proposal, but it now agrees with intent of this issue.)

[bakkot]

If that's right, then my only concern would be about function code specified via the Function constructor. Wouldn't we need to account for that, too? If so, would it be sufficient (or desirable) to tack an additional clause to this new definition?

Hm. CreateDynamicFunction, which is what Function() defers to, is defined in terms of the original grammar, so I think it's OK. That said, for the purposes of clarity I think adding "... including source text parsed by any abstract operations" would make sense.

I think @jmdyck is right that making this change would also require changing all usages of the phrasing "the source text matched by |Prod| is strict mode code" to read "all of the source text matched by |Prod| is strict mode code", and that at that point it might make sense to do a more comprehensive reform of how strictness is defined (which would hopefully also help clarify what is currently a fairly confusing part of the spec).

I'm happy with either solution.

[jmdyck]

CreateDynamicFunction, which is what Function() defers to, is defined in terms of the original grammar, so I think it's OK.

CreateDynamicFunction parses the parameters and body separately, and assigns the results (via FunctionInitialize) to the [[ECMAScriptCode]] and [[FormalParameters]] internal slots of a new function object. So that qualifies as 'function code' under the current definition, but not under the proposed definition, because there's no FunctionDeclaration (or whatever) involved.

That said, for the purposes of clarity I think adding "... including source text parsed by any abstract operations" would make sense.

That would suggest that some source text isn't parsed by abstract operations, which I don't think is true. Anyhow, it wouldn't solve the above problem.

[jugglinmike]

@jmdyck I think I follow, but I'm going to need some more hand-holding. Does this seem right?

- Function code is strict mode code if the associated |FunctionDeclaration|,
- |FunctionExpression|, |GeneratorDeclaration|, |GeneratorExpression|,
- |AsyncFunctionDeclaration|, |AsyncFunctionExpression|,
- |AsyncGeneratorDeclaration|, |AsyncGeneratorExpression|, |MethodDefinition|,
- |ArrowFunction|, or |AsyncArrowFunction| 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.
+ For all other function-defining productions (i.e. |FunctionDeclaration|,
+ |FunctionExpression|, |GeneratorDeclaration|, |GeneratorExpression|,
+ |AsyncFunctionDeclaration|, |AsyncFunctionExpression|,
+ |AsyncGeneratorDeclaration|, |AsyncGeneratorExpression|, |MethodDefinition|,
+ |ArrowFunction|, and |AsyncArrowFunction|), all parts of the production are
+ strict mode code if 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.

The parenthetical is my best effort to encapsulate the long-but-necessary list of productions so that the definition (itself fairly complex) is easier to identify. Does that make sense?

[jmdyck]

@jugglinmike: I think I need a recap...

• So far, this PR has mostly been looking at how to include BindingIdentifier in the definition of "function code". Given that, the existing "Function code is strict mode code" paragraph should be okay as is.

• But what you're now proposing is, I think, to leave the definition of "function code" as is (without BindingIdentifier), but modify the definition of "strict mode code" to include it instead.

Have I got that right? If so, then I think your suggested replacement does achieve that (modulo some nits noted below), but the result is a bit odd... Both the context (i.e., 10.2 and 10.2.1) and all the algorithm steps that say "If the function code for X is strict mode code..." set up an expectation for a sentence saying "Function code is strict mode code if ...", so it's possible people will be puzzled when it's not there.

---

For all other function-defining productions

"productions" should be "nonterminals", because the parenthetical is a list of nonterminals.

And since the context hasn't already considered any function-defining nonterminals, the word "other" doesn't mean anything here.

all parts of the production are strict mode code

Change to "all source code matching the nonterminal is strict mode code".

---

(I'm still partial to a more radical/widespread solution, but I recognize that this PR is aiming for a minimal change.)

[jugglinmike]

That's correct; I'm trying to apply your earlier recommendation. I also planned to implement the phrasing "if |declaration| is strict" from your follow-up comment, but that wasn't clear from my previous response. It's an important addition, though, and I believe it addresses a number of your concerns. Does this also encompass the "more radical/widespread change" you had in mind?

In the interest of avoiding further confusion (and at the risk of more churn), I've pushed up a new commit that updates the patch according to my understanding of our discussion so far.

the context (i.e., 10.2 and 10.2.1) [...] set up an expectation [..]

And since the context hasn't already considered any function-defining
nonterminals, the word "other" doesn't mean anything here.

I have had the preceding situation in mind: "All parts of a ClassDeclaration or a ClassExpression are strict mode code." Do you think the relationship would be more clear if we moved the new version of this situation to immediately follow that one? Or am I mistaken in thinking that they are related?

[jmdyck]

That's correct; I'm trying to apply your earlier recommendation. I also planned to implement the phrasing "if |declaration| is strict" from your follow-up comment, but that wasn't clear from my previous response.

Ah, okay.

Does this also encompass the "more radical/widespread change" you had in mind?

Not entirely. The "radical" part also included refactoring 10.2 + 10.2.1 somewhat along the lines described in old bug 4146. And the wording changes might need to be more widespread.

I have had the preceding situation in mind: "All parts of a ClassDeclaration or a ClassExpression are strict mode code."

Sorry, I forgot that those define functions.

[jmdyck]

The PR now changes occurrences of:
If the code matched by X is strict mode code, ...
to:
If X is strict, ...
but this disconnects them from the definition of strict mode, which is still specified using the former terminology.

That is, you need to decide whether strictness is a property of source code or of Parse Nodes. The status quo is the former and I suggested shifting to the latter, but this PR is in-between. As @bakkot said, shifting to the latter would require touching everywhere strict mode is referenced, but there are many such references that this PR doesn't touch.

(I think there might be a middle-ground that allows use of both phrasings, but (a) that might be confusing, and (b) the connection still needs to be made.)

[jugglinmike]

shifting to the latter would require touching everywhere strict mode is referenced,

Unless I am mistaken, this would also mean removing the term "strict mode code" and re-locating the corresponding section (out of "Types of Source Code"). Is that right?

[jmdyck]

[shifting to strictness as a property of Parse Nodes] would also mean removing the term "strict mode code"

Mostly, yes, I think so. That is, contexts with precise wording (e.g., algorithms and early error rules) would no longer use that phrase, but in 'looser' contexts (like notes), it might still be convenient.

and re-locating the corresponding section (out of "Types of Source Code").

Well, it wouldn't have to move, but it would probably make more sense if it did (maybe just to 10.3). (Tangentially, 10.2.2 "Non-ECMAScript Functions" already doesn't make sense to me as part of 10.2 "Types of Source Code".)

[jmdyck]

In contrast to recent discussion here, I think we could achieve the desired effect with a fairly small, localized change (for which I'll offer 3 alternatives). At the end of the paragraph that defines "function code", insert something like:

In addition, if the source text referred to above is:
  - the |FormalParameters| and |FunctionBody| of a |FunctionDeclaration| or |FunctionExpression|,
  - the |FormalParameters| and |GeneratorBody| of a |GeneratorDeclaration| or |GeneratorExpression|,
  - the |FormalParameters| and |AsyncFunctionBody| of an |AsyncFunctionDeclaration| or |AsyncFunctionExpression|, or
  - the |FormalParameters| and |AsyncGeneratorBody| of an |AsyncGeneratorDeclaration| or |AsyncGeneratorExpression|,
then the |BindingIdentifier| (if any) of that declaration or expression
is also included in the function code of the corresponding function.

(It's basically the same idea as @jugglinmike's original insertion "along with any |BindingIdentifier| present in the grammar production", just expanded to be more precise.)

This alternative takes advantage of the fact that |FormalParameters| is common to all four cases:

In addition, if the code that supplies the value of the function's
[[FormalParameters]] internal slot is the |FormalParameters| of a
|FunctionDeclaration|, |FunctionExpression|,
|GeneratorDeclaration|, |GeneratorExpression|,
|AsyncFunctionDeclaration|, |AsyncFunctionExpression|,
|AsyncGeneratorDeclaration|, or |AsyncGeneratorExpression|,
then the |BindingIdentifier| (if any) of that declaration or expression
is also included in the function code of the corresponding function.

Mind you, the reader might wonder why [[FormalParameters]] is singled out for this purpose. The third alternative brings back the body, but it's the least precise:

In addition, if the source text referred to above is the body and parameters of a
|FunctionDeclaration|, |FunctionExpression|,
|GeneratorDeclaration|, |GeneratorExpression|,
|AsyncFunctionDeclaration|, |AsyncFunctionExpression|,
|AsyncGeneratorDeclaration|, or |AsyncGeneratorExpression|,
then the |BindingIdentifier| (if any) of that declaration or expression
is also included in the function code of the corresponding function.

[jugglinmike]

Thanks, @jmdyck! I was starting to worry about the size of this patch, so the brevity of these alternatives is much appreciated.

It seems to me that each of the three has a different drawback, respectively: conciseness, transparency, and precision. I'm most inclined to take a hit on conciseness, so the first is my preference.

@bakkot I'm certain you have an opinion here. Would you mind sharing?

[bakkot]

@jugglinmike, @jmdyck: I like the first of the three options in this comment. I would be happy with it as a resolution to this issue, modulo the following quibbles :

Because of the wording I refer to in #632, namely, "If the source code matching this production is strict mode code" where "this production" is FunctionDeclaration : function BindingIdentifier ( FormalParameters ) { FunctionBody } or similar, I think we should include the entire production, unless we are also landing #1137. That is, I would say

In addition, if the source text referred to above is:
  - the |FormalParameters| and |FunctionBody| of a |FunctionDeclaration| or |FunctionExpression|,
  - the |FormalParameters| and |GeneratorBody| of a |GeneratorDeclaration| or |GeneratorExpression|,
  - the |FormalParameters| and |AsyncFunctionBody| of an |AsyncFunctionDeclaration| or |AsyncFunctionExpression|, or
  - the |FormalParameters| and |AsyncGeneratorBody| of an |AsyncGeneratorDeclaration| or |AsyncGeneratorExpression|,
then **the corresponding |FunctionDeclaration|, |FunctionExpression|, |GeneratorDeclaration|, |GeneratorExpression|, |AsyncFunctionDeclaration|, |AsyncGeneratorDeclaration|, or |AsyncGeneratorExpression|**
is included in the function code of the corresponding function, **with the above exception**.

(The 'above exception' being the line currently in the spec which excludes nested function code.)

I don't think this breaks anything, though it's now very wordy.

Also, parse nodes are not source text, so the first line should probably read

In addition, if the source text referred to above is **parsed as**:

Finally, I would personally add a non-normative NOTE reading

NOTE: The practical effect of this is that the Early Errors for strict mode code are applied to BindingIdentifiers which are the names of functions whose bodies contain "use strict" directives, even if the surrounding code is not strict mode code.

---

That is to say, going this route, my preferred resolution would be to add the following text after the current definition of "function code":

In addition, if the source text referred to above is parsed as:
  - the |FormalParameters| and |FunctionBody| of a |FunctionDeclaration| or |FunctionExpression|,
  - the |FormalParameters| and |GeneratorBody| of a |GeneratorDeclaration| or |GeneratorExpression|,
  - the |FormalParameters| and |AsyncFunctionBody| of an |AsyncFunctionDeclaration| or |AsyncFunctionExpression|, or
  - the |FormalParameters| and |AsyncGeneratorBody| of an |AsyncGeneratorDeclaration| or |AsyncGeneratorExpression|,
then the corresponding |FunctionDeclaration|, |FunctionExpression|, |GeneratorDeclaration|, |GeneratorExpression|, |AsyncFunctionDeclaration|, |AsyncGeneratorDeclaration|, or |AsyncGeneratorExpression|
is included in the function code of the corresponding function, with the above exception.

NOTE: The practical effect of this is that the Early Errors for strict mode code are applied to  BindingIdentifiers which are the names of functions whose bodies contain "use strict" directives, even if the surrounding code is not strict mode code.

or (my stronger preference) to land #1137 and in this patch add this text rather than the above:

In addition, if the source text referred to above is parsed as:
  - the |FormalParameters| and |FunctionBody| of a |FunctionDeclaration| or |FunctionExpression|,
  - the |FormalParameters| and |GeneratorBody| of a |GeneratorDeclaration| or |GeneratorExpression|,
  - the |FormalParameters| and |AsyncFunctionBody| of an |AsyncFunctionDeclaration| or |AsyncFunctionExpression|, or
  - the |FormalParameters| and |AsyncGeneratorBody| of an |AsyncGeneratorDeclaration| or |AsyncGeneratorExpression|,
then the |BindingIdentifier| (if any) of that declaration or expression
is also included in the function code of the corresponding function.

NOTE: The practical effect of this is that the Early Errors for strict mode code are applied to  BindingIdentifiers which are the names of functions whose bodies contain "use strict" directives, even if the surrounding code is not strict mode code.

[jmdyck]

re quibble 1 (including the corresponding decl or expr in the function code):

(a) the preceding condition means that the decl or expr is already in hand, so you don't need to go through a "corresponding" to get it. You could change "the corresponding" to just "that" (i..e, ... then that |FunctionDeclaration|, ...)

(b) As you say, Parse Nodes are not text, so it's incorrect to include |FunctionDeclaration| etc in the function code. Rather, you need to say something like ... then the source code matching that |FunctionDeclaration| ....

(c) I''m doubtful that "with the above exception" will be clear to readers.

(d) While I like the idea of moving the 'root' of function code (and thus strictness) up to the level of the decl/expr, this feels like a convoluted way to do it. With this suggestion, the definition of function code would have roughly this structure:

Function code is source code parsed for [[ECMAScriptCode]] and [[FormalParameters]].
Except that it doesn't include the function code of nested functions.
And in most cases, it actually is the source code parsed for the function itself,
but excluding the function code of nested functions.

We could certainly do better, but that would probably move us away from a small PR.

---

re quibble 2 (inserting "parsed as"):

Yup, good fix.

---

re quibble 3 (adding a note):

(a) It seems to be saying that the only practical effect of the definition of 'function code' is the effect on BindingIdentifiers, which is not true. It might be true of just the "In addition" sentence, but the note isn't clear on its scope.

(b) The note might make more sense if it pointed out that in previous editions of the spec, the function's BindingIdentifier was not included in the function's function code, so the function's strictness didn't affect it.

(c) It's a bit odd to be talking about strict mode when we don't define it until the next clause, but this is just a note, so I guess it's okay.

(d) It's got a lot of plurals. I'd prefer something like ... apply to a BindingIdentifier that is the name of a function whose body contains a "use strict" directive

---

or (my stronger preference) to land #1137 and in this patch add this text rather than the above

Yes, that would be my preference too.

[bakkot]

@jmdyck, taking those into account, how do you feel about:

In addition, if the source text referred to above is parsed as:
  - the |FormalParameters| and |FunctionBody| of a |FunctionDeclaration| or |FunctionExpression|,
  - the |FormalParameters| and |GeneratorBody| of a |GeneratorDeclaration| or |GeneratorExpression|,
  - the |FormalParameters| and |AsyncFunctionBody| of an |AsyncFunctionDeclaration| or |AsyncFunctionExpression|, or
  - the |FormalParameters| and |AsyncGeneratorBody| of an |AsyncGeneratorDeclaration| or |AsyncGeneratorExpression|,
then the source text matching the |BindingIdentifier| (if any) of that declaration or expression
is also included in the function code of the corresponding function.

NOTE: The practical effect of including the |BindingIdentifier| is that the Early Errors for strict mode code are applied to a |BindingIdentifier| that is the name of a function whose body contains a "use strict" directive, even if the surrounding code is not strict mode code.

+ landing #1137?

[jmdyck]

I'd be okay with that.

(I still think the note would make more sense if it pointed out the change from past editions, and I think "apply" would read better than "are applied to", but neither is crucial.)

[bakkot]

@jugglinmike Now that #1137 is merged, do you want to update this PR?

[jugglinmike]

Sure thing, @bakkot. The original version of this branch is available here.

Your latest suggestion included an informative note directly following the new paragraph, but this seemed a little jarring when situated within a list of definitions. I've moved the note to the end of the section and added the text "in function code" to restore the context.

[jmdyck]

I've moved the note to the end of the section and added the text "in function code" to restore the context.

Works for me.