[Bug]: Decorator execution order does not match evaluation order @babel/plugin-proposal-decorators

[pgoforth]

💻

• Would you like to work on a fix?

How are you using Babel?

babel-loader (webpack)

Input code

function logAccessorDecoratorRun(log, name) {
    log.push(`${name}: generator`);
    return function (target, { addInitializer }) {
        log.push(`${name}: wrapper`);
        addInitializer(function () {
            log.push(`${name}: addInitializer`);
        });
        const { init, set } = target;
        return {
            init(initialValue) {
                log.push(`${name}: init: ${initialValue} - ${init}`);
                return init ? init.call(this, initialValue) : initialValue;
            },
            set(val) {
                log.push(`${name}: value: ${val}`);
                return set ? set.call(this, val) : val;
            },
        };
    };
}

describe('accessor-initializer-init-setter', () => {
    it('should maintain the correct order for accessor initialization and setting', () => {
        const log = [];

        class FakeClass {
            @logAccessorDecoratorRun(log, 'first')
            @logAccessorDecoratorRun(log, 'second')
            accessor fakeField = 'fakeValue';
        }

        const instance = new FakeClass();
        instance.fakeField = 'foo';

        expect(log).toEqual([
            // Begin in order here: first -> second
            'first: generator',
            'second: generator',
            // Reverse order here: second -> first
            'second: wrapper',
            'first: wrapper',
            'second: addInitializer',
            'first: addInitializer',
            'second: init: fakeValue - undefined',
            'first: init: fakeValue - undefined',
            /*
             *  Should maintain order: second -> first
             *  ...but is swapped back to: first -> second
             *  ...and next 2 lines fail
             */
            'second: value: foo',
            'first: value: foo',
        ]);
    });
});

Configuration file name

babel.config.js

Configuration

module.exports = {
    presets: ['@babel/preset-env'],
    plugins: [
        [
            '@babel/plugin-proposal-decorators',
            {
                version: '2022-03',
            },
        ],
        '@babel/plugin-proposal-class-properties',
        '@babel/plugin-proposal-class-static-block',
        '@babel/plugin-proposal-optional-catch-binding',
        '@babel/plugin-proposal-nullish-coalescing-operator',
        '@babel/plugin-proposal-object-rest-spread',
        '@babel/plugin-proposal-private-methods',
        '@babel/plugin-proposal-optional-chaining',
        '@babel/plugin-proposal-unicode-property-regex',
        '@babel/plugin-syntax-export-default-from',
        '@babel/plugin-transform-async-to-generator',
        '@babel/plugin-transform-flow-strip-types',
    ],
};

Current and expected behavior

When using @babel/plugin-proposal-decorators at version 2022-03

Behavior

Using the following decorator:

function coerce(process) {
    return function (target) {
        const { init, set } = target;
        return {
            ...target,
            set(value) {
                return set.call(this, process(value));
            },
            init(initialValue) {
                return (init && init.call(this, process(initialValue))) ?? process(initialValue);
            },
        };
    };
}

Current

The order of the init method does not match the sequential order of the set method when more than one decorator is applied to the same initialized accessor.

class FakeClass {
    @coerce((val) => `${val}Baz`)
    @coerce((val) => `${val}Bar`)
    accessor value = 'foo';
}
const instance = new FakeClass();
console.log(instance.value); // 'fooBarBaz'
instance.value = 'fee';
console.log(instance.value); // 'feeBazBar'

Expected

The order of the init method should match the sequential order of the set method when more than one decorator is applied to the same initialized accessor.

class FakeClass {
    @coerce((val) => `${val}Bar`)
    @coerce((val) => `${val}Baz`)
    accessor value = 'foo';
}
const instance = new FakeClass();
console.log(instance.value); // 'fooBarBaz'
instance.value = 'fee';
console.log(instance.value); // 'feeBarBaz'

Environment

System:

• OS: macOS 13.3.1

Binaries:

• Node: 18.16.0 - ~/.nvm/versions/node/v18.16.0/bin/node
• npm: 9.5.1 - ~/.nvm/versions/node/v18.16.0/bin/npm

npmPackages:

• @babel/cli: ^7.18.3 => 7.20.7
• @babel/core: ^7.18.3 => 7.21.5
• @babel/eslint-parser: 7.19.1 => 7.19.1
• @babel/plugin-proposal-class-properties: ^7.18.2 => 7.18.6
• @babel/plugin-proposal-class-static-block: 7.20.7 => 7.20.7
• @babel/plugin-proposal-decorators: ^7.18.2 => 7.20.13
• @babel/plugin-proposal-object-rest-spread: ^7.20.2 => 7.20.7
• @babel/plugin-syntax-decorators: 7.19.0 => 7.19.0
• @babel/plugin-syntax-export-default-from: ^7.2.0 => 7.18.6
• @babel/preset-env: ^7.20.2 => 7.21.5
• @babel/runtime: ^7.20.6 => 7.20.13
• babel-jest: ^29.3.1 => 29.4.3
• babel-loader: ^9.1.0 => 9.1.2
• babel-merge: ^3.0.0 => 3.0.0
• eslint: ^8.12.0 => 8.34.0
• eslint-plugin-babel: ^5.3.1 => 5.3.1
• jest: ^29.3.1 => 29.4.3
• webpack: ^5.71.0 => 5.81.0

Possible solution

I can see two possible solutions:

1. Make all decorator evaluations, initialization, and execution happen in relation to the spec provided here.
   Currently they are evaluated in order, but they initialization happend inside-out, and then execution of get/set happens in order. If the initialization is changed to evaluate in spec order then the issue would be resolved.
2. Reverse the order of execution of set/get accessor methods to match initialization order.

I believe that the first option more closely matches the intention of the spec, but the issue would be resolved through the second option as well. I am presenting both because I do not know which would be easier.

Additional context

The spec only provides for the order of initial evaluation of the decorators, so I can see where there might be confusion. I believe the intention of the spec is that the decorators are not only evaluated in the specified order, but also have their calling methods maintain that same order.

[babel-bot]

Hey @pgoforth! 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.

[JLHwung]

Good question. The behaviour is expected.

The related evaluation order is specified in 15.7.6 ApplyDecoratorsToElementDefinition step m:

m. if kind is accessor, then
  i. If newValue is an Object, then
    1. Let newGetter be ? Get(newValue, "get").
    2. If IsCallable(newGetter) is true, set elementRecord.[[Get]] to newGetter.
    3. Else if newGetter is not undefined, throw a TypeError exception.
    4. Let newSetter be ? Get(newValue, "set").
    5. If IsCallable(newSetter) is true, set elementRecord.[[Set]] to newSetter.
    6. Else if newSetter is not undefined, throw a TypeError exception.
    7. Let initializer be ? Get(newValue, "init").
    8. If IsCallable(initializer) is true, append initializer to elementRecord.[[Initializers]].
    9. Else if initializer is not undefined, throw a TypeError exception.

The initializer is appended to elementRecord.[[Initializers]] while get/set overwrites the previous get/set.

Now in your case, the decorators are applied in this order:

1. decorator2, the evaluation result of logAccessorDecoratorRun(log, 'second')
2. decorator1, the evaluation result of logAccessorDecoratorRun(log, 'first')

When decorator2 is applied, the initializer which prints 'second: init: fakeValue - undefined' is appended, the previous vanilla elementRecord.[[Set]] is overwritten by a function that prints 'second: value: foo' and calls the previous [[Set]]

When decorator1 is applied, the initializer which prints 'first: init: fakeValue - undefined' is appended, the previous elementRecord.[[Set]] is overwritten by a function that prints 'first: value: foo' and calls the previous [[Set]]

At this point, the elementRecord.[[Initializers]] is [log('second: init...'), log('first: init...')], the elementRecord.[[Set]] is a compound function, which can roughly expand to:

function set(val) {
  // defined by `decorator1`
  log.push('first: value')
  return (function set(val) {
     // defined by `decorator2`
     log.push('second: value')
     return set.call(this, val)
  }).call(this, val)
}

When instance.fakeField = 'foo'; is executed, the element initializers are invoked in order, so 'second: init: fakeValue - undefined' is printed before first: .... Then the setter is invoked, which prints 'first: value' before 'second: value'.

It seems to me the set/`get case for accessor is not covered in current test cases, we can add a few new test cases.

[pgoforth]

So if I'm reading what you are saying correctly @JLHwung, the spec is being adhered to correctly, but the result is unexpected and perhaps the spec needs adjustment?

[JLHwung]

Personally I don't think the current spec behaviour is unexpected: For every class element, it has a list of initializers, but it has at most one setter, so the modified setter must overwrite what was defined before.

If you believe such behaviour is unwanted, please open a new issue in https://github.com/tc39/proposal-decorators. Babel will follow if the spec behaviour is changed.

[pgoforth]

I do not believe any behavior related to multiple initializers and one setter is inappropriate, my objection is to the order of execution of the [[Initializers]] array. Initializers are being resolved [0-n] and would be in the correct order if resolved [n-0].

[JLHwung]

Initializers are being resolved [0-n] and would be in the correct order if resolved [n-0].

Maybe you can comment on 7.3.34 InitializeFieldOrAccessor and 7.3.35 InitializeInstanceElements. It is straightforward for me that addInitializers adds an initializer to the end of the list, otherwise it would be called prependInitializer or unshiftInitializer.

Anyway I will close this issue as there is no action item for Babel at this moment.

[pzuraq]

@pgoforth the ordering is consistent in that it is always applied from innermost decorator to outermost decorator. In the case of methods, getters, and setters, this feels a bit different because of the nature of “outermost”. When a method is wrapped, the outer method can order itself before or after the inner method. For instance, you could update your example like to so to have consistent ordering:

function logAccessorDecoratorRun(log, name) {
    log.push(`${name}: generator`);
    return function (target, { addInitializer }) {
        log.push(`${name}: wrapper`);
        addInitializer(function () {
            log.push(`${name}: addInitializer`);
        });
        const { init, set } = target;
        return {
            init(initialValue) {
                log.push(`${name}: init: ${initialValue} - ${init}`);
                return init ? init.call(this, initialValue) : initialValue;
            },
            set(val) {
                let ret = set ? set.call(this, val) : val;
                log.push(`${name}: value: ${val}`);
                return ret;
            },
        };
    };
}

[pgoforth]

@JLHwung Wanted to bring this up for reference. tc39/proposal-decorators#508
A change is likely forthcoming, but I cannot speak to the details of the exact spec changes.

[JLHwung]

@pgoforth Thanks for the heads-up. As I mentioned in another thread (#15570 (comment)), this issue will be addressed in #15570.