Fix: Traverse set correct scope for function declarations in sloppy mode

[overlookmotel]

Q	A
Fixed Issues?	Fixes #13549
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link	n/a
Any Dependency Changes?	No
License	MIT

Fix for #13549.

I believe this captures all the edge cases.

• A sloppy-mode function declaration is hoisted to the furthest ancestor block which does not include another const, let, param or module binding.
• Only applies to plain functions, not async or generator functions.
• Binding location depends on whether environment in which the function is declared is strict mode, not whether the function itself is strict mode.

A few notes about implementation:

• Because it's necessary to know what other bindings exist in ancestor blocks before determining which scope the function declaration should be bound in, creating the binding has to be deferred until exiting the parent function / program. This deals with cases like this where at the time of visiting the function declaration, you don't have all the info you need to calculate the correct scope:

{
  function foo() {}
}
let foo;

• The check for whether a function is strict mode here is fairly expensive. However, it's bypassed for functions which are declared at top level in program, or directly nested in another function - which covers the vast majority of cases. The check is only run if it really needs to be.

• Because of the several conditions on correct input to _registerSloppyFunctionDeclarations(), I thought it best to prefix method name with _, so it's not part of public API.

• The test I've modified in @babel/plugin-transform-block-scoping I think maintains the intent of the test - the later function f() {} is meant to be scoped to inside the switch block.

• I am not entirely clear how traversal starting from a node which is not the program root works. I think this section covers such cases, but would appreciate input on whether I have this right.

[JLHwung]

nit: I prefer to move sloppyFunctionDeclarationStack manipulation to the Function visitor and then split BlockScoped visitor, as the current logic is implicitly determined by the execution order of Function and BlockScoped visitor.

[overlookmotel]

Yes sure, I'll be happy to make these changes. Give me a day or two please.

[JLHwung]

We can use pure function if it is not supposed to be a NodePath API.

[nicolo-ribaudo]

Nit: It took me really long do see the difference between this function name and the name of the other function. Could you rename the other one to something like _registerMultipleSloppyFunctionDeclarations? 😅

[overlookmotel]

I'm afraid I've discovered my assumptions were incorrect. I said above:

A sloppy-mode function declaration is hoisted to the furthest ancestor block which does not include another const, let, param or module binding.

This is not correct. It should be:

A sloppy-mode function declaration is hoisted to its parent function's scope if that scope and no intermediate scopes already have an existing binding.

There are some further complications if there are multiple function declarations.

Here the inner function declaration is hoisted as a redeclaration:

{
  function f() { return 2; }
}
function f() { return 1; }
console.log(f()); // Logs 2

Whereas here the more deeply nested function here does create a separate binding in its own block:

{
  function f() { return 1; }
  {
    function f() { return 2; }
    console.log(f()); // 2
  }
}
console.log(f()); // 1

So unfortunately the implementation in my PR would need a lot of changes to be correct. Going to take some time or (to be honest) maybe never happen!

[nicolo-ribaudo]

(I hate annex b)

[overlookmotel]

MDN notes that there are differences between browsers in how they handle this:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function#conditionally_created_functions

In NodeJS:

console.log(f); // undefined
const getOuterF = () => f;
{
  console.log(typeof f); // function
  console.log(typeof getOuterF()); // undefined <-- what???
  function f() {}
  console.log(typeof f); // function
  console.log(typeof getOuterF()); // function
  f = 123;
  console.log(typeof f); // number
  console.log(typeof getOuterF()); // function <-- what???
}
console.log(typeof f); // function

It appears the nested function declaration creates two bindings - one at top level, and one in the block statement.

I couldn't find a passage in ECMA spec which specifies the correct behavior. But I find the spec pretty impenetrable, so I may just have missed it. Anyone know if this exists?

[nicolo-ribaudo]

It's specified here: https://tc39.es/ecma262/#sec-block-level-function-declarations-web-legacy-compatibility-semantics

I can try to analyze how that example should work according to the spec.

[overlookmotel]

Thanks very much for swift reply. Some bedtime reading! It looks complicated, but more comprehensible than much of the spec.

[nicolo-ribaudo]

Btw, with your example I get the same result in Firefox and Node.js

[nicolo-ribaudo]

Ok, for that example you probably have to look at B.3.2.2 and 16.1.7.