Fix decorated class static field private access

[JLHwung]

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

To recap, when a class is decorated we move non-static elements into an inner class since they will be defined on the class prototype before the class is replaced in the class decorator. However, the static class element can still capture the instance private elements. By splitting the class the static class element can no longer resolve the private name defined by an instance element.

In this PR we move the closure of static element into the inner class, to ensure that they can still resolve the private name. For static private methods, we have to also replace the super usage.

This PR also respects the ignoreFunctionLength compiler assumption when generating the delegate for the static private methods.

[babel-bot]

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

[JLHwung]

This failing test is disabled. It is already failing on main. I think it should be fixed after #16341 is fixed.

[JLHwung]

Currently the Node.js 16 CI is failing but Node.js >= 18 is fine, I guess it was an engine bug, I will investigate.

[JLHwung]

Interesting.

According to CI, this test is failing on Node.js 16 + *nix, but it works on Node.js 16 + Windows.
If I only run this test on Node.js + macOS, it works fine.

So it points to an OS-specific optimizer bug. I will mark this test as node.js 18+.

[JLHwung]

Hang tight. This test can be very confusing, I have to carefully specify the receiver to make it a valid JavaScript. I think it is worthy of a compiler assumption such as DecoratorDoesNotReplaceClass, to avoid the inner class completely, because once a class is replaced in a class decorator, the common this element access could be easily broken.

[JLHwung]

Aside: WDYT about a minNodeExecVersion option? So for old Node.js version we can still transform the input and see if the output matches the fixture, but the test runner will ignore the exec.js. I guess most minNodeVersion can be replaced by minNodeExecVersion.

[nicolo-ribaudo]

Sounds good to me. I wonder if any test needs to skip compiling on old versions, or if we can just update minNodeVersion to only skip execution.

[nicolo-ribaudo]

Can you add a test case with static fields too, other than static block and methods? And one with all of them at the same time.

[nicolo-ribaudo]

Can we generate _method.apply(this, arguments), without spread?

[nicolo-ribaudo]

This is not much better than _method.apply(this, arguments) :)

[JLHwung]

Yeah, the output will be marginally smaller after minification. Do you want to always generate the spec output?

[nicolo-ribaudo]

I'd prefer to always use .apply so that the generated code doesn't have to go through the iterator protocol, but still we can avoid making sure that #method has the correct length when the assumption is enabled.

[JLHwung]

Can you add a test case with static fields too, other than static block and methods?

As for static fields test, does this test look good?

babel/packages/babel-plugin-proposal-decorators/test/fixtures/2023-11-classes/replacement-static-private-instance-element-access/exec.js

Lines 59 to 115 in 79107b5

	{
	"instance private access in a static field initializer";
	let hasX, getX, setX, hasA, getA, setA, hasM, callM, staticThis, OriginalFoo;
	class Bar {}
	const dec = (Foo) => {
	OriginalFoo = Foo;
	return Bar;
	};
	@dec
	class Foo {
	#x;
	accessor #a;
	#m() { return "#m" }
	x;
	accessor a;
	m() {}
	static field = (
	staticThis = this,
	hasX = (o) => #x in o,
	getX = (o) => o.#x,
	setX = (o, v) => o.#x = v,
	hasA = (o) => #a in o,
	getA = (o) => o.#a,
	setA = (o, v) => o.#a = v,
	hasM = (o) => #m in o,
	callM = (o) => o.#m()
	);
	}
	const foo = new OriginalFoo();
	const bar = new Foo();
	expect(hasX(foo)).toBe(true);
	expect(getX((setX(foo, "#x"), foo))).toBe("#x");
	expect(hasA(foo)).toBe(true);
	expect(getA((setA(foo, "#a"), foo))).toBe("#a");
	expect(hasM(foo)).toBe(true);
	expect(callM(foo)).toBe("#m");
	expect(hasX(bar)).toBe(false);
	expect(hasA(bar)).toBe(false);
	expect(hasM(bar)).toBe(false);
	expect(foo.hasOwnProperty("x")).toBe(true);
	expect(bar.hasOwnProperty("x")).toBe(false);
	expect(OriginalFoo.prototype.hasOwnProperty("a")).toBe(true);
	expect(Bar.prototype.hasOwnProperty("a")).toBe(false);
	expect(OriginalFoo.prototype.hasOwnProperty("m")).toBe(true);
	expect(Bar.prototype.hasOwnProperty("m")).toBe(false);
	expect(staticThis).toBe(Bar);
	}

And one with all of them at the same time.

Sure.

[nicolo-ribaudo]

As for static fields test, does this test look good?

Yes 🙃 I promise I actually reviewed this PR, I don't know how I missed it.

[liuxingbaoyu]

We can optimize it in the future.

babelHelpers.assertClassBrand(_Foo3_brand, _p3);
return _m();

or

return babelHelpers.assertClassBrand(_Foo3_brand, _p3, _m)();