minify: never give a slot the name of a pinned symbol

src/ast/scope.rs

@@ -355,6 +355,9 @@ pub struct Scope {
     // evaluated code might reference anything that it has access to.
     pub contains_direct_eval: bool,
 
+    // Set on a "with" scope and every scope that encloses it, for compute_reserved_names_for_scope.
+    pub contains_with: bool,
+
     // This is to help forbid "arguments" inside class body scopes
     pub forbid_arguments: bool,
 
@@ -387,6 +390,7 @@ impl Scope {
         label_ref: Ref::NONE,
         label_stmt_is_loop: false,
         contains_direct_eval: false,
+        contains_with: false,
         forbid_arguments: false,
         strict_mode: StrictModeKind::SloppyMode,
         is_after_const_local_prefix: false,

src/js_parser/p.rs

@@ -2152,6 +2152,19 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         }
     }
 
+    /// The renamer follows links before it reads the flag, so pin the whole chain.
+    pub(crate) fn set_must_not_be_renamed_through_links(&mut self, ref_: Ref) {
+        let mut ref_ = ref_;
+        loop {
+            let symbol = &mut self.symbols[ref_.inner_index() as usize];
+            symbol.set_must_not_be_renamed(true);
+            if !symbol.has_link() {
+                return;
+            }
+            ref_ = symbol.link.get();
+        }
+    }
+
     pub(crate) fn log_arrow_arg_errors(&mut self, errors: &mut DeferredArrowArgErrors) {
         if errors.invalid_expr_await.len > 0 {
             let r = errors.invalid_expr_await;
@@ -3558,6 +3571,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             // Check for collisions that would prevent to hoisting "var" symbols up to the enclosing function scope
             if let Some(scope_parent) = scope_ref.parent {
                 let scope_strict_mode = scope_ref.strict_mode;
+                let scope_is_with = scope_ref.kind == js_ast::scope::Kind::With;
                 // The loop below never inserts into `scope.members` itself (only ancestors), so
                 // snapshotting `(name_ptr, Member)` pairs up front preserves iteration semantics
                 // and lets us re-borrow `*scope` mutably inside the body.
@@ -3647,6 +3661,11 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                         }
                     }
 
+                    // `with (obj) var t = 2` declares `t` in the "with" scope, which the walk below starts above.
+                    if scope_is_with {
+                        self.set_must_not_be_renamed_through_links(value.ref_);
+                    }
+
                     if hash.is_none() {
                         hash = Some(Scope::get_member_hash(name));
                     }
@@ -3668,7 +3687,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                         //   assert(obj.foo === 2)
                         //
                         if scope_kind == js_ast::scope::Kind::With {
-                            self.symbols[symbol_idx].set_must_not_be_renamed(true);
+                            self.set_must_not_be_renamed_through_links(value.ref_);
                         }
 
                         if let Some(member_in_scope) =
@@ -3693,6 +3712,11 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                             {
                                 // Silently merge this symbol into the existing symbol
                                 self.symbols[symbol_idx].link.set(member_in_scope.ref_);
+                                if self.symbols[symbol_idx].must_not_be_renamed() {
+                                    self.set_must_not_be_renamed_through_links(
+                                        member_in_scope.ref_,
+                                    );
+                                }
                                 // `StringHashMap` get_or_put already stores the key on insert and
                                 // cannot hand out `&mut K` (see StringHashMapGetOrPut docs), so
                                 // no key write is needed here.
@@ -3891,6 +3915,16 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             if self.options.features.commonjs_at_runtime {
                 self.has_with_scope = true;
             }
+
+            // Marked up the chain like contains_direct_eval, for compute_reserved_names_for_scope.
+            let mut scope_iter: Option<js_ast::StoreRef<Scope>> = Some(scope);
+            while let Some(mut s) = scope_iter {
+                if s.contains_with {
+                    break;
+                }
+                s.contains_with = true;
+                scope_iter = s.parent;
+            }
         }
 
         if cfg!(debug_assertions) {

src/js_parser/scan/scan_symbols.rs

@@ -147,7 +147,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         // property on the target object of the "with" statement. We must not rename
         // it or we risk changing the behavior of the code.
         if is_inside_with_scope {
-            self.symbols[ref_.inner_index() as usize].set_must_not_be_renamed(true);
+            self.set_must_not_be_renamed_through_links(ref_);
         }
 
         // Track how many times we've referenced this symbol

src/js_printer/lib.rs

@@ -7700,7 +7700,7 @@ pub fn print_ast<'a, W: WriterTrait, const ASCII_ONLY: bool, const GENERATE_SOUR
     _writer: W,
     bump: &'a bun_alloc::Arena,
     tree: &'a Ast,
-    symbols: js_ast::symbol::Map,
+    mut symbols: js_ast::symbol::Map,
     source: &'a bun_ast::Source,
     opts: Options<'a>,
 ) -> crate::Result<usize> {
@@ -7722,6 +7722,20 @@ pub fn print_ast<'a, W: WriterTrait, const ASCII_ONLY: bool, const GENERATE_SOUR
     let module_scope = &tree.module_scope;
     let stable_source_indices = [source.index.0];
     let renamer: rename::Renamer<'_, '_> = if opts.minify_identifiers {
+        // Pinned before the reserved names are computed so no slot takes one of these names.
+        let dont_break_the_code = [tree.module_ref, tree.exports_ref, tree.require_ref];
+        for ref_ in dont_break_the_code {
+            if let Some(symbol) = symbols.get_mut(ref_) {
+                symbol.set_must_not_be_renamed(true);
+            }
+        }
+
+        for named_export in tree.named_exports.values() {
+            if let Some(symbol) = symbols.get_mut(named_export.ref_) {
+                symbol.set_must_not_be_renamed(true);
+            }
+        }
+
         let mut reserved_names = rename::compute_initial_reserved_names(opts.module_type)?;
         for child in module_scope.children.slice() {
             // `StoreRef<Scope>` has safe `DerefMut`; copy the handle to a mut
@@ -7745,21 +7759,6 @@ pub fn print_ast<'a, W: WriterTrait, const ASCII_ONLY: bool, const GENERATE_SOUR
         let module_ref = tree.module_ref;
         let parts = &tree.parts;
 
-        // `symbols` was moved into `minify_renamer`; reach it through
-        // the renamer for the post-init `must_not_be_renamed` pass.
-        let dont_break_the_code = [tree.module_ref, tree.exports_ref, tree.require_ref];
-        for ref_ in dont_break_the_code {
-            if let Some(symbol) = minify_renamer.symbols.get_mut(ref_) {
-                symbol.set_must_not_be_renamed(true);
-            }
-        }
-
-        for named_export in tree.named_exports.values() {
-            if let Some(symbol) = minify_renamer.symbols.get_mut(named_export.ref_) {
-                symbol.set_must_not_be_renamed(true);
-            }
-        }
-
         if uses_exports_ref {
             minify_renamer.accumulate_symbol_use_count(
                 &mut top_level_symbols,

src/js_printer/renamer.rs

@@ -1480,12 +1480,11 @@ pub fn compute_reserved_names_for_scope(
         }
     }
 
-    // If there's a direct "eval" somewhere inside the current scope, continue
-    // traversing down the scope tree until we find it to get all reserved names
-    if scope.contains_direct_eval {
+    // Keep traversing down the scope tree toward a direct "eval" or a "with" to get all reserved names
+    if scope.contains_direct_eval || scope.contains_with {
         for child in scope.children.slice() {
             // `StoreRef<Scope>: Deref<Target = Scope>` — safe arena-backed deref.
-            if child.contains_direct_eval {
+            if child.contains_direct_eval || child.contains_with {
                 compute_reserved_names_for_scope(child, symbols, names);
             }
         }

test/bundler/bundler_minify.test.ts

@@ -19,6 +19,133 @@ describe("bundler", () => {
     },
     run: { stdout: "top fn" },
   });
+
+  // A symbol read through a `with` body keeps its original name. The renamer
+  // must not give that name to another symbol the body can see, such as the
+  // parameter of the enclosing function: `function f(obj) { var t = 1; with
+  // (obj) { return t } }` printed as `function f(t) { var t = 1; with (t)
+  // return t }` reads the local instead of `obj.t`. Each `f_` function pins
+  // one of the 54 single-character names the minifier can produce, so
+  // whichever name the (most used) parameter slot gets, one `with` body breaks
+  // without the fix. `hoisted` and `merged` declare the pinned `var` twice, so
+  // the reference that the `with` pins is a link to the function-level symbol,
+  // which has to keep its name too. `braceless` declares the `var` in the
+  // `with` scope itself, which the hoisting walk starts above.
+  const singleCharNames = [..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_$"];
+  const withPinnedNamesSource = [
+    ...singleCharNames.map(n => `function f_${n}(obj) { var ${n} = 1; obj.p; obj.p; with (obj) { return ${n}; } }`),
+    "function hoisted(obj) { var t = 1; obj.p; obj.p; with (obj) { var t = 2; } return [t, obj.t]; }",
+    "function merged(obj) { var t = 1; obj.p; obj.p; { var t = 2; with (obj) { return t; } } }",
+    "function braceless(obj) { obj.p; obj.p; with (obj) var t = 2; return [t, obj.t]; }",
+    "const bad = [];",
+    ...singleCharNames.map(n => `if (f_${n}({}) !== 1 || f_${n}({ ${n}: 9 }) !== 9) bad.push("${n}");`),
+    `if (JSON.stringify([hoisted({}), hoisted({ t: 9 })]) !== "[[2,null],[1,2]]") bad.push("hoisted");`,
+    `if (merged({}) !== 2 || merged({ t: 9 }) !== 9) bad.push("merged");`,
+    `if (JSON.stringify([braceless({}), braceless({ t: 9 })]) !== "[[2,null],[null,2]]") bad.push("braceless");`,
+    "console.log(JSON.stringify(bad));",
+  ].join("\n");
+  itBundled("minify/WithStatementPinnedNameNotReusedByEnclosingScope", {
+    files: {
+      "/entry.js": withPinnedNamesSource,
+    },
+    format: "cjs",
+    minifyIdentifiers: true,
+    run: { stdout: "[]" },
+  });
+  // `bun build --no-bundle` names symbols through the same reserved set.
+  itBundled("minify/WithStatementPinnedNameNotReusedByEnclosingScopeNoBundle", {
+    files: {
+      "/entry.js": withPinnedNamesSource,
+    },
+    bundling: false,
+    format: "cjs",
+    minifyIdentifiers: true,
+    run: { stdout: "[]" },
+  });
+  // `bun build --no-bundle` keeps the export names. They were pinned after the
+  // reserved names were computed, so a local in a nested function could take
+  // one of them and shadow the export it reads.
+  itBundled("minify/NoBundleExportNameNotReusedByLocal", {
+    files: {
+      "/entry.js": [
+        ...singleCharNames.map(n => `export const ${n} = "${n}";`),
+        "export function check() {",
+        "  let local = 0; local; local;",
+        "  const bad = [];",
+        ...singleCharNames.map(n => `  if (${n} !== "${n}") bad.push("${n}");`),
+        "  return bad;",
+        "}",
+        "console.log(JSON.stringify(check()));",
+      ].join("\n"),
+    },
+    bundling: false,
+    minifyIdentifiers: true,
+    run: { stdout: "[]" },
+  });
+  // Every kind of export takes one-character names here, and each test below
+  // declares 54 other names, one for each one-character name the minifier can
+  // produce. So every export name is wanted by one of them, whatever the
+  // character frequency of the file is.
+  const exportKinds: ((n: string) => [declaration: string, read: string])[] = [
+    n => [`export const ${n} = "${n}";`, n],
+    n => [`export let ${n} = "${n}";`, n],
+    n => [`export var ${n} = "${n}";`, n],
+    n => [`export function ${n}() { return "${n}"; }`, `${n}()`],
+    n => [`export class ${n} { static value = "${n}"; }`, `${n}.value`],
+    n => [`const ${n} = "${n}"; export { ${n} };`, n],
+    n => [`export const { ${n} } = { ${n}: "${n}" };`, n],
+  ];
+  const everyExportKind = singleCharNames.map((n, i) => exportKinds[i % exportKinds.length](n));
+  const exportDeclarations = everyExportKind.map(([declaration]) => declaration);
+  const exportChecks = singleCharNames.map((n, i) => `if (${everyExportKind[i][1]} !== "${n}") bad.push("${n}");`);
+  // A parameter, a local and a catch binding of a function that reads the
+  // exports: `export const t = ...; export function label(item) { return
+  // t(item.name) }` printed as `function label(t) { return t(t.name) }`.
+  itBundled("minify/NoBundleExportNameNotReusedInFunction", {
+    files: {
+      "/entry.js": [
+        ...exportDeclarations,
+        `export function check(${singleCharNames.map((_, i) => `param${i} = ${i}`).join(", ")}) {`,
+        "  const bad = [];",
+        `  let local = [${singleCharNames.map((_, i) => `param${i}`).join(", ")}].join();`,
+        `  if (local !== "${singleCharNames.map((_, i) => i).join()}") bad.push("params " + local);`,
+        "  try { throw local.length; } catch (caught) { if (caught !== local.length) bad.push('caught ' + caught); }",
+        ...exportChecks,
+        "  return bad;",
+        "}",
+        "console.log(JSON.stringify(check()));",
+      ].join("\n"),
+    },
+    bundling: false,
+    minifyIdentifiers: true,
+    run: { stdout: "[]" },
+  });
+  // An import and a private top-level name: `import { a } from "./x.js";
+  // export const t = a` printed as `import { a as t } from "./x.js"; export
+  // const t = t`, which does not parse.
+  itBundled("minify/NoBundleExportNameNotReusedAtTopLevel", {
+    files: {
+      "/entry.js": [
+        `import fromDefault, { fromNamed } from "./imported.js";`,
+        `import * as fromNamespace from "./imported.js";`,
+        `function privateFunction() { return "function"; }`,
+        `class PrivateClass { static value = "class"; }`,
+        ...singleCharNames.map((_, i) => `const private${i} = ${i};`),
+        ...exportDeclarations,
+        "const bad = [];",
+        `const imports = [fromDefault, fromNamed, fromNamespace.fromNamed, privateFunction(), PrivateClass.value].join();`,
+        `if (imports !== "default,named,named,function,class") bad.push("imports " + imports);`,
+        `const privates = [${singleCharNames.map((_, i) => `private${i}`).join(", ")}].join();`,
+        `if (privates !== "${singleCharNames.map((_, i) => i).join()}") bad.push("privates " + privates);`,
+        ...exportChecks,
+        "console.log(JSON.stringify(bad));",
+      ].join("\n"),
+      "/imported.js": `export const fromNamed = "named"; export default "default";`,
+    },
+    bundling: false,
+    minifyIdentifiers: true,
+    run: { stdout: "[]" },
+  });
   itBundled("minify/TemplateStringFolding", {
     files: {
       "/entry.js": /* js */ `

test/bundler/transpiler/transpiler.test.js

@@ -5340,6 +5340,30 @@ describe.concurrent("minify.identifiers on an empty source or a data loader", ()
   });
 });
 
+// The exports keep their names and take every one-character name the minifier
+// can produce, so the import and the parameter each need a longer name.
+// `label(item)` printed as `function label(t) { return t(t.name) }`.
+it.each(["transformSync", "transform"])(
+  "minify.identifiers: %s gives no import or parameter the name of an export",
+  async method => {
+    const exportNames = [..."abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ_$"];
+    const output = await new Bun.Transpiler({ loader: "js", minify: { identifiers: true } })[method](
+      [
+        `import { imported } from "./imported.js";`,
+        ...exportNames.map(name => `export const ${name} = "${name}";`),
+        `export function label(item) { return t(item.name) + " #" + item.id + imported; }`,
+      ].join("\n"),
+    );
+    const [, importAlias] = output.match(/import \{ imported as ([\w$]+) \}/);
+    const [, parameter] = output.match(/function label\(([\w$]+)\)/);
+    expect({
+      importAlias: exportNames.includes(importAlias) ? `${importAlias} is an export` : "ok",
+      parameter: exportNames.includes(parameter) ? `${parameter} is an export` : "ok",
+      body: output.includes(`return t(${parameter}.name) + " #" + ${parameter}.id + ${importAlias};`),
+    }).toEqual({ importAlias: "ok", parameter: "ok", body: true });
+  },
+);
+
 it("runtime transpiler stack overflows", async () => {
   expect(async () => await import("./fixtures/lots-of-for-loop.js")).toThrow(`Maximum call stack size exceeded`);
 });