Implement decorators as presented at 2023-01 TC39 meeting

[nicolo-ribaudo]

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]

@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]

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

[nicolo-ribaudo]

@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]

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.

[nicolo-ribaudo]

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]

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

[JLHwung]

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

[JLHwung]

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]

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]

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]

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]

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]

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

[JLHwung]

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]

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]

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]

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