Fix decorator evaluation private environment #16325
Conversation
JLHwung
added
PR: Bug Fix 🐛
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56452 |
| } | ||
|
|
||
| expect([...A].map(fn => fn(B)).join(",")).toBe("B#X,B#X,B#X,B#X,B#X,B#X,B#X,B#X,B#X,B#X"); | ||
| } |
There was a problem hiding this comment.
This edge case test is disabled because I can't figure out how to transform it.
There was a problem hiding this comment.
In this case, can we introduce a new public element and delete it immediately after defined?
class C {
// for demo purpose, the `delete` expression can be directly inserted into staticInitializerAssignments
@(function(_, context) { context.addInitializer(function() { delete this._ } })
static [(/* decorator evaluations */, "_")];
}Of course using delete is a performance killer, but this approach will be only invoked when there is no public class elements, which should be quite rare. By doing so we can get rid of all function context variable replacement since the decorator expressions are guaranteed to be inserted within a computed key.
There was a problem hiding this comment.
I think this case is rare enough that:
- the perf hit is ok
- the perf it doesn't actually matter, because you are not going to access properties on instances of this class (because it, well... has no properties)
for delete, let's put it in a static { delete this._ } block at the beginning of the class?
There was a problem hiding this comment.
That sounds good to me.
We can even do it when someone needs it. :)
There was a problem hiding this comment.
for delete, let's put it in a static { delete this._ } block at the beginning of the class?
I plan to insert the delete expression right before the applyDecs call, since its parent block is already the first class element.
There was a problem hiding this comment.
This test is now enabled and merged with decorator-evaluation-yield-private-super-property/exec.js
It also happens to catch a bug in the helper-replace-supers, currently Babel is incorrectly replacing the super in the decorators of the given class method. This should have been handled in the environmentVisitor but the root class method path is not visited because the visitSelf argument of traverse.node is not exposed to the path.traverse.
| } | ||
|
|
||
| B.m(); | ||
| expect(receivedName).toBe("B"); |
There was a problem hiding this comment.
This test was renamed to "decorator-evaluation-this" and then augmented.
| var _initStatic, _init_a, _init_a2, _get_a, _set_a, _init_computedKey, _init_computedKey2, _init_computedKey3, _init_computedKey4, _init_computedKey5, _init_computedKey6, _computedKey, _init_computedKey7, _Foo; | ||
| let _computedKey; | ||
| var _initStatic, _init_a, _init_a2, _get_a, _set_a, _init_computedKey, _init_computedKey2, _init_computedKey3, _init_computedKey4, _init_computedKey5, _init_computedKey6, _init_computedKey7, _Foo; |
There was a problem hiding this comment.
This change is intentional as it will fix an integration issue with the class properties transform. For context see https://github.com/babel/babel/pull/10029/files#r287785963
In this REPL example
function noop() {}
const classes = [];
for (let i = 0; i < 5; ++i) {
classes.push(
class A {
@noop
[i] = "my property";
}
);
}
var result = [];
for (const clazz of classes) {
result.push(Object.getOwnPropertyNames(new clazz())[0]);
}
console.log(result.join())when we apply the decorators, static block and class properties plugin, it should print 0,1,2,3,4 but currently it prints 4,4,4,4,4.
I can add a new test case for this issue.
If the output size is the concern, I can open a new PR to register all variable as the let binding, as it will introduce much more test output changes.
There was a problem hiding this comment.
Yes please add a test!
Regarding to output size, we should first aim at correctness and then optimization.
There was a problem hiding this comment.
Thanks! I didn't notice this was a bug fix, I thought the increased size didn't matter.
|
"I suggest review this PR by commits." ~ 13 commits 😛 |
| const expressions = path.get("expressions"); | ||
| return getComputedKeyCompletion(expressions[expressions.length - 1]); | ||
| } | ||
| return skipTransparentExprWrappers(path); |
There was a problem hiding this comment.
Should this recourse if the result of skipTransparentExprWrappers is a SequenceExpression?
| const key = fieldPath.get("key") as NodePath<t.Expression>; | ||
| expressions.push(key.node); |
There was a problem hiding this comment.
If key is already a sequence expression maybe we should do expression.push(...key.node.expressions)?
Or -- this is probably so rare that it doesn't matter, but in this case could we have a test showing that we generated a "nested" sequence expression?
There was a problem hiding this comment.
If a key is already a sequence expression, it could be generated by another Babel plugin. The intention here is try to reuse the uid memoiser, for example the computed keys transform can reuse the memoiser _computedKeys generated by the decorator transform:
class C {
[decs = f(), _computedKey = g(), another_decs = h(), _computedKey];
}but at this point there is no class features running before decorators, so there aren't much practical examples. I will add a test case for nested sequence expression.
| ); | ||
| } | ||
| } else { | ||
| // If the computed key is an uid reference, treat it as |
There was a problem hiding this comment.
Why if we get to this else branch the key is an uid reference?
EDIT: Oh I see -- could you rename memoiseComputedKey to memoiseComputedKeyIfNotUid, or add a comment before the call saying that it returns undefined if and only if it's a uid?
| if (isUidReference) { | ||
| return; |
There was a problem hiding this comment.
Nit: Move this early return to before const isMemoiseAssignment.
| } | ||
|
|
||
| expect([...A].map(fn => fn(B)).join(",")).toBe("B#X,B#X,B#X,B#X,B#X,B#X,B#X,B#X,B#X,B#X"); | ||
| } |
There was a problem hiding this comment.
I think this case is rare enough that:
- the perf hit is ok
- the perf it doesn't actually matter, because you are not going to access properties on instances of this class (because it, well... has no properties)
for delete, let's put it in a static { delete this._ } block at the beginning of the class?
|
|
||
| const computedKeysPath = applyDecoratorWrapperPath.get("body")[0]; | ||
|
|
||
| // Capture lexical this and super call, replace their usage in computed key assignments |
There was a problem hiding this comment.
Would
work here? If yes, can we export that function and use it for Babel 8, rather than duplicating the logic forever?Or maybe we could even "polyfill" it for Babel 7, doing terrible stuff:
function hoistFunctionEnvironment(path) {
Object.defineProperty(path, "isArrowFunctionExpression", {
get() {
delete path.isArrowFunctionExpression;
return () => true;
}
});
return path.unwrapFunctionEnvironment();
}There was a problem hiding this comment.
Good point! I didn't realize we have such a utility in the traverse library. However I am down to the temporary property approach and hopefully we can remove all the lexical variables capturing logic here.
| @@ -1659,6 +1672,32 @@ function transformClass( | |||
| } | |||
|
|
|||
| originalClass.body.body.unshift(t.staticBlock([])); | |||
| if (computedKeyAssignments.length > 0) { | |||
| // todo: the following branch will fail on 2023-05 version | |||
| if (version === "2023-11") { | |||
There was a problem hiding this comment.
process.env.BABEL_8_BREAKING || version === "2023-11"
There was a problem hiding this comment.
This check is eventually removed in bdee20d.
| static m() { | ||
| @noop | ||
| class C { | ||
| @(receivedName = super.name, noop) p; |
There was a problem hiding this comment.
Can you add a test with a decorator that references both super and a private field in the same decorator?
There was a problem hiding this comment.
Does this test look good?
Edit: I can add a super-call flavor if you mean super().
There was a problem hiding this comment.
This is ok, I somehow missed it.
This change ensures that 1) non static computed key will share the same function context when moved into the non-static class 2) evaluations of decorators and computed keys will always precede static computed keys, given that they have been moved into computed key assignments before
so that we can still preserve the production params [yield], [await] inherited from the outer scope
|
test262 seems to be relevant, while the other CI failures are not. Considering that this PR rebase may be complicated, we can postpone the #16326 merge. |
JLHwung
force-pushed
the
fix-decorator-evaluation-private-environment
branch
from
0b8f0bc
to
d837c27
Compare
| static { | ||
| _xDecs = dec; | ||
| delete this._; |
There was a problem hiding this comment.
New PR idea: We can optimize them, potentially reusing the usesFnContext diagnostic info of each decorators sequence.
In this PR we move the element decorator evaluation from the preface of class declaration into computed key.
Previously they are evaluated outside the class, immediate after class decorator evaluations. However, because the element decorators are defined within the class syntax, they should have access to the private variables defined in the current class.
The current evaluation storage:
If a class has only private elements and static blocks, we will insert the decorator evaluations before theapplyDecscall. We will replace all function context usage before moving them into the static block. However, this is still not safe because decorators expressions / computed keys inherit the outer level[Yield]and[Await]production parameter, while the static block where we insertapplyDecscall can not be a generator / async function. Given that the scenaraio of private-only class is rare, I would like to trade it with this general private fix.I suggest review this PR by commits.