Decorators broken with private fields, generated code has syntax error

[evanw]

Bug Report

🔎 Search Terms

decorator private field syntax error

🕗 Version & Regression Information

• This is the behavior in every version I tried

⏯ Playground Link

Playground link with relevant code

💻 Code

function decorator(cb: () => any): any {
  return () => console.log(cb())
}

class Foo {
  static #bar = 123
  @decorator(() => Foo.#bar) foo() {}
}

new Foo

🙁 Actual behavior

I expect this code to either be a compile error or to compile successfully and print 123 when run.

🙂 Expected behavior

The code compiles without any errors but the generated code contains a syntax error:

"use strict";
var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var _a, _Foo_bar;
function decorator(cb) {
    return () => console.log(cb());
}
class Foo {
    foo() { }
}
_a = Foo;
_Foo_bar = { value: 123 };
__decorate([
    decorator(() => Foo.)
], Foo.prototype, "foo", null);
new Foo;

Context: I discovered this issue while investigating how TypeScript handles various decorator edge cases, which I'm doing because I'm trying to replicate TypeScript's behavior into esbuild's TypeScript transpiler for this issue: evanw/esbuild#2147.

[evanw]

After fixing esbuild's issues with parameter decorator scope myself, I believe the correct thing to do would be for TypeScript to make this case a compile error. My reasoning is that it appears that TypeScript evaluates decorator expressions in the scope that encloses the class declaration (since that's where the generated code will be inserted). By that logic, any private names in the class body should be inaccessible here since they are not in scope. So private names can still be allowed in decorator expressions but they need to be in scope, which is determined by where the class declaration itself lives.

[evanw]

The PR for this does fix the code in the bug report. However, it introduces an inconsistency where TypeScript now sometimes evaluates decorators outside the class body and sometimes evaluates them inside the class body. Also TypeScript's type checker seems to allow things to happen both outside and inside the class body even though the code will only ever be evaluated in one of those places, which is confusing. For example:

async function foo() {
  console.log('before')
  class Foo {
    static #bar = '#bar'

    // Either of these decorators work individually, but using
    // them both together generates code with a syntax error
    @((() => { console.log(Foo.#bar) }) as PropertyDecorator)
    @(await new Promise<PropertyDecorator>(r => setTimeout(() => r(() => {}), 1000)))

    static foo = 'foo'
  }
  console.log('after')
}
foo()

TypeScript generates code that looks like this, which contains a syntax error:

async function foo() {
    console.log('before');
    class Foo {
        static #bar = '#bar';
        // Either of these decorators work individually, but using
        // them both together generates code with a syntax error
        static { this.foo = 'foo'; }
        static {
            __decorate([
                (() => { console.log(Foo.#bar); }),
                (await new Promise(r => setTimeout(() => r(() => { }), 1000)))
            ], Foo, "foo", void 0);
        }
    }
    console.log('after');
}
foo();

TypeScript's checker allows both await and private names in decorator expressions even though await only works outside the class body and private names only work inside the class body. So after this PR, decorator expressions seem to no longer have well-defined evaluation semantics.

I'm trying to figure out what to do about this in esbuild. I guess the semantics right now are sort of "decorator evaluation happens inside the class body (and therefore await should be forbidden) if any member decorator contains a private name (even if it's not for the class with the decorator), otherwise decorator evaluation happens outside the class body (and therefore await is allowed)." Basically treating hasClassElementWithDecoratorContainingPrivateIdentifierInExpression as something that determines the behavior of the language. Another option would be to try to make this work anyway by initializing an async callback in the static block and then awaiting it right after the class body. Or maybe there's even another approach.

Can someone from the TypeScript team describe the semantics of decorator evaluation? Are decorators evaluated inside or outside of the class body? What's the right approach for esbuild to take in this case?

See also: evanw/esbuild#3230

[RyanCavanaugh]

@rbuckton can you comment on the above?

[rbuckton]

Decorator expressions should behave like expressions in computed property names, where they are evaluated outside of the class body using the surrounding [Await] and [Yield] parser contexts. Decorators that are lexically declared inside of the class body should have access to private names. This is a complex interaction that we do not quite handle correctly.

There are several different approaches we can take:

1. Emit all private static elements as WeakMap/WeakSet when await or yield are detected.
2. Perform all decorator expression evaluation inside of a computed property name when await or yield are detected.

We've generally fallen back to down-level emit (1) in similar circumstances, but (2) is potentially viable given that you usually have some named element to use a decorator (barring classes with only private elements):

// source
async function f() {
  class Foo {
    static #bar = '#bar';
    @(dec1(() => Foo.#bar))
    @(await Promise.resolve(dec2()))
    static foo = 'foo';
  }
}

// downlevel-ish (--experimentalDecorators)
async function f() {
  var _a;
  class Foo {
    static #bar = '#bar';
    static [(_a = [dec1(() => Foo.#bar), await Promise.resolve(dec2())], "foo")] = 'foo';
    static {
      __decorate(_a, Foo, "foo", void 0);
    }
  }
}

We do something similar to (2) today when we emit down-level stage 3 decorators if a static method has a computed property name, though it still doesn't work with await/yield because we use an arrow to enclose all of the temporary variables to avoid scope pollution since some of them are persistent (like instance initializers): TypeScript Playground

[evanw]

Decorator expressions should behave like expressions in computed property names

That doesn't appear to be exactly how TypeScript experimental decorators behave. For example, the code in the esbuild bug reported to me essentially looks like this:

let ArrayMaxSize = (x: number): PropertyDecorator => () => console.log(x)

class DocumentSearchOptions {
  static MaxFiltersSize = 20

  @ArrayMaxSize(DocumentSearchOptions.MaxFiltersSize)
  filters = []
}

TypeScript runs experimental decorators after class initialization, so referencing the class name from within a decorator works. However, expressions in computed property names run before class initialization, so referencing the class name from within a computed property name doesn't work. For example:

class DocumentSearchOptions {
  static MaxFiltersSize = 20

  static [(DocumentSearchOptions.MaxFiltersSize, 'filters')] = []
}

This code will throw an error when run:

Uncaught ReferenceError: can't access lexical declaration 'DocumentSearchOptions' before initialization

The bug report I got for esbuild is happening because I gave computed property name semantics to decorator expressions. It might be that this is correct for the new JavaScript decorators because JavaScript decorators do have computed property name semantics. But TypeScript experimental decorators appear to not have computed property name semantics.

Edit: Applying the transform you suggested to your code example also introduces a ReferenceError where there wasn't one before. You can run the code in this playground link to see this live. The reference to Foo works with the current experimental decorator transform but moving evaluation into a computed property causes a ReferenceError. People are apparently relying on this existing behavior of referencing the class name in a decorator so I'm guessing it's not behavior that can be removed.

[rbuckton]

TypeScript runs experimental decorators after class initialization, so referencing the class name from within a decorator works. However, expressions in computed property names run before class initialization, so referencing the class name from within a computed property name doesn't work.

Ah, that's true. My comment about evaluation order is true of Stage 3 decorators, not --experimentalDecorators. Our legacy decorator emit has issues when combined with static initialization, and we've generally issued errors when you try to combine the two in various ways (i.e., it's an error to use this in a static initializer of a decorated class).

For legacy decorators and --target es5, we would emit the decorators into a function closure, so using yield and await never really worked in that case:

var C = /** @class */ (function () {
    function C() {
    }
    C = __decorate([
        dec
    ], C);
    return C;
}());

The easiest thing to do for --experimentalDecorators would be to make yield/await in a decorator a syntax error.