Skip to content

Memoize class binding when compiling private methods and static elements#15701

Merged
JLHwung merged 4 commits intobabel:mainfrom
JLHwung:fix-15696
Jul 19, 2023
Merged

Memoize class binding when compiling private methods and static elements#15701
JLHwung merged 4 commits intobabel:mainfrom
JLHwung:fix-15696

Conversation

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jun 13, 2023

Q                       A
Fixed Issues? Fixes #15696
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

In this PR we memoise the class binding when it is referenced in private methods or static elements, either via this or the class id. Previously we replaced this with class id, but as #15696 point out,

Class declarations in JavaScript generate an outer binding name, which turns out to be mutable (and can be reassigned). Separately, JavaScript also generates an immutable inner binding name that is only in scope within the class body.

it will fail when the outer binding is reassigned.

I suggest reviewing this PR by commits as there are many fixture changes.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Class Fields Spec: Private Methods labels Jun 13, 2023
@babel-bot
Copy link
Collaborator

babel-bot commented Jun 13, 2023

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

return injectSuperRef;
};

const classRefForInnerBinding =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When ref is not defined and classRefFlags.ForInnerBinding is not enabled, e.g. compiling a plain class class C {} without private methods / static elements, we will generate a uid identifier but we do not insert it to the final AST. In this case the memoizer variable _class0, _class1, etc. is not continuous.

@nicolo-ribaudo
Copy link
Member

Is only generating the extra ref when the class is reassigned doable?

@nicolo-ribaudo
Copy link
Member

I pushed some commits to do #15701 (comment), wdyt about this approach?

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 22, 2023

@nicolo-ribaudo Thanks for working on this. I didn't wrap it under the reassignment check because

  • It generally does not work with the case where the reassignment is introduced by a plugin. The replacement methods like NodePath#insertAfter or NodePath#replaceWith will not requeue the binding node. And it's asking a bit too much for plugin authors to do so.
  • The extra assignment does not necessarily bloat the output size. When the class id is long and class bindings are frequently used, the reassignment is doing a favor for the compressor as all the reference identifiers are now able to be mangled. Because of the compressor I didn't go ahead and fix the uuid continuity.

Btw TypeScript memoizes the class binding without checking if it is reassigned, too: https://www.typescriptlang.org/play?#code/MYGwhgzhAEBiD29oG8BQ0PQgFzNglsNAMQjQC8ciA3KgL5A

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I reverted my changes, the code is good

Copy link
Contributor Author

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Just rebased this PR and it looks good to me.

@JLHwung JLHwung merged commit 3caeeb1 into babel:main Jul 19, 2023
@JLHwung JLHwung deleted the fix-15696 branch July 19, 2023 21:59
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Class Fields Spec: Private Methods

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Incorrect transpilation of class name references in static contexts

3 participants