Was there a purpose for non-lexical ordering of decorator applications?

[pzuraq]

Given how many discussions we've had around ordering, I want to be clear up front that this issue is NOT a intended re-litigation of any of the previous discussions.

This is about a specific ordering that happens in the spec currently that does not appear to have been really discussed anywhere, and that I can't remember the reason for. I do not think this ordering must change, but it is a bit odd and I thought I'd open this issue to see if anyone does remember the reason for this, or if it was an oversight during the writing of the spec.

To recap, these are the main steps of decorator application:

1. Decorator expressions are evaluated to get the final functions that need to be applied
2. Class elements are evaluated to pass into decorators
3. Decorator functions are called with their respective class elements
4. For methods, the result of the decorator function is defined on the class immediately
5. For static fields, the initializer returned by the decorator is called after class decorators are run and we have the final class, and the result of the initializer is set on the class instance
6. For instance fields, the result of the decorator is called is called during class instance evaluation, and the result of the initializer is set on the class instance

We're talking about step 3 in this issue, which is the following in spec:

        1. 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_.
            1. Let _result_ be Completion(ApplyDecoratorsAndDefineMethod(_F_, _e_, _extraInitializers_, *true*)).
            1. If _result_ is an abrupt completion, then
              1. Set the running execution context's PrivateEnvironment to _outerPrivateEnvironment_.
              1. Return ? _result_.
        1. For each element _e_ of _instanceElements_, do
          1. If _e_.[[Kind]] is not ~field~, then
            1. If _e_.[[Kind]] is ~accessor~, let _extraInitializers_ be _e_.[[ExtraInitializers]]; otherwise, let _extraInitializers_ be _instanceMethodExtraInitializers_.
            1. Let _result_ be Completion(ApplyDecoratorsAndDefineMethod(_proto_, _e_, _extraInitializers_, *false*)).
            1. If _result_ is an abrupt completion, then
              1. Set the running execution context's PrivateEnvironment to _outerPrivateEnvironment_.
              1. Return ? _result_.
        1. For each element _e_ of _staticElements_, do
          1. If _e_.[[Kind]] is ~field~, then
            1. Let _result_ be Completion(ApplyDecoratorsToElementDefinition(_F_, _e_, _e_.[[ExtraInitializers]], *true*)).
            1. If _result_ is an abrupt completion, then
              1. Set the running execution context's PrivateEnvironment to _outerPrivateEnvironment_.
              1. Return ? _result_.
        1. For each element _e_ of _instanceElements_, do
          1. If _e_.[[Kind]] is ~field~, then
            1. Let _result_ be Completion(ApplyDecoratorsToElementDefinition(_proto_, _e_, _e_.[[ExtraInitializers]], *false*)).
            1. If _result_ is an abrupt completion, then
              1. Set the running execution context's PrivateEnvironment to _outerPrivateEnvironment_.
              1. Return ? _result_.

As you can see, we iterate through the static elements of the class twice, and the instance elements of the class twice, so the ordering is:

1. Static method, getter, setter, and accessor decorators are applied, and the methods are defined
2. Instance method, getter, setter, and accessor decorators are applied, and the methods are defined
3. Static field decorators are applied, but NOT defined
4. Instance field decorators are applied, but NOT defined

I can't remember the reason for this change, vs running all decorator applications in lexical order. This change happened around the time we added addInitializer and removed @init: from the proposal, so I think that it could have been that I was trying to encode the ordering of initializers added via addInitializer here, and I didn't realize at the time that this would impact decorator application order in an observable way (or potentially I thought that it would, and it seemed correct at the time to do so).

Reviewing this again, I can't really see a good reason to order these non-lexically. Applications happen at class definition time, so they shouldn't impact the actual order of assignment to the class/prototype/instance, nor the order of initialization via addInitializer, especially now that addInitializer for fields and accessors run immediately after the field/accessor is assigned. The only observable differences between this ordering and purely lexical ordering are:

1. Decorator call order, which could reference some shared global state that could in theory be used to affect behavior?
2. Metadata from the metadata proposal will be affected by methods/accessors before fields, so that could also affect behavior

I'm not sure if there are any real use cases here though. In theory, you could side-channel information like type info from one decorator to another via metadata, which is a totally valid use case, but it's hard to imagine that this ordering would help that in particular. Would you really need to have all method metadata before fields run? What about accessor fields? They behave more like fields anyways, so wouldn't they also benefit from method metadata as well? It feels like for any case where you would want to side channel this information, you might run into issues with ordering (e.g. method2 wants metadata from method1, which would be an issue in either case) so I'm not sure how ensuring fields have access to method metadata would help that.

Like I said at the beginning, I'm NOT saying this is a critical issue, I just wanted to see if anyone else remembers the context for this decision, if it made sense at the time, still makes sense, etc. My feeling is that the current ordering vs lexical ordering won't have much of an impact in the majority of use cases, it just might be a bit odd for those edge cases where a decorator does want to rely on it, and it could be more complex to implement and makes the spec a bit more complex. I also don't think it would be a massive deal to change, because again, I don't think many decorators would actually be relying on this ordering, but I could definitely be wrong there!

[pzuraq]

Note to clarify: This is not about lexical ordering of decorators for a given element. If multiple decorators are applied to an element, those should definitely be applied in reverse-lexical order (e.g. from innermost to outermost), that was very intentional. This issue is solely discussing the ordering of application to elements.

[rbuckton]

IIRC, the class element ordering just mirrors the order in which class elements are defined, which maintains lexical ordering within a group, where the groups and order are:

1. static non-field
2. non-static non-field
3. static field
4. non-static field

This order can potentially cause confusion with respect to side-effects during decorator application:

let counter = 1;
function foo(target, context) {
  const incr = counter++;
  const init = value => value + counter;
  return context.kind === "field" ? init : { init };
}

class C {
  @foo x = 0;
  @foo accessor y = 0;
  @foo z = 0;
}

const obj = new C();
console.log(obj.x, obj.y, obj.z); // prints: ???

Here, the user might intuit that the @foo decorator will be applied in order of x, y, z, and thus expect that the above code would emit 1 2 3 due to the side-effect resulting from incrementing counter. However, since accessor declarations produce get and set methods, they are grouped with other non-field elements and thus does not match developer intuition.

Changing element order for decorator application to be document order instead of by group would restore this intuition.

In addition, decorator application order affects the order in which "extra" initializers are evaluated. Currently, addInitializer can only add extra initializers based on the placement of the element (i.e., static/non-static), so grouping vs. lexical order has no effect. However, in #465 we have been discussing a future add-on proposal to allow you to override addInitializer placement. If we were to advance this proposal while still maintaining the current grouping order, we would again break developer intuition:

class C {
  @((t, c) => c.addInitializer(() => console.log("A")))
  static x() {}

  @((t, c) => c.addInitializer(() => console.log("B"), "static")) // override placement
  y() {}

  @((t, c) => c.addInitializer(() => console.log("C")))
  static z() {}
}

A user might expect these callbacks to be evaluated in the order A B C. However, if we maintain grouped ordering then the callbacks would be evaluated in the order A C B.

It's important to note that, if we do change the order from groups to lexical ordering, class decorators will still continue to be applied after all class element decorators. Since class decorators receive a reference to the class constructor, all replacements resulting from static and prototype method decorators must already have been applied.

In summary, I would support changing application order to reflect the following:

1. All decorator expressions (class and class element) are evaluated in lexical document order (current behavior).
2. Within the class body, we perform decorator application against each element in lexical document order (changed behavior).
3. For a given class element, we apply each of its decorator expression in reverse order (current behavior).
4. Perform decorator application against the class in reverse order (current behavior).