Clarification on field vs. method destination

[evanw]

I'm currently working on writing some tests for decorators in preparation for adding support for them to esbuild. My reading of the specification is that decorators on the class itself can return a new object, which then becomes the ultimate value for the class binding. That happens in ClassDefinitionEvaluation here:

35. Let newF be Completion(ApplyDecoratorsToClassDefinition(F, decorators, className, classExtraInitializers)).
36. If newF is an abrupt completion, then
    1. Set the running execution context's PrivateEnvironment to outerPrivateEnvironment.
    2. Return ? newF.
37. Set F to newF.[[Value]].

When that happens, I believe that causes methods to end up on the old object (because they are added to the old value of F earlier) and fields to end up on the new object (because they are added to the new value of F later). Am I reading the specification correctly? Specifically methods are applied here:

27. For each element e of staticElements, do
    1. If e is a ClassElementDefinition Record and e.[[Kind]] is not field, then
       1. If e.[[Kind]] is accessor, let extraInitializers be e.[[ExtraInitializers]]; otherwise, let extraInitializers be staticMethodExtraInitializers.
       2. Let result be Completion(ApplyDecoratorsAndDefineMethod(F, e, extraInitializers, true)).

and fields are initialized here:

40. For each element elementRecord of staticElements, do
    1. If elementRecord is a ClassElementDefinition Record and elementRecord.[[Kind]] is field or accessor, then
       1. Let result be Completion(InitializeFieldOrAccessor(F, elementRecord)).

Note that this means accessors (which are methods that reference fields) end up being split between the old object and the new object, with the getter/setter on the old object and the storage field on the new object.

Is this intentional? Neither TypeScript nor Babel seem to do this. I wanted to check that my interpretation is correct and that this is by design before locking in my implementation and tests like that. I don't use decorators myself so I don't have any intuition for whether this is a useful thing to do or not. I'm just trying to make an accurate implementation of this specification. It just seems kind of weird that e.g. decorating a class could seemingly break all private accessors in that class.

For example:

let oldObj
let runAsserts

const dec = cls => {
  oldObj = cls
  return class { }
}

const newObj = @dec class {
  static accessor x = 1
  static accessor #y = 2

  static {
    runAsserts = () => {
      assert(newObj !== oldObj)

      // Public accessors
      {
        // Did "oldObj" end up with a getter and setter but not storage?
        assertThrows(() => oldObj.x)

        // Did "newObj" end up with storage but not a getter or setter?
        assert(newObj.x === undefined)

        // I think the only way to access the storage on "newObj" is by using the getter from "oldObj"?
        const get = Object.getOwnPropertyDescriptor(oldObj, 'x').get
        assert(get.call(newObj) === 1)
      }

      // Private accessors
      {
        // Did "oldObj" end up with a getter and setter but not storage?
        // If so, then this throws because the getter can't access the storage.
        assertThrows(() => oldObj.#y)

        // Did "newObj" end up with storage but not a getter or setter?
        // If so, this throws because the getter isn't present.
        assertThrows(() => newObj.#y)

        // Maybe there's no way to access "#y" here at all?
      }
    }
  }
}

runAsserts()

function assert(it) {
  if (!it) console.log(`Failure: ${it}`)
}

function assertThrows(it) {
  try { it() } catch { return }
  console.log(`Expected ${it} to throw`)
}

[nicolo-ribaudo]

This is intentional: #329

Babel implements this for normal fields, but not for the accessor internal private field: this is a problem, because it makes it impossible to use an accessor with decorators that return a subclass. EDIT: It looks like Babel implements this as defined in the proposal: https://babeljs.io/repl#?browsers=defaults%2C%20not%20ie%2011%2C%20not%20ie_mob%2011&build=&builtIns=false&corejs=3.6&spec=false&loose=false&code_lz=AICg1gBAvAfBDGAbAhgZ1RApgDwC6YDsATDSAbwF8BKAKCTQwEEIyaIJVdlcBLeCZPHiZ0AewBOEbNAgAmANw0KNGkA&debug=false&forceAllTransforms=false&modules=false&shippedProposals=false&circleciRepo=&evaluate=true&fileSize=false&timeTravel=false&sourceType=script&lineWrap=true&presets=&prettier=false&targets=Node-0.10&version=7.24.5&externalPlugins=%40babel%2Fplugin-proposal-decorators%407.24.1&assumptions=%7B%7D

Also I wonder, can static private methods be called in any way when there is a class decorator that returns a subclass?

[pzuraq]

This is intentional, class decorators should have access to the class when it is as complete as possible. The only reason that class decorators do not run after fields/static blocks is because you can end up in a split world, e.g.

class C {
  static f = new C();
}

const c = new C();

If decorators ran after fields are assigned, then c and C.f would not be the same value if C was replaced by a class decorator. This would be very confusing, more so than not having the completely finalized class.

Class extra initializers (addInitializer) plug that timing gap, they enable decorators to run some code after all static class fields have been assigned.

[evanw]

It looks like Babel implements this as defined in the proposal

It seems like perhaps Babel's implementation of MakeAutoAccessorGetter and MakeAutoAccessorSetter are incorrectly bound to the original class object instead of this. Compare this auto-accessor to this manual accessor. The tests are the same in both and I'd expect both to behave the same, but the first one fails and the second one passes. My reading of MakeAutoAccessorGetter is that it's supposed to use this, which can then change at run-time:

1. Let getterClosure be a new Abstract Closure with no parameters that captures privateStateName and performs the following steps when called:
   1. Let o be the this value.
   2. Return ? PrivateGet(o, privateStateName).
2. Let getter be CreateBuiltinFunction(getterClosure, 0, "get", « »).
3. Perform SetFunctionName(getter, name, "get").
4. Perform MakeMethod(getter, homeObject).
5. Return getter.

One easy way to tell is that the getter (which can be accessed via Object.getOwnPropertyDescriptor) shouldn't be able to be applied to arbitrary objects that don't have the underlying accessor storage private name.

Note: I'm using Firefox and it considers the code that Babel generates to be a syntax error (specifically reference to undeclared private field or method). Chrome and Safari both run the code that Babel generates, so I'm guessing this is a bug in Firefox. I'll have to report that as well. But anyway don't use Firefox to test this.

[nicolo-ribaudo]

Chrome and Safari both run the code that Babel generates, so I'm guessing this is a bug in Firefox. I'll have to report that as well. But anyway don't use Firefox to test this.

(off topic for this discussion, I'll hide it when you react to this comment)

Already reported, and it's already fixed in Firefox nightly I think :) When running with preset-env Babel applies a plugin to workaround that firefox bug -- it's not enabled in that REPL link because I only added the decorators plugin.

[nicolo-ribaudo]

@pzuraq About this in auto accessor, it seems like the original intention was to not use this? #468 (comment)

[pzuraq]

@nicolo-ribaudo yes that was the intention, I think I misunderstood what the _homeObject_ was doing when I closed that thread. I'll work on an update to the spec, hopefully for the next meeting in June.

[evanw]

I don't see anything about decorators on the June agenda. Does this need to be added to that agenda for it to be addressed? Sorry, I'm not too familiar on how TC39 meetings work.

[pzuraq]

@evanw unfortunately I didn't get around to making the updates in time. I don't work full time on TC39 stuff anymore unfortunately, haven't for the past couple of years, so I have been pushing along the spec on mostly personal time. I usually find a chunk of time every several months or so to do maintenance, address issues, etc. This last chunk was spent writing the tests for decorators in test262, which were important for implementations to be able to get all the nuances of timing and such done correctly.

There are a few small updates like this one that need to make it into the spec, and that writing the tests has helped identify, so I'll be bundling those together for the next plenary after June. Hopefully now that we have tests, this will be the last round of updates.