From 1e301ebc9557e5a355eb2a36915de6c1efdce7fe Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Fri, 1 Apr 2022 15:16:23 -0400 Subject: [PATCH] fix #2147: ts decorators use scope enclosing class --- CHANGELOG.md | 93 +++++++++++------ internal/bundler/bundler_ts_test.go | 37 +++++++ internal/bundler/snapshots/snapshots_ts.txt | 53 +++++++++- internal/js_parser/js_parser.go | 109 ++++++++++++-------- internal/js_parser/ts_parser.go | 15 ++- internal/js_parser/ts_parser_test.go | 8 ++ 6 files changed, 240 insertions(+), 75 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f1ee7c99e9..7b50b794b9d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,53 +2,88 @@ ## Unreleased -* Change the context of TypeScript parameter decorators +* Change the context of TypeScript parameter decorators ([#2147](https://github.com/evanw/esbuild/issues/2147)) While TypeScript parameter decorators are expressions, they are not evaluated where they exist in the code. They are moved to after the class declaration and evaluated there instead. Specifically this TypeScript code: ```ts - class Foo { - foo(@bar() baz) {} + class Class { + method(@decorator() arg) {} } ``` becomes this JavaScript code: ```js - class Foo { - foo(baz) {} + class Class { + method(arg) {} } __decorate([ - __param(0, bar()) - ], Foo.prototype, "foo", null); + __param(0, decorator()) + ], Class.prototype, "method", null); ``` - One consequence of this is that whether `await` is allowed or not depends on whether the class declaration itself is inside an `async` function or not. The TypeScript compiler allows code that does this: + This has several consequences: - ```ts - async function fn(foo) { - class Foo { - foo(@bar(await foo) baz) {} - } - return Foo - } - ``` + * Whether `await` is allowed inside a decorator expression or not depends on whether the class declaration itself is in an `async` context or not. With this release, you can now use `await` inside a decorator expression when the class declaration is either inside an `async` function or is at the top-level of an ES module and top-level await is supported. Note that the TypeScript compiler currently has a bug regarding this edge case: https://github.com/microsoft/TypeScript/issues/48509. + + ```ts + // Using "await" inside a decorator expression is now allowed + async function fn(foo: Promise) { + class Class { + method(@decorator(await foo) arg) {} + } + return Class + } + ``` - because that becomes the following valid JavaScript: + Also while TypeScript currently allows `await` to be used like this in `async` functions, it doesn't currently allow `yield` to be used like this in generator functions. It's not yet clear whether this behavior with `yield` is a bug or by design, so I haven't made any changes to esbuild's handling of `yield` inside decorator expressions in this release. - ```js - async function fn(foo) { - class Foo { - foo(baz) {} - } - __decorate([ - __param(0, bar(await foo)) - ], Foo.prototype, "foo", null); - return Foo; - } - ``` + * Since the scope of a decorator expression is the scope enclosing the class declaration, they cannot access private identifiers. Previously this was incorrectly allowed but with this release, esbuild no longer allows this. Note that the TypeScript compiler currently has a bug regarding this edge case: https://github.com/microsoft/TypeScript/issues/48515. - Previously using `await` like this wasn't allowed. With this release, esbuild now handles `await` correctly in TypeScript parameter decorators. Note that the TypeScript compiler currently has some bugs regarding this behavior: https://github.com/microsoft/TypeScript/issues/48509. Also while TypeScript currently allows `await` to be used like this in `async` functions, it doesn't currently allow `yield` to be used like this in generator functions. It's not yet clear whether this behavior with `yield` is a bug or by design, so I haven't made any changes to esbuild's handling of `yield` inside decorator expressions. + ```ts + // Using private names inside a decorator expression is no longer allowed + class Class { + static #priv = 123 + method(@decorator(Class.#priv) arg) {} + } + ``` + + * Since the scope of a decorator expression is the scope enclosing the class declaration, identifiers inside parameter decorator expressions should never be resolved to a parameter of the enclosing method. Previously this could happen, which was a bug with esbuild. This bug no longer happens in this release. + + ```ts + // Name collisions now resolve to the outer name instead of the inner name + let arg = 1 + class Class { + method(@decorator(arg) arg = 2) {} + } + ``` + + Specifically previous versions of esbuild generated the following incorrect JavaScript (notice the use of `arg2`): + + ```js + let arg = 1; + class Class { + method(arg2 = 2) { + } + } + __decorateClass([ + __decorateParam(0, decorator(arg2)) + ], Class.prototype, "method", 1); + ``` + + This release now generates the following correct JavaScript (notice the use of `arg`): + + ```js + let arg = 1; + class Class { + method(arg2 = 2) { + } + } + __decorateClass([ + __decorateParam(0, decorator(arg)) + ], Class.prototype, "method", 1); + ``` ## 0.14.29 diff --git a/internal/bundler/bundler_ts_test.go b/internal/bundler/bundler_ts_test.go index e014aa7f459..9440958e15f 100644 --- a/internal/bundler/bundler_ts_test.go +++ b/internal/bundler/bundler_ts_test.go @@ -864,6 +864,43 @@ func TestTypeScriptDecoratorsKeepNames(t *testing.T) { }) } +// See: https://github.com/evanw/esbuild/issues/2147 +func TestTypeScriptDecoratorScopeIssue2147(t *testing.T) { + ts_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.ts": ` + let foo = 1 + class Foo { + method1(@dec(foo) foo = 2) {} + method2(@dec(() => foo) foo = 3) {} + } + + class Bar { + static x = class { + static y = () => { + let bar = 1 + @dec(bar) + @dec(() => bar) + class Baz { + @dec(bar) method1() {} + @dec(() => bar) method2() {} + method3(@dec(() => bar) bar) {} + method4(@dec(() => bar) bar) {} + } + return Baz + } + } + } + `, + }, + entryPaths: []string{"/entry.ts"}, + options: config.Options{ + Mode: config.ModePassThrough, + AbsOutputFile: "/out.js", + }, + }) +} + func TestTSExportDefaultTypeIssue316(t *testing.T) { ts_suite.expectBundled(t, bundled{ files: map[string]string{ diff --git a/internal/bundler/snapshots/snapshots_ts.txt b/internal/bundler/snapshots/snapshots_ts.txt index d8cd84fd9d4..0f73f778b60 100644 --- a/internal/bundler/snapshots/snapshots_ts.txt +++ b/internal/bundler/snapshots/snapshots_ts.txt @@ -832,6 +832,57 @@ export var x; x2.z = x2.y; })(x || (x = {})); +================================================================================ +TestTypeScriptDecoratorScopeIssue2147 +---------- /out.js ---------- +var _a; +let foo = 1; +class Foo { + method1(foo2 = 2) { + } + method2(foo2 = 3) { + } +} +__decorateClass([ + __decorateParam(0, dec(foo)) +], Foo.prototype, "method1", 1); +__decorateClass([ + __decorateParam(0, dec(() => foo)) +], Foo.prototype, "method2", 1); +class Bar { +} +Bar.x = (_a = class { +}, _a.y = () => { + let bar = 1; + let Baz = class { + method1() { + } + method2() { + } + method3(bar2) { + } + method4(bar2) { + } + }; + __decorateClass([ + dec(bar) + ], Baz.prototype, "method1", 1); + __decorateClass([ + dec(() => bar) + ], Baz.prototype, "method2", 1); + __decorateClass([ + __decorateParam(0, dec(() => bar)) + ], Baz.prototype, "method3", 1); + __decorateClass([ + __decorateParam(0, dec(() => bar)) + ], Baz.prototype, "method4", 1); + Baz = __decorateClass([ + dec(bar), + dec(() => bar) + ], Baz); + return Baz; +}, _a); + ================================================================================ TestTypeScriptDecorators ---------- /out.js ---------- @@ -1074,7 +1125,7 @@ var k_default = class { } }; __decorateClass([ - __decorateParam(0, x2(() => 0)), + __decorateParam(0, x(() => 0)), __decorateParam(0, y(() => 1)) ], k_default.prototype, "foo", 1); diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 4a1ca4d13c1..ca9d97235b5 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -90,6 +90,8 @@ type parser struct { localTypeNames map[string]bool tsEnums map[js_ast.Ref]map[string]js_ast.TSEnumValue constValues map[js_ast.Ref]js_ast.ConstValue + classPropValue js_ast.E + classPropTSDecoratorScope *js_ast.Scope // This is the reference to the generated function argument for the namespace, // which is different than the reference to the namespace itself: @@ -544,6 +546,7 @@ const ( // arrow expressions. type fnOrArrowDataParse struct { arrowArgErrors *deferredArrowArgErrors + tsDecoratorScope *js_ast.Scope asyncRange logger.Range needsAsyncLoc logger.Loc await awaitOrYield @@ -558,9 +561,6 @@ type fnOrArrowDataParse struct { // In TypeScript, forward declarations of functions have no bodies allowMissingBodyForTypeScript bool - - // Allow TypeScript decorators in function arguments - allowTSDecorators bool } // This is function-specific information used during visiting. It is saved and @@ -1011,20 +1011,22 @@ func (p *parser) popScope() { } func (p *parser) popAndDiscardScope(scopeIndex int) { + // Unwind any newly-added scopes in reverse order + for i := len(p.scopesInOrder) - 1; i >= scopeIndex; i-- { + scope := p.scopesInOrder[i].scope + parent := scope.Parent + last := len(parent.Children) - 1 + if parent.Children[last] != scope { + panic("Internal error") + } + parent.Children = parent.Children[:last] + } + // Move up to the parent scope - toDiscard := p.currentScope - parent := toDiscard.Parent - p.currentScope = parent + p.currentScope = p.currentScope.Parent // Truncate the scope order where we started to pretend we never saw this scope p.scopesInOrder = p.scopesInOrder[:scopeIndex] - - // Remove the last child from the parent scope - last := len(parent.Children) - 1 - if parent.Children[last] != toDiscard { - panic("Internal error") - } - parent.Children = parent.Children[:last] } func (p *parser) popAndFlattenScope(scopeIndex int) { @@ -1659,7 +1661,8 @@ func (p *parser) parseStringLiteral() js_ast.Expr { } type propertyOpts struct { - tsDecorators []js_ast.Expr + tsDecorators []js_ast.Expr + tsDecoratorScope *js_ast.Scope asyncRange logger.Range tsDeclareRange logger.Range @@ -1668,11 +1671,10 @@ type propertyOpts struct { isGenerator bool // Class-related options - isStatic bool - isTSAbstract bool - isClass bool - classHasExtends bool - allowTSDecorators bool + isStatic bool + isTSAbstract bool + isClass bool + classHasExtends bool } func (p *parser) parseProperty(kind js_ast.PropertyKind, opts propertyOpts, errors *deferredErrors) (js_ast.Property, bool) { @@ -2032,7 +2034,7 @@ func (p *parser) parseProperty(kind js_ast.PropertyKind, opts propertyOpts, erro yield: yield, allowSuperCall: opts.classHasExtends && isConstructor, allowSuperProperty: true, - allowTSDecorators: opts.allowTSDecorators, + tsDecoratorScope: opts.tsDecoratorScope, isConstructor: isConstructor, // Only allow omitting the body if we're parsing TypeScript class @@ -5145,7 +5147,7 @@ func (p *parser) parseFn(name *js_ast.LocRef, classKeyword logger.Range, data fn } var tsDecorators []js_ast.Expr - if data.allowTSDecorators { + if data.tsDecoratorScope != nil { oldAwait := p.fnOrArrowDataParse.await oldNeedsAsyncLoc := p.fnOrArrowDataParse.needsAsyncLoc @@ -5196,7 +5198,7 @@ func (p *parser) parseFn(name *js_ast.LocRef, classKeyword logger.Range, data fn p.fnOrArrowDataParse.needsAsyncLoc = oldFnOrArrowData.needsAsyncLoc } - tsDecorators = p.parseTypeScriptDecorators() + tsDecorators = p.parseTypeScriptDecorators(data.tsDecoratorScope) p.fnOrArrowDataParse.await = oldAwait p.fnOrArrowDataParse.needsAsyncLoc = oldNeedsAsyncLoc @@ -5369,7 +5371,7 @@ func (p *parser) parseClassStmt(loc logger.Loc, opts parseStmtOpts) js_ast.Stmt } classOpts := parseClassOpts{ - allowTSDecorators: true, + tsDecoratorScope: p.currentScope, isTypeScriptDeclare: opts.isTypeScriptDeclare, } if opts.tsDecorators != nil { @@ -5394,7 +5396,7 @@ func (p *parser) parseClassStmt(loc logger.Loc, opts parseStmtOpts) js_ast.Stmt type parseClassOpts struct { tsDecorators []js_ast.Expr - allowTSDecorators bool + tsDecoratorScope *js_ast.Scope isTypeScriptDeclare bool } @@ -5444,10 +5446,10 @@ func (p *parser) parseClass(classKeyword logger.Range, name *js_ast.LocRef, clas scopeIndex := p.pushScopeForParsePass(js_ast.ScopeClassBody, bodyLoc) opts := propertyOpts{ - isClass: true, - allowTSDecorators: classOpts.allowTSDecorators, - classHasExtends: extendsOrNil.Data != nil, - classKeyword: classKeyword, + isClass: true, + tsDecoratorScope: classOpts.tsDecoratorScope, + classHasExtends: extendsOrNil.Data != nil, + classKeyword: classKeyword, } hasConstructor := false @@ -5459,8 +5461,8 @@ func (p *parser) parseClass(classKeyword logger.Range, name *js_ast.LocRef, clas // Parse decorators for this property firstDecoratorLoc := p.lexer.Loc() - if opts.allowTSDecorators { - opts.tsDecorators = p.parseTypeScriptDecorators() + if opts.tsDecoratorScope != nil { + opts.tsDecorators = p.parseTypeScriptDecorators(opts.tsDecoratorScope) } else { opts.tsDecorators = nil p.logInvalidDecoratorError(classKeyword) @@ -6060,7 +6062,7 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt { // Parse decorators before class statements, which are potentially exported if p.options.ts.Parse { scopeIndex := len(p.scopesInOrder) - tsDecorators := p.parseTypeScriptDecorators() + tsDecorators := p.parseTypeScriptDecorators(p.currentScope) // If this turns out to be a "declare class" statement, we need to undo the // scopes that were potentially pushed while parsing the decorator arguments. @@ -8985,7 +8987,7 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_ } } - p.visitFn(&s2.Fn, s2.Fn.OpenParenLoc) + p.visitFn(&s2.Fn, s2.Fn.OpenParenLoc, nil) stmts = append(stmts, stmt) // Optionally preserve the name @@ -9516,7 +9518,7 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_ } case *js_ast.SFunction: - p.visitFn(&s.Fn, s.Fn.OpenParenLoc) + p.visitFn(&s.Fn, s.Fn.OpenParenLoc, nil) // Strip this function declaration if it was overwritten if p.symbols[s.Fn.Name.Ref.InnerIndex].Flags.Has(js_ast.RemoveOverwrittenFunctionDeclaration) && !s.IsExport { @@ -10073,15 +10075,28 @@ func (p *parser) captureValueWithPossibleSideEffects( }, wrapFunc } -func (p *parser) visitTSDecorators(tsDecorators []js_ast.Expr) []js_ast.Expr { - for i, decorator := range tsDecorators { - tsDecorators[i] = p.visitExpr(decorator) +func (p *parser) visitTSDecorators(tsDecorators []js_ast.Expr, tsDecoratorScope *js_ast.Scope) []js_ast.Expr { + if tsDecorators != nil { + // TypeScript decorators cause us to temporarily revert to the scope that + // encloses the class declaration, since that's where the generated code + // for TypeScript decorators will be inserted. + oldScope := p.currentScope + p.currentScope = tsDecoratorScope + + for i, decorator := range tsDecorators { + tsDecorators[i] = p.visitExpr(decorator) + } + + // Avoid "popScope" because this decorator scope is not hierarchical + p.currentScope = oldScope } + return tsDecorators } func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class) js_ast.Ref { - class.TSDecorators = p.visitTSDecorators(class.TSDecorators) + tsDecoratorScope := p.currentScope + class.TSDecorators = p.visitTSDecorators(class.TSDecorators, tsDecoratorScope) if class.Name != nil { p.recordDeclaredSymbol(class.Name.Ref) @@ -10223,7 +10238,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class) js_ast continue } - property.TSDecorators = p.visitTSDecorators(property.TSDecorators) + property.TSDecorators = p.visitTSDecorators(property.TSDecorators, tsDecoratorScope) // Special-case certain expressions to allow them here switch k := property.Key.Data.(type) { @@ -10289,6 +10304,8 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class) js_ast } if property.ValueOrNil.Data != nil { + p.classPropValue = property.ValueOrNil.Data + p.classPropTSDecoratorScope = tsDecoratorScope if nameToKeep != "" { wasAnonymousNamedExpr := p.isAnonymousNamedExpr(property.ValueOrNil) property.ValueOrNil = p.maybeKeepExprSymbolName(p.visitExpr(property.ValueOrNil), nameToKeep, wasAnonymousNamedExpr) @@ -10378,8 +10395,9 @@ func fnBodyContainsUseStrict(body []js_ast.Stmt) (logger.Loc, bool) { } type visitArgsOpts struct { - body []js_ast.Stmt - hasRestArg bool + body []js_ast.Stmt + tsDecoratorScope *js_ast.Scope + hasRestArg bool // This is true if the function is an arrow function or a method isUniqueFormalParameters bool @@ -10408,7 +10426,7 @@ func (p *parser) visitArgs(args []js_ast.Arg, opts visitArgsOpts) { for i := range args { arg := &args[i] - arg.TSDecorators = p.visitTSDecorators(arg.TSDecorators) + arg.TSDecorators = p.visitTSDecorators(arg.TSDecorators, opts.tsDecoratorScope) p.visitBinding(arg.Binding, bindingOpts{ duplicateArgCheck: duplicateArgCheck, }) @@ -13449,7 +13467,11 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } case *js_ast.EFunction: - p.visitFn(&e.Fn, expr.Loc) + var tsDecoratorScope *js_ast.Scope + if e == p.classPropValue { + tsDecoratorScope = p.classPropTSDecoratorScope + } + p.visitFn(&e.Fn, expr.Loc, tsDecoratorScope) name := e.Fn.Name // Remove unused function names when minifying @@ -13811,7 +13833,7 @@ func (p *parser) handleIdentifier(loc logger.Loc, e *js_ast.EIdentifier, opts id return js_ast.Expr{Loc: loc, Data: e} } -func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc) { +func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, tsDecoratorScope *js_ast.Scope) { oldFnOrArrowData := p.fnOrArrowDataVisit oldFnOnlyData := p.fnOnlyDataVisit p.fnOrArrowDataVisit = fnOrArrowDataVisit{ @@ -13845,6 +13867,7 @@ func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc) { hasRestArg: fn.HasRestArg, body: fn.Body.Block.Stmts, isUniqueFormalParameters: fn.IsUniqueFormalParameters, + tsDecoratorScope: tsDecoratorScope, }) p.pushScopeForVisitPass(js_ast.ScopeFunctionBody, fn.Body.Loc) if fn.Name != nil { diff --git a/internal/js_parser/ts_parser.go b/internal/js_parser/ts_parser.go index ca0bcdb6305..5064fb1a6b5 100644 --- a/internal/js_parser/ts_parser.go +++ b/internal/js_parser/ts_parser.go @@ -889,9 +889,16 @@ func (p *parser) skipTypeScriptTypeStmt(opts parseStmtOpts) { p.lexer.ExpectOrInsertSemicolon() } -func (p *parser) parseTypeScriptDecorators() []js_ast.Expr { +func (p *parser) parseTypeScriptDecorators(tsDecoratorScope *js_ast.Scope) []js_ast.Expr { var tsDecorators []js_ast.Expr + if p.options.ts.Parse { + // TypeScript decorators cause us to temporarily revert to the scope that + // encloses the class declaration, since that's where the generated code + // for TypeScript decorators will be inserted. + oldScope := p.currentScope + p.currentScope = tsDecoratorScope + for p.lexer.Token == js_lexer.TAt { loc := p.lexer.Loc() p.lexer.Next() @@ -908,7 +915,11 @@ func (p *parser) parseTypeScriptDecorators() []js_ast.Expr { value.Loc = loc tsDecorators = append(tsDecorators, value) } + + // Avoid "popScope" because this decorator scope is not hierarchical + p.currentScope = oldScope } + return tsDecorators } @@ -920,7 +931,7 @@ func (p *parser) logInvalidDecoratorError(classKeyword logger.Range) { // Parse and discard decorators for error recovery scopeIndex := len(p.scopesInOrder) - p.parseTypeScriptDecorators() + p.parseTypeScriptDecorators(p.currentScope) p.discardScopesUpTo(scopeIndex) } } diff --git a/internal/js_parser/ts_parser_test.go b/internal/js_parser/ts_parser_test.go index 9531ac42894..891b0e7b3b1 100644 --- a/internal/js_parser/ts_parser_test.go +++ b/internal/js_parser/ts_parser_test.go @@ -501,6 +501,14 @@ func TestTSPrivateIdentifiers(t *testing.T) { expectParseErrorTS(t, "class Foo { @dec static #foo() {} }", ": ERROR: Expected identifier but found \"#foo\"\n") expectParseErrorTS(t, "class Foo { @dec static get #foo() {} }", ": ERROR: Expected identifier but found \"#foo\"\n") expectParseErrorTS(t, "class Foo { @dec static set #foo() {x} }", ": 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. + expectParseErrorTS(t, "class Foo { static #foo; @dec(Foo.#foo) bar }", ": ERROR: Private name \"#foo\" must be declared in an enclosing class\n") + expectParseErrorTS(t, "class Foo { static #foo; @dec(Foo.#foo) bar() {} }", ": ERROR: Private name \"#foo\" must be declared in an enclosing class\n") + expectParseErrorTS(t, "class Foo { static #foo; bar(@dec(Foo.#foo) x) {} }", ": ERROR: Private name \"#foo\" must be declared in an enclosing class\n") } func TestTSInterface(t *testing.T) {