tc39/proposal-decorators

Normative updates to the spec, March 2023

pzuraq opened this issue · 7 comments

This issue is a meta issue for tracking a number of normative updates that are being proposed to the spec in the upcoming March plenary.

  1. Remove the dynamic assignment of [[HomeObject]] from decorator method application/evaluation (PR, Issue)

    This dynamic assignment/update of a function's [[HomeObject]] is a new behavior that has not occurred before in the spec, and is even asserted against. It results in unexpected behavior and does not have an actual use-case. This decision was likely due to a misunderstanding of what the MakeMethods purpose was. This change removes the reassignment entirely.

  2. Call decorators with their natural this value instead of undefined (PR, Issue)

    Currently decorators are always called with an undefined this value, even when it might appear that they should have one (e.g. @foo.bar should be called with foo as the receiver). This change threads the receiver through so the decorator can be called with it.

  3. Throw an error if the value passed to addInitializer is not callable (PR)

    addInitializer expects to receive a function, and all of the spec text downstream from it assumes this as well, so this assertion is necessary to ensure that the value is actually callable.

  4. Set the name of the addInitializer function (PR)

    Sets the name of addInitializer, which is currently an empty string.

  5. Remove SetFunctionName from decoration (PR)

    Currently, SetFunctionName is called on the returned decorated methods. This two main problems:

    1. It messes up stack traces, because the actual function name will be different than the code, which would be confusing when trying to debug.
    2. SetFunctionName asserts against ever being called twice for the same method.
  6. "Bind" static accessors directly to the class itself. (PR pending, issue)

    Static accessors are implemented conceptually as a sugar on top of private fields. A getter and setter are generated which access private storage on the same class. For static accessors, this results in them not working when inherited by a child class, since static fields and accessors are not redefined on child classes.

The proposed update is to "bind" static accessors to their original class, e.g. instead of being sugar for:

class A {
  static #x = 42
  static get x() { return this.#x }
  static set x(v) { this.#x = v }
}

They would desugar to:

class A {
  static #x = 42
  static get x() { return A.#x }
  static set x(v) { A.#x = v }
}

5. Remove assert from SetFunctionName so function names can be updated (PR)

I'm not sure I agree with this one, since you can return a shared function:

function noop() {}
function conditional(cond) {
  return function (target, context) {
    return cond ? target : noop;
  };
}

class C {
  @conditional(true) static a() {}
  @conditional(false) static b() {}
  @conditional(false) static c() {}
}

console.log(C.a.name, C.b.name, C.c.name);

Without this change, this would log a noop noop. After this change, this would log a c c, and would affect every class using @conditional(false), not just the local one.

There's no way to tell whether the returned function is safe to be renamed in this fashion. I'd much rather someone explicitly do this instead if changing the name is truly important:

function dec(m) {
  function x() {
    return m.call(this, ...arguments);
  }
  Object.defineProperty(x, "name", {
    ...Object.getOwnPropertyDescriptor(x, "name"),
    value: typeof m.name === "symbol" ? `[${m.name.toString()}]` : m.name
  });
  return x;
}

Yes, it's a bit more cumbersome, but it's also more predictable.

I think I'm coming around to that with more discussion that's been going on, I was worried this would result in a lot of problematic debugging behavior but it does seem like changing the name would actually mess with stack traces even more (see discussion here: #502)

Probably should include static accessors on this list as well.

All of these changes gained consensus in the plenary today, I will be merging them into the updated spec as soon as I have time

Hello, @pzuraq, can you at least describe final consensus? Seems like this issues blocks at least esbuild implementation, which is blocks all users of vite from using new decorators...

pzuraq commented

As noted above, all of the proposed changes were accepted. I’ve been very busy lately so haven’t had time to update the spec, but I’ll try to get to it today.

pzuraq commented

All updates have been merged. For update 6, see the linked PR for more details(comment)