From 2430220a3a90c295e9b5a69cd1e9e9e86b83d864 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 6 Nov 2026 08:32:41 +0000 Subject: [PATCH] js_parser: keep a dead for-in/for-of head with two or more names as written, and print a dead require() as null The dead-code filter keeps a dead `var` by rewriting its declaration to the flat list of the names it declares. It did the same to the head of a for-in or for-of loop. A loop head takes exactly one declaration, so a dead `for (var [a, b] of x)` was printed as `for (var a, b of x)`: a SyntaxError in a valid file. The for-in and for-of arms now leave a head that declares two or more names as it is written. A head that declares one name or none takes the same path as before, so no output that was valid changes. A `require("x")` in dead control flow was replaced with a missing expression, which prints as nothing: `while (require("x"))` in a dead branch became `while ()`, and a kept head `[a = require("x"), b]` would print as `[a = , b]`. It is now `null`, as it is for `import()` and `require.resolve()`, and as esbuild prints it. The transpiler cache version is 52, because an older entry can hold the invalid text. --- src/js_parser/p.rs | 5 +- src/js_parser/scan/scan_side_effects.rs | 31 +++- src/jsc/RuntimeTranspilerCache.rs | 5 +- test/bundler/esbuild/dce.test.ts | 31 ++++ test/bundler/transpiler/transpiler.test.js | 180 +++++++++++++++++++++ 5 files changed, 245 insertions(+), 7 deletions(-) diff --git a/src/js_parser/p.rs b/src/js_parser/p.rs index 3859c3391621..e92ee74d330b 100644 --- a/src/js_parser/p.rs +++ b/src/js_parser/p.rs @@ -1630,10 +1630,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O // We don't want to spend time scanning the required files if they will // never be used. if self.is_control_flow_dead { - return Expr { - data: null_expr_data(), - loc: arg.loc, - }; + return self.new_expr(E::Null {}, arg.loc); } str_.resolve_rope_if_needed(self.arena); diff --git a/src/js_parser/scan/scan_side_effects.rs b/src/js_parser/scan/scan_side_effects.rs index f541f38bad57..9324e9fabac5 100644 --- a/src/js_parser/scan/scan_side_effects.rs +++ b/src/js_parser/scan/scan_side_effects.rs @@ -570,6 +570,33 @@ impl SideEffects { } } + /// True at the second name that the pattern declares. + fn declares_second_name(binding: Binding, has_first: &mut bool) -> bool { + match binding.data { + bun_ast::binding::Data::BIdentifier(_) => core::mem::replace(has_first, true), + bun_ast::binding::Data::BArray(array) => (array.items.slice().iter()) + .any(|item| Self::declares_second_name(item.binding, has_first)), + bun_ast::binding::Data::BObject(obj) => (obj.properties.slice().iter()) + .any(|item| Self::declares_second_name(item.value, has_first)), + _ => false, + } + } + + /// A for-in/for-of head takes one declaration: the `var a, b` of the `SLocal` arm is a + /// SyntaxError there, so a head that declares two or more names stays as it is written. + fn should_keep_for_in_or_of_head(init: Stmt, bump: &Bump) -> bool { + if let StmtData::SLocal(local) = init.data + && local.kind == bun_ast::S::Kind::KVar + { + let mut has_first = false; + let mut decls = local.decls.slice().iter(); + if decls.any(|decl| Self::declares_second_name(decl.binding, &mut has_first)) { + return true; + } + } + Self::should_keep_stmt_in_dead_control_flow(init, bump) + } + fn should_keep_stmts_in_dead_control_flow(stmts: bun_ast::StmtNodeList, bump: &Bump) -> bool { for child in stmts.slice() { if Self::should_keep_stmt_in_dead_control_flow(*child, bump) { @@ -693,12 +720,12 @@ impl SideEffects { } StmtData::SForIn(for_) => { - Self::should_keep_stmt_in_dead_control_flow(for_.init, bump) + Self::should_keep_for_in_or_of_head(for_.init, bump) || Self::should_keep_stmt_in_dead_control_flow(for_.body, bump) } StmtData::SForOf(for_) => { - Self::should_keep_stmt_in_dead_control_flow(for_.init, bump) + Self::should_keep_for_in_or_of_head(for_.init, bump) || Self::should_keep_stmt_in_dead_control_flow(for_.body, bump) } diff --git a/src/jsc/RuntimeTranspilerCache.rs b/src/jsc/RuntimeTranspilerCache.rs index cafb4f0281bf..710760e9fa1d 100644 --- a/src/jsc/RuntimeTranspilerCache.rs +++ b/src/jsc/RuntimeTranspilerCache.rs @@ -85,7 +85,10 @@ bun_core::declare_scope!(cache, visible); /// Version 50: The body of a class with lowered class decorators names the class by the /// binding that holds what the decorators return. /// Version 51: A `bun:` specifier that names no builtin is printed as it is written. -const EXPECTED_VERSION: u32 = 51; +/// Version 52: A `for-in` or `for-of` loop that never runs keeps a head that declares two or more +/// names as it is written, and a `require("x")` that never runs is `null`. Older entries can hold +/// a SyntaxError for both. +const EXPECTED_VERSION: u32 = 52; /// Source files smaller than this are not written to / read from the on-disk /// transpiler cache. Originally 50 KiB, which excluded almost every file in a diff --git a/test/bundler/esbuild/dce.test.ts b/test/bundler/esbuild/dce.test.ts index f3f159d484c3..a7d2f52e5c64 100644 --- a/test/bundler/esbuild/dce.test.ts +++ b/test/bundler/esbuild/dce.test.ts @@ -1390,6 +1390,37 @@ describe("bundler", () => { dce: true, minifySyntax: true, }); + // Not from esbuild. With --minify-syntax esbuild 0.28.2 prints these heads as `for (var a, b of x)`, + // a SyntaxError: a loop head takes one declaration. Without the flag it does not trim dead code. + for (const minifySyntax of [false, true]) { + itBundled(`dce/DeadForInOrForOfHead${minifySyntax ? "MinifySyntax" : ""}`, { + files: { + "/entry.js": /* js */ ` + function afterJump(x) { + return (a = 1) + (b = 2); + for (var [a, b] of x); + } + function inDeadBranch(x) { + if (false) { + for (var { a, b = require("./missing.js") } in require("./missing.js")); + } + return (a = 1) + (b = 2); + } + // Each function assigns its own "var" names, so they do not become globals. + console.log(afterJump([]), inDeadBranch({}), "a" in globalThis, "b" in globalThis); + `, + }, + minifySyntax, + onAfterBundle(api) { + const file = api.readFile("/out.js"); + expect(file).toContain("for (var [a, b] of x)"); + expect(file).toContain("for (var { a, b = null } in null)"); + }, + run: { + stdout: "3 3 false false", + }, + }); + } itBundled("dce/RemoveTrailingReturn", { todo: true, files: { diff --git a/test/bundler/transpiler/transpiler.test.js b/test/bundler/transpiler/transpiler.test.js index 4724a1bd2a31..77b8d5a918b3 100644 --- a/test/bundler/transpiler/transpiler.test.js +++ b/test/bundler/transpiler/transpiler.test.js @@ -3969,6 +3969,186 @@ console.log(resolve.length) "if (undefined) {\n let y = Math.random();\n}", ); }); + + // A dead loop stays when its head declares a `var` name. The head takes one declaration, + // so `for (var a, b of x)` is a SyntaxError. + // The default options trim an `if (false)`. `minify.syntax` also trims after a jump. + const expectDeadLoop = (loop, printed) => { + expectPrinted_( + `async function f(x) { if (false) { ${loop} ; } }`, + `async function f(x) {\n if (false) {\n ${printed}\n ;\n }\n}`, + ); + expectPrintedMin_( + `async function f(x) { return; ${loop} ; }`, + `async function f(x) {\n return;\n ${printed}\n ;\n}`, + ); + }; + + it("keeps a dead for-in or for-of head that declares two or more names as it is written", () => { + for (const head of [ + "var [a, b]", + "var { a, b }", + "var [, a, , b]", + "var [a, [b]]", + "var [[a], b]", + "var [[a, b]]", + "var { x: [a, b] }", + "var { x: { a }, y: [b] }", + "var [{ a }, {}, { b }]", + "var [a = 1, b = g()]", + "var { [k]: a, b }", + "var [a, ...b]", + "var { a, ...b }", + "var [a, a]", + "var [a, { b, c: [d] }]", + ]) { + expectDeadLoop(`for (${head} in x)`, `for (${head} in x)`); + expectDeadLoop(`for (${head} of x)`, `for (${head} of x)`); + expectDeadLoop(`for await (${head} of x)`, `for await (${head} of x)`); + } + }); + + it("keeps only the name of a dead for-in or for-of head that declares one name", () => { + for (const head of [ + "var a", + "var [a]", + "var { a }", + "var [a = g()]", + "var { [k]: a }", + "var [, a]", + "var [a, {}]", + "var { x: [], a }", + "var { a, x: [] }", + "var [[a]]", + "var { x: { a } }", + "var [[], [{ a }]]", + "var [...a]", + "var { ...a }", + ]) { + expectDeadLoop(`for (${head} in x)`, "for (var a in x)"); + expectDeadLoop(`for (${head} of x)`, "for (var a of x)"); + expectDeadLoop(`for await (${head} of x)`, "for await (var a of x)"); + } + }); + + it("keeps a head with two names where another dead statement holds the loop", () => { + const onOneLine = (code, transpiler_) => parsed(code, true, false, transpiler_).replaceAll(/\s+/g, " "); + for (const stmt of [ + "label: for (var [a, b] of x) ;", + "while (x) for (var [a, b] of x) ;", + "do for (var [a, b] of x) ; while (x);", + "for (;; ) for (var [a, b] of x) ;", + "for (let k in x) for (var [a, b] of x) ;", + "for (let v of x) for (var [a, b] of x) ;", + "try { for (var [a, b] of x) ; } catch {}", + "try {} catch { for (var [a, b] of x) ; }", + "try {} finally { for (var [a, b] of x) ; }", + "if (x) for (var [a, b] of x) ;", + "if (x) ; else for (var [a, b] of x) ;", + "switch (x) { case 1: for (var [a, b] of x) ; }", + ]) { + const inDeadBranch = `function f(x) { if (false) { ${stmt} } }`; + expect(onOneLine(inDeadBranch, transpiler)).toBe(inDeadBranch); + for (const jump of ["return;", "throw x;"]) { + const afterJump = `function f(x) { ${jump} ${stmt} }`; + expect(onOneLine(afterJump, transpilerMinifySyntax)).toBe(afterJump); + } + } + for (const jump of ["break;", "continue;"]) { + const afterJump = `function f(x) { while (x) { ${jump} for (var [a, b] of x) ; } }`; + expect(onOneLine(afterJump, transpilerMinifySyntax)).toBe(afterJump); + } + }); + + it("removes a dead for-in or for-of loop that declares no `var` name", () => { + for (const head of [ + "var []", + "var [, {}]", + "var { x: [] }", + "var [[], [{}]]", + "let [a, b]", + "const { a, b }", + "a", + "[a, b]", + ]) { + for (const loop of [`for (${head} in x)`, `for (${head} of x)`]) { + expectPrintedMin_(`function f(x) { return; ${loop} ; }`, "function f(x) {\n return;\n}"); + // The body keeps the loop, and the head stays as it is written. + expectPrintedMin_( + `function f(x) { return; ${loop} { var y = 1; } }`, + `function f(x) {\n return;\n ${loop}\n var y;\n}`, + ); + } + } + // The head of a `for (;;)` takes a list, so it still gets the names alone. + expectPrintedMin_( + "function f(x) { return; for (var [a, b] = x; a < b; a++) ; }", + "function f(x) {\n return;\n for (var a, b;a < b; a++)\n ;\n}", + ); + }); + + it.concurrent.each([ + [ + "a dead for-of loop after a return", + `function f(x) { + return (a = 1) + (b = 2); + for (var [a, b] of x); + }`, + ], + [ + "a dead for-in loop in an if (false)", + `function f(x) { + if (false) { + for (var { a, b } in x); + } + return (a = 1) + (b = 2); + }`, + ], + [ + "a dead require() call in the head of a dead loop", + `function f(x) { + if (false) { + for (var [a = require("./missing.js"), b] of require("./missing.js")); + } + return (a = 1) + (b = 2); + }`, + ], + [ + "a dead require() call in another dead statement that stays", + `function f(x) { + if (false) { + while (require("./missing.js")) var a; + switch (require("./missing.js")) { + case require("./missing.js").y: + var b; + } + } + return (a = 1) + (b = 2); + }`, + ], + [ + // Only the name of the head stays, so the file loads as a module too. + "sloppy-only code in a dead head that declares one name", + `function f(x) { + if (false) { + for (var [a = delete q] of x); + for (var { [delete q]: b } in x); + } + return (a = 1) + (b = 2); + }`, + ], + ])("runs a file with %s", async (_, source) => { + using dir = tempDir("dead-for-in-of-head", { + // `f` assigns its own `var` names, so they do not become globals. + "entry.js": source + "\nconsole.log(f([]), typeof a, typeof b);", + }); + expect(await bunRun(join(String(dir), "entry.js"))).toEqual({ + stdout: "3 undefined undefined", + stderr: "", + exitCode: 0, + signalCode: null, + }); + }); }); });