Skip to content

Commit e7a9256

Browse files
evanwevanw
committed
fix #3756: regression with --keep-names 
1 parent 33cbbea commit e7a9256

File tree

5 files changed

+167
-38
lines changed

5 files changed

+167
-38
lines changed

‎CHANGELOG.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,24 @@
11
# Changelog
22

3+
## Unreleased
4+
5+
* Fix a regression with `--keep-names` ([#3756](https://github.com/evanw/esbuild/issues/3756))
6+
7+
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:
8+
9+
```js
10+
// Original code
11+
x = { get y() {} }
12+
13+
// Output from version 0.21.0 (with --keep-names)
14+
x = { get y: /* @__PURE__ */ __name(function() {
15+
}, "y") };
16+
17+
// Output from this version (with --keep-names)
18+
x = { get y() {
19+
} };
20+
```
21+
322
## 0.21.0
423

524
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.

‎internal/bundler_tests/bundler_default_test.go

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5535,21 +5535,36 @@ func TestKeepNamesAllForms(t *testing.T) {
55355535
({ fn = function() {} } = {});
55365536
`,
55375537
"/do-not-keep.js": `
5538-
// Methods
5538+
// Class methods
55395539
class Foo0 { fn() {} }
5540-
class Foo1 { get fn() {} }
5541-
class Foo2 { set fn(_) {} }
5542-
class Foo3 { static fn() {} }
5543-
class Foo4 { static get fn() {} }
5544-
class Foo5 { static set fn(_) {} }
5545-
5546-
// Private methods
5540+
class Foo1 { *fn() {} }
5541+
class Foo2 { get fn() {} }
5542+
class Foo3 { set fn(_) {} }
5543+
class Foo4 { async fn() {} }
5544+
class Foo5 { static fn() {} }
5545+
class Foo6 { static *fn() {} }
5546+
class Foo7 { static get fn() {} }
5547+
class Foo8 { static set fn(_) {} }
5548+
class Foo9 { static async fn() {} }
5549+
5550+
// Class private methods
55475551
class Bar0 { #fn() {} }
5548-
class Bar1 { get #fn() {} }
5549-
class Bar2 { set #fn(_) {} }
5550-
class Bar3 { static #fn() {} }
5551-
class Bar4 { static get #fn() {} }
5552-
class Bar5 { static set #fn(_) {} }
5552+
class Bar1 { *#fn() {} }
5553+
class Bar2 { get #fn() {} }
5554+
class Bar3 { set #fn(_) {} }
5555+
class Bar4 { async #fn() {} }
5556+
class Bar5 { static #fn() {} }
5557+
class Bar6 { static *#fn() {} }
5558+
class Bar7 { static get #fn() {} }
5559+
class Bar8 { static set #fn(_) {} }
5560+
class Bar9 { static async #fn(_) {} }
5561+
5562+
// Object methods
5563+
const Baz0 = { fn() {} }
5564+
const Baz1 = { *fn() {} }
5565+
const Baz2 = { get fn() {} }
5566+
const Baz3 = { set fn(_) {} }
5567+
const Baz4 = { async fn() {} }
55535568
`,
55545569
},
55555570
entryPaths: []string{

‎internal/bundler_tests/snapshots/snapshots_default.txt

Lines changed: 74 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2806,37 +2806,65 @@ class Foo1 {
28062806
static {
28072807
__name(this, "Foo1");
28082808
}
2809-
get fn() {
2809+
*fn() {
28102810
}
28112811
}
28122812
class Foo2 {
28132813
static {
28142814
__name(this, "Foo2");
28152815
}
2816-
set fn(_) {
2816+
get fn() {
28172817
}
28182818
}
28192819
class Foo3 {
28202820
static {
28212821
__name(this, "Foo3");
28222822
}
2823-
static fn() {
2823+
set fn(_) {
28242824
}
28252825
}
28262826
class Foo4 {
28272827
static {
28282828
__name(this, "Foo4");
28292829
}
2830-
static get fn() {
2830+
async fn() {
28312831
}
28322832
}
28332833
class Foo5 {
28342834
static {
28352835
__name(this, "Foo5");
28362836
}
2837+
static fn() {
2838+
}
2839+
}
2840+
class Foo6 {
2841+
static {
2842+
__name(this, "Foo6");
2843+
}
2844+
static *fn() {
2845+
}
2846+
}
2847+
class Foo7 {
2848+
static {
2849+
__name(this, "Foo7");
2850+
}
2851+
static get fn() {
2852+
}
2853+
}
2854+
class Foo8 {
2855+
static {
2856+
__name(this, "Foo8");
2857+
}
28372858
static set fn(_) {
28382859
}
28392860
}
2861+
class Foo9 {
2862+
static {
2863+
__name(this, "Foo9");
2864+
}
2865+
static async fn() {
2866+
}
2867+
}
28402868
class Bar0 {
28412869
static {
28422870
__name(this, "Bar0");
@@ -2848,37 +2876,75 @@ class Bar1 {
28482876
static {
28492877
__name(this, "Bar1");
28502878
}
2851-
get #fn() {
2879+
*#fn() {
28522880
}
28532881
}
28542882
class Bar2 {
28552883
static {
28562884
__name(this, "Bar2");
28572885
}
2858-
set #fn(_) {
2886+
get #fn() {
28592887
}
28602888
}
28612889
class Bar3 {
28622890
static {
28632891
__name(this, "Bar3");
28642892
}
2865-
static #fn() {
2893+
set #fn(_) {
28662894
}
28672895
}
28682896
class Bar4 {
28692897
static {
28702898
__name(this, "Bar4");
28712899
}
2872-
static get #fn() {
2900+
async #fn() {
28732901
}
28742902
}
28752903
class Bar5 {
28762904
static {
28772905
__name(this, "Bar5");
28782906
}
2907+
static #fn() {
2908+
}
2909+
}
2910+
class Bar6 {
2911+
static {
2912+
__name(this, "Bar6");
2913+
}
2914+
static *#fn() {
2915+
}
2916+
}
2917+
class Bar7 {
2918+
static {
2919+
__name(this, "Bar7");
2920+
}
2921+
static get #fn() {
2922+
}
2923+
}
2924+
class Bar8 {
2925+
static {
2926+
__name(this, "Bar8");
2927+
}
28792928
static set #fn(_) {
28802929
}
28812930
}
2931+
class Bar9 {
2932+
static {
2933+
__name(this, "Bar9");
2934+
}
2935+
static async #fn(_) {
2936+
}
2937+
}
2938+
const Baz0 = { fn() {
2939+
} };
2940+
const Baz1 = { *fn() {
2941+
} };
2942+
const Baz2 = { get fn() {
2943+
} };
2944+
const Baz3 = { set fn(_) {
2945+
} };
2946+
const Baz4 = { async fn() {
2947+
} };
28822948

28832949
================================================================================
28842950
TestKeepNamesClassStaticName

‎internal/js_parser/js_parser.go

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,6 @@ type parser struct {
9494
localTypeNames map[string]bool
9595
tsEnums map[ast.Ref]map[string]js_ast.TSEnumValue
9696
constValues map[ast.Ref]js_ast.ConstValue
97-
propMethodValue js_ast.E
9897
propDerivedCtorValue js_ast.E
9998
propMethodDecoratorScope *js_ast.Scope
10099

@@ -11634,6 +11633,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
1163411633
// We need to explicitly assign the name to the property initializer if it
1163511634
// will be transformed such that it is no longer an inline initializer.
1163611635
nameToKeep := ""
11636+
isLoweredPrivateMethod := false
1163711637
if private, ok := property.Key.Data.(*js_ast.EPrivateIdentifier); ok {
1163811638
if !property.Kind.IsMethodDefinition() || p.privateSymbolNeedsToBeLowered(private) {
1163911639
nameToKeep = p.symbols[private.Ref.InnerIndex].OriginalName
@@ -11645,7 +11645,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
1164511645
// inside the constructor where "super" is valid, so those don't need to
1164611646
// be rewritten.
1164711647
if property.Kind.IsMethodDefinition() && p.privateSymbolNeedsToBeLowered(private) {
11648-
p.fnOrArrowDataVisit.shouldLowerSuperPropertyAccess = true
11648+
isLoweredPrivateMethod = true
1164911649
}
1165011650
} else if !property.Kind.IsMethodDefinition() && !property.Flags.Has(js_ast.PropertyIsComputed) {
1165111651
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
1165511655

1165611656
// Handle methods
1165711657
if property.ValueOrNil.Data != nil {
11658-
p.propMethodValue = property.ValueOrNil.Data
1165911658
p.propMethodDecoratorScope = result.bodyScope
1166011659

1166111660
// 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
1167111670
}
1167211671
}
1167311672

11674-
property.ValueOrNil = p.visitExpr(property.ValueOrNil)
11673+
property.ValueOrNil, _ = p.visitExprInOut(property.ValueOrNil, exprIn{
11674+
isMethod: true,
11675+
isLoweredPrivateMethod: isLoweredPrivateMethod,
11676+
})
1167511677
}
1167611678

1167711679
// Handle initialized fields
@@ -12395,6 +12397,9 @@ func (p *parser) maybeRewritePropertyAccess(
1239512397
}
1239612398

1239712399
type exprIn struct {
12400+
isMethod bool
12401+
isLoweredPrivateMethod bool
12402+
1239812403
// This tells us if there are optional chain expressions (EDot, EIndex, or
1239912404
// ECall) that are chained on to this expression. Because of the way the AST
1240012405
// 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
1413114136
if innerClassNameRef == ast.InvalidRef {
1413214137
innerClassNameRef = p.generateTempRef(tempRefNeedsDeclareMayBeCapturedInsideLoop, "")
1413314138
}
14134-
p.propMethodValue = property.ValueOrNil.Data
1413514139
p.fnOnlyDataVisit.isInStaticClassContext = true
1413614140
p.fnOnlyDataVisit.innerClassNameRef = &innerClassNameRef
1413714141
}
@@ -14143,7 +14147,10 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
1414314147
p.nameToKeepIsFor = property.ValueOrNil.Data
1414414148
}
1414514149

14146-
property.ValueOrNil, _ = p.visitExprInOut(property.ValueOrNil, exprIn{assignTarget: in.assignTarget})
14150+
property.ValueOrNil, _ = p.visitExprInOut(property.ValueOrNil, exprIn{
14151+
isMethod: property.Kind.IsMethodDefinition(),
14152+
assignTarget: in.assignTarget,
14153+
})
1414714154

1414814155
p.fnOnlyDataVisit.innerClassNameRef = oldInnerClassNameRef
1414914156
p.fnOnlyDataVisit.isInStaticClassContext = oldIsInStaticClassContext
@@ -15061,8 +15068,9 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
1506115068
}
1506215069

1506315070
p.visitFn(&e.Fn, expr.Loc, visitFnOpts{
15064-
isClassMethod: e == p.propMethodValue,
15065-
isDerivedClassCtor: e == p.propDerivedCtorValue,
15071+
isMethod: in.isMethod,
15072+
isDerivedClassCtor: e == p.propDerivedCtorValue,
15073+
isLoweredPrivateMethod: in.isLoweredPrivateMethod,
1506615074
})
1506715075
name := e.Fn.Name
1506815076

@@ -15072,8 +15080,8 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
1507215080
e.Fn.Name = nil
1507315081
}
1507415082

15075-
// Optionally preserve the name
15076-
if p.options.keepNames {
15083+
// Optionally preserve the name for functions, but not for methods
15084+
if p.options.keepNames && (!in.isMethod || in.isLoweredPrivateMethod) {
1507715085
if name != nil {
1507815086
expr = p.keepExprSymbolName(expr, p.symbols[name.Ref.InnerIndex].OriginalName)
1507915087
} else if nameToKeep != "" {
@@ -16331,8 +16339,9 @@ func (p *parser) handleIdentifier(loc logger.Loc, e *js_ast.EIdentifier, opts id
1633116339
}
1633216340

1633316341
type visitFnOpts struct {
16334-
isClassMethod bool
16335-
isDerivedClassCtor bool
16342+
isMethod bool
16343+
isDerivedClassCtor bool
16344+
isLoweredPrivateMethod bool
1633616345
}
1633716346

1633816347
func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, opts visitFnOpts) {
@@ -16343,21 +16352,18 @@ func (p *parser) visitFn(fn *js_ast.Fn, scopeLoc logger.Loc, opts visitFnOpts) {
1634316352
isAsync: fn.IsAsync,
1634416353
isGenerator: fn.IsGenerator,
1634516354
isDerivedClassCtor: opts.isDerivedClassCtor,
16346-
shouldLowerSuperPropertyAccess: fn.IsAsync && p.options.unsupportedJSFeatures.Has(compat.AsyncAwait),
16355+
shouldLowerSuperPropertyAccess: (fn.IsAsync && p.options.unsupportedJSFeatures.Has(compat.AsyncAwait)) || opts.isLoweredPrivateMethod,
1634716356
}
1634816357
p.fnOnlyDataVisit = fnOnlyDataVisit{
1634916358
isThisNested: true,
1635016359
isNewTargetAllowed: true,
1635116360
argumentsRef: &fn.ArgumentsRef,
1635216361
}
1635316362

16354-
if opts.isClassMethod {
16363+
if opts.isMethod {
1635516364
decoratorScope = p.propMethodDecoratorScope
1635616365
p.fnOnlyDataVisit.innerClassNameRef = oldFnOnlyData.innerClassNameRef
1635716366
p.fnOnlyDataVisit.isInStaticClassContext = oldFnOnlyData.isInStaticClassContext
16358-
if oldFnOrArrowData.shouldLowerSuperPropertyAccess {
16359-
p.fnOrArrowDataVisit.shouldLowerSuperPropertyAccess = true
16360-
}
1636116367
}
1636216368

1636316369
if fn.Name != nil {

‎scripts/end-to-end-tests.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3772,6 +3772,29 @@ for (let flags of [[], ['--minify', '--keep-names']]) {
37723772
if (foo.Foo.name !== 'Foo') throw 'fail: ' + foo.Foo.name
37733773
`,
37743774
}),
3775+
3776+
// See: https://github.com/evanw/esbuild/issues/3756
3777+
test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), {
3778+
'in.js': `(() => { let obj = { fn() {} }; if (obj.fn.name !== 'fn') throw 'fail: ' + obj.fn.name })()`,
3779+
}),
3780+
test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), {
3781+
'in.js': `(() => { let obj = { *fn() {} }; if (obj.fn.name !== 'fn') throw 'fail: ' + obj.fn.name })()`,
3782+
}),
3783+
test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), {
3784+
'in.js': `(() => { let obj = { async fn() {} }; if (obj.fn.name !== 'fn') throw 'fail: ' + obj.fn.name })()`,
3785+
}),
3786+
test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), {
3787+
'in.js': `(() => {
3788+
let obj = { get fn() {} }, { get } = Object.getOwnPropertyDescriptor(obj, 'fn')
3789+
if (get.name !== 'get fn') throw 'fail: ' + get.name
3790+
})()`,
3791+
}),
3792+
test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), {
3793+
'in.js': `(() => {
3794+
let obj = { set fn(_) {} }, { set } = Object.getOwnPropertyDescriptor(obj, 'fn')
3795+
if (set.name !== 'set fn') throw 'fail: ' + set.name
3796+
})()`,
3797+
}),
37753798
)
37763799
}
37773800
tests.push(

0 commit comments

Comments
 (0)