Skip to content

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

@pzuraq

Description

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions