From ccc8e8b31f90fc0464d12cf125ecf2df4d2a68ae Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 19 Oct 2022 13:48:33 -0400 Subject: [PATCH] fix #2619: bug with single-use substitutions --- CHANGELOG.md | 44 +++++++++++++++++++++++++++ internal/js_parser/js_parser.go | 34 ++++++++++++++++----- internal/js_parser/js_parser_test.go | 37 ++++++++++++++++++----- scripts/js-api-tests.js | 45 ++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0472bd18c69..849a539d414 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 7f45c5b75a0..3be17b863b4 100644 --- a/internal/js_parser/js_parser.go +++ b/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 } diff --git a/internal/js_parser/js_parser_test.go b/internal/js_parser/js_parser_test.go index 0d3a94dc493..4c711150bd7 100644 --- a/internal/js_parser/js_parser_test.go +++ b/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) { diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js index 4cf12c4faff..d6fc3785ee3 100644 --- a/scripts/js-api-tests.js +++ b/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;