Rewrite transform-block-scoping plugin

[nicolo-ribaudo]

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

This PR rewrites the transform-block-scoping plugin, to:

• make it easier to follow what's going on, avoiding the stateful BlockScoping class and just relying on plain visitors
• clearly separate Annex B.3.3 behavior
• make the transform-block-scoping behavior consistent regardless of the context (function scope vs global scope; for vs while; loop scope vs block scope inside loop)

Lets see if it breaks any test262 tests

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53644/

[nicolo-ribaudo]

We now only insert the {...} if the block-scoping plugins needs to inject new nodes, so there are a lot of changes like this one where we don't inject the block.

[nicolo-ribaudo]

This test already passes with the old plugin, but it was untested (I accidentally broke it and all the tests were still passing).

[nicolo-ribaudo]

The old plugins sometimes injected a hoisted _loop function that is reused for every iteration, and other times it injected an IIFE. Not it always injects a single hoisted function.

[nicolo-ribaudo]

i isn't captured by a closure, so we don't need to pass it as a parameter.

[nicolo-ribaudo]

No need to rename result to _result, since it doesn't conflict.

[nicolo-ribaudo]

This is removed (and it was unnecessary) because now the loop block-scoped variable is transformed before transforming for-of.

[nicolo-ribaudo]

This test is failing with the old plugin, because the const checks and the tdz checks didn't interact well with each other. Now they are handled together in validation.ts.

[nicolo-ribaudo]

The plugin now always keeps var, because we cannot safely remove it anyway. For example:

maybeCall(() => a);
let { a, b } = c;

becomes

var a = babelHelpers.temporalUndefined;
maybeCall(() => _tdzCheck(a));
var { a, b } = c;

We can easily remove var from var a = babelHelpers.temporalUndefined; I didn't do it in this PR to avoid diffs in every file.

[liuxingbaoyu]

I agree with this particular scenario: it's safer when scope tracking isn't 100% reliable.

[nicolo-ribaudo]

I don't know why we had a second void 0

[liuxingbaoyu]

Unrelated to this PR: here it seems numbers can be safely removed.

[nicolo-ribaudo]

Yeah we can check if the initializer is without side effects.

[liuxingbaoyu]

I'm not sure if putting the function declaration inside the loop is better in this case, maybe a little bit of a performance hit, but it improves readability. (especially when there is a complex loop body)
Of course the new behavior is better for me than the old one anyway.

[nicolo-ribaudo]

Keeping the single _loop function also has the advantage over the old behavior that we can avoid multiple closures (example: https://babeljs.io/repl#?browsers=since%202015&build=&builtIns=false&corejs=3.21&spec=false&loose=false&code_lz=GYewTgBAFA3DCUEDeAoCEDOB3AlgFwGMALaARkVXXQIEMMBTCUgLjSvQBt68IAPGNuyiIAvAD4-A9AF822fMTIVBEWgyat2nbpJXphEcbpkpZQA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=unambiguous&lineWrap=true&presets=env%2Creact%2Cstage-2&prettier=false&targets=&version=7.20.4&externalPlugins=&assumptions=%7B%7D)

And yes, I believe that the function is hoisted for performance reasons, even if it's a small difference "readability of the output" has lower priority :P

[nicolo-ribaudo]

TODO: fix this

let a = 1;

function f() {
  { console.log(a); }
  { let a = 2; }
}

EDIT: done

[nicolo-ribaudo]

CI finally green, all the regressions caught by test262 have been fixed!

[JLHwung]

Awesome job! Will do a second round later.

[JLHwung]

Suggested change

	// declaration in the loop head now, to avoid having
	// declaration in the loop head now, to avoid

[JLHwung]

Since it is visiting scopeable elements, it should be merged with the environment visitor to ensure class keys / decorators are properly excluded from the Scope.

[nicolo-ribaudo]

Can you give me a code example? Since on Declaration we do path.skip(), this should never visit class keys/decorators.

[JLHwung]

The check can be copied to the start of the outer loop body in transformStatementList.

[JLHwung]

Should isStrict return true when we see a ClassBody in its ancestry?

[liuxingbaoyu]

Awesome!
I'm feeling good about the changes, just a little concerned about how this will change in the real world.
This PR uses referencePaths, which may be interfered by third-party plugins. (Some plugins completely ignore the scope information, and even cloneNode is not used correctly, these situations exist in the code I originally wrote)
But I think this should continue, this PR itself greatly simplifies and clarifies the plugin logic, while avoiding a lot of complicated edge logic, which will have a very positive impact in the future.
And if traverse itself is problematic, let's improve it.
I don't know how this affects debugging with source maps, but if someone reports this it will be easy to fix.

This fixes a tdz issue, marked as a bug fix?

[nicolo-ribaudo]

I prefer to keep it as "internal" because it's much more significantly a refactor than a bug fix, and refactors are also usually more risky.

[ndelangen]

I'm afraid this likely caused a regression: #15300