js_parser: keep typeof name and the lookups of a body-declared var inside with

src/aot/compile/removable/unused.rs

@@ -1,5 +1,6 @@
 use bun_sema::bind::Parent;
 use bun_sema::fold::is_removed_where_never_run;
+use bun_sema::import_sites::is_in_with_scope;
 use bun_sema::util::SharedSort;
 
 use super::*;
@@ -211,7 +212,9 @@ impl Removable<'_, '_> {
                 UnOp::Typeof => {
                     let name = hir.skip_type_assertions(operand);
                     if matches!(hir[name].kind, ExprKind::Ident(_)) {
-                        return self.is_name_initialized(name);
+                        // Inside a `with` the lookup of the name can run a trap or a getter of the object.
+                        return !is_in_with_scope(self.bound, name)
+                            && self.is_name_initialized(name);
                     }
                     if matches!(decisions.decision(name), Decision::GlobalPath(&[_])) {
                         return false;

src/ast/e.rs

@@ -144,6 +144,7 @@ bitflags::bitflags! {
         /// Also if we know a typeof operator was originally an identifier, then
         /// we know that this typeof operator always has no side effects (even if
         /// we consider the identifier by itself to have a side effect).
+        /// An identifier with `must_keep_due_to_with_stmt` is the exception.
         ///
         /// Note that there *is* actually a case where "typeof x" can throw an error:
         /// when "x" is being referenced inside of its TDZ (temporal dead zone). TDZ
@@ -421,6 +422,7 @@ impl Identifier {
     /// If we're inside a "with" statement, this identifier may be a property
     /// access. In that case it would be incorrect to remove this identifier since
     /// the property access may be a getter or setter with side effects.
+    /// A "typeof" of it stays too: the lookup alone can run a proxy trap of the object.
     #[inline]
     pub const fn must_keep_due_to_with_stmt(self) -> bool {
         self.ref_.user_bit(0)

src/ast/scope.rs

@@ -351,7 +351,7 @@ 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.
+    // Set on a "with" scope and every scope that encloses it, for the renamer and find_symbol.
     pub contains_with: bool,
 
     // This is to help forbid "arguments" inside class body scopes

src/js_parser/p.rs

@@ -2775,8 +2775,13 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 }
                 js_ast::StmtData::SLocal(mut local) => {
                     if local.decls.len_u32() > 0 {
+                        let is_var = local.kind == js_ast::LocalKind::KVar;
                         let first = &mut local.decls.slice_mut()[0];
-                        if matches!(first.binding.data, js_ast::b::B::BIdentifier(_)) {
+                        if let js_ast::b::B::BIdentifier(id) = first.binding.data {
+                            // Below "with", a `var` is looked up before its initializer runs.
+                            if is_var && self.var_store_asks_with_target(id.r#ref) {
+                                return false;
+                            }
                             if let Some(value) = first.value.as_mut() {
                                 break 'brk js_ast::StoreRef::from_bump(value);
                             }
@@ -3807,7 +3812,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 self.has_with_scope = true;
             }
 
-            // Marked up the chain like contains_direct_eval, for compute_reserved_names_for_scope.
+            // Marked up the chain like contains_direct_eval, for the renamer and find_symbol.
             let mut scope_iter: Option<js_ast::StoreRef<Scope>> = Some(scope);
             while let Some(mut s) = scope_iter {
                 if s.contains_with {
@@ -6311,19 +6316,19 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 // if the result is unused and the operand has no side effects. However, it
                 // has a special case where if the operand is an identifier expression such
                 // as "typeof x" and "x" doesn't exist, no reference error is thrown so the
-                // operation has no side effects.
+                // operation has no side effects, unless "x" has `must_keep_due_to_with_stmt`.
                 //
                 // Note that there *is* actually a case where "typeof x" can throw an error:
                 // when "x" is being referenced inside of its TDZ (temporal dead zone). TDZ
                 // checks are not yet handled correctly by bun or esbuild, so this possibility is
                 // currently ignored.
                 js_ast::op::Code::UnTypeof => {
-                    if matches!(ex.value.data, js_ast::ExprData::EIdentifier(_))
+                    if let js_ast::ExprData::EIdentifier(ident) = ex.value.data
                         && ex
                             .flags
                             .contains(E::UnaryFlags::WAS_ORIGINALLY_TYPEOF_IDENTIFIER)
                     {
-                        return true;
+                        return !ident.must_keep_due_to_with_stmt();
                     }
                     return self.expr_can_be_removed_if_unused_without_dce_check(&ex.value);
                 }
@@ -8047,7 +8052,17 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         self.relocated_top_level_vars
             .push(LocRef { loc, ref_: r#ref });
         self.record_usage(r#ref);
-        Expr::init_identifier(r#ref, loc)
+        self.var_store_target(loc, r#ref)
+    }
+
+    /// The target of the assignment that replaces a `var` declaration with an initializer.
+    pub(crate) fn var_store_target(&self, loc: bun_ast::Loc, r#ref: Ref) -> Expr {
+        let target = E::Identifier::init(r#ref)
+            .with_must_keep_due_to_with_stmt(self.var_store_asks_with_target(r#ref));
+        Expr {
+            loc,
+            data: js_ast::ExprData::EIdentifier(target),
+        }
     }
 
     // One statement could potentially expand to several statements

src/js_parser/scan/scan_side_effects.rs

@@ -206,11 +206,14 @@ impl SideEffects {
                         // "typeof x" must not be transformed into if "x" since doing so could
                         // cause an exception to be thrown. Instead we can just remove it since
                         // "typeof x" is special-cased in the standard to never throw.
-                        if matches!(un.value.data, ExprData::EIdentifier(_))
+                        if let ExprData::EIdentifier(ident) = un.value.data
                             && un
                                 .flags
                                 .contains(E::UnaryFlags::WAS_ORIGINALLY_TYPEOF_IDENTIFIER)
                         {
+                            if ident.must_keep_due_to_with_stmt() {
+                                return Some(expr);
+                            }
                             return None;
                         }
 

src/js_parser/scan/scan_symbols.rs

@@ -21,7 +21,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         // Every `break 'brk` below assigns `declare_loc` first; the one
         // early-`return` builds its own `FindSymbolResult` without reading it.
         let declare_loc: bun_ast::Loc;
-        let mut is_inside_with_scope = false;
+        // The scope that lists the symbol.
+        let mut found_in: Option<js_ast::StoreRef<Scope>> = None;
         let mut scope_use = true;
         // This function can show up in profiling.
         // That's part of why we do this.
@@ -36,11 +37,6 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             let mut did_forbid_arguments = false;
 
             while let Some(scope) = current {
-                // Track if we're inside a "with" statement body
-                if scope.kind == js_ast::scope::Kind::With {
-                    is_inside_with_scope = true;
-                }
-
                 // Forbid referencing "arguments" inside class bodies
                 if scope.forbid_arguments && !did_forbid_arguments && name == b"arguments" {
                     let r = js_lexer::range_of_identifier(self.source, loc);
@@ -55,6 +51,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 // Is the symbol a member of this scope?
                 if let Some(member) = scope.get_member_with_hash(name, hash) {
                     declare_loc = member.loc;
+                    found_in = current;
                     // Unbound globals live in the module scope.
                     scope_use = self.track_scope_uses
                         && (scope.parent.is_some()
@@ -74,6 +71,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                         if let Some(member) = exported.get(name) {
                             if member.data.is_enum() == ts.is_enum_scope {
                                 declare_loc = member.loc;
+                                found_in = current;
                                 // If this is an identifier from a sibling TypeScript namespace, then we're
                                 // going to have to generate a property access instead of a simple reference.
                                 // Lazily-generate an identifier that represents this property access.
@@ -108,7 +106,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                 return Ok(FindSymbolResult {
                     r#ref: Ref::NONE,
                     declare_loc: Some(loc),
-                    is_inside_with_scope,
+                    is_inside_with_scope: self.module_scope().contains_with
+                        && self.lookup_asks_with_target(None, Ref::NONE),
                 });
             }
 
@@ -145,6 +144,10 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             break 'brk new_ref;
         };
 
+        // Only a file that has a "with" statement takes the walk.
+        let is_inside_with_scope =
+            self.module_scope().contains_with && self.lookup_asks_with_target(found_in, ref_);
+
         // If we had to pass through a "with" statement body to get to the symbol
         // declaration, then this reference could potentially also refer to a
         // property on the target object of the "with" statement. We must not rename
@@ -164,4 +167,46 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             is_inside_with_scope,
         })
     }
+
+    /// True when a store to `ref_`, a `var` of the current scope, asks a "with" target.
+    pub(crate) fn var_store_asks_with_target(&self, ref_: Ref) -> bool {
+        self.module_scope().contains_with
+            && self.lookup_asks_with_target(Some(self.current_scope), ref_)
+    }
+
+    /// `found_in` lists `ref_`. A `var` belongs to its function scope, so its walk goes on to there.
+    #[cold]
+    #[inline(never)]
+    fn lookup_asks_with_target(
+        &self,
+        found_in: Option<js_ast::StoreRef<Scope>>,
+        ref_: Ref,
+    ) -> bool {
+        let mut current = Some(self.current_scope);
+        while let Some(scope) = current {
+            if scope.kind == js_ast::scope::Kind::With {
+                return true;
+            }
+            if current == found_in {
+                break;
+            }
+            current = scope.parent;
+        }
+        if current.is_none()
+            || self.symbols[ref_.inner_index() as usize].kind != js_ast::symbol::Kind::Hoisted
+        {
+            return false;
+        }
+
+        while let Some(scope) = current {
+            if scope.kind_stops_hoisting() {
+                return false;
+            }
+            if scope.kind == js_ast::scope::Kind::With {
+                return true;
+            }
+            current = scope.parent;
+        }
+        false
+    }
 }

src/js_parser/visit/visit_stmt.rs

@@ -2053,7 +2053,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
                             if let Some(val) = decl.value {
                                 let id_ref = b_id.r#ref;
                                 stmts.push(Stmt::assign(
-                                    Expr::init_identifier(id_ref, decl.binding.loc),
+                                    p.var_store_target(decl.binding.loc, id_ref),
                                     val,
                                 ));
                                 decl.value = None;

src/jsc/RuntimeTranspilerCache.rs

@@ -85,7 +85,8 @@ 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: Inside `with`, `typeof name` and the lookups of a body-declared `var` are kept.
+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

src/sema/import_sites.rs

@@ -16,12 +16,11 @@ mod written_arms;
 pub use call_sites::{
     RequireCall, RequireCallForm, does_import_meta_resolve_ask_resolver,
     ends_in_call_that_reads_frame_of_caller, find_module_printed_as_require, find_require_call,
-    is_linked_by_bundler, is_module_call_of_plugin, is_name_read_by_transpiler,
+    is_in_with_scope, is_linked_by_bundler, is_module_call_of_plugin, is_name_read_by_transpiler,
     is_name_written_out,
 };
 pub(crate) use call_sites::{
-    call_site_arguments, call_site_kind, is_free_common_js, is_in_with_scope, is_process,
-    may_construct_worker,
+    call_site_arguments, call_site_kind, is_free_common_js, is_process, may_construct_worker,
 };
 pub use changed_files::{
     TextsOfFile, find_texts_of_exports, write_from_other_folder, write_start_from_other_folder,

src/sema/import_sites/call_sites.rs

@@ -583,7 +583,8 @@ pub(crate) fn is_process(
             && is_free_name(hir, bound, follow_settled_bindings(hir, bound, object), global_objects))
 }
 
-pub(crate) fn is_in_with_scope(bound: &Bound<'_>, ident: ExprId) -> bool {
+/// The innermost scope that makes the lookup of the name `ident` dynamic is a `with` statement or lies in one.
+pub fn is_in_with_scope(bound: &Bound<'_>, ident: ExprId) -> bool {
     let mut scope = bound.dynamic_lookup_scope(NodeData::Expr(ident));
     while let Some(at) = scope {
         let data = &bound.scopes[at.idx()];

test/aot/parallel/language/store-to-a-var-of-a-with-block-after-a-call.cjs

@@ -0,0 +1,28 @@
+// Inside `with`, the declaration `var result = made` asks the object for `result` when it runs: behind the call that gave the object that property. So it stores in the object, as the assignment does, in a function and at the top of the file.
+function make(object) {
+  object.result = "property";
+  return "made";
+}
+function assign(object) {
+  with (object) {
+    var result;
+    const made = make(object);
+    result = made;
+  }
+  return result;
+}
+function declare(object) {
+  with (object) {
+    const made = make(object);
+    var result = made;
+  }
+  return result;
+}
+const objects = [{}, {}, {}];
+console.log(String(assign(objects[0])), objects[0].result);
+console.log(String(declare(objects[1])), objects[1].result);
+with (objects[2]) {
+  const made = make(objects[2]);
+  var result = made;
+}
+console.log(String(result), objects[2].result);

test/aot/parallel/tree-shaking/unused-typeof-of-a-global-beside-a-direct-eval.cjs

@@ -0,0 +1,19 @@
+// Beside a direct `eval`, the `typeof` of a name that nothing uses is left out, as without the `eval`: the getter of the global does not run. Only `with` keeps such a `typeof`. The marked call makes the compiler ask what is left of each expression that nothing uses.
+const seen = [];
+Object.defineProperty(globalThis, "watchedGlobal", {
+  get() {
+    seen.push("getter");
+    return 1;
+  },
+  configurable: true,
+});
+function pure() {}
+/* @__PURE__ */ pure();
+var yes = 1;
+function run() {
+  eval("0");
+  yes && typeof watchedGlobal;
+  yes ? typeof watchedGlobal : 0;
+}
+run();
+console.log("reads:", seen.join(" "));

test/aot/parallel/tree-shaking/unused-typeof-of-a-name-inside-a-with-block.cjs

@@ -0,0 +1,29 @@
+// Inside `with` the `typeof` of a name looks the name up in the object, also where nothing uses what it gives: behind `&&`, `||`, `??` and in an arm of `?:`. The marked call makes the compiler ask what is left of each expression that nothing uses.
+const seen = [];
+const scope = new Proxy(
+  {},
+  {
+    has(target, key) {
+      if (/^p[0-9]$/.test(key)) seen.push(key);
+      return false;
+    },
+  },
+);
+function pure() {}
+/* @__PURE__ */ pure();
+var yes = 1,
+  no = 0,
+  nothing = null;
+with (scope) {
+  yes && typeof p1;
+  no || typeof p2;
+  nothing ?? typeof p3;
+  yes ? typeof p4 : typeof p5;
+  {
+    var p6;
+    yes && typeof p6;
+  }
+  label: if (no) var p7;
+  else yes && typeof p7;
+}
+console.log(seen.join(" "));

test/bundler/bundler_minify.test.ts

@@ -88,6 +88,45 @@ describe("bundler", () => {
     minifyIdentifiers: false,
     run: { stdout: `[["arg,dep",1],["arg,dep",9]]` },
   });
+  // A name in a `with` body is looked up on the object first, which can run a
+  // proxy trap. An unused `typeof` of such a name stays, and so does an unused
+  // read of a `var` that the body declares.
+  const withLookupsSource = /* js */ `
+    const lookups = [];
+    const scope = new Proxy({}, {
+      has(target, key) {
+        if (/^[pv][0-9]+$/.test(key)) lookups.push(key);
+        return false;
+      },
+    });
+    function use() {}
+    with (scope) {
+      typeof p1;
+      typeof p2 === "undefined";
+      if (typeof p3) {}
+      use(!typeof p4, void typeof p5, (typeof p6, 0));
+      { var v1; v1; typeof v1; }
+      for (var v2; ; ) { v2; break; }
+      label: do var v3 = 1; while (v3, 0);
+    }
+    console.log(lookups.join(" "));
+  `;
+  itBundled("minify/WithStatementKeepsTypeofAndVarReads", {
+    files: {
+      "/entry.js": withLookupsSource,
+    },
+    format: "cjs",
+    run: { stdout: "p1 p2 p3 p4 p5 p6 v1 v1 v2 v3 v3" },
+  });
+  itBundled("minify/WithStatementKeepsTypeofAndVarReadsMinified", {
+    files: {
+      "/entry.js": withLookupsSource,
+    },
+    format: "cjs",
+    minifySyntax: true,
+    minifyIdentifiers: true,
+    run: { stdout: "p1 p2 p3 p4 p5 p6 v1 v1 v2 v3 v3" },
+  });
   // `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.