ci(gc): make the root-dominance gate able to fail, baseline it honestly, and document the invariant - #7212
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe PR adds a GC root-dominance CI gate, generated IR corpus, allowlist enforcement, seeded checks, rooting documentation, and a dynamic closure-call lowering fix. The fix roots and rereads receivers, callees, and arguments across moving-GC collection points. ChangesGC root dominance validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 1, 2026
jdalton
force-pushed
the
feat/gc-root-invariant-gate
branch
from
August 1, 2026 18:40
6478f3c to
f4b15b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
scripts/gc_root_dominance_check.py (4)
755-778: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the Ruff findings before lint runs.
Ruff reports
RUF023on__slots__order andB904on the two raises inside theexceptblocks. If the repository lint job enables these rules, the job fails.🛠️ Proposed fix
- __slots__ = ("fingerprint", "count", "issue", "justification", "matched") + __slots__ = ("count", "fingerprint", "issue", "justification", "matched")except FileNotFoundError: - raise AllowlistError(f"{path}: no such file") + raise AllowlistError(f"{path}: no such file") from None except json.JSONDecodeError as exc: - raise AllowlistError(f"{path}: not valid JSON: {exc}") + raise AllowlistError(f"{path}: not valid JSON: {exc}") from exc🤖 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 755 - 778, Update the class __slots__ declaration to follow Ruff’s required ordering, and in load_allowlist add explicit exception chaining to both AllowlistError raises inside the FileNotFoundError and JSONDecodeError handlers using the caught exceptions. Preserve the existing error messages and validation behavior.Source: Linters/SAST tools
822-847: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAn over-stated
countis not detected.
stale_entriesreports only entries withmatched == 0. An entry withcount: 2that matches one violation stays valid. The unused quota then absorbs a future new violation of the same shape without any diff. The allowlist header states thatcountpins the multiplicity, so under-matching deserves the same treatment as a stale entry.♻️ Proposed check
def stale_entries(entries): - """Entries that matched nothing. Always an error -- see the header note.""" - return [e for e in entries.values() if e.matched == 0] + """Entries whose quota is not fully used. Always an error. + + `matched == 0` means the bug is fixed or the corpus shrank. `matched < + count` means the entry carries spare quota that would silently absorb the + next new violation of the same shape. + """ + return [e for e in entries.values() if e.matched < e.count]Note that this change requires the
countvalues inscripts/gc_root_dominance_allowlist.jsonto match the observed hits exactly, and it changes self-test arm 1 expectations.🤖 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 822 - 847, Update stale_entries to treat allowlist entries as stale unless their matched count exactly equals the declared count, so under-stated or over-stated quotas are rejected rather than leaving unused capacity. Adjust scripts/gc_root_dominance_allowlist.json counts to match observed violations exactly and update self-test arm 1 expectations accordingly.
1385-1404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mainduplicates_scan.Lines 1385-1404 repeat the parse,
compute_poll_reaching, bind count, andcheck_funcwalk that_scanalready performs. The two copies must stay in step, or the self-test and the real run diverge silently. Extend_scanto also returnparsedand the function count, then call it here.🤖 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 1385 - 1404, The main flow duplicates the scanning logic already implemented by _scan, creating two paths that can diverge. Extend _scan to return parsed and the function count alongside its existing results, then update main to consume those values and remove its duplicate parsing, compute_poll_reaching, bind-count, and check_func traversal.
1002-1020: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSpread the seed sample across modules.
The loop walks files in sorted order and stops as soon as
sites >= want_sites. With--seeded-violations 40over 117 modules, all 40 sites can come from the first one or two files. The gate then proves the checker still fires on those modules only, which is narrower than the breadth the workflow comment claims. Cap the sites taken per file so the sample spans modules.♻️ Proposed sampling
- for p in sorted(paths): + per_file_cap = max(1, want_sites // 8) + for p in sorted(paths): if sites >= want_sites: break + taken_here = 0 with open(p, "r", errors="replace") as fh: lines = fh.read().splitlines() try: base, _ = _scan([p], moving_only, anchor) except MalformedIR: continue for at in _seed_sites(lines): - if sites >= want_sites: + if sites >= want_sites or taken_here >= per_file_cap: breakIncrement
taken_herenext tosites += 1.🤖 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 1002 - 1020, Limit the number of seeded violations collected from each file in the loop over sorted paths so no module can consume the entire want_sites quota; track a per-file count such as taken_here alongside sites and stop processing that file once its cap is reached, while preserving the existing global sites >= want_sites termination and incrementing the per-file count with each accepted mutation..github/workflows/gc-root-dominance.yml (1)
125-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
MIN_COMPILEDexplicitly in the workflow, or drop the env override from the script.The script reads
MIN_COMPILEDfrom the environment with a default of 90. The workflow does not set it, so the floor is whatever the script's default is today. The script header states that the floor must change together withPATTERNSin a reviewable diff, but an environment override can bypass that rule. State the value here next to the other floors, so all four numbers are in one place.🤖 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 @.github/workflows/gc-root-dominance.yml around lines 125 - 145, Set the MIN_COMPILED environment variable explicitly in the “Check root-store dominance” workflow step to the intended compiled-file floor, alongside the existing --min-files, --min-binds, and --min-funcs thresholds. Keep the script invocation and other floors unchanged so all threshold values are reviewable in the workflow.scripts/gc_root_dominance_corpus.sh (2)
74-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate
sourcesbefore compiling.Two patterns can match the same file. For example, a file named
test_gap_static_class_x.tsmatches only one prefix today, but any future prefix overlap adds the same source twice. A duplicate inflatescompiledand makesMIN_COMPILEDweaker than it looks. Sort and dedupe after the loop.♻️ Proposed dedup
if [ "${`#sources`[@]}" -eq 0 ]; then echo "::error::no corpus sources matched any pattern; the glob list is stale" >&2 exit 2 fi + +# Overlapping prefixes would otherwise count one source twice and weaken the floor. +IFS=$'\n' read -r -d '' -a sources < <(printf '%s\n' "${sources[@]}" | sort -u && printf '\0')🤖 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_corpus.sh` around lines 74 - 91, Deduplicate the collected source paths before compilation: after the pattern loop building sources, sort the array and remove duplicate entries, preserving the existing stale-pattern validation and ensuring compiled counts reflect unique files.
110-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the failure output of a skipped source.
The compile output goes to
/dev/null, so a skipped source reports only its name. When the corpus drops belowMIN_COMPILED, the CI log then states that sources stopped compiling but not why. Write the output to the scratch directory and print the tail for each skip.♻️ Proposed diagnostics
if ! env PERRY_GC_MOVING_LOOP_POLLS=1 \ PERRY_INLINE_SHADOW_SLOT=0 \ PERRY_NO_AUTO_OPTIMIZE=1 \ "$PERRY_BIN" compile "$src" -o "$scratch/$name" --trace llvm \ - >/dev/null 2>&1; then + >"$scratch/$name.log" 2>&1; then skipped=$((skipped + 1)) skipped_names+=("$name") + echo " --- $name failed to compile (last 5 lines) ---" >&2 + tail -n 5 "$scratch/$name.log" >&2 continue fi🤖 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_corpus.sh` around lines 110 - 118, Update the compile failure handling in the corpus loop around PERRY_GC_MOVING_LOOP_POLLS so compiler output is captured in a per-source file under the scratch directory instead of redirected to /dev/null. When compilation fails, print the tail of that file before recording the source as skipped and continuing, while preserving the existing skipped count and skipped_names behavior.
🤖 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 `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 142-146: Update the dominance-direction description in the checker
invariant documentation to state that it reports an earlier root store that does
not dominate a later collection point. Replace the current reversed wording
while preserving the surrounding explanation of unrecognised calls and false
positives.
In `@docs/src/internals/rfc-rooting-by-construction.md`:
- Around line 9-13: In the introductory paragraph, fix the incomplete sentence
ending after “a crash five GC cycles later” by removing the stray “objects” or
rewriting the sentence so it is grammatically complete and clear.
- Around line 4-5: Update the issue references in the “Problem” line of
rfc-rooting-by-construction so the `#7192` entry does not parse as a Markdown
heading, while preserving all referenced issue numbers; keep the references on
the preceding line, escape the leading hash, or format them as inline code.
- Around line 65-73: Resolve the inconsistency between the `Plain` value
semantics and the migration documentation: either redefine `Plain` with a
genuinely copyable register representation, or remove its `Copy`/“imposes
nothing” claims and update non-GC callers to explicitly clone or move values.
Keep the migration guidance aligned with the chosen ownership behavior.
- Around line 65-73: Update the RFC’s Raw and Rooted handle designs to carry
provenance for their owning Emitter and ShadowFrame, rather than relying on
PhantomData<&'e Emitter> alone. Revise Raw::root and Rooted::get to validate and
preserve emitter branding, and ensure Rooted handles remain tied to the
allocating ShadowFrame so they cannot outlive or be used with another popped
frame.
In `@scripts/gc_root_dominance_allowlist.json`:
- Around line 22-23: Update the fingerprint format documentation near
Violation.fingerprint to state that the collector component is the
alphabetically first collector, matching sorted({c.callee for c in
self.collectors})[0], rather than the first collector in program order. Clarify
that entries must be copied from the checker’s verbose output.
In `@scripts/gc_root_dominance_check.py`:
- Around line 1303-1308: Update the failure message in the CLEAN fixture
self-test near _mutate to describe inserting or splicing a call immediately
above the root store, rather than sinking the store below the collecting call.
Keep the existing violation expectation and error-reporting behavior unchanged.
- Around line 1459-1481: Update the allowlist hygiene flow around stale_entries
and remaining so both stale-entry and unallowed-violation reports are emitted in
the same run. Avoid returning immediately from either condition; evaluate both,
preserve their existing diagnostics, and return status 2 when stale entries
exist, otherwise status 1 when remaining violations exist.
---
Nitpick comments:
In @.github/workflows/gc-root-dominance.yml:
- Around line 125-145: Set the MIN_COMPILED environment variable explicitly in
the “Check root-store dominance” workflow step to the intended compiled-file
floor, alongside the existing --min-files, --min-binds, and --min-funcs
thresholds. Keep the script invocation and other floors unchanged so all
threshold values are reviewable in the workflow.
In `@scripts/gc_root_dominance_check.py`:
- Around line 755-778: Update the class __slots__ declaration to follow Ruff’s
required ordering, and in load_allowlist add explicit exception chaining to both
AllowlistError raises inside the FileNotFoundError and JSONDecodeError handlers
using the caught exceptions. Preserve the existing error messages and validation
behavior.
- Around line 822-847: Update stale_entries to treat allowlist entries as stale
unless their matched count exactly equals the declared count, so under-stated or
over-stated quotas are rejected rather than leaving unused capacity. Adjust
scripts/gc_root_dominance_allowlist.json counts to match observed violations
exactly and update self-test arm 1 expectations accordingly.
- Around line 1385-1404: The main flow duplicates the scanning logic already
implemented by _scan, creating two paths that can diverge. Extend _scan to
return parsed and the function count alongside its existing results, then update
main to consume those values and remove its duplicate parsing,
compute_poll_reaching, bind-count, and check_func traversal.
- Around line 1002-1020: Limit the number of seeded violations collected from
each file in the loop over sorted paths so no module can consume the entire
want_sites quota; track a per-file count such as taken_here alongside sites and
stop processing that file once its cap is reached, while preserving the existing
global sites >= want_sites termination and incrementing the per-file count with
each accepted mutation.
In `@scripts/gc_root_dominance_corpus.sh`:
- Around line 74-91: Deduplicate the collected source paths before compilation:
after the pattern loop building sources, sort the array and remove duplicate
entries, preserving the existing stale-pattern validation and ensuring compiled
counts reflect unique files.
- Around line 110-118: Update the compile failure handling in the corpus loop
around PERRY_GC_MOVING_LOOP_POLLS so compiler output is captured in a per-source
file under the scratch directory instead of redirected to /dev/null. When
compilation fails, print the tail of that file before recording the source as
skipped and continuing, while preserving the existing skipped count and
skipped_names behavior.
🪄 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: c982049f-302a-4f24-81eb-44a955813496
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/gc-root-dominance.yml.gitignoreCLAUDE.mdCargo.tomlchangelog.d/7212-gc-root-dominance-gate.mddocs/src/SUMMARY.mddocs/src/internals/gc-rooting-invariant.mddocs/src/internals/rfc-rooting-by-construction.mdscripts/gc_root_dominance_allowlist.jsonscripts/gc_root_dominance_check.pyscripts/gc_root_dominance_corpus.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/gc_root_dominance_check.py (2)
772-778: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the original exception in
load_allowlist.Ruff reports B904 at Lines 776 and 778. Add
from exc(orfrom None) so a JSON parse failure keeps its cause in the traceback.♻️ Proposed change
- except FileNotFoundError: - raise AllowlistError(f"{path}: no such file") - except json.JSONDecodeError as exc: - raise AllowlistError(f"{path}: not valid JSON: {exc}") + except FileNotFoundError as exc: + raise AllowlistError(f"{path}: no such file") from exc + except json.JSONDecodeError as exc: + raise AllowlistError(f"{path}: not valid JSON: {exc}") from exc🤖 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 772 - 778, Update the exception handling in load_allowlist so both AllowlistError raises explicitly chain their originating exceptions with from exc (or suppress chaining with from None where appropriate), resolving Ruff B904 while preserving the existing error messages.Source: Linters/SAST tools
1016-1027: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare violation identity, not violation count, at each seeded site.
len(after) > len(base)counts a site as caught whenever the mutant reports one more violation than the baseline. The extra violation can come from a different bind in the same module, for example if the splicedjs_call_functionwidens an unrelated window. In that case a genuine miss at the seeded site is recorded as a catch, which weakens the arm that this test exists for.Compare the sets of violations instead, and require the new one to sit in the mutated function.
♻️ Proposed change
try: after, _ = _scan([mutant], moving_only, anchor) except MalformedIR: continue sites += 1 - if len(after) > len(base): + base_keys = {(v.func, v.fingerprint) for v in base} + new_keys = {(v.func, v.fingerprint) for v in after} - base_keys + # `at` is the store line; the enclosing function is the one the + # mutant must newly report. + if new_keys or len(after) > len(base): caught += 1Consider also recording the enclosing function name from
_seed_sitesand asserting that the new violation belongs to it.🤖 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 1016 - 1027, Update the seeded-site comparison around _scan and _seed_sites to compare violation identities rather than len(after) and len(base). Record the enclosing mutated function name from _seed_sites, identify the baseline-to-mutant violation addition, and count the site as caught only when that new violation belongs to the recorded function; otherwise record it as a miss.
🤖 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 `@docs/src/internals/gc-rooting-invariant.md`:
- Around line 162-164: Update the documentation section describing
`--stale-registers` to mark it as planned rather than currently available;
explicitly state that the current command does not run this mode and that cases
3 and 4 require the planned checker.
---
Nitpick comments:
In `@scripts/gc_root_dominance_check.py`:
- Around line 772-778: Update the exception handling in load_allowlist so both
AllowlistError raises explicitly chain their originating exceptions with from
exc (or suppress chaining with from None where appropriate), resolving Ruff B904
while preserving the existing error messages.
- Around line 1016-1027: Update the seeded-site comparison around _scan and
_seed_sites to compare violation identities rather than len(after) and
len(base). Record the enclosing mutated function name from _seed_sites, identify
the baseline-to-mutant violation addition, and count the site as caught only
when that new violation belongs to the recorded function; otherwise record it as
a miss.
🪄 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: a74fad94-0129-4b8d-9168-d3fac2f10fcb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/workflows/gc-root-dominance.yml.gitignoreCLAUDE.mdCargo.tomlchangelog.d/7212-gc-root-dominance-gate.mddocs/src/SUMMARY.mddocs/src/internals/gc-rooting-invariant.mddocs/src/internals/rfc-rooting-by-construction.mdscripts/gc_root_dominance_allowlist.jsonscripts/gc_root_dominance_check.pyscripts/gc_root_dominance_corpus.sh
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/src/SUMMARY.md
- Cargo.toml
- scripts/gc_root_dominance_corpus.sh
- scripts/gc_root_dominance_allowlist.json
- CLAUDE.md
- .github/workflows/gc-root-dominance.yml
- .gitignore
- changelog.d/7212-gc-root-dominance-gate.md
There was a problem hiding this comment.
Actionable comments posted: 3
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 (1)
2134-2141: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit this script: it now exceeds the 2000-line-per-file cap.
The file extends past line 2000 (the reviewed content ends at line 2141), so
scripts/check_file_size.shfails. Move self-contained parts into sibling modules, for example the stale-register analysis (check_func_stale,run_stale,RECEIVER_SINKS), the unrooted-alloca check (check_func_unrooted_allocas,_scan_unrooted), and the self-test fixtures, then import them here.As per coding guidelines: "Run
scripts/check_file_size.shand respect the 2000-line-per-file cap".🤖 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 2134 - 2141, Split scripts/gc_root_dominance_check.py into sibling modules so the main file is at most 2000 lines, moving self-contained components such as RECEIVER_SINKS with check_func_stale/run_stale, check_func_unrooted_allocas with _scan_unrooted, and self-test fixtures. Import and update call sites in the original script to preserve existing behavior, then verify with scripts/check_file_size.sh.Source: Coding guidelines
CLAUDE.md (1)
11-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not update release metadata in this PR.
Line 11 changes the maintainer-owned
Current Versionvalue. Restore the previous value. Keep the user-facing change in a PR-keyedchangelog.d/<PR>-<slug>.mdfragment.Based on learnings, contributors must not edit the
Current Versionline inCLAUDE.md.🤖 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 `@CLAUDE.md` at line 11, Restore the previous value of the Current Version entry in CLAUDE.md, leaving maintainer-owned release metadata unchanged. Keep the user-facing change only in the PR-keyed changelog.d/<PR>-<slug>.md fragment.Source: Learnings
🧹 Nitpick comments (3)
CLAUDE.md (1)
253-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
CLAUDE.mdconcise.Line 253 adds changelog-like history and detailed implementation behavior even though it links to
docs/src/internals/gc-rooting-invariant.md. Keep the invariant, checker, CI, and allowlist pointers here. Move detailed failure history and per-issue status to the linked documentation or a PR-keyedchangelog.d/fragment.As per coding guidelines, keep
CLAUDE.mdconcise and put detailed changelogs inchangelog.d/.🤖 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 `@CLAUDE.md` at line 253, Condense the “Root-store dominance in codegen” entry in CLAUDE.md to the invariant plus pointers to scripts/gc_root_dominance_check.py, gc-root-dominance.yml, scripts/gc_root_dominance_corpus.sh, the allowlist, and the linked rooting documentation. Remove the detailed failure history, issue-by-issue status, runtime behavior, and `#7211` discussion; move any required changelog details to an appropriate changelog.d fragment.Source: Coding guidelines
scripts/gc_root_dominance_check.py (2)
1301-1319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
window_hits_genericin the unrooted-alloca check.The nested
window_hitsclosure at lines 1509-1520 repeats this logic. Two copies of the CFG-window rule can drift, and a drift changes what one mode reports without changing the other. Callwindow_hits_generic(f, st, ld)fromcheck_func_unrooted_allocasand delete the closure.🤖 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 1301 - 1319, In check_func_unrooted_allocas, remove the nested window_hits closure and replace its callers with window_hits_generic(f, st, ld). Reuse the existing generic CFG-window logic so unrooted-alloca checks follow the same collection rules.
1258-1270: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrecompute a use index instead of rescanning the whole function per fixpoint iteration.
The
while grewloop walks every block and every instruction of the function on each iteration, and this runs once per heap-value source. Over a 117-module corpus the cost grows withsources × chain_length × instructions. Build one map from operand register to the instructions that use it, then propagate the chain from that map.♻️ Sketch
+ # register -> [Insn] that use it, built once per function. + users = defaultdict(list) + for b in f.blocks: + for ins in f.insns[b]: + for r in operand_regs(ins.text): + users[r].append(ins)Then seed a worklist with
src.resultand pop from it, addingins.resultwhenis_transparent(ins)holds.🤖 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 1258 - 1270, Replace the repeated full-function scan in the chain propagation block with a precomputed operand-to-using-instructions index for the current function. In the forward closure starting at src.result, use a worklist to visit each discovered register, inspect only indexed users, and add transparent instruction results that are not already in chain; preserve the existing is_transparent and uses semantics while eliminating the grew-based rescans.
🤖 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 `@changelog.d/7214-closure-calln-stale-registers.md`:
- Around line 116-118: Reflow the prose in the changelog entry so line 117 does
not begin with the issue reference “#7161”; keep the existing wording and
emphasis intact while moving the reference later in the sentence or otherwise
ensuring the line starts with normal text.
In `@CLAUDE.md`:
- Line 253: Revise the “Root-store dominance in codegen” explanation to
distinguish failed rooting mechanisms: an out-of-frame js_shadow_slot_bind may
be a no-op, while a plain alloca_entry is not rewritten by the collector, so do
not describe all cases as rooted slots holding dangling pointers. Also replace
the claim that failures only occur with PERRY_GC_MOVING_LOOP_POLLS=1 with
wording that identifies loop polls as increasing in-loop coverage, while
preserving allocating calls and explicit gc() as collection points.
In `@scripts/gc_root_dominance_check.py`:
- Around line 1936-1950: Update the argument validation around the existing
max-stale and fatal-sinks checks to reject --stale-registers when
--unrooted-allocas is also enabled. Use argparse’s error path with a clear
requires-style message, ensuring both mutually exclusive check modes cannot be
silently combined.
---
Outside diff comments:
In `@CLAUDE.md`:
- Line 11: Restore the previous value of the Current Version entry in CLAUDE.md,
leaving maintainer-owned release metadata unchanged. Keep the user-facing change
only in the PR-keyed changelog.d/<PR>-<slug>.md fragment.
In `@scripts/gc_root_dominance_check.py`:
- Around line 2134-2141: Split scripts/gc_root_dominance_check.py into sibling
modules so the main file is at most 2000 lines, moving self-contained components
such as RECEIVER_SINKS with check_func_stale/run_stale,
check_func_unrooted_allocas with _scan_unrooted, and self-test fixtures. Import
and update call sites in the original script to preserve existing behavior, then
verify with scripts/check_file_size.sh.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 253: Condense the “Root-store dominance in codegen” entry in CLAUDE.md to
the invariant plus pointers to scripts/gc_root_dominance_check.py,
gc-root-dominance.yml, scripts/gc_root_dominance_corpus.sh, the allowlist, and
the linked rooting documentation. Remove the detailed failure history,
issue-by-issue status, runtime behavior, and `#7211` discussion; move any required
changelog details to an appropriate changelog.d fragment.
In `@scripts/gc_root_dominance_check.py`:
- Around line 1301-1319: In check_func_unrooted_allocas, remove the nested
window_hits closure and replace its callers with window_hits_generic(f, st, ld).
Reuse the existing generic CFG-window logic so unrooted-alloca checks follow the
same collection rules.
- Around line 1258-1270: Replace the repeated full-function scan in the chain
propagation block with a precomputed operand-to-using-instructions index for the
current function. In the forward closure starting at src.result, use a worklist
to visit each discovered register, inspect only indexed users, and add
transparent instruction results that are not already in chain; preserve the
existing is_transparent and uses semantics while eliminating the grew-based
rescans.
🪄 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: 137f8b5f-8090-409c-888a-0c1c13cfe18a
📒 Files selected for processing (13)
CLAUDE.mdchangelog.d/7206-stale-receiver-registers.mdchangelog.d/7214-closure-calln-stale-registers.mdcrates/perry-codegen/src/expr/index_get.rscrates/perry-codegen/src/expr/temp_root.rscrates/perry-codegen/src/lower_call/console_promise.rsdocs/src/internals/gc-rooting-invariant.mdscripts/gc_root_dominance_check.pytest-files/test_gap_gc_closure_call_argument_rooting.tstest-files/test_gap_gc_closure_call_callee_rooting.tstest-files/test_gap_gc_closure_call_this_rooting.tstest-files/test_gap_gc_index_get_receiver_rooting.tstest-files/test_gap_gc_method_receiver_rooting.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/internals/gc-rooting-invariant.md
…e_callN
The generic dynamic-value-call lowering held THREE classes of GC value in bare
SSA registers across work that can collect. `js_closure_callN` is the central
dispatch path -- `f(g())`, `o.m(g())`, `curry(1)(2)`, every call whose callee is
a value rather than a statically resolved function -- and this is the site
An SSA register is not a GC root. Under PERRY_GC_MOVING_LOOP_POLLS=1 a back-edge
poll inside an argument runs an evacuating minor: each held value SURVIVES (the
capture cell, shadow slot or module global it was read from is a root) and
therefore MOVES. The collector rewrites that location; the register keeps naming
from-space.
* the CALLEE, held across the whole argument list. The checked unbox masks a
pre-move address and js_closure_callN reads a closure header out of
abandoned memory: "TypeError: value is not a function".
* the `this` RECEIVER, held across the read of the callee off it AND the
argument list. PerryTS#7206 fixed this operand on the sibling
js_native_call_method_by_id dispatch; this is the generic one.
* each already-lowered ARGUMENT, held across the arguments after it AND
across the rebind unbox.
The three live windows differ, so they are computed separately:
receiver | the callee read + every argument
callee | every argument
argument i | the arguments after i + the rebind unbox
That last window is why this is not a copy of PerryTS#7206's fix.
js_closure_unbox_callee_checked_rebind calls clone_closure_rebind_this, which
ALLOCATES a replacement closure (closure/dynamic_props.rs:1040) when the callee
captures `this`. It sits below the last argument and above js_closure_callN, so
the arguments are re-read below it -- hence RootedOperands::reread_one, which
re-reads one operand at a caller-chosen point instead of the whole group at one.
Hoisting the unbox above the argument list would remove the window instead, but
its throw is observable and the spec evaluates arguments before it. On the
>16-arity path the argument stores into the stack buffer moved below the unbox
for the same reason: a stack buffer is not a root, so filling it above an
allocating rebind freezes pre-move addresses one indirection further out.
Receiverless calls take js_closure_unbox_callee_checked, a tag check and a mask
that allocates nothing, so `f(x, y)` on inert operands emits exactly its old IR.
Temp roots, not re-lowering: re-lowering the callee or receiver would observe an
assignment made by an argument, a miscompile rather than a rooting fix.
Three gap tests, one per held value, each red on the parent under a GENUINE
POLLS=1 build and green after. The flag is compile-time since PerryTS#7161 AND
runtime-armed (gc_moving_loop_polls_enabled(), gc/policy.rs:1759) -- setting
only one is a false green, and the first cut of these tests passed 10/10 for
exactly that reason.
callee / this / argument, POLLS=1 parent: TypeError 10/10 each
this: bad 0 10/10 each
all three, POLLS=1 + PERRY_GEN_GC=0 bad 0 5/5
all three, default (no polls) bad 0 5/5
Cost over the 141-module sfw-registry corpus, measured rather than assumed
because this is the hottest emitted call path. operand_protection emits nothing
for an operand whose window cannot collect, which is why the delta is small:
linked binary 39,216,688 -> 39,233,200 B (+0.042%), emitted IR 1,999,570 ->
2,001,607 lines (+0.10%), js_gc_temp_root_push sites 8,394 -> 8,885.
scripts/gc_root_dominance_check.py gains js_closure_unbox_callee_checked in
NONCOLLECTING, citing closure/unbox.rs:25 -- a tag check and a low-48 mask, no
allocation, no user code, no poll. It sits between every dynamic call's last
argument and its js_closure_callN, so its absence reported the whole argument
list of every 1-arg dynamic call as stale: 372 of the 729 fatal-sink hits were
that single false positive, all marked MOVING: no. The _rebind variant is
deliberately NOT added -- it allocates, and the fix above depends on it counting
as a collection point. Fatal-sink slice against the corrected list: 231 -> 205.
cargo test -p perry-codegen: failing set IDENTICAL to the parent (6
loop_safepoint_purity, 16 native_proof_regressions, 3 native_proof_buffer_views,
1 shadow_slot_hygiene, 1 typed_shape_descriptors), measured on the parent commit
rather than assumed; one lib unit test red on the parent passes here. The
bind-anchored gate reports the same single non-moving residual PerryTS#7192 left.
WHAT THIS DOES NOT CLOSE: sfw-registry --help under a genuine POLLS=1 build is
still red -- 3/10 pass, 7/10 SIGSEGV -- so PerryTS#7161's stopgap STAYS. Its default arm
is clean 10/10, so nothing was traded away. Two concrete leads are written up in
the changelog fragment: `prev_this` in this same lowering is the same bug
unfixed (js_implicit_this_set returns a value read from the scanned, rewritten
IMPLICIT_THIS cell and holds it across the entire user call), and the remaining
205 fatal sinks are no longer dominated by one class, with the spread dispatch
(expr/call_spread.rs) the obvious next site.
Refs PerryTS#7154, PerryTS#7206, PerryTS#7192, PerryTS#7198, PerryTS#7184, PerryTS#7161, PerryTS#7114, PerryTS#6951, PerryTS#519.
…ration - changelog fragment was PR-misnumbered `7207-`; PerryTS#7207 is a different, already merged change. Renamed to `7214-`. Content already referenced PerryTS#7206 correctly and is unchanged. - `cargo fmt --all -- --check` is a required check on `lint`; one hand-wrapped `roots.push` call needed re-wrapping. No behaviour change. - Registered the three witnesses in the GC x repsel corpus, next to PerryTS#7206's pair. All three are moving-only: clean on the shipped default on both sides of the fix, so they belong with the `requires=move` rows and prove nothing on `default`.
…ly, and document the invariant Squashed. See PR description for the seeded-violation proof and the allowlist rationale.
…riant writeup Merge-time corrections to statements PerryTS#7207 invalidated while this PR was in review: - CLAUDE.md called `lower_call/new.rs`'s inline-ctor `this_slot` "still open". PerryTS#7207 closed it. Point at `--unrooted-allocas` as the detector for that shape and name PerryTS#7210 as where its remaining hits are tracked. - The rooting-invariant doc documented `--stale-registers` but not `--unrooted-allocas`, so the one mode the bind-anchored check is structurally blind to had no entry. Add it, and state plainly that the gate does NOT run it and that its hits are deliberately outside the allowlist — the allowlist covers the bind-anchored shape only. - "all five known shapes" -> "every known shape", so the pointer cannot go stale the next time one is found.
proggeramlug
force-pushed
the
feat/gc-root-invariant-gate
branch
from
August 1, 2026 21:59
ce00cb0 to
7e5d0e4
Compare
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 1, 2026
… allowlist fingerprint; restore the workspace version Follow-ups to the review threads on PerryTS#7196 and PerryTS#7212, both of which merged before the findings were worked through. Cargo.toml — PerryTS#7196 set `[workspace.package].version` back to 0.5.1277, undoing the 0.5.1278 bump PerryTS#7199 had landed four commits earlier. Nothing since has touched the line, so main is currently shipping a version number it already released. Restored. docs/src/internals/gc-rooting-invariant.md — the checker description read "reports any root store that does not dominate a preceding collection point", which is vacuous: a store can never dominate anything that precedes it, so the sentence is true of every store in the program. What the checker actually reports is the collection point — `window_hits(origin, bind)` collects the calls that can run between the instruction producing a GC value and the `js_shadow_slot_bind` that publishes it. Reworded to name the collection point as the reported object and to keep the true relation (the root store must dominate it), which is the rule stated at the top of the same page. Also noted that the gate command shown does not pass `--stale-registers`, so cases 3 and 4 only surface when it is run by hand. CLAUDE.md — the shape summary claimed all three failures present as "a rooted slot holding a dangling pointer", contradicting its own two preceding clauses: PerryTS#7184's bind is a silent no-op so nothing is bound, and the `alloca_entry` shape is never a slot at all. Split per shape. It also said the class "only bites under PERRY_GC_MOVING_LOOP_POLLS=1"; the checker's own POLL_CAPABLE_RUNTIME set treats js_object_set_field_by_name, js_object_get_property and js_call_function as moving-capable with no poll involved, and PerryTS#7211's allowlist entry is exactly such a window. Polls widen in-loop coverage; they are not a precondition. docs/src/internals/rfc-rooting-by-construction.md — `Plain(String)` cannot be `Copy`; the migration section costed it as if it were. It is `Clone`, which still imposes nothing on the caller. Added the emitter/frame branding gap to "What it cannot catch": PhantomData<&'e Emitter> records a lifetime, not an instance, and `Rooted` carries a bare SlotIdx, so the design as written catches ordering mistakes but not provenance ones. scripts/gc_root_dominance_allowlist.json — the fingerprint format line said "<first collector>". It is `sorted(set(callees))[0]`, the alphabetically first, not the first in program order. A hand-derived entry that guesses program order matches nothing, and an entry that matches nothing fails the build — so the misleading line pointed straight at a red gate. scripts/gc_root_dominance_check.py, three surgical changes: * the self-test failure text described sinking the root store; `_mutate` splices _SEED_CALL above the store and moves nothing. * `--stale-registers` returned before the `--unrooted-allocas` block, so passing both ran one pass and silently skipped the other. Now an argparse error, matching the --max-stale and --fatal-sinks guards directly above it. * the stale-allowlist-entry report returned 2 before the uncovered- violation report could run. Fix one violation and introduce another in the same PR and the log showed only the bookkeeping problem. Both reports now print; the exit code is unchanged (2 when an entry is stale, 1 when only uncovered violations remain). Verified: `--self-test` OK. Against the parent, a corpus with one stale entry and two uncovered violations printed only the stale entry and hid both violations; it now prints all three and still exits 2. Refs PerryTS#7196, PerryTS#7212, PerryTS#7199, PerryTS#7211.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Perry's code generator can emit a program in which a value the garbage collector needs to know about is not registered with it across a point where collection can happen. When that happens the compiled program keeps a pointer to memory the collector has already recycled, and the failure shows up cycles later, in a different function, as a confusing
TypeError: value is not a function— or as a silently wrong answer. #7198 added a CI job that finds this statically, by reading the LLVM IR Perry emits and checking that every root registration happens before anything that can collect.That job has been failing on
mainsince the day it merged, and it blocked nothing. It is not in branch protection's list of required checks, so a red result never turned a merge red. It found five real instances of the bug and reported them into the void for the life of #7198. This PR makes the gate honest: it is baselined against those five findings (now tracked as #7211) with a named, per-site allowlist rather than a number, the corpus it reads grew from 41 to 117 modules, and the checker now proves on every run that it can still fail. After merge, one repo admin action is still required — see the fold below.This is durability work for the bug class behind #7154 / #7184 / #7192 / #7206. The individual bugs are being fixed elsewhere; this is about making sure new ones cannot land quietly.
The finding that shaped this PR — the gate was already red on
main, and had been since it mergedRunning
main's own checker overmain's own corpus:It blocked nothing because
gc-root-dominanceis not in branch protection's required contexts — hazard 2, and precisely the corollary CLAUDE.md warns about ("a new gate has never been green, so promoting it immediately blocks every open PR. Run it once, then promote. Leaving that second step undone is how (2) happens.").The five hits are real and are now #7211:
Expr::ClassExprFreshroots its class object only when it believes the static initializers can collect, and never asks whether its own emittedjs_object_set_field_by_namecan.class C { static tag = tag }therefore goes stale. Not fixed here —crates/perry-codegen/src/expr/is under concurrent edit.The change — a reproducible corpus, a named allowlist instead of a threshold, and two new ways for the checker to prove it still works
Four pieces, each closing a different way this gate could have gone quietly useless.
scripts/gc_root_dominance_corpus.sh(new) — corpus emission moves out of the YAML so reproducing a CI failure is one command. A retyped invocation that dropsPERRY_GC_MOVING_LOOP_POLLS=1produces IR in which the bug is not expressible, and the local run then "cannot reproduce" a real finding. Corpus grows from 41 to 117 .ll files / 1993 functions / 2501 root stores across 99 sources, chosen for the lowerings this invariant runs through (class expressions, constructors, object/array literals, computed reads, property stores, closures, dynamic dispatch, representation selection). A stale glob is a hard error; the compiled-source count has an explicit floor.scripts/gc_root_dominance_allowlist.json(new) — replaces "any violation fails" with one named entry per known-remaining hit, each carrying an issue and a written justification. Deliberately not a number: a threshold cannot tell a new violation from an old one, and the cheapest way to green a red build is to raise it by one. Three enforced properties:count--seeded-violations N— after checking, splices synthetic collection points into the real corpus IR between an allocation and its root store, and requires every one to be reported.--self-testonly proves the checker fires on frozen hand-written fixtures; it keeps passing if Perry's emitted IR drifts to a shape the parser can no longer read, at which point the gate reports a sereneviolations: 0over IR it is not analysing. This is the arm that catches that.--min-funcs,checked N functions / M modules— breadth is asserted and printed, so a silently-empty or silently-shrunken run is visible rather than indistinguishable from a clean one.Docs —
docs/src/internals/gc-rooting-invariant.mdstates the rule plainly with all five real bugs as case studies, the symptom each produces, and how to check your work (including thePERRY_GC_PROTECT_FROMSPACE_DEPTHfalse-green caveat: the default of 4 is not enough, use 800). Indexed inSUMMARY.md.Proof it can fail — seven arms run against the real corpus on this branch
Ran: every arm below, against the real corpus on this branch.
main, allowlistedcountlowered below actual--self-testalso gained arms for the allowlist's anti-absorption properties (partial coverage, quota, stale entries, under-documented and duplicate entries) and for the corpus mutator itself, so the machinery is proven before it is pointed at real IR.Cost: corpus emission ~80s (99 compiles), the check itself ~3s over 1993 functions. Fine for per-PR.
The four gate hazards — where each one stands after this PR
continue-on-error, no|| true, no pipe between the checker and the shell's exit statusconcurrencycancels pull-request runs only,mainruns queue--self-test,--seeded-violationsagainst real IR, and--min-files/--min-binds/--min-funcsAction required after merge — an admin has to add the job to required contexts, and I cannot do that
With the allowlist the job is green on
main. A repo admin needs to addgc-root-dominanceto branch protection's required contexts. Until that happens this is documentation, not a gate — which is exactly how the #7211 hits went unread for the life of #7198.Note for whoever merges the in-flight lowering fixes — delete the allowlist entries in the same PR as the fix
The allowlist is keyed to specific sites with justifications, never a total, so a later fix cannot leave a stale threshold behind. But it will go red with
error: allowlist entries matched nothingonce #7211 is fixed — that is the intended prompt. Delete the corresponding entries in the same PR as the fix. The four current entries all retire together.Also in this PR — an RFC for making the bug unrepresentable rather than merely detectable
docs/src/internals/rfc-rooting-by-construction.md— a design proposal expressing V8'sHandle/HandleScopeidea through Rust's borrow checker. Unrooted values becomeRaw<'e>holding a shared borrow of the emitter; emitting anything that can collect takes&mut, so using aRawacross a collection point is a compile error. Covers the API, a bug-by-bug verdict (4 of the 5 caught by construction; #7184 needs the shadow frame to own its own slot numbering), migration cost (~2500 builder call sites, 300-500 of them genuinely GC-managed, with an incremental escape-hatch path), performance (emitted IR identical; the honest risk is more rooting than strictly needed), and five things it explicitly cannot catch.Verdict: feasible and worth doing, incrementally, after the in-flight lowering work lands. Not prototyped here for exactly that reason — a proof-of-concept worth anything has to touch
expr/andlower_call/, which are under concurrent edit.Dependency note — independent of #7206, but this gate should adopt its
--stale-registersmode afterwardIndependent of #7206. That PR also touches
scripts/gc_root_dominance_check.py(adding--stale-registers), so expect a small merge conflict in the argument parser; the changes are in different functions. Once #7206 lands,--stale-registersshould be added to this gate's invocation — it is the mode that found the receiver and computed-base bugs, and this gate does not yet run it.Summary by CodeRabbit
Bug Fixes
this), and argument values from causing incorrect results.Documentation
Quality Improvements