Implement decorators as presented at `2023-01` TC39 meeting #15405

nicolo-ribaudo · 2023-02-03T14:48:55Z

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

The first few commits are just boilerplate to add a new decorators version:

added the version: "2023-01" option
copied the previous helper into applyDecs2301
wrapped the previous helper in a function to avoid naming conflicts between functions in different helpers
copied the tests from the previous version

The important commits are:

[3cf9ebd] Update signature of access.get and access.set, implementing the first part of context.access API implementation feedback. tc39/proposal-decorators#494
[8b7fd2b] Add .has method to context.access, implementing the second part of context.access API implementation feedback. tc39/proposal-decorators#494
[584c281] Brand check the receiver of private access.get/access.set, an existing bug that I spotted and could easily fixed thanks to the new access to .has.
[7194dc7] Parse decorators before/after export if decoratorsBeforeExport is not specified

When reviewing the tests, please review these important commits one by one to properly see the relative diff.

These changes got consensus during the last TC39 meeting, but there is no spec yet for them. This PR shouldn't be merged until then (@DanielRosenwasser @rbuckton ping me if you need a review for the spec PR!).

@DanielRosenwasser @rbuckton When defining .has for public class members, I used HasProperty instead of HasOwnProperty so that it's consistent with the usage of Get in the .get method. Does that match your expectations?

Some questions for reviewers:

right now this PR allows decorators both before&after export only when the decoratorsBeforeExport option is not explicitly set to true or false. Technically it's backward compatible to just ignore that option and always allow non-legacy decorators in both places; should we do it? It would slightly simplify the implementation.
right now @babel/generator always prints decorators after export (unless the generator option decoratorsBeforeExport: true is specified). We don't make any guarantee regarding the output format so I think this is fine, however swapping decorators with the export keyword when printing has the unfortunate effect of removing some comments. Do you think we should print decorators in the same position as they appeared in the input source code?

rbuckton · 2023-02-03T15:59:14Z

@DanielRosenwasser @rbuckton When defining .has for public class members, I used HasProperty instead of HasOwnProperty so that it's consistent with the usage of Get in the .get method. Does that match your expectations?

Yes, the expectation is to match <name> in arg, which uses HasProperty for non-private names and PrivateElementFind for private names.

babel-bot · 2023-02-03T16:05:12Z

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

nicolo-ribaudo · 2023-02-03T18:16:32Z

@liuxingbaoyu I don't think that the Jest failure is related, but the last commit in that test is yours :P

https://github.com/facebook/jest/blob/main/packages/jest-core/src/__tests__/collectHandles.test.js

JLHwung

Changes lgtm. Left some comments about emitting footprints and context access performance.

JLHwung · 2023-02-03T18:18:18Z

packages/babel-plugin-proposal-decorators/src/transformer-2023-01.ts

+            injectHasChecks &&
+              (template.expression.ast`
+                _ => ${t.cloneNode(key)} in _
+              ` as t.ArrowFunctionExpression),


Can we generate staticPrivateHas from the first argument of applyDecs? It seems to
me it suffices to just memoise

_ => babelHelpers.checkInRHS(_) === Class

Then we can consider generate only privateHas and reuse it for other private elements. The assumption here is that no plugins are removing private elements without preserving the privateIn checks. So the interface can be:

const privateHas = _ => #p in _; applyDecs2301(Class, privateHas, [decs])

If that assumption is not practical, we can insert our own private class brand.

Edit: response hidden because the way we compile `in` checks for static private fields is already broken in this edge case anyway

Technically you can do something like this:

let A; let accessX; let accessY; try { class _A { @((d, c) => (accessX = c.access, d)) static #x; static { A = _A; throw 0; } @((d, c) => (accessX = c.access, d)) static #y; } } catch {} assert(accessX.has(A) === true); assert(accessY.has(A) === false);

but we can probably assume by default that class evaluation finishes (or if it throws, you are not going to use the class binding for anything).

JLHwung · 2023-02-03T19:00:42Z

packages/babel-helpers/src/helpers/applyDecs2301.js

 }

+function assertInstanceIfPrivate(has, target) {
+  if (has && !has(target)) {


assertInstanceIfPrivate is a runtime call. Can we call assertInstanceIfPrivate only for private element? So we don't have to check has.

JLHwung · 2023-02-03T19:03:39Z

packages/babel-helpers/src/helpers/applyDecs2301.js

    // Assert: If kind === 0, then isPrivate is true.
    var t = kind === 0 /* FIELD */ || kind === 1; /* ACCESSOR */
    if (t || kind === 3 /* GETTER */) {
      get = function (target) {


We can generate a get with assertInstanceIfPrivate if isPrivate, so multiple context.access.get call does not require multiple if(has) check.

I mean:

if (isPrivate) { get = function (target) { desc.get.call(assertInstanceIfPrivate(hasPrivateBrand, target)); } } else { get = function (target) { desc.get.call(target); } }

because isPrivate is known at compile time, the check should not be duplicated in context.access.get.

nicolo-ribaudo · 2023-02-04T10:15:45Z

I also added a commit to (only with 2023-01, to avoid potentially breaking changes to the previous versions) use arrow functions instead of plain functions when generating the get/set functions for private fields. This saves a few bytes per private field when compiling to a target that supports arrow functions natively.

liuxingbaoyu · 2023-02-06T07:34:09Z

right now this PR allows decorators both before&after export only when the decoratorsBeforeExport option is not explicitly set to true or false. Technically it's backward compatible to just ignore that option and always allow non-legacy decorators in both places; should we do it? It would slightly simplify the implementation.

Both look fine to me.

right now @babel/generator always prints decorators after export (unless the generator option decoratorsBeforeExport: true is specified). We don't make any guarantee regarding the output format so I think this is fine, however swapping decorators with the export keyword when printing has the unfortunate effect of removing some comments. Do you think we should print decorators in the same position as they appeared in the input source code?

I tend to keep same positions.
At the same time we can provide a way to allow users to know the location without node.loc. (New AST properties? New node.extra?)

But this brings up a new question, how does @babel/types work?
When a plugin creates a new node, it is difficult to know whether the existing one comes before or after, and having both in one file is not ideal.

rbuckton · 2023-02-06T14:29:37Z

This PR shouldn't be merged until then (@DanielRosenwasser @rbuckton ping me if you need a review for the spec PR!).

I have a PR up against @pzuraq's ecma262 PR: pzuraq/ecma262#4

rbuckton · 2023-02-06T16:11:08Z

right now @babel/generator always prints decorators after export (unless the generator option decoratorsBeforeExport: true is specified). We don't make any guarantee regarding the output format so I think this is fine, however swapping decorators with the export keyword when printing has the unfortunate effect of removing some comments. Do you think we should print decorators in the same position as they appeared in the input source code?

I tend to keep same positions. At the same time we can provide a way to allow users to know the location without node.loc. (New AST properties? New node.extra?)

FWIW, TypeScript will preserve the original order for --target ESNext since there is a minor semantic difference between @dec export class and export @dec class. The former will not include @dec in a call to Function.prototype.toString, while the latter will.

nicolo-ribaudo · 2023-02-06T18:32:53Z

Oh right, thanks for the heads up! It's definitely something we have to preserve then.

JLHwung · 2023-02-06T18:54:36Z

packages/babel-helpers/src/helpers/applyDecs2301.js

+        initializers = staticInitializers;
+      }
+      if (!staticBrand) {
+        staticBrand = function (_) {


The staticBrand should be assigned only when we see a private static element.

class A { static p } class B { @dec static p } function dec(_, context) { // Should return true though A != B // to mirror the semantic Reflect.has(%, "p") context.access.has(A) }

nicolo-ribaudo · 2023-02-09T17:28:02Z

The proposal has been updated!

liuxingbaoyu

I think it might be better to keep the order in node.extra, but it doesn't matter.

nicolo-ribaudo · 2023-02-13T15:12:08Z

I think it might be better to keep the order in node.extra, but it doesn't matter.

We usually only store stuff in node.extra when someone (either Babel or Prettier usually) needs it; we can add it when we have a real-world use case :)

fisker · 2023-02-17T01:12:33Z

We usually only store stuff in node.extra when someone (either Babel or Prettier usually)

Prettier use the same approach, checking the declaration and first decorator has same start location https://github.com/prettier/prettier/blob/5ea9a8f81f40abfd12dbc4106e3080c7608e9dd5/src/language-js/print/decorators.js#L77

… not specified

nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories Spec: Decorators labels Feb 3, 2023

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

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

nicolo-ribaudo force-pushed the deco-2023-01 branch from d3bed4d to 584c281 Compare February 3, 2023 16:02

nicolo-ribaudo marked this pull request as ready for review February 3, 2023 17:00

JLHwung approved these changes Feb 3, 2023

View reviewed changes

nicolo-ribaudo force-pushed the deco-2023-01 branch from bfa119e to d1484ad Compare February 4, 2023 09:51

nicolo-ribaudo mentioned this pull request Feb 6, 2023

Add docs for 2023-01 decorators babel/website#2715

Merged

nicolo-ribaudo added PR: Needs Review A pull request awaiting more approvals and removed PR: Needs Docs labels Feb 6, 2023

JLHwung reviewed Feb 6, 2023

View reviewed changes

rbuckton mentioned this pull request Feb 8, 2023

Add normative changes from January 2023 plenary pzuraq/ecma262#4

Merged

liuxingbaoyu approved these changes Feb 11, 2023

View reviewed changes

nicolo-ribaudo added PR: Needs Review A pull request awaiting more approvals PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release and removed PR: Needs Review A pull request awaiting more approvals labels Feb 13, 2023

nicolo-ribaudo force-pushed the deco-2023-01 branch from d22c139 to 1e5ccdb Compare February 13, 2023 15:16

nicolo-ribaudo removed the PR: Needs Review A pull request awaiting more approvals label Feb 13, 2023

nicolo-ribaudo added 18 commits February 18, 2023 15:13

Copy 2022-03 tests to 2023-01

c4326da

Add 2023-01 version to decorators plugin

7a2693a

Create new version of the helper

85ef645

Update makefile

f6cbbc9

Update signature of access.get and access.set

05bad66

Add .has method to context.access

ee612e9

Brand check the receiver of private access.get/access.set

e98a291

Parse decorators before/after export if decoratorsBeforeExport is…

fa11406

… not specified

Add comments attachment tests to generator

09c23e8

make build

9798774

Don't pass decoratorsBeforeExport with version: 2023-01

6212054

Fix Babel 8

725ae19

Update flow and ts parser tests

97b3b88

Review

5c7d777

Use arrow functions for get/set fns

a38aded

Preserve decorators position when printing

700ba8f

Review by JLHwung

0797d93

Don't run test with removed option in babel 8

8afed11

nicolo-ribaudo force-pushed the deco-2023-01 branch from 1e5ccdb to 8afed11 Compare February 18, 2023 14:14

Update 3rd party parser tests

0801742

nicolo-ribaudo changed the title ~~Implement decorators version presented at 2023-01 TC39 meeting~~ Implement decorators as presented at 2023-01 TC39 meeting Feb 18, 2023

nicolo-ribaudo merged commit eae47c9 into babel:main Feb 18, 2023

nicolo-ribaudo deleted the deco-2023-01 branch February 18, 2023 15:50

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

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

Uh oh!

Implement decorators as presented at 2023-01 TC39 meeting #15405

Implement decorators as presented at 2023-01 TC39 meeting #15405

Uh oh!

Conversation

nicolo-ribaudo commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Feb 3, 2023

Uh oh!

babel-bot commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JLHwung left a comment

Choose a reason for hiding this comment

Uh oh!

JLHwung Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLHwung Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

JLHwung Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

JLHwung Feb 6, 2023

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Feb 4, 2023

Uh oh!

liuxingbaoyu commented Feb 6, 2023

Uh oh!

rbuckton commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Feb 6, 2023

Uh oh!

nicolo-ribaudo commented Feb 6, 2023

Uh oh!

JLHwung Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Feb 9, 2023

Uh oh!

liuxingbaoyu left a comment

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo commented Feb 13, 2023

Uh oh!

fisker commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Implement decorators as presented at `2023-01` TC39 meeting #15405

Implement decorators as presented at `2023-01` TC39 meeting #15405

nicolo-ribaudo commented Feb 3, 2023 •

edited

Loading

babel-bot commented Feb 3, 2023 •

edited

Loading

nicolo-ribaudo commented Feb 3, 2023 •

edited

Loading

nicolo-ribaudo Feb 4, 2023 •

edited

Loading

rbuckton commented Feb 6, 2023 •

edited

Loading

JLHwung Feb 6, 2023 •

edited

Loading

fisker commented Feb 17, 2023 •

edited

Loading