From e7a9256d330d30e638698cc1755898476c3679f8 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Tue, 7 May 2024 12:04:40 -0400 Subject: [PATCH] fix #3756: regression with `--keep-names` --- CHANGELOG.md | 19 +++++ .../bundler_tests/bundler_default_test.go | 41 +++++++--- .../snapshots/snapshots_default.txt | 82 +++++++++++++++++-- internal/js_parser/js_parser.go | 40 +++++---- scripts/end-to-end-tests.js | 23 ++++++ 5 files changed, 167 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9190626a22..bb788df3fa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,24 @@ # Changelog +## Unreleased + +* Fix a regression with `--keep-names` ([#3756](https://github.com/evanw/esbuild/issues/3756)) + + The previous release introduced a regression with the `--keep-names` setting and object literals with `get`/`set` accessor methods, in which case the generated code contained syntax errors. This release fixes the regression: + + ```js + // Original code + x = { get y() {} } + + // Output from version 0.21.0 (with --keep-names) + x = { get y: /* @__PURE__ */ __name(function() { + }, "y") }; + + // Output from this version (with --keep-names) + x = { get y() { + } }; + ``` + ## 0.21.0 This release doesn't contain any deliberately-breaking changes. However, it contains a very complex new feature and while all of esbuild's tests pass, I would not be surprised if an important edge case turns out to be broken. So I'm releasing this as a breaking change release to avoid causing any trouble. As usual, make sure to test your code when you upgrade. diff --git a/internal/bundler_tests/bundler_default_test.go b/internal/bundler_tests/bundler_default_test.go index e7e619a30bc..5b574ac38f4 100644 --- a/internal/bundler_tests/bundler_default_test.go +++ b/internal/bundler_tests/bundler_default_test.go @@ -5535,21 +5535,36 @@ func TestKeepNamesAllForms(t *testing.T) { ({ fn = function() {} } = {}); `, "/do-not-keep.js": ` - // Methods + // Class methods class Foo0 { fn() {} } - class Foo1 { get fn() {} } - class Foo2 { set fn(_) {} } - class Foo3 { static fn() {} } - class Foo4 { static get fn() {} } - class Foo5 { static set fn(_) {} } - - // Private methods + class Foo1 { *fn() {} } + class Foo2 { get fn() {} } + class Foo3 { set fn(_) {} } + class Foo4 { async fn() {} } + class Foo5 { static fn() {} } + class Foo6 { static *fn() {} } + class Foo7 { static get fn() {} } + class Foo8 { static set fn(_) {} } + class Foo9 { static async fn() {} } + + // Class private methods class Bar0 { #fn() {} } - class Bar1 { get #fn() {} } - class Bar2 { set #fn(_) {} } - class Bar3 { static #fn() {} } - class Bar4 { static get #fn() {} } - class Bar5 { static set #fn(_) {} } + class Bar1 { *#fn() {} } + class Bar2 { get #fn() {} } + class Bar3 { set #fn(_) {} } + class Bar4 { async #fn() {} } + class Bar5 { static #fn() {} } + class Bar6 { static *#fn() {} } + class Bar7 { static get #fn() {} } + class Bar8 { static set #fn(_) {} } + class Bar9 { static async #fn(_) {} } + + // Object methods + const Baz0 = { fn() {} } + const Baz1 = { *fn() {} } + const Baz2 = { get fn() {} } + const Baz3 = { set fn(_) {} } + const Baz4 = { async fn() {} } `, }, entryPaths: []string{ diff --git a/internal/bundler_tests/snapshots/snapshots_default.txt b/internal/bundler_tests/snapshots/snapshots_default.txt index 1b921d955a8..fb8c4ad0d17 100644 --- a/internal/bundler_tests/snapshots/snapshots_default.txt +++ b/internal/bundler_tests/snapshots/snapshots_default.txt @@ -2806,37 +2806,65 @@ class Foo1 { static { __name(this, "Foo1"); } - get fn() { + *fn() { } } class Foo2 { static { __name(this, "Foo2"); } - set fn(_) { + get fn() { } } class Foo3 { static { __name(this, "Foo3"); } - static fn() { + set fn(_) { } } class Foo4 { static { __name(this, "Foo4"); } - static get fn() { + async fn() { } } class Foo5 { static { __name(this, "Foo5"); } + static fn() { + } +} +class Foo6 { + static { + __name(this, "Foo6"); + } + static *fn() { + } +} +class Foo7 { + static { + __name(this, "Foo7"); + } + static get fn() { + } +} +class Foo8 { + static { + __name(this, "Foo8"); + } static set fn(_) { } } +class Foo9 { + static { + __name(this, "Foo9"); + } + static async fn() { + } +} class Bar0 { static { __name(this, "Bar0"); @@ -2848,37 +2876,75 @@ class Bar1 { static { __name(this, "Bar1"); } - get #fn() { + *#fn() { } } class Bar2 { static { __name(this, "Bar2"); } - set #fn(_) { + get #fn() { } } class Bar3 { static { __name(this, "Bar3"); } - static #fn() { + set #fn(_) { } } class Bar4 { static { __name(this, "Bar4"); } - static get #fn() { + async #fn() { } } class Bar5 { static { __name(this, "Bar5"); } + static #fn() { + } +} +class Bar6 { + static { + __name(this, "Bar6"); + } + static *#fn() { + } +} +class Bar7 { + static { + __name(this, "Bar7"); + } + static get #fn() { + } +} +class Bar8 { + static { + __name(this, "Bar8"); + } static set #fn(_) { } } +class Bar9 { + static { + __name(this, "Bar9"); + } + static async #fn(_) { + } +} +const Baz0 = { fn() { +} }; +const Baz1 = { *fn() { +} }; +const Baz2 = { get fn() { +} }; +const Baz3 = { set fn(_) { +} }; +const Baz4 = { async fn() { +} }; ================================================================================ TestKeepNamesClassStaticName diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index de85ed6b909..626cf0ba430 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -94,7 +94,6 @@ type parser struct { localTypeNames map[string]bool tsEnums map[ast.Ref]map[string]js_ast.TSEnumValue constValues map[ast.Ref]js_ast.ConstValue - propMethodValue js_ast.E propDerivedCtorValue js_ast.E propMethodDecoratorScope *js_ast.Scope @@ -11634,6 +11633,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul // We need to explicitly assign the name to the property initializer if it // will be transformed such that it is no longer an inline initializer. nameToKeep := "" + isLoweredPrivateMethod := false if private, ok := property.Key.Data.(*js_ast.EPrivateIdentifier); ok { if !property.Kind.IsMethodDefinition() || p.privateSymbolNeedsToBeLowered(private) { nameToKeep = p.symbols[private.Ref.InnerIndex].OriginalName @@ -11645,7 +11645,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul // inside the constructor where "super" is valid, so those don't need to // be rewritten. if property.Kind.IsMethodDefinition() && p.privateSymbolNeedsToBeLowered(private) { - p.fnOrArrowDataVisit.shouldLowerSuperPropertyAccess = true + isLoweredPrivateMethod = true } } else if !property.Kind.IsMethodDefinition() && !property.Flags.Has(js_ast.PropertyIsComputed) { if str, ok := property.Key.Data.(*js_ast.EString); ok { @@ -11655,7 +11655,6 @@ 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 = result.bodyScope // Propagate the name to keep from the method into the initializer @@ -11671,7 +11670,10 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul } } - property.ValueOrNil = p.visitExpr(property.ValueOrNil) + property.ValueOrNil, _ = p.visitExprInOut(property.ValueOrNil, exprIn{ + isMethod: true, + isLoweredPrivateMethod: isLoweredPrivateMethod, + }) } // Handle initialized fields @@ -12395,6 +12397,9 @@ func (p *parser) maybeRewritePropertyAccess( } type exprIn struct { + isMethod bool + isLoweredPrivateMethod bool + // This tells us if there are optional chain expressions (EDot, EIndex, or // ECall) that are chained on to this expression. Because of the way the AST // works, chaining expressions on to this expression means they are our @@ -14131,7 +14136,6 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO if innerClassNameRef == ast.InvalidRef { innerClassNameRef = p.generateTempRef(tempRefNeedsDeclareMayBeCapturedInsideLoop, "") } - p.propMethodValue = property.ValueOrNil.Data p.fnOnlyDataVisit.isInStaticClassContext = true p.fnOnlyDataVisit.innerClassNameRef = &innerClassNameRef } @@ -14143,7 +14147,10 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO p.nameToKeepIsFor = property.ValueOrNil.Data } - property.ValueOrNil, _ = p.visitExprInOut(property.ValueOrNil, exprIn{assignTarget: in.assignTarget}) + property.ValueOrNil, _ = p.visitExprInOut(property.ValueOrNil, exprIn{ + isMethod: property.Kind.IsMethodDefinition(), + assignTarget: in.assignTarget, + }) p.fnOnlyDataVisit.innerClassNameRef = oldInnerClassNameRef p.fnOnlyDataVisit.isInStaticClassContext = oldIsInStaticClassContext @@ -15061,8 +15068,9 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } p.visitFn(&e.Fn, expr.Loc, visitFnOpts{ - isClassMethod: e == p.propMethodValue, - isDerivedClassCtor: e == p.propDerivedCtorValue, + isMethod: in.isMethod, + isDerivedClassCtor: e == p.propDerivedCtorValue, + isLoweredPrivateMethod: in.isLoweredPrivateMethod, }) name := e.Fn.Name @@ -15072,8 +15080,8 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO e.Fn.Name = nil } - // Optionally preserve the name - if p.options.keepNames { + // Optionally preserve the name for functions, but not for methods + if p.options.keepNames && (!in.isMethod || in.isLoweredPrivateMethod) { if name != nil { expr = p.keepExprSymbolName(expr, p.symbols[name.Ref.InnerIndex].OriginalName) } else if nameToKeep != "" { @@ -16331,8 +16339,9 @@ func (p *parser) handleIdentifier(loc logger.Loc, e *js_ast.EIdentifier, opts id } type visitFnOpts struct { - isClassMethod bool - isDerivedClassCtor bool + isMethod bool + isDerivedClassCtor bool + isLoweredPrivateMethod bool } func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, opts visitFnOpts) { @@ -16343,7 +16352,7 @@ func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, opts visitFnOpts) { isAsync: fn.IsAsync, isGenerator: fn.IsGenerator, isDerivedClassCtor: opts.isDerivedClassCtor, - shouldLowerSuperPropertyAccess: fn.IsAsync && p.options.unsupportedJSFeatures.Has(compat.AsyncAwait), + shouldLowerSuperPropertyAccess: (fn.IsAsync && p.options.unsupportedJSFeatures.Has(compat.AsyncAwait)) || opts.isLoweredPrivateMethod, } p.fnOnlyDataVisit = fnOnlyDataVisit{ isThisNested: true, @@ -16351,13 +16360,10 @@ func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, opts visitFnOpts) { argumentsRef: &fn.ArgumentsRef, } - if opts.isClassMethod { + if opts.isMethod { decoratorScope = p.propMethodDecoratorScope p.fnOnlyDataVisit.innerClassNameRef = oldFnOnlyData.innerClassNameRef p.fnOnlyDataVisit.isInStaticClassContext = oldFnOnlyData.isInStaticClassContext - if oldFnOrArrowData.shouldLowerSuperPropertyAccess { - p.fnOrArrowDataVisit.shouldLowerSuperPropertyAccess = true - } } if fn.Name != nil { diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index d09aaf17474..5c3dc61c703 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -3772,6 +3772,29 @@ for (let flags of [[], ['--minify', '--keep-names']]) { if (foo.Foo.name !== 'Foo') throw 'fail: ' + foo.Foo.name `, }), + + // See: https://github.com/evanw/esbuild/issues/3756 + test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), { + 'in.js': `(() => { let obj = { fn() {} }; if (obj.fn.name !== 'fn') throw 'fail: ' + obj.fn.name })()`, + }), + test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), { + 'in.js': `(() => { let obj = { *fn() {} }; if (obj.fn.name !== 'fn') throw 'fail: ' + obj.fn.name })()`, + }), + test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), { + 'in.js': `(() => { let obj = { async fn() {} }; if (obj.fn.name !== 'fn') throw 'fail: ' + obj.fn.name })()`, + }), + test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), { + 'in.js': `(() => { + let obj = { get fn() {} }, { get } = Object.getOwnPropertyDescriptor(obj, 'fn') + if (get.name !== 'get fn') throw 'fail: ' + get.name + })()`, + }), + test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), { + 'in.js': `(() => { + let obj = { set fn(_) {} }, { set } = Object.getOwnPropertyDescriptor(obj, 'fn') + if (set.name !== 'set fn') throw 'fail: ' + set.name + })()`, + }), ) } tests.push(