fix(codegen): root a call receiver and a computed-read base across their sibling operands

[jdalton]

Two ways a compiled program could crash, or quietly read the wrong value, when the garbage collector moved an object in the middle of a single expression. A method call like recv.m(f()) and a computed read like o[f()] both evaluate the object first and the second operand after — that is the order the spec requires — so the object sat in a plain CPU register while f() ran. If f() allocated enough to trigger a moving collection, the object was relocated and the register was left pointing at the old address. The call then threw TypeError: value is not a function, or the read segfaulted.

Both sites are fixed, both have a regression test that is red on the parent commit and green here, and both were found by a static check over the emitted LLVM IR rather than by a runtime probe — which matters, because at the moment of the collection there is nothing wrong for a runtime probe to see.

This only bites under PERRY_GC_MOVING_LOOP_POLLS=1, which has been off by default since #7161.

The invariant — rooting the place a value came from does not save the register you copied it into

#7192 fixed values whose root store was emitted late. These two are different: they are never rooted at all. A reference is read out of a root, kept in a bare SSA register across a collection point, and used below it.

The reference survives the collection — the closure capture cell, shadow slot or module global holding it is a root — so it moves. The collector rewrites that location and the register keeps naming from-space. This is exactly the "property (2) — a rewritten location — is worthless without property (3), reading that location again below the collection point" that expr/temp_root.rs's module header describes, and the same fix #7192 applied to the property/element STORE receiver.

Site one: the method-call receiver — a stale receiver makes method lookup resolve against abandoned memory, so the call throws TypeError: value is not a function

In lower_call/console_promise.rs, the js_native_call_method_by_id dispatch. In the sfw-registry corpus this is zod's classic/schemas.ts:301:

inst.regex = (...args) => inst.check(checks.regex(...args));

inst is read out of the arrow's capture cell, held across checks.regex(...args) — a real user call, so it polls — then used as .check's receiver. The IR before this change:

%r12 = call i64 @js_closure_get_capture_bits(i64 %r11, i32 0)   ; inst
%r13 = bitcast i64 %r12 to double                                ; a register, not a root
%r19 = call double @js_closure_call_apply_with_spread(...)       ; runs user code -> polls -> minor
%r24 = call double @js_typed_feedback_native_call_method_by_id(..., double %r13, ...)  ; STALE

The arguments are rooted too — each one before the next is lowered, per RootedOperands' incremental contract — so an earlier argument cannot go stale across a later one either.

Site two: the computed-read base — a stale base walks the keys array of freed memory, segfaulting inside get_field_by_name_object_tail

In expr/index_get.rs, the dynamic-string-key arm and the last-resort runtime-tag-check arm. These are the READ counterpart of #7192's index_set / property_set guard, which only covered the store side.

In sfw-registry this is zod's core/checks.ts:68, numericOriginMap[typeof def.value] — a module-global base with a key expression that reads a property and therefore can collect. The failure is either a SIGSEGV or, worse, a silently wrong value.

Why a temp root rather than re-lowering — re-lowering would observe an assignment the sibling operand made, which is a miscompile rather than a rooting fix

Both sites use temp_root::guard_store_operand / reread_store_operand / release_store_operand (#7198's generalized naming).

The guard emits nothing when the sibling expression cannot collect, so an inert argument list or key keeps its previous IR byte for byte. It is released after the dispatch, because the dispatcher allocates while reading these values.

The instrument: --stale-registers — a ranked lead list, not a gate, so it exits 0 unless you hand it a budget

The shipped check anchors on a shadow-slot bind, so it can only see values that are eventually rooted. Neither site above is, which is why #7192's checker reported the corpus at 1 violation while both of these were live.

--stale-registers checks the more general invariant the module header already states. It classifies every heap-value source — an allocation, or a read of a collector-rewritten location (a shadow-slot load, a closure capture cell, a temp-root slot, a module global, a mutable-capture box) — follows it forward through bit-level identity ops, and reports the first real use sitting below a collecting call. --fatal-sinks narrows to uses that dereference the value, where a relocation is fatal rather than merely wrong. It found both sites independently of any runtime probe, and it agreed with lldb on the exact instruction in each case.

It is not calibrated to zero: the raw count is dominated by values it cannot prove are pointers, and even the fatal-sink slice still carries the known-unfixed js_closure_call* class. A mode that returned non-zero on any hit would be a check that can never pass — the mirror image of CLAUDE.md's four "a gate that cannot fail" hazards, and just as reliably ignored. So the exit status is explicit:

Invocation	Exit	Meaning
--stale-registers	0	Diagnostic. Prints the counts and the ranked leads.
--stale-registers --max-stale N	1 above N	Opt-in ratchet, for a slice that has been calibrated.
--max-stale or --fatal-sinks without --stale-registers	2	Usage error. A silently ignored budget, or a filter that never reached the report, is a disarmed knob.
--stale-registers over an empty corpus	2	--min-files is checked first; nothing was scanned.
The bind-anchored gate gc-root-dominance.yml runs	1 on any violation	Unchanged by this PR.

--self-test asserts the top three rows against the planted and control fixtures, so the contract is checked on every PR rather than described in a comment. Reverting the default to return 1 if total else 0 fails three of those arms.

Validation — both gap tests flip red to green and stay green under a non-moving collector, so the failure tracks collector mode rather than luck

Ran.

	parent	this PR
test_gap_gc_method_receiver_rooting.ts, POLLS=1	TypeError: value is not a function 5/5	bad 0 10/10
test_gap_gc_index_get_receiver_rooting.ts, POLLS=1	TypeError: Cannot read properties of undefined 4/4	bad 0 10/10
both, POLLS=1 + PERRY_GEN_GC=0	bad 0	bad 0
both, default (no polls)	bad 0	bad 0 5/5
static checker, fatal-sink slice, 141-module sfw-registry corpus	986	729
cargo test -p perry-codegen	6 loop_safepoint_purity failures	identical to main
python3 scripts/gc_root_dominance_check.py --self-test	—	OK, including the four new exit-status arms

The entire js_typed_feedback_native_call_method_by_id fatal-sink class (257) is gone.

Did not run. cargo test -p perry-runtime — this PR touches only perry-codegen, which perry-runtime does not depend on. The full parity sweep is likewise not re-run; the two new tests are gap tests and are covered by the gap gate.

Trade-off. The temp-root guard adds a root/re-read/release triple on the receiver and base paths whenever the sibling operand can collect. That is the cost of correctness under a moving collector, and it is skipped entirely when the sibling cannot collect, so inert argument lists and constant keys emit exactly the IR they did before.

What this does not close — sfw-registry --help under moving polls is still red at 5/10, so #7161's stopgap stays

Measured at the merge base plus this PR, compiled and run with PERRY_GC_MOVING_LOOP_POLLS=1: red 5/10 (SIGSEGV). Its default arm is clean 10/10. Before these two fixes the same build was red 8/10, and before them the failure was the deterministic TypeError at zod schemas.ts:301 — so this is progress along the same chain, not a resolution.

The remaining 593 fatal-sink hits are all js_closure_callN: the generic dynamic-value-call lowering in console_promise.rs, which holds the callee and the this receiver and each argument in registers across the argument list. Three stale registers in the single hottest call path. That is the next site of this class and it is deliberately not fixed here — it is the central dispatch path and deserves its own change with its own repro.

Refs #7154, #7192, #7198, #7184, #7161, #7114, #6951.

Summary by CodeRabbit

• Bug Fixes

  • Fixed rare errors when garbage collection relocates objects during computed property reads or method calls.
  • Improved reliability when evaluating allocating expressions before accessing object receivers or arguments.

• Tests

  • Added regression coverage for moving-garbage-collection scenarios involving computed properties and dynamic method calls.

• Documentation

  • Documented the garbage-collection safety fixes and updated the release version to 0.5.1279.

[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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6d738b12-dcba-4eb5-9896-3333e5a7f774

📥 Commits

Reviewing files that changed from the base of the PR and between 1f9d710 and 08a665d.

📒 Files selected for processing (1)

• test-parity/gc_repsel_corpus.txt

---

📝 Walkthrough

Walkthrough

The change roots receivers and arguments across potentially collecting expressions and dispatch. It adds moving-GC regression tests and extends gc_root_dominance_check.py with stale-register analysis, filtering, budgets, CLI validation, and self-tests.

Changes

Stale GC receiver handling

Layer / File(s)	Summary
Receiver rooting and regression coverage crates/perry-codegen/src/expr/index_get.rs, crates/perry-codegen/src/lower_call/console_promise.rs, test-files/*gc*receiver*, test-parity/gc_repsel_corpus.txt, changelog.d/7206-stale-receiver-registers.md	Computed reads and native method calls root receivers and arguments, reread relocated values, and release roots after dispatch. Moving-GC tests and parity entries cover both paths.
Stale-register source and use analysis scripts/gc_root_dominance_check.py	The checker models heap-value sources and aliases, detects uses after collection windows, reports stale values, and filters fatal sinks.
Diagnostic CLI and self-tests scripts/gc_root_dominance_check.py, changelog.d/7206-stale-receiver-registers.md	The checker adds stale-analysis options and --max-stale budget handling. Self-tests validate reporting, budgets, invalid combinations, and clean fixtures.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

• GC: the remaining unrooted-alloca hazards after #7207 — class-keys pointer caches, interleaved staging arrays, inlined-callee param slots #7210: Addresses related stale receiver and argument handling across allocating lowering paths.
• GC: heap values in plain alloca_entry slots are neither marked nor rewritten (inline-ctor this_slot, closure-capture staging array) #7202: Covers stale heap values across GC and relates to the new stale-use analysis.

Possibly related PRs

• PerryTS/perry#6941: Adds related GC-rooting fixes in code generation.
• PerryTS/perry#7192: Provides related temporary-rooting and dominance-checker work.
• PerryTS/perry#7196: Adds related diagnostics for stale moving-GC pointers.

Suggested reviewers: proggeramlug

Sequence Diagram(s)

sequenceDiagram
  participant Codegen
  participant GC
  participant RuntimeDispatch
  participant RegressionTest
  Codegen->>Codegen: Root receiver and arguments
  Codegen->>GC: Evaluate allocating key or argument
  GC-->>Codegen: Relocate rooted values
  Codegen->>Codegen: Reread rooted values
  Codegen->>RuntimeDispatch: Dispatch with refreshed values
  RuntimeDispatch-->>RegressionTest: Return computed result
  RegressionTest->>RegressionTest: Check repeated moving-GC results

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the primary fix: rooting call receivers and computed-read bases across sibling operand evaluation.
Description check	✅ Passed	The description thoroughly covers the problem, implementation, tests, validation results, related issues, and remaining limitations, despite omitting the template checklist.
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.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

test-files/test_gap_gc_method_receiver_rooting.ts (1)

31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Both gap tests can lose their collection window to constant folding. In each test the allocation-heavy helper returns a value that does not depend on the objects it allocates, so a later optimizer can delete the loop and the allocations. The test would then pass without reaching an evacuating minor, and the regression would go undetected.

• test-files/test_gap_gc_method_receiver_rooting.ts#L31-L37: derive churn's return value from the allocated bits elements and from x, instead of the constant bits.length - 600.
• test-files/test_gap_gc_index_get_receiver_rooting.ts#L28-L34: make the pushed objects depend on v and derive the "hit" decision from a read of an allocated element, instead of the constant bits.length === 4000.

🤖 Prompt for 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.

In `@test-files/test_gap_gc_method_receiver_rooting.ts` around lines 31 - 37, The
allocation loops in both gap-test helpers must produce observable values. In
test-files/test_gap_gc_method_receiver_rooting.ts lines 31-37, update churn so
its return depends on x and values read from allocated bits elements, not only
bits.length - 600; in test-files/test_gap_gc_index_get_receiver_rooting.ts lines
28-34, make pushed objects depend on v and derive the "hit" result from reading
an allocated element instead of the constant bits.length === 4000.

scripts/gc_root_dominance_check.py (1)

875-901: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Build and index SSA uses once before expanding each heap source.

def_of is allocated at lines 875-879 but not used by this stale-use closure. Lines 891-901 scan every instruction for every register in chain on every fixpoint iteration, which makes the stale-use check sources × instructions × iterations. Move a users index into the per-function setup and expand chain with a worklist over matched register users; reuse operand_regs() so the existing SSA operand matching is preserved.

🤖 Prompt for 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.

In `@scripts/gc_root_dominance_check.py` around lines 875 - 901, Build a
per-function SSA users index alongside def_of, mapping each register returned by
operand_regs(ins.text) to the instructions that use it. In the heap-source
closure around heap_source_kind, replace repeated full-function scans and grew
iterations with a worklist seeded by src.result, processing indexed users once
and adding results only for is_transparent instructions. Preserve the existing
chain semantics and operand matching.

🤖 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 `@scripts/gc_root_dominance_check.py`:
- Around line 978-983: Update run_stale() so --stale-registers remains
diagnostic by returning 0 after reporting the stale-register counts, regardless
of whether total is nonzero. Preserve the existing ranked output and main()
flow, without introducing a gating threshold.

---

Nitpick comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 875-901: Build a per-function SSA users index alongside def_of,
mapping each register returned by operand_regs(ins.text) to the instructions
that use it. In the heap-source closure around heap_source_kind, replace
repeated full-function scans and grew iterations with a worklist seeded by
src.result, processing indexed users once and adding results only for
is_transparent instructions. Preserve the existing chain semantics and operand
matching.

In `@test-files/test_gap_gc_method_receiver_rooting.ts`:
- Around line 31-37: The allocation loops in both gap-test helpers must produce
observable values. In test-files/test_gap_gc_method_receiver_rooting.ts lines
31-37, update churn so its return depends on x and values read from allocated
bits elements, not only bits.length - 600; in
test-files/test_gap_gc_index_get_receiver_rooting.ts lines 28-34, make pushed
objects depend on v and derive the "hit" result from reading an allocated
element instead of the constant bits.length === 4000.

🪄 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: df51bd99-1f3f-4d3f-9c82-92954edc80a4

📥 Commits

Reviewing files that changed from the base of the PR and between 3a983ca and 5c9e328.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (8)

• CLAUDE.md
• Cargo.toml
• changelog.d/7200-stale-receiver-registers.md
• crates/perry-codegen/src/expr/index_get.rs
• crates/perry-codegen/src/lower_call/console_promise.rs
• scripts/gc_root_dominance_check.py
• test-files/test_gap_gc_index_get_receiver_rooting.ts
• test-files/test_gap_gc_method_receiver_rooting.ts

[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 `@scripts/gc_root_dominance_check.py`:
- Around line 1141-1168: Validate ns.fatal_sinks alongside ns.max_stale in the
argument validation after ns.parse_args(), rejecting it with ap.error unless
ns.stale_registers is enabled. Ensure --fatal-sinks alone cannot reach the
bind-anchored check in main().

🪄 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: 3efdc0eb-0463-4073-a64d-46aa0d311ead

📥 Commits

Reviewing files that changed from the base of the PR and between 5c9e328 and 8560ca0.

📒 Files selected for processing (6)

• changelog.d/7206-stale-receiver-registers.md
• crates/perry-codegen/src/expr/index_get.rs
• crates/perry-codegen/src/lower_call/console_promise.rs
• scripts/gc_root_dominance_check.py
• test-files/test_gap_gc_index_get_receiver_rooting.ts
• test-files/test_gap_gc_method_receiver_rooting.ts

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

• test-files/test_gap_gc_index_get_receiver_rooting.ts
• crates/perry-codegen/src/expr/index_get.rs
• crates/perry-codegen/src/lower_call/console_promise.rs

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

scripts/gc_root_dominance_check.py (1)

865-931: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

--fatal-sinks can miss a genuinely fatal use when a non-fatal use of the same source is found first.

check_func_stale stops at the first non-transparent real use of a source below a collection point, then run_stale filters that single result post-hoc with v.fatal_sink (Line 964-965). If the first dominated use of a source is benign (for example, a store to a non-root slot) but a later use on a different path or block is a genuine receiver/callee dereference, the search never reaches it, since it already committed to the first candidate and broke out of both loops. --fatal-sinks exists specifically to surface the dereference case, so under this flag the search should keep looking past a non-fatal candidate instead of stopping there.

🔧 Proposed fix: thread `fatal_only` through the search so it keeps looking until a fatal use is found

-def check_func_stale(module, f, poll_reaching=frozenset(), moving_only=False):
+def check_func_stale(module, f, poll_reaching=frozenset(), moving_only=False,
+                      fatal_only=False):
     """Report registers holding a heap value that are USED below a collection
     point without being re-read from a root."""
     ...
                     v = StaleUse(module, f.name, src, kind, use, hits,
                                  src.result, poll_reaching)
                     if moving_only and not v.moving:
                         continue
+                    if fatal_only and not v.fatal_sink:
+                        continue
                     out.append(v)
                     break

             for v in check_func_stale(mod, f, poll_reaching, moving_only):
-                if fatal_only and not v.fatal_sink:
-                    continue
+            for v in check_func_stale(mod, f, poll_reaching, moving_only,
+                                       fatal_only):
                 total += 1

This keeps default (non---fatal-sinks) behavior unchanged and only makes the fatal-sinks search more thorough.

🤖 Prompt for 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.

In `@scripts/gc_root_dominance_check.py` around lines 865 - 931, Update
check_func_stale to accept and propagate a fatal_only option, and when enabled
continue scanning past non-fatal StaleUse candidates until finding a fatal sink;
preserve the current first-match behavior when fatal_only is disabled. Update
run_stale to pass its --fatal-sinks state into check_func_stale and avoid
relying on post-hoc filtering that prevents later uses from being examined.

🤖 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 `@scripts/gc_root_dominance_check.py`:
- Around line 1141-1171: Prevent --any-def from being silently ignored with
--stale-registers by rejecting that combination during argument validation,
alongside the existing --max-stale and --fatal-sinks checks. For --min-binds,
either detect explicit use and reject it in stale-register mode, or restructure
its default so explicit overrides can be distinguished; at minimum update the
--min-binds help text to state that it applies only outside --stale-registers
mode.

---

Nitpick comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 865-931: Update check_func_stale to accept and propagate a
fatal_only option, and when enabled continue scanning past non-fatal StaleUse
candidates until finding a fatal sink; preserve the current first-match behavior
when fatal_only is disabled. Update run_stale to pass its --fatal-sinks state
into check_func_stale and avoid relying on post-hoc filtering that prevents
later uses from being examined.

🪄 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: 40537a0f-0c99-435b-b951-46550c2f5886

📥 Commits

Reviewing files that changed from the base of the PR and between 8560ca0 and bf8bede.

📒 Files selected for processing (6)

• changelog.d/7206-stale-receiver-registers.md
• crates/perry-codegen/src/expr/index_get.rs
• crates/perry-codegen/src/lower_call/console_promise.rs
• scripts/gc_root_dominance_check.py
• test-files/test_gap_gc_index_get_receiver_rooting.ts
• test-files/test_gap_gc_method_receiver_rooting.ts

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

• crates/perry-codegen/src/lower_call/console_promise.rs
• crates/perry-codegen/src/expr/index_get.rs
• test-files/test_gap_gc_index_get_receiver_rooting.ts
• changelog.d/7206-stale-receiver-registers.md
• test-files/test_gap_gc_method_receiver_rooting.ts

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

--any-def and --min-binds are silently ignored when combined with --stale-registers.

Lines 1167-1170 already guard --max-stale and --fatal-sinks against being used without --stale-registers, with the stated rationale that "a knob that is silently ignored is a disarmed knob." The reverse gap exists for --any-def and --min-binds: when --stale-registers is set, the stale path returns at Line 1205-1207 before anchor (built from --any-def) or n_binds/ns.min_binds are ever consulted, so both flags have no effect and no diagnostic is printed.

--any-def is a simple boolean and can be guarded the same way as the existing checks:

🔧 Proposed fix for `--any-def`

     if ns.max_stale is not None and not ns.stale_registers:
         ap.error("--max-stale requires --stale-registers")
     if ns.fatal_sinks and not ns.stale_registers:
         ap.error("--fatal-sinks requires --stale-registers")
+    if ns.any_def and ns.stale_registers:
+        ap.error("--any-def has no effect with --stale-registers")

--min-binds defaults to 1, so an explicit --min-binds 1 cannot be distinguished from the default with a plain truthiness check. If detecting an explicit override is not worth the default=None restructuring, at minimum update its help text to state it only applies outside --stale-registers mode, so a user passing --min-binds 50 --stale-registers is not misled into believing the corpus size was validated.

🤖 Prompt for 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.

In `@scripts/gc_root_dominance_check.py` around lines 1141 - 1171, Prevent
--any-def from being silently ignored with --stale-registers by rejecting that
combination during argument validation, alongside the existing --max-stale and
--fatal-sinks checks. For --min-binds, either detect explicit use and reject it
in stale-register mode, or restructure its default so explicit overrides can be
distinguished; at minimum update the --min-binds help text to state that it
applies only outside --stale-registers mode.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

scripts/gc_root_dominance_check.py (2)

1369-1389: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Assert the --unrooted-allocas gate exit status.

Lines 1369-1389 only test _scan_unrooted(). They do not execute the CLI gate in Lines 1499-1542. A change that makes the final return always succeed will keep this self-test green. Add probes that assert exit 1 for the planted fixture and exit 0 for the rooted control.

As per coding guidelines, “A CI gate must assert that the behavior it measures actually executed, not merely that the test completed without throwing; verify gate exit codes directly rather than relying on piped wrapper commands.”

🤖 Prompt for 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.

In `@scripts/gc_root_dominance_check.py` around lines 1369 - 1389, Extend the
self-test near the _scan_unrooted checks to invoke the --unrooted-allocas CLI
gate for both the planted unrooted fixture and the rooted control, and assert
exit status 1 and 0 respectively. Use direct subprocess exit-code checks rather
than piped wrapper commands, while preserving the existing scanner assertions.

Source: Coding guidelines

---

1438-1442: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reject combined analysis modes.

If callers pass --stale-registers --unrooted-allocas, Lines 1487-1489 return from stale analysis before the unrooted-alloca gate runs. The default stale mode exits 0, so this invocation can pass while unrooted-alloca violations exist. Reject this flag combination, or explicitly run both analyses.

As per coding guidelines, “A CI gate must assert that the behavior it measures actually executed, not merely that the test completed without throwing; verify gate exit codes directly rather than relying on piped wrapper commands.”

🤖 Prompt for 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.

In `@scripts/gc_root_dominance_check.py` around lines 1438 - 1442, Reject the
incompatible --stale-registers and --unrooted-allocas combination during
argument validation before either analysis runs. Ensure the command exits
nonzero with a clear diagnostic, preventing the stale-registers early return
from skipping the unrooted-alloca check; preserve each mode’s existing behavior
when used alone.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 1369-1389: Extend the self-test near the _scan_unrooted checks to
invoke the --unrooted-allocas CLI gate for both the planted unrooted fixture and
the rooted control, and assert exit status 1 and 0 respectively. Use direct
subprocess exit-code checks rather than piped wrapper commands, while preserving
the existing scanner assertions.
- Around line 1438-1442: Reject the incompatible --stale-registers and
--unrooted-allocas combination during argument validation before either analysis
runs. Ensure the command exits nonzero with a clear diagnostic, preventing the
stale-registers early return from skipping the unrooted-alloca check; preserve
each mode’s existing behavior when used alone.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f18d3f6-2fd9-4d1a-bdac-d57e69fc983a

📥 Commits

Reviewing files that changed from the base of the PR and between bf8bede and 1f9d710.

📒 Files selected for processing (2)

• crates/perry-codegen/src/expr/index_get.rs
• scripts/gc_root_dominance_check.py

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

• crates/perry-codegen/src/expr/index_get.rs