From 5160cdf91b558381da0fbd2335358ca46fa1cea8 Mon Sep 17 00:00:00 2001 From: Andrew Brassington <3682072+machineloop@users.noreply.github.com> Date: Sun, 21 Jun 2026 20:03:18 -0400 Subject: [PATCH] fix(hir): keep upgrade-callback wsId tagged ("ws","Client") across its own param binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `server.on('upgrade', (req, wsId, head) => …)` pre-scans and registers the second param `wsId` as a ("ws","Client") native instance (prescans.rs) so wsId.send(...) /.on(...)/.close() inside the handler dispatch through the Client-class NATIVE_MODULE_TABLE rows (→ js_ws_send_client_i64 etc.). But the arrow's OWN param binding then calls shadow_native_instance_if_present("wsId"), which tombstones any native tag colliding with a param name (the leaked-instance guard for minified bundles). That erased the FRESH, intended tag, so wsId.send no longer dispatched to the native fn — wsId.send/.on silently no-opped: the handler calls wsId.send but the frame never goes out, and inbound frames never reach wsId.on (a dead post-upgrade WebSocket channel). Fix: each pre-scan marks the param name it just tagged as protected (ctx.protect_native_param); shadow_native_instance_if_present consumes that protection one-shot to skip the tombstone for exactly that param. Covers the upgrade `wsId`, the http-client-callback `res`, and the request 'socket' `sock`. Adds a perry-hir test asserting wsId.send/.on lower to ws Client NativeMethodCalls. Refs #577 --- crates/perry-hir/src/lower/context.rs | 43 ++++++++++- .../perry-hir/src/lower/expr_call/prescans.rs | 29 +++++++- .../perry-hir/src/lower/lowering_context.rs | 18 +++++ .../tests/ws_upgrade_param_native_dispatch.rs | 71 +++++++++++++++++++ 4 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs diff --git a/crates/perry-hir/src/lower/context.rs b/crates/perry-hir/src/lower/context.rs index 9a25fb771e..449b13dc70 100644 --- a/crates/perry-hir/src/lower/context.rs +++ b/crates/perry-hir/src/lower/context.rs @@ -123,6 +123,7 @@ impl LoweringContext { native_instances_index: HashMap::new(), module_native_instances_index: HashMap::new(), func_return_native_instances_index: HashMap::new(), + prescan_protected_native_params: std::collections::HashMap::new(), native_modules_index: HashMap::new(), module_shadow_stack: Vec::new(), class_statics_index: HashMap::new(), @@ -1194,12 +1195,16 @@ impl LoweringContext { .map(|(root, sub)| (root.as_str(), sub.as_str())) } + /// Register `local_name` as a native instance of `module_name`/`class_name`. + /// Returns `false` (a no-op) when the package is a `perry.compilePackages` + /// override (#5137) — callers that gate a side effect on successful + /// registration (e.g. `protect_native_param`) must check this. pub(crate) fn register_native_instance( &mut self, local_name: String, module_name: String, class_name: String, - ) { + ) -> bool { // #5137: if the user opted this package into `perry.compilePackages`, // its real npm source is being compiled and the binding resolves to // the compiled-from-source class. Registering a native instance here @@ -1211,7 +1216,7 @@ impl LoweringContext { // is used. `is_native_module` already makes the same back-off for the // import-resolution side (#665). if is_compile_package_override(&module_name) { - return; + return false; } // Push the new index onto this name's shadow stack (innermost last). let idx = self.native_instances.len(); @@ -1221,6 +1226,7 @@ impl LoweringContext { .push(idx); self.native_instances .push((local_name, module_name, class_name)); + true } /// Shadow any prior native-instance tag for `local_name` by pushing a @@ -1248,11 +1254,38 @@ impl LoweringContext { /// saw `e.map`/`e.length`/`e.constructor` all read 0 while `e[0]` and /// `Array.prototype.map.call(e)` worked → `(number).join is not a function`). pub(crate) fn shadow_native_instance_if_present(&mut self, name: &str) { + // A pre-scan registered this exact param name as a native instance for + // the callback now being lowered (e.g. `wsId` for `server.on('upgrade', + // (req, wsId, head) => …)`). That tag is the FRESH, intended one — the + // param IS the native instance — so its own param binding must NOT + // tombstone it. One-shot: consume the protection so a later, unrelated + // param of the same name still shadows a genuinely stale tag. + // Consume only when this binding is at the depth the pre-scan anchored + // the protection to (the callback's own param scope). A same-named + // binding at any other depth must NOT consume it; `exit_scope` drops a + // never-consumed entry when the callback scope unwinds. + if self.prescan_protected_native_params.get(name).copied() == Some(self.scope_depth) { + self.prescan_protected_native_params.remove(name); + return; + } if self.lookup_native_instance(name).is_some() { self.shadow_native_instance(name.to_string()); } } + /// Mark `name` as a pre-scan-designated native-instance param so the very + /// next `shadow_native_instance_if_present(name)` (the callback's own param + /// binding) skips tombstoning it. The pre-scan runs in the CALLER's scope, + /// one level above the callback, so anchor to `scope_depth + 1` (the + /// callback's param scope): the consume matches at exactly that depth, and a + /// never-consumed entry is dropped when the callback scope exits — it can't + /// linger into the caller scope and shadow a later, unrelated same-named + /// binding. See `prescan_protected_native_params`. + pub(crate) fn protect_native_param(&mut self, name: String) { + self.prescan_protected_native_params + .insert(name, self.scope_depth + 1); + } + /// Truncate `native_instances` back to `mark`, keeping the /// `native_instances_index` shadow stacks in sync: every recorded index /// `>= mark` is popped (these belong to bindings whose scope is exiting), @@ -1470,6 +1503,12 @@ impl LoweringContext { pub(crate) fn exit_scope(&mut self, mark: (usize, usize, usize)) { debug_assert!(self.scope_depth > 0, "exit_scope called at module depth"); self.scope_depth = self.scope_depth.saturating_sub(1); + // Drop any pre-scan param protections registered in a scope deeper than + // the one we just returned to — their expected consumer (the callback's + // param binding) never fired, so they must not linger and skip a later + // same-named binding's tombstone. + self.prescan_protected_native_params + .retain(|_, depth| *depth <= self.scope_depth); self.scope_local_marks.pop(); // #wall5: restore native-module shadowing for this scope. if let Some(m) = self.scope_module_shadow_marks.pop() { diff --git a/crates/perry-hir/src/lower/expr_call/prescans.rs b/crates/perry-hir/src/lower/expr_call/prescans.rs index 4e07c9f2ec..7c87723506 100644 --- a/crates/perry-hir/src/lower/expr_call/prescans.rs +++ b/crates/perry-hir/src/lower/expr_call/prescans.rs @@ -79,7 +79,16 @@ pub(super) fn run_call_prescans( // perry-ext-http's `js_http_on` for client-side IncomingMessage // handles too via the cross-module on-listener path). if let Some(res_name) = pre_scan_node_http_client_callback_params(ctx, call) { - ctx.register_native_instance(res_name, "http".to_string(), "IncomingMessage".to_string()); + // Protect only when registration actually happened — it no-ops under a + // `perry.compilePackages` override, and protecting without a fresh tag + // would let a later same-named param skip tombstoning a stale one. + if ctx.register_native_instance( + res_name.clone(), + "http".to_string(), + "IncomingMessage".to_string(), + ) { + ctx.protect_native_param(res_name); + } } // Issue #577 Phase 4 — `httpServer.on('upgrade', (req, wsId, head) => …)` @@ -88,7 +97,16 @@ pub(super) fn run_call_prescans( // `wsId.close()` inside the handler dispatch via the Client-class // entries in NATIVE_MODULE_TABLE. if let Some(ws_id_name) = pre_scan_node_http_upgrade_params(ctx, call) { - ctx.register_native_instance(ws_id_name, "ws".to_string(), "Client".to_string()); + // The arrow's own `wsId` param binding would otherwise tombstone this + // fresh tag via shadow_native_instance_if_present — protect it so + // `wsId.send`/`.on` inside the handler keep the Client-class dispatch. + // Protect only when registration actually happened (it no-ops under a + // compile-package override), else a later same-named param would skip + // tombstoning a genuinely stale tag. + if ctx.register_native_instance(ws_id_name.clone(), "ws".to_string(), "Client".to_string()) + { + ctx.protect_native_param(ws_id_name); + } } // Issue #2211 — `request.on('socket', sock => …)` on a `ClientRequest` @@ -98,7 +116,12 @@ pub(super) fn run_call_prescans( // handler dispatches through the class-filtered Socket rows in // NATIVE_MODULE_TABLE. if let Some(sock_name) = pre_scan_node_http_client_request_socket_params(ctx, call) { - ctx.register_native_instance(sock_name, "net".to_string(), "Socket".to_string()); + // Protect only on successful registration (no-ops under a + // compile-package override) — see the `wsId` site above. + if ctx.register_native_instance(sock_name.clone(), "net".to_string(), "Socket".to_string()) + { + ctx.protect_native_param(sock_name); + } } // perry/ui reactive Text: `Text(\`...${state.value}...\`)` where at least one diff --git a/crates/perry-hir/src/lower/lowering_context.rs b/crates/perry-hir/src/lower/lowering_context.rs index be1846a4b1..6d09d2df07 100644 --- a/crates/perry-hir/src/lower/lowering_context.rs +++ b/crates/perry-hir/src/lower/lowering_context.rs @@ -421,6 +421,24 @@ pub struct LoweringContext { /// truncated). The old lookup scanned FORWARD (first-match-wins), so the /// index keeps the FIRST pushed index per name (`entry().or_insert`). pub(crate) func_return_native_instances_index: HashMap, + /// Param names a pre-scan registered as native instances for the callback + /// it is ABOUT to lower (e.g. the `wsId` of `server.on('upgrade', (req, + /// wsId, head) => …)` tagged `("ws","Client")`). The callback's own param + /// binding then calls `shadow_native_instance_if_present(param)` to tombstone + /// stale leaked native tags — but here the tag is the FRESH, intended one, + /// so that shadow would wrongly erase it (the `wsId.send`/`.on` dead-channel + /// bug). One-shot per name: `shadow_native_instance_if_present` consumes the + /// entry and skips the tombstone exactly once. + /// + /// Each entry is anchored to the CALLBACK's param `scope_depth` (one below + /// the caller scope the pre-scan runs in); the consume matches at exactly + /// that depth and `exit_scope` drops entries deeper than the surviving depth. + /// So a protection whose expected consumer never fires (an unusual callback + /// shape, or a binding path that doesn't route through + /// `shadow_native_instance_if_present`) is cleared when the callback scope + /// exits and cannot leak into the caller scope to wrongly skip a later, + /// unrelated same-named binding's tombstone. + pub(crate) prescan_protected_native_params: std::collections::HashMap, /// Perf index for `native_modules` (push-only, never truncated). The old /// `lookup_native_module` scanned FORWARD (first-match-wins), so the index /// keeps the FIRST pushed index per name (`entry().or_insert`). diff --git a/crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs b/crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs new file mode 100644 index 0000000000..efbfbc7b2b --- /dev/null +++ b/crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs @@ -0,0 +1,71 @@ +// Regression: the `wsId` parameter of an HTTP `'upgrade'` handler +// (`server.on('upgrade', (req, wsId, head) => …)`) must keep the +// `("ws", "Client")` native-instance tag the upgrade pre-scan assigns it, so +// `wsId.send(...)` / `wsId.on(...)` inside the handler lower to Client-class +// `Expr::NativeMethodCall { module: "ws", … }`. +// +// The arrow's own `wsId` param binding calls `shadow_native_instance_if_present` +// to tombstone any STALE native tag leaked from an outer scope — but here the +// tag is the FRESH, intended one the pre-scan just registered. Without the +// one-shot protection (`protect_native_param` / `prescan_protected_native_params`) +// that binding wrongly erased it, and the method calls fell back to generic +// dynamic dispatch (the post-upgrade "dead channel": `.send` / `.on` no longer +// routed to the ws Client shims). +// +// The handler arrow lowers to an inline `Expr::Closure { body, .. }` nested as +// the second argument of the `http.on` call (closures here are NOT lifted into +// `module.functions`). perry-hir's public walker (`walk_expr_children`) skips +// closure bodies and there is no public statement walker, so we assert over the +// module's `Debug` rendering instead: only `Expr::NativeMethodCall` carries a +// `module: ""` field, so each `module: "ws"` occurrence is a surviving +// Client-class dispatch. Without the fix the calls degrade to `Expr::Call` and +// no `module: "ws"` appears. + +use perry_diagnostics::SourceCache; +use perry_hir::{clear_current_module_source, fix_local_native_instances, lower_module}; +use perry_parser::parse_typescript_with_cache; + +fn lower(src: &str) -> perry_hir::Module { + let mut cache = SourceCache::new(); + let parsed = + parse_typescript_with_cache(src, "/tmp/ws_upgrade_param.ts", &mut cache).expect("parse"); + let mut module = + lower_module(&parsed.module, "test", "/tmp/ws_upgrade_param.ts").expect("lower"); + clear_current_module_source(); + fix_local_native_instances(&mut module); + module +} + +#[test] +fn upgrade_handler_wsid_keeps_client_class_dispatch_across_its_own_binding() { + let module = lower( + r#" + import { createServer } from "node:http"; + + const server = createServer((req: any, res: any) => { + res.end("ok"); + }); + + server.on("upgrade", (req: any, wsId: any, _head: any) => { + wsId.send("perry-hello"); + wsId.on("message", (_msg: any) => {}); + }); + "#, + ); + + let dump = format!("{module:#?}"); + let ws_dispatches = dump.matches("module: \"ws\"").count(); + + // Both `wsId.send` and `wsId.on` must survive as ws Client NativeMethodCalls. + assert!( + ws_dispatches >= 2, + "expected wsId.send + wsId.on to lower as ws Client-class NativeMethodCalls, \ + but found {ws_dispatches} `module: \"ws\"` dispatch(es). Lowered HIR:\n{dump}" + ); + // `method: "send"` is unique to the ws Client call (the only other native + // call here is `http.on`), so its presence pins the discriminating case. + assert!( + dump.contains("method: \"send\""), + "wsId.send must dispatch as a ws NativeMethodCall. Lowered HIR:\n{dump}" + ); +}