fix #3396: js decorator pretty-printing bugs

evanw · evanw · commit 65a4439ff7c4 · 2023-09-23T14:12:53.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,41 @@
 
 ## Unreleased
 
+* Fix printing of JavaScript decorators in tricky cases ([#3396](https://github.com/evanw/esbuild/issues/3396))
+
+    This release fixes some bugs where esbuild's pretty-printing of JavaScript decorators could incorrectly produced code with a syntax error. The problem happened because esbuild sometimes substitutes identifiers for other expressions in the pretty-printer itself, but the decision about whether to wrap the expression or not didn't account for this. Here are some examples:
+
+    ```js
+    // Original code
+    import { constant } from './constants.js'
+    import { imported } from 'external'
+    import { undef } from './empty.js'
+    class Foo {
+      @constant()
+      @imported()
+      @undef()
+      foo
+    }
+
+    // Old output (with --bundle --format=cjs --packages=external --minify-syntax)
+    var import_external = require("external");
+    var Foo = class {
+      @123()
+      @(0, import_external.imported)()
+      @(void 0)()
+      foo;
+    };
+
+    // New output (with --bundle --format=cjs --packages=external --minify-syntax)
+    var import_external = require("external");
+    var Foo = class {
+      @(123())
+      @((0, import_external.imported)())
+      @((void 0)())
+      foo;
+    };
+    ```
+
 * Allow pre-release versions to be passed to `target` ([#3388](https://github.com/evanw/esbuild/issues/3388))
 
     People want to be able to pass version numbers for unreleased versions of node (which have extra stuff after the version numbers) to esbuild's `target` setting and have esbuild do something reasonable with them. These version strings are of course not present in esbuild's internal feature compatibility table because an unreleased version has not been released yet (by definition). With this release, esbuild will now attempt to accept these version strings passed to `target` and do something reasonable with them.
diff --git a/internal/bundler_tests/snapshots/snapshots_dce.txt b/internal/bundler_tests/snapshots/snapshots_dce.txt
@@ -409,32 +409,32 @@ var fn = () => {
 };
 
 // keep-these.js
-var Class = @(fn) class {
+var Class = @fn class {
 };
 var Field = class {
-  @(fn)
+  @fn
   field;
 };
 var Method = class {
-  @(fn)
+  @fn
   method() {
   }
 };
 var Accessor = class {
-  @(fn)
+  @fn
   accessor accessor;
 };
 var StaticField = class {
-  @(fn)
+  @fn
   static field;
 };
 var StaticMethod = class {
-  @(fn)
+  @fn
   static method() {
   }
 };
 var StaticAccessor = class {
-  @(fn)
+  @fn
   static accessor accessor;
 };
 
diff --git a/internal/bundler_tests/snapshots/snapshots_default.txt b/internal/bundler_tests/snapshots/snapshots_default.txt
@@ -929,8 +929,8 @@ _ = class {
   #bar;
   classes = [
     class {
-      @(import_somewhere.imported)
-      @(0, import_somewhere.imported)()
+      @import_somewhere.imported
+      @((0, import_somewhere.imported)())
       imported;
     },
     class {
@@ -940,12 +940,12 @@ _ = class {
     },
     class {
       @(123)
-      @123()
+      @(123())
       constant;
     },
     class {
       @(void 0)
-      @(void 0)()
+      @((void 0)())
       undef;
     },
     class {
@@ -977,7 +977,7 @@ _ = class {
   #bar;
   classes = [
     class {
-      @(imported)
+      @imported
       @imported()
       imported;
     },
@@ -988,12 +988,12 @@ _ = class {
     },
     class {
       @(123)
-      @123()
+      @(123())
       constant;
     },
     class {
       @(void 0)
-      @(void 0)()
+      @((void 0)())
       undef;
     },
     class {
diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go
@@ -964,15 +964,25 @@ const (
 func (p *printer) printDecorators(decorators []js_ast.Decorator, how printDecorators) {
 	for _, decorator := range decorators {
 		wrap := false
+		wasCallTarget := false
 		expr := decorator.Value
 
 	outer:
 		for {
+			isCallTarget := wasCallTarget
+			wasCallTarget = false
+
 			switch e := expr.Data.(type) {
-			case *js_ast.EIdentifier, *js_ast.ECall:
+			case *js_ast.EIdentifier:
 				// "@foo"
 				break outer
 
+			case *js_ast.ECall:
+				// "@foo()"
+				expr = e.Target
+				wasCallTarget = true
+				continue
+
 			case *js_ast.EDot:
 				// "@foo.bar"
 				if p.canPrintIdentifier(e.Name) {
@@ -993,6 +1003,29 @@ func (p *printer) printDecorators(decorators []js_ast.Decorator, how printDecora
 				// "@(foo[bar])"
 				break
 
+			case *js_ast.EImportIdentifier:
+				ref := ast.FollowSymbols(p.symbols, e.Ref)
+				symbol := p.symbols.Get(ref)
+
+				if symbol.ImportItemStatus == ast.ImportItemMissing {
+					// "@(void 0)"
+					break
+				}
+
+				if symbol.NamespaceAlias != nil && isCallTarget && e.WasOriginallyIdentifier {
+					// "@((0, import_ns.fn)())"
+					break
+				}
+
+				if value := p.options.ConstValues[ref]; value.Kind != js_ast.ConstValueNone {
+					// "@(<inlined constant>)"
+					break
+				}
+
+				// "@foo"
+				// "@import_ns.fn"
+				break outer
+
 			default:
 				// "@(foo + bar)"
 				// "@(() => {})"
@@ -3028,11 +3061,8 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 		} else if symbol.NamespaceAlias != nil {
 			wrap := p.callTarget == e && e.WasOriginallyIdentifier
 			if wrap {
-				if p.options.MinifyWhitespace {
-					p.print("(0,")
-				} else {
-					p.print("(0, ")
-				}
+				p.print("(0,")
+				p.printSpace()
 			}
 			p.printSpaceBeforeIdentifier()
 			p.addSourceMapping(expr.Loc)
