fix #3230, fix #3326, fix #3394: update decorators

CHANGELOG.md

@@ -2,6 +2,33 @@
 
 ## Unreleased
 
+* Adjust TypeScript experimental decorator behavior ([#3230](https://github.com/evanw/esbuild/issues/3230), [#3326](https://github.com/evanw/esbuild/issues/3326), [#3394](https://github.com/evanw/esbuild/issues/3394))
+
+    With this release, esbuild will now allow TypeScript experimental decorators to access both static class properties and `#private` class names. For example:
+
+    ```js
+    const check =
+      <T,>(a: T, b: T): PropertyDecorator =>
+        () => console.log(a === b)
+
+    async function test() {
+      class Foo {
+        static #foo = 1
+        static bar = 1 + Foo.#foo
+        @check(Foo.#foo, 1) a: any
+        @check(Foo.bar, await Promise.resolve(2)) b: any
+      }
+    }
+
+    test().then(() => console.log('pass'))
+    ```
+
+    This will now print `true true pass` when compiled by esbuild. Previously esbuild evaluated TypeScript decorators outside of the class body, so it didn't allow decorators to access `Foo` or `#foo`. Now esbuild does something different, although it's hard to concisely explain exactly what esbuild is doing now (see the background section below for more information).
+
+    Note that TypeScript's experimental decorator support is currently buggy: TypeScript's compiler passes this test if only the first `@check` is present or if only the second `@check` is present, but TypeScript's compiler fails this test if both checks are present together. I haven't changed esbuild to match TypeScript's behavior exactly here because I'm waiting for TypeScript to fix these bugs instead.
+
+    Some background: TypeScript experimental decorators don't have consistent semantics regarding the context that the decorators are evaluated in. For example, TypeScript will let you use `await` within a decorator, which implies that the decorator runs outside the class (since `await` isn't supported inside a class body), but TypeScript will also let you use `#private` names, which implies that the decorator runs inside the class body (since `#private` names are only supported inside a class body). The value of `this` in a decorator is also buggy (the run-time value of `this` changes if any decorator in the class uses a `#private` name but the type of `this` doesn't change, leading to the type checker no longer matching reality). These inconsistent semantics make it hard for esbuild to implement this feature as decorator evaluation happens in some superposition of both inside and outside the class body that is particular to the internal implementation details of the TypeScript compiler.
+
 * Forbid `--keep-names` when targeting old browsers ([#3477](https://github.com/evanw/esbuild/issues/3477))
 
     The `--keep-names` setting needs to be able to assign to the `name` property on functions and classes. However, before ES6 this property was non-configurable, and attempting to assign to it would throw an error. So with this release, esbuild will no longer allow you to enable this setting and also target a really old browser.

internal/js_parser/js_parser.go

@@ -201,7 +201,7 @@ type parser struct {
 	// references. Then, during the second pass when we are about to enter into
 	// a class, we conservatively decide to lower all private names in that class
 	// which are used in a brand check anywhere in the file.
-	classPrivateBrandChecksToLower map[string]bool
+	lowerAllOfThesePrivateNames map[string]bool
 
 	// Temporary variables used for lowering
 	tempLetsToDeclare         []ast.Ref
@@ -210,6 +210,9 @@ type parser struct {
 
 	lexer js_lexer.Lexer
 
+	// Private field access in a decorator lowers all private fields in that class
+	parseExperimentalDecoratorNesting int
+
 	// Temporary variables used for lowering
 	tempRefCount         int
 	topLevelTempRefCount int
@@ -2073,7 +2076,9 @@ func (p *parser) parseProperty(startLoc logger.Loc, kind js_ast.PropertyKind, op
 		if opts.tsDeclareRange.Len != 0 {
 			p.log.AddError(&p.tracker, opts.tsDeclareRange, "\"declare\" cannot be used with a private identifier")
 		}
-		key = js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EPrivateIdentifier{Ref: p.storeNameInRef(p.lexer.Identifier)}}
+		name := p.lexer.Identifier
+		key = js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EPrivateIdentifier{Ref: p.storeNameInRef(name)}}
+		p.reportPrivateNameUsage(name.String)
 		p.lexer.Next()
 
 	case js_lexer.TOpenBracket:
@@ -3360,10 +3365,10 @@ func (p *parser) parsePrefix(level js_ast.L, errors *deferredErrors, flags exprF
 
 		// Make sure to lower all matching private names
 		if p.options.unsupportedJSFeatures.Has(compat.ClassPrivateBrandCheck) {
-			if p.classPrivateBrandChecksToLower == nil {
-				p.classPrivateBrandChecksToLower = make(map[string]bool)
+			if p.lowerAllOfThesePrivateNames == nil {
+				p.lowerAllOfThesePrivateNames = make(map[string]bool)
 			}
-			p.classPrivateBrandChecksToLower[name.String] = true
+			p.lowerAllOfThesePrivateNames[name.String] = true
 		}
 
 		return js_ast.Expr{Loc: loc, Data: &js_ast.EPrivateIdentifier{Ref: p.storeNameInRef(name)}}
@@ -4072,6 +4077,7 @@ func (p *parser) parseSuffix(left js_ast.Expr, level js_ast.L, errors *deferredE
 				}
 				name := p.lexer.Identifier
 				nameLoc := p.lexer.Loc()
+				p.reportPrivateNameUsage(name.String)
 				p.lexer.Next()
 				ref := p.storeNameInRef(name)
 				left = js_ast.Expr{Loc: left.Loc, Data: &js_ast.EIndex{
@@ -4177,6 +4183,7 @@ func (p *parser) parseSuffix(left js_ast.Expr, level js_ast.L, errors *deferredE
 					// "a?.#b"
 					name := p.lexer.Identifier
 					nameLoc := p.lexer.Loc()
+					p.reportPrivateNameUsage(name.String)
 					p.lexer.Next()
 					ref := p.storeNameInRef(name)
 					left = js_ast.Expr{Loc: left.Loc, Data: &js_ast.EIndex{
@@ -6117,7 +6124,6 @@ func (p *parser) parseClassStmt(loc logger.Loc, opts parseStmtOpts) js_ast.Stmt
 	}
 
 	classOpts := parseClassOpts{
-		decoratorScope:      p.currentScope,
 		isTypeScriptDeclare: opts.isTypeScriptDeclare,
 	}
 	if opts.deferredDecorators != nil {
@@ -6148,7 +6154,6 @@ func (p *parser) parseClassExpr(decorators []js_ast.Decorator) js_ast.Expr {
 
 	opts := parseClassOpts{
 		decorators:       decorators,
-		decoratorScope:   p.currentScope,
 		decoratorContext: decoratorInClassExpr,
 	}
 	p.pushScopeForParsePass(js_ast.ScopeClassName, classKeyword.Loc)
@@ -6177,7 +6182,6 @@ func (p *parser) parseClassExpr(decorators []js_ast.Decorator) js_ast.Expr {
 
 type parseClassOpts struct {
 	decorators          []js_ast.Decorator
-	decoratorScope      *js_ast.Scope
 	decoratorContext    decoratorContextFlags
 	isTypeScriptDeclare bool
 }
@@ -6229,7 +6233,7 @@ func (p *parser) parseClass(classKeyword logger.Range, name *ast.LocRef, classOp
 
 	opts := propertyOpts{
 		isClass:          true,
-		decoratorScope:   classOpts.decoratorScope,
+		decoratorScope:   p.currentScope,
 		decoratorContext: classOpts.decoratorContext,
 		classHasExtends:  extendsOrNil.Data != nil,
 		classKeyword:     classKeyword,
@@ -6244,7 +6248,7 @@ func (p *parser) parseClass(classKeyword logger.Range, name *ast.LocRef, classOp
 
 		// Parse decorators for this property
 		firstDecoratorLoc := p.lexer.Loc()
-		opts.decorators = p.parseDecorators(opts.decoratorScope, classKeyword, opts.decoratorContext)
+		opts.decorators = p.parseDecorators(p.currentScope, classKeyword, opts.decoratorContext)
 
 		// This property may turn out to be a type in TypeScript, which should be ignored
 		if property, ok := p.parseProperty(p.saveExprCommentsHere(), js_ast.PropertyNormal, opts, nil); ok {
@@ -6607,7 +6611,9 @@ func (p *parser) parseDecorators(decoratorScope *js_ast.Scope, classKeyword logg
 			//   }
 			//
 			// This matches the behavior of the TypeScript compiler.
+			p.parseExperimentalDecoratorNesting++
 			value = p.parseExprWithFlags(js_ast.LNew, exprFlagDecorator)
+			p.parseExperimentalDecoratorNesting--
 		} else {
 			// JavaScript's decorator syntax is more restrictive. Parse it using a
 			// special parser that doesn't allow normal expressions (e.g. "?.").
@@ -6650,10 +6656,12 @@ func (p *parser) parseDecorator() js_ast.Expr {
 		p.lexer.Next()
 
 		if p.lexer.Token == js_lexer.TPrivateIdentifier {
+			name := p.lexer.Identifier
 			memberExpr.Data = &js_ast.EIndex{
 				Target: memberExpr,
-				Index:  js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EPrivateIdentifier{Ref: p.storeNameInRef(p.lexer.Identifier)}},
+				Index:  js_ast.Expr{Loc: p.lexer.Loc(), Data: &js_ast.EPrivateIdentifier{Ref: p.storeNameInRef(name)}},
 			}
+			p.reportPrivateNameUsage(name.String)
 			p.lexer.Next()
 		} else {
 			memberExpr.Data = &js_ast.EDot{
@@ -11270,8 +11278,7 @@ type visitClassResult struct {
 }
 
 func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaultNameRef ast.Ref, nameToKeep string) (result visitClassResult) {
-	decoratorScope := p.currentScope
-	class.Decorators = p.visitDecorators(class.Decorators, decoratorScope)
+	class.Decorators = p.visitDecorators(class.Decorators, p.currentScope)
 
 	if class.Name != nil {
 		p.recordDeclaredSymbol(class.Name.Ref)
@@ -11321,10 +11328,10 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
 
 	// Conservatively lower all private names that have been used in a private
 	// brand check anywhere in the file. See the comment on this map for details.
-	if p.classPrivateBrandChecksToLower != nil {
+	if p.lowerAllOfThesePrivateNames != nil {
 		for _, prop := range class.Properties {
 			if private, ok := prop.Key.Data.(*js_ast.EPrivateIdentifier); ok {
-				if symbol := &p.symbols[private.Ref.InnerIndex]; p.classPrivateBrandChecksToLower[symbol.OriginalName] {
+				if symbol := &p.symbols[private.Ref.InnerIndex]; p.lowerAllOfThesePrivateNames[symbol.OriginalName] {
 					symbol.Flags |= ast.PrivateSymbolMustBeLowered
 					recomputeClassLoweringInfo = true
 				}
@@ -11422,7 +11429,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
 			continue
 		}
 
-		property.Decorators = p.visitDecorators(property.Decorators, decoratorScope)
+		property.Decorators = p.visitDecorators(property.Decorators, result.bodyScope)
 
 		// Visit the property key
 		if private, ok := property.Key.Data.(*js_ast.EPrivateIdentifier); ok {
@@ -11523,7 +11530,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
 		// Handle methods
 		if property.ValueOrNil.Data != nil {
 			p.propMethodValue = property.ValueOrNil.Data
-			p.propMethodDecoratorScope = decoratorScope
+			p.propMethodDecoratorScope = result.bodyScope
 
 			// Propagate the name to keep from the method into the initializer
 			if p.options.keepNames && nameToKeep != "" {
@@ -12450,6 +12457,15 @@ func isEvalOrArguments(name string) bool {
 	return name == "eval" || name == "arguments"
 }
 
+func (p *parser) reportPrivateNameUsage(name string) {
+	if p.parseExperimentalDecoratorNesting > 0 {
+		if p.lowerAllOfThesePrivateNames == nil {
+			p.lowerAllOfThesePrivateNames = make(map[string]bool)
+		}
+		p.lowerAllOfThesePrivateNames[name] = true
+	}
+}
+
 func (p *parser) isValidAssignmentTarget(expr js_ast.Expr) bool {
 	switch e := expr.Data.(type) {
 	case *js_ast.EIdentifier:

internal/js_parser/ts_parser_test.go

@@ -878,13 +878,12 @@ func TestTSPrivateIdentifiers(t *testing.T) {
 	expectParseErrorExperimentalDecoratorTS(t, "class Foo { @dec static get #foo() {} }", "<stdin>: ERROR: Expected identifier but found \"#foo\"\n")
 	expectParseErrorExperimentalDecoratorTS(t, "class Foo { @dec static set #foo() {x} }", "<stdin>: ERROR: Expected identifier but found \"#foo\"\n")
 
-	// Decorators are not able to access private names, since they use the scope
-	// that encloses the class declaration. Note that the TypeScript compiler has
-	// a bug where it doesn't handle this case and generates invalid code as a
-	// result: https://github.com/microsoft/TypeScript/issues/48515.
-	expectParseErrorExperimentalDecoratorTS(t, "class Foo { static #foo; @dec(Foo.#foo) bar }", "<stdin>: ERROR: Private name \"#foo\" must be declared in an enclosing class\n")
-	expectParseErrorExperimentalDecoratorTS(t, "class Foo { static #foo; @dec(Foo.#foo) bar() {} }", "<stdin>: ERROR: Private name \"#foo\" must be declared in an enclosing class\n")
-	expectParseErrorExperimentalDecoratorTS(t, "class Foo { static #foo; bar(@dec(Foo.#foo) x) {} }", "<stdin>: ERROR: Private name \"#foo\" must be declared in an enclosing class\n")
+	// Decorators are now able to access private names, since the TypeScript
+	// compiler was changed to move them into a "static {}" block within the
+	// class body: https://github.com/microsoft/TypeScript/pull/50074
+	expectParseErrorExperimentalDecoratorTS(t, "class Foo { static #foo; @dec(Foo.#foo) bar }", "")
+	expectParseErrorExperimentalDecoratorTS(t, "class Foo { static #foo; @dec(Foo.#foo) bar() {} }", "")
+	expectParseErrorExperimentalDecoratorTS(t, "class Foo { static #foo; bar(@dec(Foo.#foo) x) {} }", "")
 }
 
 func TestTSInterface(t *testing.T) {

scripts/end-to-end-tests.js

@@ -5463,6 +5463,53 @@ for (let flags of [['--target=es2022'], ['--target=es6'], ['--bundle', '--target
         if (!staticMethod) throw 'fail: staticMethod'
       `,
     }),
+
+    // https://github.com/evanw/esbuild/issues/3326
+    test(['in.ts', '--outfile=node.js'].concat(flags), {
+      'in.ts': `
+        const log: string[] = []
+        class Test1 {
+          static deco(target: any, key: any, desc: any): any { log.push('Test1') }
+          @Test1.deco static test(): void { }
+        }
+        class Test2 {
+          static deco(target: any, key: any, desc: any): any { log.push('Test2') }
+          @Test2.deco static test(): Test2 { return new Test2(); }
+        }
+        @Test3.deco
+        class Test3 {
+          static deco(target: any): any { log.push('Test3') }
+        }
+        if (log + '' !== 'Test1,Test2,Test3') throw 'fail: ' + log
+      `,
+      'tsconfig.json': `{
+        "compilerOptions": {
+          "experimentalDecorators": true,
+        },
+      }`,
+    }),
+
+    // https://github.com/evanw/esbuild/issues/3394
+    test(['in.ts', '--outfile=node.js'].concat(flags), {
+      'in.ts': `
+        const dec = (arg: number): ParameterDecorator => () => { answer = arg }
+        let answer = 0
+
+        class Foo {
+          static #foo = 123
+          static bar = 234
+          method(@dec(Foo.#foo + Foo.bar) arg: any) {
+          }
+        }
+
+        if (answer !== 357) throw 'fail: ' + answer
+      `,
+      'tsconfig.json': `{
+        "compilerOptions": {
+          "experimentalDecorators": true,
+        },
+      }`,
+    }),
   )
 
   // https://github.com/evanw/esbuild/issues/3177