From 47b5e7ceb0e54f409019474c4285ec89e688e3ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sun, 6 Sep 2026 09:21:19 +0200 Subject: [PATCH] fix(hir): scope a bare-assignment native-instance tag to the binding (#9847) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lower_assign` handles `X = .(...)`. Its class-name match ends in a catch-all, and it registered the result through `push_module_native_instance` — keyed on the identifier TEXT and scoped to the whole module, never truncated. So any method on any recognised native module, assigned to a variable, claimed that spelling for the rest of the program. Minified bundles reuse single letters everywhere, which makes the collision the normal case rather than a corner. `cli_2.1.112.js` (claude-code) compiles as one module, imports `child_process` as `fA1`, contains `let O; try { O = fA1.spawn(z.file, z.args, z.options) }` inside one helper, and binds the name `O` 5,381 times. Every later `O` was typed `child_process::Instance` — including the `for (let { segment: O } of ...)` binding in `string-width` that holds a grapheme STRING, whose `O.codePointAt(0)` lowered as `NativeMethodCall { module: "child_process", class_name: Some("Instance") }` and reached the right answer only because native-instance dispatch falls through to a generic path on a string receiver — once per grapheme, in the loop that dominates a claude-code turn. The tag now keys on the `LocalId` the assignment target resolves to. This is the same correction #7775 already made in this file for `new Proxy` bindings (`proxy_locals` -> `proxy_local_ids`) after a proxy bound to `a` in one function made every other function's `a.prop` lower to `js_proxy_get`. Scope-truncating the assignment path would NOT have worked: the module-wide table exists for the cross-function case — a module-level `let client;` assigned inside `init()` and read inside `handler()` — and truncating at scope exit would have dropped exactly that. Keyed on the binding it reaches just as far, because both functions resolve `client` to the same `LocalId`, while a same-named binding in another scope is simply a different binding. `lookup_native_instance` gains an id-keyed arm ahead of the module-wide one, short-circuited when the module has no bare-assignment handle at all (the arm sits on the miss path of every identifier property access). A target that resolves to no local — a bare global — still registers and resolves by name; that is the same hole #7775 documented, kept for the same reason and strictly no worse than the previous behaviour, which used it for every assignment. Nothing pattern-matches `child_process` or `codePointAt`: the mislowered call disappears because the tag never reaches that binding. --- .../native-instance-binding-scope-9847.md | 34 +++ crates/perry-hir/src/lower/context.rs | 52 +++++ crates/perry-hir/src/lower/expr_assign.rs | 47 +++- .../perry-hir/src/lower/lowering_context.rs | 22 ++ .../tests/native_instance_binding_scope.rs | 214 ++++++++++++++++++ 5 files changed, 364 insertions(+), 5 deletions(-) create mode 100644 changelog.d/native-instance-binding-scope-9847.md create mode 100644 crates/perry-hir/tests/native_instance_binding_scope.rs diff --git a/changelog.d/native-instance-binding-scope-9847.md b/changelog.d/native-instance-binding-scope-9847.md new file mode 100644 index 0000000000..8ff1dd9f42 --- /dev/null +++ b/changelog.d/native-instance-binding-scope-9847.md @@ -0,0 +1,34 @@ +**A native-instance tag created by a bare assignment now follows the binding it +assigns to, not the identifier's spelling** (#9847). One +`O = childProcess.spawn(...)` in a bundled helper used to type *every* binding +named `O` in the module as a `child_process` instance. + +`lower_assign` registered the tag through `push_module_native_instance`, whose +key is the identifier text and whose scope is the whole module, and its +class-name match ends in a catch-all — so any method on any native module, +assigned to a variable, claimed that name for the rest of the program. Minified +bundles reuse single letters everywhere, so the collision is the normal case, +not a corner: `cli_2.1.112.js` (claude-code) compiles as one module, contains +`let O; try { O = fA1.spawn(z.file, z.args, z.options) }`, and binds the name +`O` 5,381 more times. Among them is the `for (let { segment: O } of ... )` +binding in `string-width`, which holds a grapheme **string**; its +`O.codePointAt(0)` lowered as +`NativeMethodCall { module: "child_process", class_name: Some("Instance") }` +and reached the right answer only because native-instance dispatch falls +through to a generic path on a string receiver — once per grapheme, in the +loop that dominates a claude-code turn. + +The tag is now keyed on the `LocalId` the assignment target resolves to. That +is the same correction #7775 made for `new Proxy` bindings (`proxy_locals` → +`proxy_local_ids`) and it keeps the cross-function reach the module-wide table +existed for: a module-level `let client;` assigned inside `init()` and read +inside `handler()` resolves to the same binding in both. A target that resolves +to no local at all — a bare global — still falls back to the name-keyed table, +which is strictly no worse than the previous behaviour that used it for +everything. + +This is a mis-typing fix, not a wrong-answer fix: the mislowered calls already +degraded gracefully. But they degraded through a dispatch that had no business +seeing them, and a `child_process` method table that ever gained a +`codePointAt`, `length` or `test` entry would have captured string calls +silently. diff --git a/crates/perry-hir/src/lower/context.rs b/crates/perry-hir/src/lower/context.rs index 338b27193f..2647b616cc 100644 --- a/crates/perry-hir/src/lower/context.rs +++ b/crates/perry-hir/src/lower/context.rs @@ -150,6 +150,7 @@ impl LoweringContext { namespace_vars: Vec::new(), current_namespace: None, module_native_instances: Vec::new(), + local_id_native_instances: HashMap::new(), uses_fetch: false, uses_webassembly: false, react_default_import_local: None, @@ -1638,10 +1639,41 @@ impl LoweringContext { // zero-arg FFI calls. .filter(|(_, module, class)| !exposes_plain_object_fields(module, class)) .map(|(_, module, class)| (module.as_str(), class.as_str())) + .or_else(|| { + // #9847: a bare assignment (`O = cp.spawn(...)`) tags the + // RESOLVED binding, not the spelling. Consulted before the + // name-keyed module-wide table below, so a same-named binding + // in another function is simply a different binding and cannot + // inherit the tag. A different binding has a different + // `LocalId`, so no scope-exit truncation is needed here. + // + // This arm sits on the miss path of every identifier property + // access, so short-circuit the common module that has no + // bare-assignment native handle at all before touching the + // locals index. + if self.local_id_native_instances.is_empty() { + return None; + } + let id = self.lookup_local(name)?; + self.local_id_native_instances + .get(&id) + .filter(|(module, class)| !exposes_plain_object_fields(module, class)) + .map(|(module, class)| (module.as_str(), class.as_str())) + }) .or_else(|| { // Check module-level instances (survive scope exits). // Same last-match-wins rule for consistency — the index stores // the LAST pushed entry per name. + // + // #9847 KNOWN HOLE, stated plainly (the same one #7775 left for + // proxies): a receiver that resolves to NO local — a bare + // global, or a module-level binding referenced from a function + // body lowered before that binding was pre-registered — is + // still answered by spelling alone. That arm is kept because + // dropping it would regress the genuine cross-function handles + // that reach the lowering only through a name, and it is + // strictly no worse than the pre-#9847 behaviour, which used it + // for every assignment. self.module_native_instances_index .get(name) .map(|&idx| &self.module_native_instances[idx]) @@ -1684,6 +1716,26 @@ impl LoweringContext { .insert(entry.0.clone(), idx); self.module_native_instances.push(entry); } + + /// #9847: tag the RESOLVED binding `id` as holding a native instance. + /// + /// Used by the bare-assignment path (`O = cp.spawn(...)`) in place of + /// `push_module_native_instance`, whose name key was module-wide: in a + /// minified single-module bundle a single native handle poisoned every + /// homonym in the program. Keyed on the `LocalId` the target resolves to, + /// this keeps the cross-function reach the module-wide table was there to + /// provide (a module-level `let client;` assigned inside `init()` and read + /// inside `handler()` resolves to the SAME id in both) while making a + /// same-named binding in another scope a different binding. + pub(crate) fn register_local_id_native_instance( + &mut self, + id: LocalId, + module_name: String, + class_name: String, + ) { + self.local_id_native_instances + .insert(id, (module_name, class_name)); + } } // Internal anchor — keeps the file's outer impl block intact while diff --git a/crates/perry-hir/src/lower/expr_assign.rs b/crates/perry-hir/src/lower/expr_assign.rs index c07f56e2bb..123f562e05 100644 --- a/crates/perry-hir/src/lower/expr_assign.rs +++ b/crates/perry-hir/src/lower/expr_assign.rs @@ -195,11 +195,48 @@ pub(super) fn lower_assign(ctx: &mut LoweringContext, assign: &ast::AssignExpr) _ => Some("Instance"), }; if let Some(class_name) = class_name { - ctx.push_module_native_instance(( - var_name.clone(), - module_name.to_string(), - class_name.to_string(), - )); + // #9847: tag the BINDING this assigns + // to, not its spelling. The catch-all + // arm above makes any method on any + // native module tag the target, and + // `push_module_native_instance` keyed + // that on the identifier text for the + // whole module — so one + // `O = cp.spawn(...)` in a bundled + // helper typed all 5,381 bindings named + // `O` in claude-code's single-module + // bundle as `child_process::Instance`, + // including a `for (let {segment: O} of + // …)` binding holding a grapheme + // string. When the target resolves to a + // local we key on its `LocalId`, which + // keeps the cross-function reach the + // module-wide table existed for (a + // module-level `let client;` assigned + // inside one function and read inside + // another resolves to the same id in + // both) without the homonym collision. + // An unresolvable target — a bare + // global with no binding — has no id to + // key on and keeps the old name-keyed + // registration; see the matching arm in + // `lookup_native_instance`. + match ctx.lookup_local(&var_name) { + Some(local_id) => { + ctx.register_local_id_native_instance( + local_id, + module_name.to_string(), + class_name.to_string(), + ); + } + None => { + ctx.push_module_native_instance(( + var_name.clone(), + module_name.to_string(), + class_name.to_string(), + )); + } + } } } } diff --git a/crates/perry-hir/src/lower/lowering_context.rs b/crates/perry-hir/src/lower/lowering_context.rs index d96b0c504e..f69b838286 100644 --- a/crates/perry-hir/src/lower/lowering_context.rs +++ b/crates/perry-hir/src/lower/lowering_context.rs @@ -447,7 +447,29 @@ pub struct LoweringContext { pub(crate) current_namespace: Option, /// Module-level native instances that survive scope exits. /// Used for variables assigned from native calls inside functions (e.g., `mongoClient = await MongoClient.connect(uri)`). + /// + /// NAME-keyed and module-wide, so it is no longer authoritative for the + /// bare-assignment form: see `local_id_native_instances`, which supersedes + /// it whenever the assignment target resolves to a binding (#9847). This + /// stays the fallback for a target that resolves to no local at all. pub(crate) module_native_instances: Vec<(String, String, String)>, + /// #9847: the RESOLVED bindings that hold a native instance produced by a + /// bare assignment (`O = cp.spawn(...)`), so "this binding is a + /// `child_process` handle" stops meaning "something in this module spells + /// it this way". + /// + /// `module_native_instances` alone tagged the NAME for the whole module and + /// was never truncated, so one `O = cp.spawn(...)` in a bundled helper made + /// every other `O` in a 13 MB single-module bundle a + /// `child_process::Instance` — including a `for (let {segment: O} of …)` + /// binding holding a grapheme string, whose `O.codePointAt(0)` then lowered + /// as `NativeMethodCall{module:"child_process", class_name:Some("Instance")}` + /// and only worked because native-instance dispatch falls through to a + /// generic path on a string receiver. Same defect class as #7775's + /// `proxy_locals` → `proxy_local_ids`, and keyed the same way: a different + /// binding has a different `LocalId`, so a stale entry can never be reached + /// through a homonym, and no scope-exit truncation is needed. + pub(crate) local_id_native_instances: HashMap, /// Whether this module uses fetch() — requires perry-stdlib pub(crate) uses_fetch: bool, /// Issue #76 — set when any `WebAssembly.*` HIR variant is lowered. diff --git a/crates/perry-hir/tests/native_instance_binding_scope.rs b/crates/perry-hir/tests/native_instance_binding_scope.rs new file mode 100644 index 0000000000..9218938824 --- /dev/null +++ b/crates/perry-hir/tests/native_instance_binding_scope.rs @@ -0,0 +1,214 @@ +//! #9847 — a native-instance tag created by a bare assignment +//! (`O = cp.spawn(...)`) must follow the BINDING it assigns to, not the +//! identifier's spelling. +//! +//! Before the fix, `lower_assign` registered the tag through +//! `push_module_native_instance`, whose key is the identifier text and whose +//! scope is the whole module. `cli_2.1.112.js` (claude-code) compiles as ONE +//! module containing `let O; try { O = fA1.spawn(...) }`, and `O` is a binding +//! 5,381 times in that file — so every `O` in the 13 MB program was typed +//! `child_process::Instance`, including the `for (let {segment: O} of ...)` +//! binding that holds a grapheme STRING in the hottest loop of a turn. Its +//! `O.codePointAt(0)` lowered as +//! `NativeMethodCall{module:"child_process", class_name:Some("Instance")}` +//! and reached the right answer only because native-instance dispatch falls +//! through to a generic path on a string receiver — once per grapheme. +//! +//! Note the ORDER dependency these fixtures encode deliberately: the tag can +//! only reach a function lowered AFTER the poisoning assignment, so `spawner` +//! precedes `widthLike` in every fixture below. With the order reversed the +//! defect does not reproduce and the test could not fail. + +use perry_diagnostics::SourceCache; +use perry_hir::lower_module; +use perry_parser::parse_typescript_with_cache; + +fn lower(src: &str) -> perry_hir::Module { + let src = src.to_string(); + std::thread::Builder::new() + .stack_size(32 * 1024 * 1024) + .spawn(move || { + let mut cache = SourceCache::new(); + let parsed = + parse_typescript_with_cache(&src, "native_instance_binding_scope.ts", &mut cache) + .expect("parse should succeed"); + lower_module( + &parsed.module, + "test", + "native_instance_binding_scope.ts", + ) + .expect("lowering should succeed") + }) + .expect("spawn lower thread") + .join() + .expect("lower thread panicked") +} + +/// Debug-format just one function's body, so an assertion about `widthLike` +/// cannot be satisfied (or broken) by a node belonging to `spawner`. +fn body_of(module: &perry_hir::Module, name: &str) -> String { + let func = module + .functions + .iter() + .find(|f| f.name == name) + .unwrap_or_else(|| { + panic!( + "function `{name}` not found; module has {:?}", + module + .functions + .iter() + .map(|f| f.name.as_str()) + .collect::>() + ) + }); + format!("{:#?}", func.body) +} + +/// The whole defect and both halves of the contract in one module: a +/// module-level handle read from a second function, a function-local handle, +/// and an unrelated binding that merely shares the local handle's spelling. +/// +/// DO NOT REORDER `spawner` AND `widthLike` "for readability". The tag can only +/// reach a function lowered AFTER the assignment that creates it, so with +/// `widthLike` first the defect does not reproduce and +/// `a_same_named_binding_in_another_function_does_not_inherit_the_tag` passes +/// on the unfixed compiler — a test that cannot fail. This order was checked +/// against a pre-fix binary: `widthLike` lowered +/// `NativeMethodCall{module:"child_process", method:"codePointAt"}` there, and +/// the reversed order lowered the correct `PropertyGet`. +const FIXTURE: &str = r#" +import * as cp from "child_process"; + +let client: any; + +export function init(): void { + client = cp.spawn("true", []); +} + +export function handler(): void { + client.kill(); +} + +export function spawner(): any { + let O: any; + O = cp.spawn("false", []); + O.kill(); + return O; +} + +export function widthLike(q: any): number { + let Y = 0; + for (let { segment: O } of q) { + Y += O.codePointAt(0) >= 4352 ? 2 : 1; + } + return Y; +} +"#; + +#[test] +fn a_same_named_binding_in_another_function_does_not_inherit_the_tag() { + let module = lower(FIXTURE); + let width_like = body_of(&module, "widthLike"); + + assert!( + !width_like.contains("method: \"codePointAt\""), + "the for-of `segment` binding holds a string and must not lower its \ + `.codePointAt` through child_process native-instance dispatch just \ + because an unrelated function spells its spawn handle `O` too: \ + {width_like}" + ); + assert!( + width_like.contains("property: \"codePointAt\""), + "`O.codePointAt(0)` should lower as an ordinary property call: \ + {width_like}" + ); +} + +#[test] +fn a_function_local_native_handle_still_dispatches_natively() { + let module = lower(FIXTURE); + let spawner = body_of(&module, "spawner"); + + assert!( + spawner.contains("method: \"kill\""), + "the binding actually assigned from `cp.spawn(...)` must keep native \ + dispatch — scoping the tag to the binding must not lose the real \ + case: {spawner}" + ); + assert!( + spawner.contains("module: \"child_process\""), + "and it must still be tagged as child_process: {spawner}" + ); +} + +#[test] +fn a_module_level_handle_assigned_in_one_function_is_seen_in_another() { + let module = lower(FIXTURE); + let handler = body_of(&module, "handler"); + + // This is what the module-wide table existed for: `client` is bound at + // module level, assigned inside `init`, and read inside `handler`. Keyed + // on the resolved binding it reaches just as far, because both functions + // resolve `client` to the same LocalId. + assert!( + handler.contains("method: \"kill\"") && handler.contains("module: \"child_process\""), + "a module-level handle assigned in another function must still \ + dispatch natively: {handler}" + ); +} + +/// The issue's one-identifier A/B, as a test: two sources differing only in +/// whether the spawner's variable is spelled `O`. After the fix the two must +/// lower `widthLike` identically. +#[test] +fn renaming_the_spawner_variable_no_longer_changes_an_unrelated_function() { + const ARM: &str = r#" +import * as cp from "child_process"; + +export function unrelatedSpawner(): any { + let NAME: any; + try { NAME = cp.spawn("true", []); } catch (M) { NAME = null; } + return NAME; +} + +export function widthLike(q: any): number { + let Y = 0; + for (let { segment: O } of q) { + Y += O.codePointAt(0) >= 4352 ? 2 : 1; + } + return Y; +} +"#; + + // The rename is 3 characters longer, so every `byte_offset` in the file + // shifts. Those are source positions, not lowering decisions — normalise + // them, and nothing else, so the comparison is about the shape of the + // lowered code. (Before the fix this comparison failed on the node itself: + // `NativeMethodCall{module:"child_process", ...}` vs `Call{PropertyGet}`.) + let normalise = |body: String| { + body.split("byte_offset: ") + .enumerate() + .map(|(i, part)| { + if i == 0 { + return part.to_string(); + } + let rest = part.trim_start_matches(|c: char| c.is_ascii_digit()); + format!("byte_offset: N{rest}") + }) + .collect::() + }; + + let arm_a = normalise(body_of(&lower(&ARM.replace("NAME", "notO")), "widthLike")); + let arm_b = normalise(body_of(&lower(&ARM.replace("NAME", "O")), "widthLike")); + + assert_eq!( + arm_a, arm_b, + "renaming a spawn handle in an unrelated function must not change how \ + `widthLike` lowers" + ); + assert!( + !arm_b.contains("method: \"codePointAt\""), + "and neither arm may route the string read through native dispatch: \ + {arm_b}" + ); +}