fix(hir): keep upgrade-callback wsId tagged ("ws","Client") across its own param binding

[machineloop]

Summary

When lowering an HTTP 'upgrade' handler — server.on('upgrade', (req, wsId, head) => …) — the upgrade pre-scan registers the wsId parameter as a ("ws", "Client") native instance, so wsId.send(...) / wsId.on(...) inside the handler dispatch through the Client-class rows of NATIVE_MODULE_TABLE.

But the arrow's own wsId parameter binding then calls shadow_native_instance_if_present, whose job is to tombstone a stale native tag leaked from an outer scope. Here the tag is the fresh, intended one the pre-scan just registered, so that binding wrongly erased it and the method calls degraded to generic dynamic dispatch (.send / .on no longer routed to the ws Client shims).

This protects a pre-scan-designated parameter from being tombstoned by its own binding, one-shot per name — so a later, unrelated parameter of the same name still shadows a genuinely stale tag.

Changes

• crates/perry-hir/src/lower/lowering_context.rs: add the prescan_protected_native_params set.
• crates/perry-hir/src/lower/context.rs: shadow_native_instance_if_present consumes-and-skips the tombstone for a protected name; add protect_native_param.
• crates/perry-hir/src/lower/expr_call/prescans.rs: mark the pre-scan-registered upgrade / native-instance params (res, wsId, sock) as protected.
• crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs: assert that wsId.send / wsId.on in the handler lower to ws NativeMethodCalls.

Related issue

Refs #577

Test plan

cargo test -p perry-hir --test ws_upgrade_param_native_dispatch
cargo build --release -p perry-hir

The handler arrow lowers to an inline Expr::Closure (closures here are not lifted into module.functions), and walk_expr_children skips closure bodies, so the test asserts over the module's Debug rendering — only NativeMethodCall emits a module: field. Verified it fails without the protection (the tag is tombstoned, the calls lower as generic Expr::Calls → 0 ws dispatches) and passes with it.

• cargo build --release clean (affected crate)
• cargo test --workspace … passes — the affected crate's targeted tests pass; the full workspace has pre-existing, unrelated failures on main not touched by this change.
• Added a #[test] in the affected crate
• (if CLI / stdlib / runtime API changed) Updated docs/src/ — n/a (HIR lowering only)
• (if touching a platform UI backend) — n/a

Checklist

• I have NOT bumped the workspace version or edited CLAUDE.md / CHANGELOG.md (maintainer handles these at merge)
• My commits follow the loose feat: / fix: / docs: / chore: prefix convention used in the log
• I've read CONTRIBUTING.md and agree to the Code of Conduct

Summary by CodeRabbit

• Bug Fixes

  • Improved pre-scan native parameter handling for callback lowering so protections are consumed only once and only when native-instance registration succeeds.
  • Prevents incorrect native-class dispatch for HTTP callbacks, WebSocket server upgrade handlers (wsId), and socket event handlers (sock), preserving expected behavior for methods like send() and on().

• Tests

  • Added a regression test covering upgrade handler lowering to confirm wsId retains Client-class native dispatch for both send() and on().

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a prescan_protected_native_params: HashMap<String, usize> field to LoweringContext with scope-depth anchoring to enable one-shot protection of pre-scanned callback parameters from tombstoning. register_native_instance now returns bool to signal success; protect_native_param inserts a name at current scope depth; shadow_native_instance_if_present consumes the entry exactly once and skips tombstoning when present; exit_scope clears unconsumed entries deeper than the current depth. Three prescan branches for res, wsId, and sock conditionally invoke protection on successful registration. A regression test verifies wsId retains ws::Client dispatch inside HTTP upgrade handlers.

Changes

Native-param protection against tombstoning

Layer / File(s)	Summary
HashMap field definition and initialization crates/perry-hir/src/lower/lowering_context.rs, crates/perry-hir/src/lower/context.rs	Adds prescan_protected_native_params: HashMap<String, usize> field to LoweringContext with documentation describing scope-depth anchoring and automatic cleanup semantics, and initializes it in with_class_id_start.
Registration return type, one-shot protection, and scope cleanup crates/perry-hir/src/lower/context.rs	register_native_instance returns bool (false for compile-package overrides, true on success), enabling conditional protection. shadow_native_instance_if_present performs a one-shot removal check and returns early when protected, skipping tombstoning. protect_native_param inserts name with current scope depth. exit_scope clears unconsumed protections deeper than the returned-to scope depth.
Prescan call sites: conditional protection for res, wsId, and sock crates/perry-hir/src/lower/expr_call/prescans.rs	Three existing prescan branches now clone the param name, register it as a native instance (res → ("http","IncomingMessage"), wsId → ("ws","Client"), sock → ("net","Socket")), and conditionally call ctx.protect_native_param only when registration returns true, preventing the binding from tombstoning the tag.
Regression test: wsId keeps ws::Client dispatch crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs	Adds a detailed regression comment, a lower test helper, and a test verifying that wsId.send/wsId.on inside an HTTP upgrade handler lower to NativeMethodCall with module: "ws" (at least two occurrences) after the fix.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~14 minutes

Possibly related PRs

• PerryTS/perry#5493: Both PRs change perry-hir's native-instance tagging for WebSocket wsId dispatch; this PR adds scope-depth-anchored one-shot protection during callback parameter lowering, while the related PR adds cross-function native-param hints to propagate the ("ws","Client") tag across helper call boundaries.

Poem

🐇 A name slipped away — tombstoned mid-scope,
The wsId fell dark, lost all ws hope.
One map with a depth-key, a one-shot shield,
protect_native_param sealed.
Now wsId.send sings in the night—
module: "ws" glows, the dispatch shines bright! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically identifies the main change: fixing the wsId parameter to retain its ws Client native dispatch across parameter binding in HTTP upgrade callbacks.
Description check	✅ Passed	The PR description is comprehensive and well-structured, covering summary, changes, related issue, test plan, and checklist as required by the template.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/perry-hir/src/lower/expr_call/prescans.rs`:
- Around line 82-83: The protect_native_param call is currently unconditional
after register_native_instance, but since register_native_instance can be a
no-op (via compile-package override path in context.rs), calling
protect_native_param without verifying successful registration can preserve
stale native tags and cause mis-dispatch in callback method calls. Guard each
protect_native_param invocation (for res_name and other parameters) behind a
check that its corresponding register_native_instance call actually succeeded.
Apply this fix to all three affected locations: the register_native_instance and
protect_native_param calls around line 82-83, the block around lines 92-96, and
the block around lines 106-107.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b06810ce-e237-4ac4-b800-5f2ff73d9d20

📥 Commits

Reviewing files that changed from the base of the PR and between 3c34fee and 4c0b2ca.

📒 Files selected for processing (4)

• crates/perry-hir/src/lower/context.rs
• crates/perry-hir/src/lower/expr_call/prescans.rs
• crates/perry-hir/src/lower/lowering_context.rs
• crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs

[proggeramlug]

Review — targeted and sensible; one latent fragility

The ordering is correct: the pre-scan runs while lowering the server.on('upgrade', …) call, before its closure-argument's param bindings, so protect → consume nests properly. The fix is well-scoped.

Latent fragility — name-keyed, no scope tie. prescan_protected_native_params is keyed by name only and is consumed by the next shadow_native_instance_if_present(name) for that name. If the expected param binding never fires that call (a pre-scan that returns Some(name) but the callback shape differs, or any binding path that doesn't route through shadow_native_instance_if_present), the protection lingers in the set and would wrongly skip the tombstone of a later, unrelated same-named binding. It's a narrow window, but it's the kind of state-leak that bites later. A scope-anchored entry (or asserting the protection is consumed before the lowering scope exits) would be more robust.

Test asserts over the Debug rendering (module: "ws" string match) — brittle, but acceptable given the closures here aren't walkable. Confirmed the fail-without/pass-with claim is plausible from the lowering path.

[machineloop]

Thanks @proggeramlug — addressed the latent fragility in 9e05943.

You're right that a name-only key could linger if the expected consumer never fires. prescan_protected_native_params is now a HashMap<String, usize> anchoring each protection to the scope_depth at which it was registered, and exit_scope drops any entries deeper than the surviving depth:

self.prescan_protected_native_params
    .retain(|_, depth| *depth <= self.scope_depth);

So a protection whose consumer never runs (an unusual callback shape, or a binding path that skips shadow_native_instance_if_present) is cleaned up when its enclosing scope exits, instead of leaking to a later unrelated same-named binding. The one-shot consume (HashMap::remove(..).is_some()) is otherwise unchanged, the happy-path test still passes, and the full perry-hir suite is green.

(Agreed the Debug-string assertion is brittle; it's the pragmatic option given the handler arrow lowers to an inline Expr::Closure that walk_expr_children skips and there's no public statement walker — noted in the test comment.)

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/perry-hir/src/lower/context.rs`:
- Around line 1271-1279: The protect_native_param method currently anchors the
protection to the current scope_depth, but this is the caller's scope, not the
callback scope where the parameter binding actually occurs. The prescan runs
before entering the callback scope, so storing the current scope_depth means the
entry survives exit_scope and can leak into unrelated bindings with the same
name. Change protect_native_param to store scope_depth + 1 instead (the
callback's scope depth), ensuring the prescan_protected_native_params entry is
cleaned up when exiting the actual callback parameter scope, not the caller's
scope. This prevents stale native tags from incorrectly skipping tombstoning for
later unrelated bindings.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: dfb28285-7ce3-42c3-b844-af304cf79b66

📥 Commits

Reviewing files that changed from the base of the PR and between cc8fb18 and 9e05943.

📒 Files selected for processing (4)

• crates/perry-hir/src/lower/context.rs
• crates/perry-hir/src/lower/expr_call/prescans.rs
• crates/perry-hir/src/lower/lowering_context.rs
• crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/perry-hir/tests/ws_upgrade_param_native_dispatch.rs
• crates/perry-hir/src/lower/expr_call/prescans.rs