fix #3568: can reorder primitive past side-effect

Author: evanw

CHANGELOG.md

@@ -43,6 +43,27 @@
     }
     ```
 
+* Minifier: allow reording a primitive past a side-effect ([#3568](https://github.com/evanw/esbuild/issues/3568))
+
+    The minifier previously allowed reordering a side-effect past a primitive, but didn't handle the case of reordering a primitive past a side-effect. This additional case is now handled:
+
+    ```js
+    // Original code
+    function f() {
+      let x = false;
+      let y = x;
+      const boolean = y;
+      let frag = $.template(`<p contenteditable="${boolean}">hello world</p>`);
+      return frag;
+    }
+
+    // Old output (with --minify)
+    function f(){const e=!1;return $.template(`<p contenteditable="${e}">hello world</p>`)}
+
+    // New output (with --minify)
+    function f(){return $.template('<p contenteditable="false">hello world</p>')}
+    ```
+
 * Provide the `stop()` API in node to exit esbuild's child process ([#3558](https://github.com/evanw/esbuild/issues/3558))
 
     You can now call `stop()` in esbuild's node API to exit esbuild's child process to reclaim the resources used. It only makes sense to do this for a long-lived node process when you know you will no longer be making any more esbuild API calls. It is not necessary to call this to allow node to exit, and it's advantageous to not call this in between calls to esbuild's API as sharing a single long-lived esbuild child process is more efficient than re-creating a new esbuild child process for every API call. This API call used to exist but was removed in [version 0.9.0](https://github.com/evanw/esbuild/releases/v0.9.0). This release adds it back due to a user request.

internal/js_ast/js_ast_helpers.go

@@ -1696,7 +1696,7 @@ func FoldStringAddition(left Expr, right Expr, kind StringAdditionKind) Expr {
 //
 // This function intentionally avoids mutating the input AST so it can be
 // called after the AST has been frozen (i.e. after parsing ends).
-func InlineStringsAndNumbersIntoTemplate(loc logger.Loc, e *ETemplate) Expr {
+func InlinePrimitivesIntoTemplate(loc logger.Loc, e *ETemplate) Expr {
 	// Can't inline strings if there's a custom template tag
 	if e.TagOrNil.Data != nil {
 		return Expr{Loc: loc, Data: e}
@@ -1709,10 +1709,8 @@ func InlineStringsAndNumbersIntoTemplate(loc logger.Loc, e *ETemplate) Expr {
 		if value, ok := part.Value.Data.(*EInlinedEnum); ok {
 			part.Value = value.Value
 		}
-		if value, ok := part.Value.Data.(*ENumber); ok {
-			if str, ok := TryToStringOnNumberSafely(value.Value, 10); ok {
-				part.Value.Data = &EString{Value: helpers.StringToUTF16(str)}
-			}
+		if str, ok := ToStringWithoutSideEffects(part.Value.Data); ok {
+			part.Value.Data = &EString{Value: helpers.StringToUTF16(str)}
 		}
 		if str, ok := part.Value.Data.(*EString); ok {
 			if len(parts) == 0 {

internal/js_parser/js_parser.go

@@ -8732,34 +8732,40 @@ func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt
 				// by now since they are scoped to this block which we just finished
 				// visiting.
 				if prevS, ok := result[len(result)-1].Data.(*js_ast.SLocal); ok && prevS.Kind != js_ast.LocalVar {
-					// The variable must be initialized, since we will be substituting
-					// the value into the usage.
-					if last := prevS.Decls[len(prevS.Decls)-1]; last.ValueOrNil.Data != nil {
-						// The binding must be an identifier that is only used once.
-						// Ignore destructuring bindings since that's not the simple case.
-						// Destructuring bindings could potentially execute side-effecting
-						// code which would invalidate reordering.
-						if id, ok := last.Binding.Data.(*js_ast.BIdentifier); ok {
-							// Don't do this if "__name" was called on this symbol. In that
-							// case there is actually more than one use even though it says
-							// there is only one. The "__name" use isn't counted so that
-							// tree shaking still works when names are kept.
-							if symbol := p.symbols[id.Ref.InnerIndex]; symbol.UseCountEstimate == 1 && !symbol.Flags.Has(ast.DidKeepName) {
-								// Try to substitute the identifier with the initializer. This will
-								// fail if something with side effects is in between the declaration
-								// and the usage.
-								if p.substituteSingleUseSymbolInStmt(stmt, id.Ref, last.ValueOrNil) {
-									// Remove the previous declaration, since the substitution was
-									// successful.
-									if len(prevS.Decls) == 1 {
-										result = result[:len(result)-1]
-									} else {
-										prevS.Decls = prevS.Decls[:len(prevS.Decls)-1]
-									}
+					last := prevS.Decls[len(prevS.Decls)-1]
+
+					// The binding must be an identifier that is only used once.
+					// Ignore destructuring bindings since that's not the simple case.
+					// Destructuring bindings could potentially execute side-effecting
+					// code which would invalidate reordering.
+					if id, ok := last.Binding.Data.(*js_ast.BIdentifier); ok {
+						// Don't do this if "__name" was called on this symbol. In that
+						// case there is actually more than one use even though it says
+						// there is only one. The "__name" use isn't counted so that
+						// tree shaking still works when names are kept.
+						if symbol := p.symbols[id.Ref.InnerIndex]; symbol.UseCountEstimate == 1 && !symbol.Flags.Has(ast.DidKeepName) {
+							replacement := last.ValueOrNil
+
+							// The variable must be initialized, since we will be substituting
+							// the value into the usage.
+							if replacement.Data == nil {
+								replacement = js_ast.Expr{Loc: last.Binding.Loc, Data: js_ast.EUndefinedShared}
+							}
 
-									// Loop back to try again
-									continue
+							// Try to substitute the identifier with the initializer. This will
+							// fail if something with side effects is in between the declaration
+							// and the usage.
+							if p.substituteSingleUseSymbolInStmt(stmt, id.Ref, replacement) {
+								// Remove the previous declaration, since the substitution was
+								// successful.
+								if len(prevS.Decls) == 1 {
+									result = result[:len(result)-1]
+								} else {
+									prevS.Decls = prevS.Decls[:len(prevS.Decls)-1]
 								}
+
+								// Loop back to try again
+								continue
 							}
 						}
 					}
@@ -9437,10 +9443,9 @@ func (p *parser) substituteSingleUseSymbolInExpr(
 			if value, status := p.substituteSingleUseSymbolInExpr(part.Value, ref, replacement, replacementCanBeRemoved); status != substituteContinue {
 				e.Parts[i].Value = value
 
-				// If we substituted a string or number, merge it into the template
-				switch value.Data.(type) {
-				case *js_ast.EString, *js_ast.ENumber:
-					expr = js_ast.InlineStringsAndNumbersIntoTemplate(expr.Loc, e)
+				// If we substituted a primitive, merge it into the template
+				if js_ast.IsPrimitiveLiteral(value.Data) {
+					expr = js_ast.InlinePrimitivesIntoTemplate(expr.Loc, e)
 				}
 				return expr, status
 			}
@@ -9454,7 +9459,7 @@ func (p *parser) substituteSingleUseSymbolInExpr(
 	}
 
 	// We can always reorder past primitive values
-	if js_ast.IsPrimitiveLiteral(expr.Data) {
+	if js_ast.IsPrimitiveLiteral(expr.Data) || js_ast.IsPrimitiveLiteral(replacement.Data) {
 		return expr, substituteContinue
 	}
 
@@ -13268,7 +13273,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 		// it may no longer be a template literal after this point (it may turn into
 		// a plain string literal instead).
 		if p.shouldFoldTypeScriptConstantExpressions || p.options.minifySyntax {
-			expr = js_ast.InlineStringsAndNumbersIntoTemplate(expr.Loc, e)
+			expr = js_ast.InlinePrimitivesIntoTemplate(expr.Loc, e)
 		}
 
 		shouldLowerTemplateLiteral := p.options.unsupportedJSFeatures.Has(compat.TemplateLiteral)

internal/js_parser/js_parser_test.go

@@ -4974,15 +4974,23 @@ func TestMangleInlineLocals(t *testing.T) {
 	check("let x = 1; return void x", "let x = 1;")
 	check("let x = 1; return typeof x", "return typeof 1;")
 
+	// Can substitute into template literals
+	check("let x = 1; return `<${x}>`", "return `<1>`;")
+	check("let x = 1n; return `<${x}>`", "return `<1>`;")
+	check("let x = null; return `<${x}>`", "return `<null>`;")
+	check("let x = undefined; return `<${x}>`", "return `<undefined>`;")
+	check("let x = false; return `<${x}>`", "return `<false>`;")
+	check("let x = true; return `<${x}>`", "return `<true>`;")
+
 	// Check substituting a side-effect free value into normal binary operators
 	check("let x = 1; return x + 2", "return 1 + 2;")
 	check("let x = 1; return 2 + x", "return 2 + 1;")
 	check("let x = 1; return x + arg0", "return 1 + arg0;")
 	check("let x = 1; return arg0 + x", "return arg0 + 1;")
 	check("let x = 1; return x + fn()", "return 1 + fn();")
-	check("let x = 1; return fn() + x", "let x = 1;\nreturn fn() + x;")
+	check("let x = 1; return fn() + x", "return fn() + 1;")
 	check("let x = 1; return x + undef", "return 1 + undef;")
-	check("let x = 1; return undef + x", "let x = 1;\nreturn undef + x;")
+	check("let x = 1; return undef + x", "return undef + 1;")
 
 	// Check substituting a value with side-effects into normal binary operators
 	check("let x = fn(); return x + 2", "return fn() + 2;")

internal/js_printer/js_printer.go

@@ -2999,7 +2999,7 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 			if replaced != nil {
 				copy := *e
 				copy.Parts = replaced
-				switch e2 := js_ast.InlineStringsAndNumbersIntoTemplate(logger.Loc{}, &copy).Data.(type) {
+				switch e2 := js_ast.InlinePrimitivesIntoTemplate(logger.Loc{}, &copy).Data.(type) {
 				case *js_ast.EString:
 					p.printQuotedUTF16(e2.Value, printQuotedAllowBacktick)
 					return