fix: privateFieldsAsProperties global counter (#15389) #15393

fwienber · 2023-02-01T14:34:07Z

When babel-helpers are inlined, the global variable id was defined and initialized to zero in every generated file. This could lead to name clashes between #-private members in subclass and superclass, which leads to incorrect semantics.
The new solution uses Symbols to create a unique private slot that cannot name-clash. I assume that in old environments that don't support Symbol natively, (babel-)polyfills must be loaded.

For details and discussion of other solutions, see #15389.

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

babel-bot · 2023-02-01T14:37:03Z

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

fwienber · 2023-02-01T14:42:52Z

Next, I would like to add tests, but there are some specialties involved to create a reproducer (see my codesandbox.io example):

enable inlining of babel-helpers. This could probably be achieved by setting "externalHelpers": false in options.json.
provide two input files, so that the helper code is inlined twice. Unfortunately, I don't see any way to achieve that with the current test setup.

liuxingbaoyu · 2023-02-03T16:08:58Z

It might be a bit early to work on this PR now. It's best to let us agree on which method to choose in the end first, to be able to use the best method and avoid wasting some time.
Anyway, thanks for your contribution!

fwienber · 2023-02-03T16:22:48Z

It might be a bit early to work on this PR now. It's best to let us agree on which method to choose in the end first, to be able to use the best method and avoid wasting some time. Anyway, thanks for your contribution!

This is why this PR is a "draft". I think a (draft) PR is the better place to discuss and try out concrete solutions, because it literally contains the code changes. I felt the issue was getting too long and into too much detail. But if you want, we can continue the discussion there.

When babel-helpers are inlined, classPrivateFieldLooseKey() defines a global variable `id` and initialized it to zero in every generated file. This can lead to name clashes between #-private members in subclass and superclass, which again leads to incorrect semantics. In the new solution, instead of trying to produce a globally unique private property name string, a Symbol is created. The helper function classPrivateFieldLooseKey() is no longer used, but kept for backwards-compatibility. In environments where Symbol is not available (Internet Explorer), a polyfill must be included. For details and discussion of other solutions, see babel#15389 For easier review, the test output updates follow in a separate commit.

In all test output files (output.js and output.mjs), replaced 'babelHelpers.classPrivateFieldLooseKey' by 'Symbol'. Tests are green.

fwienber · 2023-02-09T18:40:15Z

All tests are green, ready for review.

fwienber · 2023-02-14T10:46:49Z

@liuxingbaoyu what do you think? In the discussion in the corresponding issue, it seemed to me that everyone agrees that emitting Symbol (in contrast to using it directly in helper code) is fine.

liuxingbaoyu

My take on this PR is that we can try it out (see if anyone reports this after it's released, as it's technically a breaking change)

Even though there are two approvals now, if you want to review it! @nicolo-ribaudo

liuxingbaoyu · 2023-02-14T15:13:20Z

...ate-class-features-plugin/test/fixtures/plugin-proposal-private-methods/loose-true/output.js

@@ -1,4 +1,4 @@
-var _privateMethod = /*#__PURE__*/babelHelpers.classPrivateFieldLooseKey("privateMethod");
+var _privateMethod = /*#__PURE__*/Symbol("privateMethod");


It looks like /*#__PURE__*/ is no longer needed.
https://github.com/babel/babel/pull/15393/files#diff-328fd0199b81baa72df150181152575e627b62d082281c6b5ac8f0075a7174b6R88

I'm just about to remove the PURE comment for the new Symbol output and wondered whether it is needed for the existing standard solution, which generates new WeakMap() also with a PURE annotation. Shouldn't optimizer tools like Uglify know that creating a (built-in) WeakMap has no side-effects?

I'm talking about output like this:
https://github.com/babel/babel/blob/main/packages/babel-helper-create-class-features-plugin/test/fixtures/plugin-proposal-private-methods/loose-false/output.js#L1

nicolo-ribaudo · 2023-02-14T15:48:36Z

I'm on the fence with this. In theory it should be good, but technically it's a breaking change. Maybe we could introduce this under a new privateFieldsAsSymbols assumption, and leave the existing behavior intact. We can then deprecate the old assumption documenting the problem, and force people to switch in Babel 8.

We will release a new minor roughly at the end of the week, and we could inclue the new assumption in that version.

liuxingbaoyu · 2023-02-14T15:52:31Z

Will the new assumptions be enabled by default? I'm afraid a lot of people haven't noticed it.

nicolo-ribaudo · 2023-02-14T15:54:25Z

Well, not even the old privateFieldsAsProperties was enabled by default. A possible timeline is:

7.21.0 - introduce the new assumption and document the problem in privateFieldsAsProperties
7.22.0 - if @babel/plugin-transform-runtime is not enabled, warn that privateFieldsAsProperties is deprecated
8.0.0 - remove privateFieldsAsProperties

fwienber · 2023-02-16T14:16:33Z

Sounds good!
So how do we proceed with this? I could try to make the corresponding code changes and adapt the PR. Or is anyone else going to take over? I don't want to retreat, just want to avoid duplicate work.

liuxingbaoyu · 2023-02-16T14:24:38Z

Feel free to do it!

nicolo-ribaudo · 2023-02-16T14:28:37Z

Feel free to make the changes here! If you need an example of how to define a new assumption option, you can check 7e50ee2.

nicolo-ribaudo · 2023-02-16T14:37:34Z

I can work on docs for this PR.

.

fwienber · 2023-02-16T15:36:36Z

Well, not even the old privateFieldsAsProperties was enabled by default. A possible timeline is:

7.21.0 - introduce the new assumption and document the problem in privateFieldsAsProperties

7.22.0 - if @babel/plugin-transform-runtime is not enabled, warn that privateFieldsAsProperties is deprecated

8.0.0 - remove privateFieldsAsProperties

What about the loose property of plugin-proposal-class-properties? If I get it right, it currently enables privateFieldsAsProperties. I am aware that "assumptions" are now the preferred way to do this, so would the behavior of using "loose" stay as-is in Babel 7 and change to enable privateFieldsAsSymbols instead in Babel 8?

nicolo-ribaudo · 2023-02-16T15:42:56Z

Yes. Or we might even just remove the loose option in Babel 8.

fwienber · 2023-02-17T07:38:12Z

More questions: If in 7.21.0/7.220, both privateFieldsAsProperties and privateFieldsAsSymbols are set, which one should win? Should there be a warning in that case that the other assumption was ineffective?

fwienber · 2023-02-17T11:48:01Z

Since the new approach to introduce a dedicated assumption for using Symbol properties is quite different, I started a fresh branch and created a new PR #15435. I copied all relevant information (e.g. link to documentation PR), I hope I didn't miss anything. Let's continue there.

fwienber force-pushed the fix-15389-privateFieldsAsProperties branch from b49fc7f to a2656af Compare February 3, 2023 13:07

This comment was marked as outdated.

Sign in to view

fwienber force-pushed the fix-15389-privateFieldsAsProperties branch from a2656af to c5b62b3 Compare February 9, 2023 09:37

fwienber mentioned this pull request Feb 9, 2023

[Bug]: Assumption privateFieldsAsProperties w/o transform-runtime plugin can lead to private field name clashes #15389

Closed

1 task

fwienber force-pushed the fix-15389-privateFieldsAsProperties branch from c5b62b3 to 367a6e4 Compare February 9, 2023 10:55

fwienber added 2 commits February 9, 2023 19:06

test output updates for previous change

34245f2

In all test output files (output.js and output.mjs), replaced 'babelHelpers.classPrivateFieldLooseKey' by 'Symbol'. Tests are green.

fwienber force-pushed the fix-15389-privateFieldsAsProperties branch from 367a6e4 to 34245f2 Compare February 9, 2023 18:07

fwienber marked this pull request as ready for review February 9, 2023 18:36

JLHwung approved these changes Feb 14, 2023

View reviewed changes

liuxingbaoyu previously approved these changes Feb 14, 2023

View reviewed changes

liuxingbaoyu added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Feb 14, 2023

liuxingbaoyu added the PR: Needs Docs label Feb 16, 2023

liuxingbaoyu added PR: New Feature 🚀 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 16, 2023

nicolo-ribaudo added Spec: Class Fields area: assumptions labels Feb 16, 2023

nicolo-ribaudo added this to the v7.21.0 milestone Feb 16, 2023

liuxingbaoyu self-requested a review February 16, 2023 14:40

nicolo-ribaudo mentioned this pull request Feb 16, 2023

Add docs for privateFieldsAsSymbols babel/website#2721

Merged

Merge branch 'babel:main' into fix-15389-privateFieldsAsProperties

bffe4a4

fwienber mentioned this pull request Feb 17, 2023

feat: Implement privateFieldsAsSymbols assumption for classes #15435

Merged

fwienber closed this Feb 17, 2023

liuxingbaoyu removed this from the v7.21.0 milestone Feb 17, 2023

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 20, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 20, 2023

JLHwung removed the PR: Needs Docs label Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: privateFieldsAsProperties global counter (#15389) #15393

fix: privateFieldsAsProperties global counter (#15389) #15393

fwienber commented Feb 1, 2023 •

edited by nicolo-ribaudo

babel-bot commented Feb 1, 2023 •

edited

fwienber commented Feb 1, 2023

This comment was marked as outdated.

liuxingbaoyu commented Feb 3, 2023 •

edited

fwienber commented Feb 3, 2023

This comment was marked as outdated.

fwienber commented Feb 9, 2023

fwienber commented Feb 14, 2023

liuxingbaoyu left a comment •

edited

liuxingbaoyu Feb 14, 2023

fwienber Feb 17, 2023

fwienber Feb 17, 2023

nicolo-ribaudo commented Feb 14, 2023

liuxingbaoyu commented Feb 14, 2023

nicolo-ribaudo commented Feb 14, 2023

fwienber commented Feb 16, 2023

liuxingbaoyu commented Feb 16, 2023

nicolo-ribaudo commented Feb 16, 2023

nicolo-ribaudo commented Feb 16, 2023

fwienber commented Feb 16, 2023 •

edited

nicolo-ribaudo commented Feb 16, 2023

fwienber commented Feb 17, 2023

fwienber commented Feb 17, 2023

		@@ -1,4 +1,4 @@
		var _privateMethod = /#__PURE__/babelHelpers.classPrivateFieldLooseKey("privateMethod");
		var _privateMethod = /#__PURE__/Symbol("privateMethod");

fix: privateFieldsAsProperties global counter (#15389) #15393

fix: privateFieldsAsProperties global counter (#15389) #15393

Conversation

fwienber commented Feb 1, 2023 • edited by nicolo-ribaudo

babel-bot commented Feb 1, 2023 • edited

fwienber commented Feb 1, 2023

This comment was marked as outdated.

liuxingbaoyu commented Feb 3, 2023 • edited

fwienber commented Feb 3, 2023

This comment was marked as outdated.

fwienber commented Feb 9, 2023

fwienber commented Feb 14, 2023

liuxingbaoyu left a comment • edited

Choose a reason for hiding this comment

liuxingbaoyu Feb 14, 2023

Choose a reason for hiding this comment

fwienber Feb 17, 2023

Choose a reason for hiding this comment

fwienber Feb 17, 2023

Choose a reason for hiding this comment

nicolo-ribaudo commented Feb 14, 2023

liuxingbaoyu commented Feb 14, 2023

nicolo-ribaudo commented Feb 14, 2023

fwienber commented Feb 16, 2023

liuxingbaoyu commented Feb 16, 2023

nicolo-ribaudo commented Feb 16, 2023

nicolo-ribaudo commented Feb 16, 2023

fwienber commented Feb 16, 2023 • edited

nicolo-ribaudo commented Feb 16, 2023

fwienber commented Feb 17, 2023

fwienber commented Feb 17, 2023

fwienber commented Feb 1, 2023 •

edited by nicolo-ribaudo

babel-bot commented Feb 1, 2023 •

edited

liuxingbaoyu commented Feb 3, 2023 •

edited

liuxingbaoyu left a comment •

edited

fwienber commented Feb 16, 2023 •

edited