fix #2619: bug with single-use substitutions

CHANGELOG.md

@@ -1,5 +1,49 @@
 # Changelog
 
+## Unreleased
+
+* Fix minifier correctness bug with single-use substitutions ([#2619](https://github.com/evanw/esbuild/issues/2619))
+
+    When minification is enabled, esbuild will attempt to eliminate variables that are only used once in certain cases. For example, esbuild minifies this code:
+
+    ```js
+    function getEmailForUser(name) {
+      let users = db.table('users');
+      let user = users.find({ name });
+      let email = user?.get('email');
+      return email;
+    }
+    ```
+
+    into this code:
+
+    ```js
+    function getEmailForUser(e){return db.table("users").find({name:e})?.get("email")}
+    ```
+
+    However, this transformation had a bug where esbuild did not correctly consider the "read" part of binary read-modify-write assignment operators. For example, it's incorrect to minify the following code into `bar += fn()` because the call to `fn()` might modify `bar`:
+
+    ```js
+    const foo = fn();
+    bar += foo;
+    ```
+
+    In addition to fixing this correctness bug, this release also improves esbuild's output in the case where all values being skipped over are primitives:
+
+    ```js
+    function toneMapLuminance(r, g, b) {
+      let hdr = luminance(r, g, b)
+      let decay = 1 / (1 + hdr)
+      return 1 - decay
+    }
+    ```
+
+    Previous releases of esbuild didn't substitute these single-use variables here, but esbuild will now minify this to the following code starting with this release:
+
+    ```js
+    function toneMapLuminance(e,n,a){return 1-1/(1+luminance(e,n,a))}
+    ```
+
 ## 0.15.11
 
 * Fix various edge cases regarding template tags and `this` ([#2610](https://github.com/evanw/esbuild/issues/2610))

internal/js_parser/js_parser.go

@@ -8467,17 +8467,30 @@ func (p *parser) substituteSingleUseSymbolInExpr(
 				return expr, status
 			}
 		} else if !p.exprCanBeRemovedIfUnused(e.Left) {
-			// Do not reorder past a side effect
+			// Do not reorder past a side effect in an assignment target, as that may
+			// change the replacement value. For example, "fn()" may change "a" here:
+			//
+			//   let a = 1;
+			//   foo[fn()] = a;
+			//
+			return expr, substituteFailure
+		} else if e.Op.BinaryAssignTarget() == js_ast.AssignTargetUpdate && !replacementCanBeRemoved {
+			// If this is a read-modify-write assignment and the replacement has side
+			// effects, don't reorder it past the assignment target. The assignment
+			// target is being read so it may be changed by the side effect. For
+			// example, "fn()" may change "foo" here:
+			//
+			//   let a = fn();
+			//   foo += a;
+			//
 			return expr, substituteFailure
 		}
 
-		// Do not substitute our unconditionally-executed value into a branching
-		// short-circuit operator unless the value itself has no side effects
-		if replacementCanBeRemoved || !e.Op.IsShortCircuit() {
-			if value, status := p.substituteSingleUseSymbolInExpr(e.Right, ref, replacement, replacementCanBeRemoved); status != substituteContinue {
-				e.Right = value
-				return expr, status
-			}
+		// If we get here then it should be safe to attempt to substitute the
+		// replacement past the left operand into the right operand.
+		if value, status := p.substituteSingleUseSymbolInExpr(e.Right, ref, replacement, replacementCanBeRemoved); status != substituteContinue {
+			e.Right = value
+			return expr, status
 		}
 
 	case *js_ast.EIf:
@@ -8612,6 +8625,11 @@ func (p *parser) substituteSingleUseSymbolInExpr(
 		return expr, substituteContinue
 	}
 
+	// We can always reorder past primitive values
+	if isPrimitiveLiteral(expr.Data) {
+		return expr, substituteContinue
+	}
+
 	// Otherwise we should stop trying to substitute past this point
 	return expr, substituteFailure
 }

internal/js_parser/js_parser_test.go

@@ -4219,6 +4219,26 @@ 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;")
 
+	// 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 x + undef", "return 1 + undef;")
+	check("let x = 1; return undef + x", "let x = 1;\nreturn undef + x;")
+
+	// Check substituting a value with side-effects into normal binary operators
+	check("let x = fn(); return x + 2", "return fn() + 2;")
+	check("let x = fn(); return 2 + x", "return 2 + fn();")
+	check("let x = fn(); return x + arg0", "return fn() + arg0;")
+	check("let x = fn(); return arg0 + x", "let x = fn();\nreturn arg0 + x;")
+	check("let x = fn(); return x + fn2()", "return fn() + fn2();")
+	check("let x = fn(); return fn2() + x", "let x = fn();\nreturn fn2() + x;")
+	check("let x = fn(); return x + undef", "return fn() + undef;")
+	check("let x = fn(); return undef + x", "let x = fn();\nreturn undef + x;")
+
 	// Cannot substitute into mutating unary operators
 	check("let x = 1; ++x", "let x = 1;\n++x;")
 	check("let x = 1; --x", "let x = 1;\n--x;")
@@ -4236,7 +4256,7 @@ func TestMangleInlineLocals(t *testing.T) {
 	check("let x = 1; arg0 += x", "arg0 += 1;")
 	check("let x = 1; arg0 ||= x", "arg0 ||= 1;")
 	check("let x = fn(); arg0 = x", "arg0 = fn();")
-	check("let x = fn(); arg0 += x", "arg0 += fn();")
+	check("let x = fn(); arg0 += x", "let x = fn();\narg0 += x;")
 	check("let x = fn(); arg0 ||= x", "let x = fn();\narg0 ||= x;")
 
 	// Cannot substitute past mutating binary operators when the left operand has side effects
@@ -4247,12 +4267,6 @@ func TestMangleInlineLocals(t *testing.T) {
 	check("let x = fn(); y.z += x", "let x = fn();\ny.z += x;")
 	check("let x = fn(); y.z ||= x", "let x = fn();\ny.z ||= x;")
 
-	// Cannot substitute code without side effects past non-mutating binary operators when the left operand has side effects
-	check("let x = 1; fn() + x", "let x = 1;\nfn() + x;")
-
-	// Cannot substitute code with side effects past non-mutating binary operators
-	check("let x = y(); arg0 + x", "let x = y();\narg0 + x;")
-
 	// Can substitute code without side effects into branches
 	check("let x = arg0; return x ? y : z;", "return arg0 ? y : z;")
 	check("let x = arg0; return arg1 ? x : y;", "return arg1 ? arg0 : y;")
@@ -4410,6 +4424,15 @@ func TestMangleInlineLocals(t *testing.T) {
 	check("let x = arg0[foo]; (0, x)()", "let x = arg0[foo];\nx();")
 	check("let x = arg0?.foo; (0, x)()", "let x = arg0?.foo;\nx();")
 	check("let x = arg0?.[foo]; (0, x)()", "let x = arg0?.[foo];\nx();")
+
+	// Explicitly allow reordering calls that are both marked as "/* @__PURE__ */".
+	// This happens because only two expressions that are free from side-effects
+	// can be freely reordered, and marking something as "/* @__PURE__ */" tells
+	// us that it has no side effects.
+	check("let x = arg0(); arg1() + x", "let x = arg0();\narg1() + x;")
+	check("let x = arg0(); /* @__PURE__ */ arg1() + x", "let x = arg0();\n/* @__PURE__ */ arg1() + x;")
+	check("let x = /* @__PURE__ */ arg0(); arg1() + x", "let x = /* @__PURE__ */ arg0();\narg1() + x;")
+	check("let x = /* @__PURE__ */ arg0(); /* @__PURE__ */ arg1() + x", "/* @__PURE__ */ arg1() + /* @__PURE__ */ arg0();")
 }
 
 func TestTrimCodeInDeadControlFlow(t *testing.T) {

scripts/js-api-tests.js

@@ -5044,6 +5044,51 @@ let transformTests = {
     assert.strictEqual(result, 3)
   },
 
+  async singleUseExpressionSubstitution({ esbuild }) {
+    function run(code) {
+      try {
+        return JSON.stringify(new Function(code)())
+      } catch (error) {
+        return error + ''
+      }
+    }
+    let bugs = ''
+    for (let input of [
+      `let fn = () => { throw new Error }; let x = undef; return fn() + x`,
+      `let fn = () => { throw new Error }; let x = fn(); return undef + x`,
+
+      `let fn = () => arg0 = 0; let x = fn(); return arg0 + x`,
+      `let fn = () => arg0 = 0; let x = fn(); return arg0 = x`,
+      `let fn = () => arg0 = 0; let x = fn(); return arg0 += x`,
+      `let fn = () => arg0 = 0; let x = fn(); return arg0 ||= x`,
+      `let fn = () => arg0 = 0; let x = fn(); return arg0 &&= x`,
+
+      `let fn = () => arg0 = 0; let obj = [1]; let x = arg0; return obj[fn()] + x`,
+      `let fn = () => arg0 = 0; let obj = [1]; let x = arg0; return obj[fn()] = x`,
+      `let fn = () => arg0 = 0; let obj = [1]; let x = arg0; return obj[fn()] += x`,
+      `let fn = () => arg0 = 0; let obj = [1]; let x = arg0; return obj[fn()] ||= x`,
+      `let fn = () => arg0 = 0; let obj = [1]; let x = arg0; return obj[fn()] &&= x`,
+
+      `let obj = { get y() { arg0 = 0; return 1 } }; let x = obj.y; return arg0 + x`,
+      `let obj = { get y() { arg0 = 0; return 1 } }; let x = arg0; return obj.y + x`,
+
+      `let x = undef; return arg0 || x`,
+      `let x = undef; return arg0 && x`,
+      `let x = undef; return arg0 ? x : 1`,
+      `let x = undef; return arg0 ? 1 : x`,
+
+      `let fn = () => { throw new Error }; let x = fn(); return arg0 || x`,
+      `let fn = () => { throw new Error }; let x = fn(); return arg0 && x`,
+      `let fn = () => { throw new Error }; let x = fn(); return arg0 ? x : 1`,
+      `let fn = () => { throw new Error }; let x = fn(); return arg0 ? 1 : x`,
+    ]) {
+      input = `function f(arg0) { ${input} } return f(123)`
+      const { code: minified } = await esbuild.transform(input, { minify: true })
+      if (run(input) !== run(minified)) bugs += '\n  ' + input
+    }
+    if (bugs !== '') throw new Error('Single-use expression substitution bugs:' + bugs)
+  },
+
   async platformNode({ esbuild }) {
     const { code } = await esbuild.transform(`export let foo = 123`, { format: 'cjs', platform: 'node' })
     assert(code.slice(code.indexOf('let foo')), `let foo = 123;