fix #2407: undo recursive --define substitution

Author: evanw

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* Fix an accidental infinite loop with `--define` substitution ([#2407](https://github.com/evanw/esbuild/issues/2407))
+
+    This is a fix for a regression that was introduced in esbuild version 0.14.44 where certain `--define` substitutions could result in esbuild crashing with a stack overflow. The problem was an incorrect fix for #2292. The fix merged the code paths for `--define` and `--jsx-factory` rewriting since the value substitution is now the same for both. However, doing this accidentally made `--define` substitution recursive since the JSX factory needs to be able to match against `--define` substitutions to integrate with the `--inject` feature. The fix is to only do one additional level of matching against define substitutions, and to only do this for JSX factories. Now these cases are able to build successfully without a stack overflow.
+
 * Fix a cross-platform consistency bug ([#2383](https://github.com/evanw/esbuild/issues/2383))
 
     Previously esbuild would minify `0xFFFF_FFFF_FFFF_FFFF` as `0xffffffffffffffff` (18 bytes) on arm64 chips and as `18446744073709552e3` (19 bytes) on x86_64 chips. The reason was that the number was converted to a 64-bit unsigned integer internally for printing as hexadecimal, the 64-bit floating-point number `0xFFFF_FFFF_FFFF_FFFF` is actually `0x1_0000_0000_0000_0180` (i.e. it's rounded up, not down), and converting `float64` to `uint64` is implementation-dependent in Go when the input is out of bounds. This was fixed by changing the upper limit for which esbuild uses hexadecimal numbers during minification to `0xFFFF_FFFF_FFFF_F800`, which is the next representable 64-bit floating-point number below `0x1_0000_0000_0000_0180`, and which fits in a `uint64`. As a result, esbuild will now consistently never minify `0xFFFF_FFFF_FFFF_FFFF` as `0xffffffffffffffff` anymore, which means the output should now be consistent across platforms.

internal/bundler/bundler_default_test.go

@@ -4172,14 +4172,20 @@ func TestInjectJSX(t *testing.T) {
 				Parts: []string{"el"},
 			},
 		},
+		"React.Fragment": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"frag"},
+			},
+		},
 	})
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{
 			"/entry.jsx": `
-				console.log(<div/>)
+				console.log(<><div/></>)
 			`,
 			"/inject.js": `
 				export function el() {}
+				export function frag() {}
 			`,
 		},
 		entryPaths: []string{"/entry.jsx"},
@@ -4559,6 +4565,46 @@ func TestDefineOptionalChainLowered(t *testing.T) {
 	})
 }
 
+// See: https://github.com/evanw/esbuild/issues/2407
+func TestDefineInfiniteLoopIssue2407(t *testing.T) {
+	defines := config.ProcessDefines(map[string]config.DefineData{
+		"a.b": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"b", "c"},
+			},
+		},
+		"b.c": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"c", "a"},
+			},
+		},
+		"c.a": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"a", "b"},
+			},
+		},
+		"x.y": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"y"},
+			},
+		},
+	})
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				a.b()
+				x.y()
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+			Defines:       &defines,
+		},
+	})
+}
+
 func TestKeepNamesTreeShaking(t *testing.T) {
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler/snapshots/snapshots_default.txt

@@ -420,6 +420,13 @@ console.log(import_meta.y);
 
 ---------- /out/dead-code.js ----------
 
+================================================================================
+TestDefineInfiniteLoopIssue2407
+---------- /out.js ----------
+// entry.js
+b.c();
+y();
+
 ================================================================================
 TestDefineOptionalChain
 ---------- /out.js ----------
@@ -1376,9 +1383,11 @@ TestInjectJSX
 // inject.js
 function el() {
 }
+function frag() {
+}
 
 // entry.jsx
-console.log(/* @__PURE__ */ el("div", null));
+console.log(/* @__PURE__ */ el(frag, null, /* @__PURE__ */ el("div", null)));
 
 ================================================================================
 TestInjectNoBundle

internal/js_parser/js_parser.go

@@ -10754,20 +10754,25 @@ func (p *parser) instantiateDefineExpr(loc logger.Loc, expr config.DefineExpr, o
 	}
 
 	// Check both user-specified defines and known globals
-	if defines, ok := p.options.defines.DotDefines[parts[len(parts)-1]]; ok {
-	next:
-		for _, define := range defines {
-			if len(define.Parts) == len(parts) {
-				for i := range parts {
-					if parts[i] != define.Parts[i] {
-						continue next
+	if opts.matchAgainstDefines {
+		// Make sure define resolution is not recursive
+		opts.matchAgainstDefines = false
+
+		if defines, ok := p.options.defines.DotDefines[parts[len(parts)-1]]; ok {
+		next:
+			for _, define := range defines {
+				if len(define.Parts) == len(parts) {
+					for i := range parts {
+						if parts[i] != define.Parts[i] {
+							continue next
+						}
 					}
-				}
-			}
 
-			// Substitute user-specified defines
-			if define.Data.DefineExpr != nil {
-				return p.instantiateDefineExpr(loc, *define.Data.DefineExpr, opts)
+					// Substitute user-specified defines
+					if define.Data.DefineExpr != nil {
+						return p.instantiateDefineExpr(loc, *define.Data.DefineExpr, opts)
+					}
+				}
 			}
 		}
 	}
@@ -12060,6 +12065,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 			if e.TagOrNil.Data == nil {
 				e.TagOrNil = p.instantiateDefineExpr(expr.Loc, p.options.jsx.Fragment, identifierOpts{
 					wasOriginallyIdentifier: true,
+					matchAgainstDefines:     true, // Allow defines to rewrite the JSX fragment factory
 				})
 			}
 
@@ -12079,6 +12085,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 			// Call createElement()
 			target := p.instantiateDefineExpr(expr.Loc, p.options.jsx.Factory, identifierOpts{
 				wasOriginallyIdentifier: true,
+				matchAgainstDefines:     true, // Allow defines to rewrite the JSX factory
 			})
 			p.warnAboutImportNamespaceCall(target, exprKindCall)
 			return js_ast.Expr{Loc: expr.Loc, Data: &js_ast.ECall{
@@ -14280,6 +14287,7 @@ type identifierOpts struct {
 	isDeleteTarget          bool
 	preferQuotedKey         bool
 	wasOriginallyIdentifier bool
+	matchAgainstDefines     bool
 }
 
 func (p *parser) handleIdentifier(loc logger.Loc, e *js_ast.EIdentifier, opts identifierOpts) js_ast.Expr {