[Bug]: Stage 3 decorators behavior differences from TypeScript

Question

[Bug]: Stage 3 decorators behavior differences from TypeScript

evanw opened this issue 7 months ago · 10 comments

evanw commented 7 months ago

💻

Would you like to work on a fix?

How are you using Babel?

Programmatic API (babel.transform, babel.parse)

Input code

See this repo: https://github.com/evanw/decorator-tests

Configuration file name

No response

Configuration

No response

Current and expected behavior

I'm working on adding stage 3 decorators to esbuild. My first step was to write a lot of tests and run them through the Babel and TypeScript implementations. I'm sending you this issue because I figured you might be interested to learn about the behavior differences that I found. This is not a problem for me as I don't use Babel myself, so feel free to close this issue after reading it if you'd like. Here are the unexpected behaviors I found with Babel's implementation (see the repo for details):

Decorators on class expressions do not yet use the correct name (#16148).
The name provided to decorators on private methods (both static and non-static) is empty. (#16144)
The context object property access exposes the underlying getter and setter instead of the Get and Set abstract operations. (#16201)
The context object for fields (both static and non-static) is missing the addInitializer method. (#16132)
Using a private name within a decorator can cause Babel to emit invalid code containing a syntax error. (#16325)
References to the class name within a decorator return undefined instead of throwing a ReferenceError if a class decorator is present. (#16138)

Caveat: I'm just learning about this feature now, and I'm also not sure how up-to-date the specification is, so it could be that Babel's behavior for some of these is intentional. But if so, that behavior at least differs from TypeScript's implementation, which seems undesirable. FWIW I'm currently mostly planning to align esbuild's behavior to TypeScript since it seems like it matches the specification more closely, so it'd be good to know if Babel's behavior here is actually more correct (e.g. regarding access).

Environment

I can reproduce this by running Babel using https://babel.dev/repl.

Possible solution

No response

Additional context

No response

Answer 1 · 2023-11-23T15:30:23.000Z

Hey @evanw! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

Answer 2 · 2023-11-23T15:45:31.000Z

Thank you very much! If you have time, I would appreciate one examples for the following as I cannot reproduce it:

EDIT: I just noticed the linked repo :)

The name provided to decorators on private methods (both static and non-static) is empty.

Input code:

class A {
  @console.log #m() {}
}

logs

Object {
  access: Object { get: d(e), has: BoundFunctionObject }
  addInitializer: function createAddInitializerMethod(r)
  kind: "method"
  metadata: Object {  }
  name: "#m"
  private: true
  static: false
}

Answer 3 · 2023-11-23T15:50:37.000Z

Also this:

The context object property access exposes the underlying getter and setter instead of the Get and Set abstract operations.

Input code:

class A {
  @console.log accessor x = 2
}

logs:

Object {
  access: Object { get: d(e), set: p(e, t), has: m(e) }
  get: function d(e)
  has: function m(e)
  set: function p(e, t)
  arguments: null
  caller: null
  length: 2
  name: "p"
  prototype: Object { … }
  kind: "field"
  metadata: Object {  }
  name: "f"
  private: false
  static: false
}

and by looking at the number of params of the access methods, they are indeed the Get/Set/Has AOs.

I see the test failing in your repo, I'll investigate why.

Answer 4 · 2023-11-23T15:55:17.000Z

The context object for fields (both static and non-static) is missing the addInitializer method.

From the repo readme (https://github.com/tc39/proposal-decorators/tree/master#adding-initialization-logic-with-addinitializer):

The addInitializer method is available on the context object that is provided to the decorator for every type of value except class fields.

(@pzuraq is this correct?)

Answer 5 · 2023-11-23T16:12:55.000Z

Comment 1

If you have time, I would appreciate one examples for the following as I cannot reproduce it:

The name provided to decorators on private methods (both static and non-static) is empty.

Ah it looks like this was the name on the private method itself, not on the context. That was unclear. Sorry about that. Here's a test case:

const dec = (fn, ctx) => {
  console.log(fn.name, ctx.name)
}
class Foo {
  @dec #foo() { }
}

Babel prints "", "#foo" while TypeScript prints "#foo", "#foo".

Comment 2

Also this:

The context object property access exposes the underlying getter and setter instead of the Get and Set abstract operations.

Here's a reduced test case:

function check(a, b) {
  try {
    const x = a()
    if (x === b) return
    throw new Error(`${x} !== ${b}`)
  } catch (e) {
    console.log(`${a}: ${e}`)
  }
}

const getter = (_, ctx) => {
  check(() => ctx.access.has({ foo: false }), true)
  check(() => ctx.access.has({ bar: true }), false)
  check(() => ctx.access.get({ foo: 123 }), 123)
  check(() => 'set' in ctx.access, false)
}

const setter = (_, ctx) => {
  check(() => ctx.access.has({ foo: false }), true)
  check(() => ctx.access.has({ bar: true }), false)
  check(() => 'get' in ctx.access, false)
  const obj = {}
  ctx.access.set(obj, 123)
  check(() => obj.foo, 123)
  check(() => 'bar' in obj, false)
}

const accessor = (_, ctx) => {
  check(() => ctx.access.has({ foo: false }), true)
  check(() => ctx.access.has({ bar: true }), false)
  check(() => ctx.access.get({ foo: 123 }), 123)
  check(() => {
    const obj = {}
    ctx.access.set(obj, 123)
    return obj.foo
  }, 123)
}

console.log('getter')
class Getter {
  bar = 0
  @getter get foo() { return this.bar }
}

console.log('setter')
class Setter {
  bar = 0
  @setter set foo(x: number) { this.bar = x }
}

console.log('accessor')
class Accessor {
  @accessor accessor foo = 0
}

TypeScript just prints getter, setter, and accessor while Babel prints the following:

getter
() => ctx.access.get({ foo: 123 }): Error: undefined !== 123
setter
() => obj.foo: Error: undefined !== 123
() => 'bar' in obj: Error: true !== false
accessor
() => ctx.access.get({ foo: 123 }): TypeError: attempted to get private field on non-instance
() => { const obj = {}; ctx.access.set(obj, 123); return obj.foo; }: TypeError: attempted to set private field on non-instance

Comment 3

The context object for fields (both static and non-static) is missing the addInitializer method.

From the repo readme (https://github.com/tc39/proposal-decorators/tree/master#adding-initialization-logic-with-addinitializer):

The addInitializer method is available on the context object that is provided to the decorator for every type of value except class fields.

Thanks for the link. Sorry, I had missed that. Here's a small test case:

const field = (_, ctx) => {
    console.log(typeof ctx.addInitializer)
}
class Foo {
  @field foo
  @field static bar
}

Babel prints undefined while TypeScript prints function. From my understanding, addInitalizer should be on fields because of tc39/proposal-decorators#469 (comment):

The spec actually has addInitializer for fields, this was a bit of a mismatch between explainer and spec. Will be updating the explainer about this in the future.

The bug about the mismatch is here: tc39/proposal-decorators#501. It sounds like the specification is correct but in either case, one should be updated to match the other.

Answer 6 · 2023-11-23T20:47:59.000Z

@nicolo-ribaudo the Readme is wrong, the spec is more up to date and has addInitializer on all types. I’ll update the README.

Answer 7 · 2023-12-05T02:40:04.000Z

The context object property access exposes the underlying getter and setter instead of the Get and Set abstract operations.

Regarding this, FYI, babel uses a closure to always explicitly call the getter function of the class property, while ts directly accesses the property on the passed in parameter.
The simplified implementation is as follows.

const Getter = {
  bar: 0,
  get foo() {
    return this.bar;
  },
};

const key = "foo";

const desc = Object.getOwnPropertyDescriptor(Getter, key);
babelAccess = {
  get: function (_this) {
    return desc.get.call(_this);
  },
};

tsAccess = {
  get: function (_this) {
    return _this[key];
  },
};

console.log(babelAccess.get({ foo: 123 }));
console.log(tsAccess.get({ foo: 123 }));

Answer 8 · 2023-12-07T02:44:15.000Z

I also noticed a difference when improving the output code size.
ts will only check whether addInitializer is valid for all decorators of a property, while babel will check for each decorator.
Even babel will still check when an exception is thrown.

let addInitializer;

function callCapturedFunc() {
  addInitializer(() => null);
}

function decMethod(_, context) {
  ({ addInitializer } = context);
  addInitializer(() => null);
}

expect(() => {
  class C {
    @callCapturedFunc
    @decMethod
    m() {}
  }
}).toThrow('attempted to call addInitializer after decoration was finished')

let addInitializer;

function decMethod(_, context) {
  ({ addInitializer } = context);
  addInitializer(() => null);
}

try {
  class C {
    @decMethod
    m() {}
  }
} finally {}

expect(() => {
  addInitializer(() => null);
}).toThrow('attempted to call addInitializer after decoration was finished')

Answer 9 · 2023-12-21T23:05:43.000Z

Here's another one:

#16188

Answer 10 · 2024-03-11T18:07:47.000Z

Closing this issue as the reported behaviour differences have been fixed by the aforementioned PRs. Currently Babel is passing most test in https://github.com/evanw/decorator-tests, except tdz-related class binding tests.