Normative: Allow duplicate FunctionDeclarations in a block

[littledan]

A previously overlooked aspect of intersection semantics for
#sec-block-level-function-declarations-web-legacy-compatibility-semantics
is the ability to have multiple function declarations with the same name
in the same block. This patch relaxes that constraint in sloppy mode.
Because it relaxes a static semantics rule, and replaces rather than
simply adds behavior, it is in the main spec text rather than Annex B.
The patch also puts the non-normative text in Annex B in an emu-note.

Fixes #320.

[UltCombo]

Isn't the NOTE formatting redundant? Previous discussion on this topic: #341 (comment)

[littledan]

Oh, I missed that discussion. The key point seems to be, from @allenwb

However, the standards orgs kind of frown upon starting sections with long explicit NOTEs and would prefer that they be moved to follow the actual normative text. Basically, short non-normative content-free intro paragraph ok, starting with a long descriptive informative NOTE, less ok.

I'm curious as to the standards orgs' reasoning. W3C and WHATWG specs often put their non-normative sections in notes, even if they are introductory paragraphs. They use formatting which doesn't cause things to be indented as much, which reduces the overhead. But that's a discussion for another day, so I'll revert that part.

[rossberg]

Hm, I don't follow the reasoning for putting this in the main spec. I don't think it matters whether the Annex "adds" or "replaces" rules, since the difference often is solely dependent on how the spec is factored, not a deeper semantic characteristic. And IIRC, there are already other examples where the Annex replaces some rules from the main spec?

[littledan]

@rossberg-chromium The only one I see is Annex B 3.2, which is a much smaller replacement than this. I could do it that way, but I think the result would be significantly harder to read, and I don't think removing the duplicate checks would sacrifice the fundamental properties of lexical scoping that the committee has expressed the desire to maintain.

[syg]

Like @rossberg-chromium I feel more comfortable with this being in Annex B than replacing the main spec text.

I also have concerns about the interaction with Annex B.3.3 here. Without additional changes there these semantics aren't "the last declaration counts" since the var assignments from B.3.3 make the previous declarations of a function visible in the var scope as we reach their declarations.

Since we kind of have the informal agreement that Annex B.3.3 as it is right now shouldn't be implemented by vendors anyways, that oddity might be tolerable. But, I really don't want it to get any weirder.

[allenwb]

Because it relaxes a static semantics rule, and replaces rather than
simply adds behavior, it is in the main spec text rather than Annex B.

This was never the criteria for placing something (or not) in annex B.

Annex B exists for material that would not exist except for web backwards comparability requirements. In addition, non-web implementations should be discouraged from implementing Annex B features. The nature of the change or how it is specified should not be a factor in determining whether something belongs in Annex B.

Allowing duplicate FiBs for sloppy mode is clearly Annex B material. It should not be in the main spec. text. However, it is acceptable to add hooks to the main spec. that simplify the specification of Annex B material.

[littledan]

OK, I'll put this in Annex B, as that seems to be the consensus. (If we were going to go by "what just exists for web backwards compatibility", I might put other things like property access defaulting to undefined there, but that's just me :)

@syg I hope that with this PR (or its successor), we can have agreement that the new Annex B 3.3 is what browsers should implement. If not, then I'd like to understand why and incorporate all of that into a new version. Do you think your original PR is essential for what browsers should support? Maybe we should reconsider it if so.

[allenwb]

@littledan

(If we were going to go by "what just exists for web backwards compatibility", I might put other things like property access defaulting to undefined there, but that's just me :)

It's not about feature that we don't like. It's primarily about features that were never previously standardized but which the web is stuck with.

[syg]

@littledan I wanna reach agreement with all vendors on what to implement for Annex B.3.3 for sure. Are you saying we'll deal with Annex B.3.3 in follow ups and merge this PR without changing B.3.3? That's fine with me given the situation with its unimplementability right now.

[littledan]

@syg I thought maybe this would be the one and only follow-up. We might have a further follow-up, but I think that (leaving out some edge cases around eval and global declarations being weird in probably-inconsequential ways) this patch basically accomplishes it as far as covering what V8 does, and we haven't been getting any more bug reports. Apparently it's fair to say that we don't have consensus yet; I'd just personally argue that this will be sufficient and shippable. @kmiller68 @bterlson What are your thoughts here?

[syg]

@littledan I think having non-final function declarations be observable only via Annex B.3.3 var assignments is super weird. If the main spec text has "last function matters" semantics, so should Annex B.

[littledan]

Updated the patch to put everything in Annex B. Thoughts?

[allenwb]

think having non-final function declarations be observable only via Annex B.3.3 var assignments is super weird. If the main spec text has "last function matters" semantics, so should Annex B.

I agree. Annex B is about preserving legacy interoperable behavior. So we need to look very carefully at what behavior we need to preserve. For example, the following is not interoperable in legacy browsers:

var x;
if (true)  {
   function f() {console.log("first f")};
   x=f;
   x();  //logs "first f" in legacy FF, longs "second f" is other legacy browsers
   function f() {console.log("second f")};
};

In fact, the only situation where multiple declarations of a function within a block is operable is if all references to the function name occur after the last declaration. Using the last declaration of the function to initialize the binding at block entry (exactly what is done in the global scope and at the top level of functions) is sufficient for legacy interop.

We should not implement new semantics that allow any tying other than the final function in block for a given name to be visible as that is not needed for legacy interop and would be a new semantics that does not already exist in ES. We should implement the semantics I outlined in #320 (comment)

[littledan]

@allenwb The difference you're pointing out is the second point, that the first FunctionDeclaration should be ignored? I guess this would amount to something more like @syg 's original patch. Is that right? Or are there differences between your proposal and that one? Seems like your point centers around

We should not implement new semantics that allow any tying other than the final function in block for a given name to be visible as that is not needed for legacy interop and would be a new semantics that does not already exist in ES.

I was concerned about @syg 's patch because it seemed like it created more mechanism and special cases. Since browsers disagreed, and since we don't have very strong evidence that one or the other choice is needed for web compatibility, I went for the simpler option in this PR. What do you think of that reasoning?

[allenwb]

I guess this would amount to something more like @syg 's original patch.

Yes, basically Annex B version of BlockDeclarationInstantiations needs to step through the function declarations in reverse order to find the last declaration of each name and use that declaration to initialize the lexically scoped declaration.

Since browsers disagreed, and since we don't have very strong evidence that one or the other choice is needed for web compatibility, I went for the simpler option in this PR. What do you think of that reasoning?

I don't agree. We should avoid adding a new semantics that doesn't already exist in the language, even if that is the simpler approach. Note that what we have to add should make sense even if it is used in new (non-legacy) code. In this case, imagine if somebody is reviewing some code (who know why it was written this way, maybe it was by mistake) that looks like:

function f() {
  if (pred) {
      function g() {...)
      g();
      function g() {}
  }
}

How do they they reason about this (hopefully) unfamiliar arrangement of code? Ideally they think: well I know that the following code will call the 2nd declaration of g:

function f() {
    function g() {...)
    g();
    function g() {}
}

so, presumably it should work the same within a block and the 2nd declaration of g will be called.

Multiple declarations of a function within scope (global or top-level function) is something that has had a common semantics since the early days of JS/ES. Progressively updating declared function bindings as a statement list is executed is an alternative semantics that has only appeared in one browser implementation (FF). To me, that argues for following the more common/familar pattern and not the outlier.

FWIW, during ES5 development we followed the 3/4 rule: there were 4 popular browser implementations (FF, Webkit, IE, Opera) and if one browser diverged from the other 3 we went with the 3 even if the one divergent implementation was simpler or otherwise "better". We only broke that rule if there of some strongly compelling reason such as very obvious future proofings.

[syg]

I think last-function-matters semantics isn't controversial, but emergent new behavior here is, as Allen points out. I am very sympathetic to the argument that Annex B already gives us other weird, emergent behavior, but I would be careful to interpret that as license to be weird for this particular case of duplicate functions, for which we do have precedent of shipped implementations. I believe the path forward is to eat the extra spec complexity cost and ensure last-function-matters.

[littledan]

I believe this pull request received consensus on the Wednesday March 2016 TC39 meeting, rather than @syg's proposal.

[saambarati]

I'm still confused on the exact semantics we're allowing. Here is what my interpretation of what I think was decided. Can somebody confirm or correct my understanding?

function foo() {
    function outer() { return f; }
    outer() // undefined
    {
        outer() // undefined
        f() // 2
        f = 100
        f // 100
        outer() // undefined
        function f() { return 1 }
        outer() // 100
        f = 200
        f // 200
        outer() // 100
        function f() { return 2 }
        outer() // 200
    }
}

[saambarati]

@allenwb @syg @littledan
Any thoughts on the above program?

[syg]

@saambarati My reading of @littledan's PR at the time of this writing matches your interpretation, but I think the PR is wrong.

The committee reached consensus on "last function matters", meaning treating all non-final function declarations as non-existent, which I expect to extend to Annex B. That is, here's what I think should happen and @littledan should update this PR to match it:

function foo() {
    function outer() { return f; }
    outer() // undefined
    {
        outer() // undefined
        f() // 2
        f = 100
        f // 100
        outer() // undefined
        function f() { return 1 }
        outer() // undefined, because only the last declaration should
                // perform the Annex B assignment.
        f = 200
        f // 200
        outer() // undefined, as above.
        function f() { return 2 }
        outer() // 200, Annex B assignment performed.
    }
}

[saambarati]

@syg Okay cool. I agree it would be nicer that the last function matters in all observable ways, including performing the hoisting to the var binding.

[littledan]

Sorry if this sounds pedantic; I remember discussion of "last function matters", but I thought that was referring more to maintaining hoisting-style semantics rather than legacy Mozilla semantics of changing the value in the course of the execution of the block. I thought the committee consensus was to do the minimal thing to get sloppy mode working in Annex B, rather than meet possible user expectations, given that Annex B is only for compatibility, at least when it comes to the expectation that @syg raised at the time. I don't remember this issue (of whether prior function declarations should be deleted) being raised, and I don't think the committee considered strict "last function matters" semantics as we're discussing here.

The semantics that @saambarati wrote is what I had thought the committee agreed on, even if I agree it's counter-intuitive. However, I could definitely be biased, as I was thinking of the details of my own proposal as the committee discussion was ongoing. I don't have any particular opposition to the deletion semantics, though (just as I don't currently oppose @syg's earlier proposal); it would be easy for me to implement in V8, and I don't think it'll take very much spec machinery (comparable to @syg's earlier proposal, or less).

Could we have some more comments from others who were involved in the discussion? @allenwb @erights @dherman

@bterlson Please don't merge this PR until we resolve this issue.

[syg]

Let's get clarification from the folks you pinged on what the consensus was on: @saambarati's initial example semantics or mine. I don't mean to restart discussion, but I may just be misunderstanding the consensus.

[syg]

That is: let's not re-discuss intuition, I apologize for mentioning that word, and just get clarification.

[littledan]

It'd be great to get feedback on the multiple possible readings of the committee's consensus on this issue. @allenwb @dherman @erights what do you think?

[bterlson]

We have consensus on this issue - all function declarations in sloppy blocks should cause an assignment to the var binding (not just the last one) (@littledan @syg)

[littledan]

@bterlson I believe that corresponds to this patch. Is it good to merge then?