From 2aa166b623a0db059409e7ccf02ebc084fcb3abd Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 20 Dec 2023 14:17:39 -0500 Subject: [PATCH] fix #3559: fix recent class transform regression --- CHANGELOG.md | 43 ++++++++++++ internal/js_parser/js_parser_lower_class.go | 35 +++++----- internal/js_parser/ts_parser_test.go | 39 +++++++++++ scripts/end-to-end-tests.js | 72 +++++++++++++++++++++ 4 files changed, 171 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 654fb8cb12d..36aa14b179d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,48 @@ # Changelog +## Unreleased + +* Fix TypeScript-specific class transform edge case ([#3559](https://github.com/evanw/esbuild/issues/3559)) + + The previous release introduced an optimization that avoided transforming `super()` in the class constructor for TypeScript code compiled with `useDefineForClassFields` set to `false` if all class instance fields have no initializers. The rationale was that in this case, all class instance fields are omitted in the output so no changes to the constructor are needed. However, if all of this is the case _and_ there are `#private` instance fields with initializers, those private instance field initializers were still being moved into the constructor. This was problematic because they were being inserted before the call to `super()` (since `super()` is now no longer transformed in that case). This release introduces an additional optimization that avoids moving the private instance field initializers into the constructor in this edge case, which generates smaller code, matches the TypeScript compiler's output more closely, and avoids this bug: + + ```ts + // Original code + class Foo extends Bar { + #private = 1; + public: any; + constructor() { + super(); + } + } + + // Old output (with esbuild v0.19.9) + class Foo extends Bar { + constructor() { + super(); + this.#private = 1; + } + #private; + } + + // Old output (with esbuild v0.19.10) + class Foo extends Bar { + constructor() { + this.#private = 1; + super(); + } + #private; + } + + // New output + class Foo extends Bar { + #private = 1; + constructor() { + super(); + } + } + ``` + ## 0.19.10 * Fix glob imports in TypeScript files ([#3319](https://github.com/evanw/esbuild/issues/3319)) diff --git a/internal/js_parser/js_parser_lower_class.go b/internal/js_parser/js_parser_lower_class.go index 92a9e017f2e..02920874ae4 100644 --- a/internal/js_parser/js_parser_lower_class.go +++ b/internal/js_parser/js_parser_lower_class.go @@ -376,16 +376,8 @@ func (p *parser) maybeLowerSuperPropertyGetInsideCall(call *js_ast.ECall) { call.Args = append([]js_ast.Expr{thisExpr}, call.Args...) } -type lowerAllInstanceFields uint8 - -const ( - lowerAllInstanceFields_false lowerAllInstanceFields = iota - lowerAllInstanceFields_true_skipSuperCallShim - lowerAllInstanceFields_true_needSuperCallShim -) - type classLoweringInfo struct { - lowerAllInstanceFields lowerAllInstanceFields + lowerAllInstanceFields bool lowerAllStaticFields bool shimSuperCtorCalls bool } @@ -464,7 +456,7 @@ func (p *parser) computeClassLoweringInfo(class *js_ast.Class) (result classLowe } } else { if p.privateSymbolNeedsToBeLowered(private) { - result.lowerAllInstanceFields = lowerAllInstanceFields_true_needSuperCallShim + result.lowerAllInstanceFields = true // We can't transform this: // @@ -503,7 +495,7 @@ func (p *parser) computeClassLoweringInfo(class *js_ast.Class) (result classLowe } } else { if p.options.unsupportedJSFeatures.Has(compat.ClassPrivateField) { - result.lowerAllInstanceFields = lowerAllInstanceFields_true_needSuperCallShim + result.lowerAllInstanceFields = true result.lowerAllStaticFields = true } } @@ -576,16 +568,14 @@ func (p *parser) computeClassLoweringInfo(class *js_ast.Class) (result classLowe // fields because there's no way for this to call a setter in the base // class, so this isn't done for private fields. if prop.InitializerOrNil.Data != nil { - result.lowerAllInstanceFields = lowerAllInstanceFields_true_needSuperCallShim - } else if result.lowerAllInstanceFields != lowerAllInstanceFields_true_needSuperCallShim { - // We can skip the "super()" call shim if all instance fields + // We can skip lowering all instance fields if all instance fields // disappear completely when lowered. This happens when // "useDefineForClassFields" is false and there is no initializer. - result.lowerAllInstanceFields = lowerAllInstanceFields_true_skipSuperCallShim + result.lowerAllInstanceFields = true } } else if p.options.unsupportedJSFeatures.Has(compat.ClassField) { // Instance fields must be lowered if the target doesn't support them - result.lowerAllInstanceFields = lowerAllInstanceFields_true_needSuperCallShim + result.lowerAllInstanceFields = true } } } @@ -593,7 +583,7 @@ func (p *parser) computeClassLoweringInfo(class *js_ast.Class) (result classLowe // We need to shim "super()" inside the constructor if this is a derived // class and there are any instance fields that need to be lowered, since // those use "this" and we can only access "this" after "super()" is called - if result.lowerAllInstanceFields == lowerAllInstanceFields_true_needSuperCallShim && class.ExtendsOrNil.Data != nil { + if result.lowerAllInstanceFields && class.ExtendsOrNil.Data != nil { result.shimSuperCtorCalls = true } @@ -1058,8 +1048,17 @@ func (p *parser) lowerClass(stmt js_ast.Stmt, expr js_ast.Expr, result visitClas if !prop.Flags.Has(js_ast.PropertyIsMethod) { if prop.Flags.Has(js_ast.PropertyIsStatic) { mustLowerField = classLoweringInfo.lowerAllStaticFields + } else if prop.Kind == js_ast.PropertyNormal && p.options.ts.Parse && !class.UseDefineForClassFields && private == nil { + // Lower non-private instance fields (not accessors) if TypeScript's + // "useDefineForClassFields" setting is disabled. When all such fields + // have no initializers, we avoid setting the "lowerAllInstanceFields" + // flag as an optimization because we can just remove all class field + // declarations in that case without messing with the constructor. But + // we must set the "mustLowerField" flag here to cause this class field + // declaration to still be removed. + mustLowerField = true } else { - mustLowerField = classLoweringInfo.lowerAllInstanceFields != lowerAllInstanceFields_false + mustLowerField = classLoweringInfo.lowerAllInstanceFields } } diff --git a/internal/js_parser/ts_parser_test.go b/internal/js_parser/ts_parser_test.go index 456e8206935..892bbbc48b3 100644 --- a/internal/js_parser/ts_parser_test.go +++ b/internal/js_parser/ts_parser_test.go @@ -2418,6 +2418,45 @@ class A extends B { __super(2); } } +`) + + expectPrintedAssignSemanticsTS(t, "class A extends B { #x; y; constructor() { super() } }", + `class A extends B { + #x; + constructor() { + super(); + } +} +`) + + expectPrintedAssignSemanticsTS(t, "class A extends B { #x = 1; y; constructor() { super() } }", + `class A extends B { + #x = 1; + constructor() { + super(); + } +} +`) + + expectPrintedAssignSemanticsTS(t, "class A extends B { #x; y = 1; constructor() { super() } }", + `class A extends B { + constructor() { + super(); + this.y = 1; + } + #x; +} +`) + + expectPrintedAssignSemanticsTS(t, "class A extends B { #x = 1; y = 2; constructor() { super() } }", + `class A extends B { + constructor() { + super(); + this.#x = 1; + this.y = 2; + } + #x; +} `) } diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 0b978d9cf5e..bbfbd320096 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -5622,6 +5622,78 @@ for (let flags of [['--target=es2022'], ['--target=es6'], ['--bundle', '--target }, }`, }), + + // https://github.com/evanw/esbuild/issues/3559 + test(['in.ts', '--outfile=node.js'].concat(flags), { + 'in.ts': ` + class Foo extends Array { + #private: any + pass: any + constructor() { + super() + this.pass = true + } + } + if (!new Foo().pass) throw 'fail' + `, + 'tsconfig.json': `{ + "compilerOptions": { + "useDefineForClassFields": false, + }, + }`, + }), + test(['in.ts', '--outfile=node.js'].concat(flags), { + 'in.ts': ` + class Foo extends Array { + #private: any + pass = true + constructor() { + super() + } + } + if (!new Foo().pass) throw 'fail' + `, + 'tsconfig.json': `{ + "compilerOptions": { + "useDefineForClassFields": false, + }, + }`, + }), + test(['in.ts', '--outfile=node.js'].concat(flags), { + 'in.ts': ` + class Foo extends Array { + #private = 123 + pass: any + constructor() { + super() + this.pass = true + } + } + if (!new Foo().pass) throw 'fail' + `, + 'tsconfig.json': `{ + "compilerOptions": { + "useDefineForClassFields": false, + }, + }`, + }), + test(['in.ts', '--outfile=node.js'].concat(flags), { + 'in.ts': ` + class Foo extends Array { + #private = 123 + pass: any = true + constructor() { + super() + } + } + if (!new Foo().pass) throw 'fail' + `, + 'tsconfig.json': `{ + "compilerOptions": { + "useDefineForClassFields": false, + }, + }`, + }), ) // https://github.com/evanw/esbuild/issues/3177