js_parser: do not visit a statement again after a single-use value is inlined

src/js_parser/p.rs

@@ -564,9 +564,6 @@ pub struct P<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> {
     /// path which also pushes a scope, so this flag is redundant for those.)
     pub(crate) is_inside_single_stmt_body: bool,
 
-    /// We must be careful to avoid revisiting nodes that have scopes.
-    pub(crate) is_revisit_for_substitution: bool,
-
     pub(crate) method_call_must_be_replaced_with_undefined: bool,
 
     // Inside a TypeScript namespace, an "export declare" statement can be used
@@ -1270,7 +1267,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
 
     /// One use of `ns` (counted by `record_usage`) is an accounted-for read.
     pub(crate) fn note_tracked_namespace_use(&mut self, ns: Ref) {
-        if self.is_revisit_for_substitution || self.is_control_flow_dead {
+        if self.is_control_flow_dead {
             return;
         }
         let n = self.namespace_tracked_uses.entry(ns).or_default();
@@ -2093,9 +2090,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
     /// another binding (an unbound global, whose name every scope already
     /// avoids), so `scope_uses` skips it.
     pub(crate) fn record_usage_impl(&mut self, ref_: Ref, scope_use: bool) {
-        if self.is_revisit_for_substitution {
-            return;
-        }
         // The use count stored in the symbol is used for generating symbol names
         // during minification. These counts shouldn't include references inside dead
         // code regions since those will be culled.
@@ -2750,28 +2744,11 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             replacement_can_be_removed,
         ) {
             Substitution::Success(result) => {
-                if matches!(
-                    result.data,
-                    js_ast::ExprData::EBinary(_)
-                        | js_ast::ExprData::EUnary(_)
-                        | js_ast::ExprData::EIf(_)
-                ) {
-                    let prev_substituting = self.is_revisit_for_substitution;
-                    self.is_revisit_for_substitution = true;
-                    // O(n^2) and we will need to think more carefully about
-                    // this once we implement syntax compression
-                    *expr = result;
-                    self.visit_expr(expr);
-                    self.is_revisit_for_substitution = prev_substituting;
-                } else {
-                    *expr = result;
-                }
-                return true;
+                *expr = result;
+                true
             }
-            _ => {}
+            _ => false,
         }
-
-        false
     }
 
     fn substitute_single_use_symbol_in_expr(
@@ -2954,6 +2931,18 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                         return Substitution::Failure(expr);
                     }
 
+                    // The right side does not always run. esbuild has no such check.
+                    if !replacement_can_be_removed
+                        && matches!(
+                            e.op,
+                            js_ast::op::Code::BinLogicalAnd
+                                | js_ast::op::Code::BinLogicalOr
+                                | js_ast::op::Code::BinNullishCoalescing
+                        )
+                    {
+                        return Substitution::Failure(expr);
+                    }
+
                     // If we get here then it should be safe to attempt to substitute the
                     // replacement past the left operand into the right operand.
                     if let Some(done) = self.substitute_single_use_symbol_in_child(
@@ -5880,17 +5869,15 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         }
         if opts.is_call_target() || opts.is_template_tag() {
             self.symbols[ref_.inner_index() as usize].set_called_as_method(true);
-            if !self.is_revisit_for_substitution {
-                let call = self
-                    .import_symbol_property_uses
-                    .get_or_put_value(self.exports_ref, Default::default())
-                    .expect("OOM")
-                    .value_ptr
-                    .get_or_put_value(name, Default::default())
-                    .expect("OOM");
-                call.count_estimate += 1;
-                call.is_call_target = true;
-            }
+            let call = self
+                .import_symbol_property_uses
+                .get_or_put_value(self.exports_ref, Default::default())
+                .expect("OOM")
+                .value_ptr
+                .get_or_put_value(name, Default::default())
+                .expect("OOM");
+            call.count_estimate += 1;
+            call.is_call_target = true;
         }
     }
 
@@ -6616,10 +6603,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         {
             return false;
         }
-        // Same node visited again (const inlining); already counted.
-        if self.is_revisit_for_substitution {
-            return true;
-        }
         let Some(use_) = self.existing_part_use(base) else {
             return false;
         };
@@ -6640,7 +6623,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
     }
 
     pub(crate) fn ignore_usage(&mut self, r#ref: Ref) {
-        if !self.is_control_flow_dead && !self.is_revisit_for_substitution {
+        if !self.is_control_flow_dead {
             debug_assert!((r#ref.inner_index() as usize) < self.symbols.len());
             self.symbols[r#ref.inner_index() as usize].use_count_estimate = self.symbols
                 [r#ref.inner_index() as usize]
@@ -9795,7 +9778,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             scope_uses: Vec::new(),
             span_uses: Vec::new(),
             is_inside_single_stmt_body: false,
-            is_revisit_for_substitution: false,
             method_call_must_be_replaced_with_undefined: false,
             has_non_local_export_declare_inside_namespace: false,
             await_target: None,

src/js_parser/visit/mod.rs

@@ -1804,9 +1804,20 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             }
         }
 
-        if !p.options.features.minify_syntax || !p.options.features.dead_code_elimination {
+        // `s_switch` runs this once every case body has been visited.
+        if kind == StmtsKind::SwitchStmt {
             return Ok(());
         }
+        p.minify_visited_stmts(stmts);
+        Ok(())
+    }
+
+    /// `stmts` must already be visited, with `current_scope` still the list's scope.
+    pub(crate) fn minify_visited_stmts(&mut self, stmts: &mut ListManaged<'a, Stmt>) {
+        let p = self;
+        if !p.options.features.minify_syntax || !p.options.features.dead_code_elimination {
+            return;
+        }
 
         // SAFETY: current_scope is a valid arena ptr for the parse.
         if p.current_scope().parent.is_some() && !p.current_scope().contains_direct_eval {
@@ -2176,7 +2187,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
 
         // stmts.deinit(); — Drop handles freeing the old buffer (BumpVec is arena-backed).
         *stmts = output;
-        Ok(())
     }
 }
 

src/js_parser/visit/visit_expr.rs

@@ -156,14 +156,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
     }
 
     fn e_identifier(p: &mut Self, e: &mut Expr, in_: ExprIn) {
-        // The substitution revisit re-enters with an already-resolved `Ref`.
-        // `find_symbol` below only looks in `scope.members`, so a symbol that
-        // lives only in `scope.generated` (e.g. a React Compiler outlined
-        // function) would be re-bound to a fresh `Unbound` and the renamer
-        // would lose the link to its declaration.
-        if p.is_revisit_for_substitution {
-            return;
-        }
         let expr = *e;
         let mut e_ = expr
             .data
@@ -1829,16 +1821,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         p.visit_expr(&mut e_.expr);
         p.visit_expr(&mut e_.options);
 
-        // Already transposed (this is a re-visit, e.g. after the minifier
-        // substituted the expression into a later use): the import record and
-        // its namespace tracking stand; transposing again would mint a second
-        // record for the same call.
-        if e_.import_record_index != u32::MAX {
-            p.should_fold_typescript_constant_expressions =
-                prev_should_fold_typescript_constant_expressions;
-            return;
-        }
-
         // Import transposition is able to duplicate the options structure, so
         // only perform it if the expression is side effect free.
         //
@@ -2569,9 +2551,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         let expr = *e;
         let _ = in_;
         let mut e_ = expr.data.e_arrow().expect("infallible: variant checked");
-        if p.is_revisit_for_substitution {
-            return;
-        }
 
         // The struct is `Copy`, so save/restore is a plain copy.
         let old_fn_or_arrow_data = p.fn_or_arrow_data_visit;
@@ -2698,9 +2677,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         let expr = *e;
         let _ = in_;
         let mut e_ = expr.data.e_function().expect("infallible: variant checked");
-        if p.is_revisit_for_substitution {
-            return;
-        }
 
         let mut react_hook_data: Option<crate::parser::HookContext> = None;
         let prev_hook_ctx = p.react_refresh.hook_ctx_storage;
@@ -2776,9 +2752,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         let expr = *e;
         let _ = in_;
         let mut e_ = expr.data.e_class().expect("infallible: variant checked");
-        if p.is_revisit_for_substitution {
-            return;
-        }
 
         // Save name from assignment context before visiting (nested visits may overwrite it)
         let decorator_name_from_context = p.decorator_class_name;

src/js_parser/visit/visit_stmt.rs

@@ -2142,6 +2142,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             let old_is_inside_switch = p.fn_or_arrow_data_visit.is_inside_switch;
             p.fn_or_arrow_data_visit.is_inside_switch = true;
             let cases = data.cases.slice_mut();
+            let mut bodies: BumpVec<'_, StmtList<'a>> =
+                BumpVec::with_capacity_in(cases.len(), p.arena);
             for i in 0..cases.len() {
                 if let Some(val) = cases[i].value.as_mut() {
                     p.visit_expr(val);
@@ -2152,7 +2154,12 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 let mut _stmts = stmts_to_list(p.arena, cases[i].body);
                 p.visit_stmts(&mut _stmts, StmtsKind::SwitchStmt)
                     .expect("unreachable");
-                cases[i].body = list_to_stmts(_stmts);
+                bodies.push(_stmts);
+            }
+            // The cases share one scope: use counts are final only when every case is visited.
+            for (case, mut body) in cases.iter_mut().zip(bodies) {
+                p.minify_visited_stmts(&mut body);
+                case.body = list_to_stmts(body);
             }
             p.fn_or_arrow_data_visit.is_inside_switch = old_is_inside_switch;
 

src/jsc/RuntimeTranspilerCache.rs

@@ -65,7 +65,8 @@ bun_core::declare_scope!(cache, visible);
 /// the cache-HIT path reinstates #33904 for them.
 /// Version 34: An import that a plugin `onResolve` answers is printed as it is written.
 /// Version 35: Long hexadecimal, octal and binary literals are rounded to the nearest double.
-const EXPECTED_VERSION: u32 = 35;
+/// Version 36: A statement is visited once, and a `switch` case is minified after every case is visited.
+const EXPECTED_VERSION: u32 = 36;
 
 /// 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

test/bundler/bundler_dynamic_import_dce.test.ts

@@ -647,8 +647,7 @@ describe("bundler", () => {
   }
 
   // The minifier substitutes a single-use `const x = cond ? (await import()).a() : b`
-  // into its use and re-visits it; the re-visit must not mint a second,
-  // untracked record for the same import().
+  // into its use. The import() must keep its one tracked record.
   itBundled("dynamic_import_dce/MinifyRevisitDoesNotDuplicateRecord", {
     files: {
       "/entry.ts": /* ts */ `
@@ -673,6 +672,29 @@ describe("bundler", () => {
     },
   });
 
+  // The single-use `v` moves into the statement of the import(). Only the
+  // exports that the callback reads are kept.
+  itBundled("dynamic_import_dce/MinifySingleUseNextToImportThen", {
+    files: {
+      "/entry.js": /* js */ `
+        async function viaMember(g) { const v = g(); return v + await import("./member.js").then(m => m.c); }
+        async function viaPattern(g) { const v = g(); return v + await import("./pattern.js").then(({ a }) => a); }
+        console.log(await viaMember(() => "x"), await viaPattern(() => "y"));
+      `,
+      "/member.js": `export const a = "A_DROPPED"; export const b = "B_DROPPED"; export const c = "C";`,
+      "/pattern.js": `export const a = "A"; export const b = "B_DROPPED";`,
+    },
+    minifySyntax: true,
+    splitting: true,
+    target: "bun",
+    format: "esm",
+    outdir: "/out",
+    run: { file: "/out/entry.js", stdout: "xC yA" },
+    onAfterBundle(api) {
+      expect(readAllOutputs(api.outdir)).not.toContain("DROPPED");
+    },
+  });
+
   // Locals merged by hoisting (duplicate `var`, `var` over a parameter) share
   // one symbol; their exports stay.
   itBundled("dynamic_import_dce/HoistedVarDestructureKeepsBoth", {

test/bundler/bundler_jsx.test.ts

@@ -1455,4 +1455,28 @@ describe("bundler", () => {
       run: { stdout: "1" },
     });
   });
+
+  // The signature of a hook is made from the hooks that it calls. A single-use
+  // value that the minifier moves into the statement of a hook call does not
+  // add a call, so the signature is the same with and without minifySyntax.
+  for (const minifySyntax of [false, true]) {
+    itBundled(`jsx/FastRefreshSignatureNextToSingleUse${minifySyntax ? "MinifySyntax" : ""}`, {
+      files: {
+        "/index.tsx": /* js */ `
+          globalThis.$RefreshSig$ = () => fn => fn;
+          globalThis.$RefreshReg$ = () => {};
+          function useThing() { return 1; }
+          function useCounter(g) { const v = g(); return v + useThing(); }
+          console.log(useCounter(() => 1));
+        `,
+      },
+      backend: "cli",
+      reactFastRefresh: true,
+      minifySyntax,
+      onAfterBundle(api) {
+        expect(api.readFile("out.js").match(/_s\(useCounter, "([^"]+)"/)?.[1]).toBe("yRI4DTW2zYY=");
+      },
+      run: { stdout: "2" },
+    });
+  }
 });