fix #3559: fix recent class transform regression

evanw · Dec 20, 2023 · 2aa166b · 2aa166b
1 parent 55e1127
commit 2aa166b
Show file tree

Hide file tree

Showing 4 changed files with 171 additions and 18 deletions.
diff --git 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
@@ -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,24 +568,22 @@ 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
 			}
 		}
 	}
 
 	// 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
@@ -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
@@ -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