Skip to content

Commit

Permalink
fix #3756: regression with --keep-names
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed May 7, 2024
1 parent 33cbbea commit e7a9256
Show file tree
Hide file tree
Showing 5 changed files with 167 additions and 38 deletions.
19 changes: 19 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
41 changes: 28 additions & 13 deletions internal/bundler_tests/bundler_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
82 changes: 74 additions & 8 deletions internal/bundler_tests/snapshots/snapshots_default.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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
Expand Down
40 changes: 23 additions & 17 deletions internal/js_parser/js_parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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 != "" {
Expand Down Expand Up @@ -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) {
Expand All @@ -16343,21 +16352,18 @@ 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,
isNewTargetAllowed: true,
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 {
Expand Down
23 changes: 23 additions & 0 deletions scripts/end-to-end-tests.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit e7a9256

Please sign in to comment.