fix(gc): root rest-args and same-module call arguments (#7270, by @jdalton) - #7271
Merged
Conversation
#7240 fixed `lower_call/extern_func.rs`'s cross-module NON-rest arm and named two siblings it would not ship unmeasured. These are those two. `extern_func.rs`'s `has_rest` arm had TWO unprotected registers where the non-rest arm had one. The fixed parameters, as before -- except their window does not close when the last argument is lowered, because the rest array is materialized afterwards and materializing it runs `js_array_alloc` plus one `js_array_push_f64` per trailing argument. And the ACCUMULATOR, which has no analogue in the non-rest arm: `current` is a raw `*mut ArrayHeader` in a bare SSA register, threaded through the push loop, holding the only reference to every argument pushed so far while the next argument's expression -- arbitrary user code -- is lowered. Nothing rooted it, so a minor landing in that window was free to SWEEP the array, not merely move it. `func_ref.rs`'s same-module arms, all four, had the identical defect. #7240's regression test needed a two-file fixture precisely because a same-file callee does not reach `extern_func.rs` at all: it resolves through `Expr::FuncRef(fid)` into `func_ref.rs`, so the bug sat one `else` away, unreached by that PR's test. It was not folded into #7240 because `func_ref.rs` threads its lowered arguments through four specialized-ABI dispatch paths, each a fast/fallback diamond with a phi at the merge; the temp-root release has to sit in the merge block that post-dominates all five call sites. The release is emitted AFTER `implicit_this_restore`, and that order is load-bearing. `implicit_this_save` (#7211) runs below the argument lowering, so its slot sits ABOVE this group, and `js_gc_temp_root_truncate` drops `base` and everything above it. Releasing first drops the saved receiver, and `js_gc_temp_root_get` answers an out-of-range read with `0` -- so the restore would rebind the enclosing method's `this` to the NUMBER 0. That is a miscompile, not a rooting bug, and it fires whenever a same-module callee reads dynamic `this` and at least one argument takes a real slot. All five arms now share one `lower_call/mod.rs` helper. Each argument is still gated by `temp_root::operand_protection`, so a list of scalars emits the IR it emitted before. Measured per gap test, compiled AND run with `PERRY_GC_MOVING_LOOP_POLLS=1`: arm parent (6aeef5b) this commit polls only bad 0 10/10 bad 0 10/10 polls + PERRY_GC_ZEAL=1 0/10, SIGSEGV bad 0 10/10 polls + zeal + PERRY_GEN_GC=0 bad 0 10/10 bad 0 10/10 The first row is why both test files carry a `parity-env:` line: without it the harness runs them in the default configuration, the broken compiler prints `bad 0`, and the files gate nothing. Polls are off by default since #7161, so the IR has no back-edge safepoint to collect on, and without zeal the only collections are allocation-triggered, which take `ManualGcScanGuard::force_full_scan` and make the copying minor ineligible -- nothing moves, so a stale register still names a live object. `run_parity_tests.sh` applies `parity-env` to the perry compile AND the perry run, which is what `PERRY_GC_MOVING_LOOP_POLLS` needs, since it is read at both. The `PERRY_GEN_GC=0` row is the control that proves the tests track collector mode rather than being flaky. Statically, over the 116-source corpus emitted by the parent compiler and read with the parent checker (so this is the codegen delta alone): `--stale-registers --moving-only` 110 -> 62, and `--moving-only --fatal-sinks` 32 -> 0. Those 32 were all `source=alloc sink=js_array_push_f64` -- the unrooted rest accumulator. Refs #7154.
…source `--stale-registers` classified a heap-value SOURCE as an `ALLOC_RE` call or a shadow-slot load. A `load double, ptr @...str.N.handle` is neither, so the register it defines was never tracked and no stale use could be attributed to it -- which is the blind spot #7240 shipped its fix through, as that PR's own writeup says. The pattern already existed and was defined twice, in effect: `--unrooted- allocas` had `REWRITTEN_LOAD_RE` and used it, while `--stale-registers` had only `GLOBAL_ROOT_RE` and knew about `@perry_global_*` alone. The two modes disagreed about what a collector-rewritten load is, and the narrower one was wrong. There is now one definition and both modes read it. Unlike #7226's `js_implicit_this_set` and #7227's `js_regexp_new`, this could not be closed by adding a name to `ALLOC_RE`: the source is a `load`, not a `call`. Strictly additive by construction -- `GLOBAL_ROOT_RE` is consulted first, so no previously reported source changes kind. Measured over the 116-source / 136-module corpus, emitted twice, once by the parent compiler and once by the commit below, so the checker delta and the codegen delta can be read separately: corpus from mode parent this parent codegen --stale-registers 2914 4805 (+1891 strh) parent codegen --moving-only 110 158 (+48 strh) parent codegen --moving-only --fatal-sinks 32 32 this codegen --stale-registers 2858 4693 (+1835 strh) this codegen --moving-only 62 62 (+0) this codegen --moving-only --fatal-sinks 0 0 Read the two middle rows together, because that is the whole result. On the parent's IR the widening exposes 48 stale uses that reach a moving minor, and ALL 48 are in the two gap tests added in the commit below -- every one a `load double, ptr @...str.N.handle` feeding `joinRest` or `joinSameRest` below the rest-array construction, which is precisely the defect that commit fixes. There are none anywhere else in the corpus. On the fixed IR the same widening adds ZERO `--moving-only` uses. The modelling is therefore not too broad: it found one population, that population was real, and it is now empty. The CI gate is untouched -- `gc-root-dominance.yml` runs the bind-anchored mode, not `--stale-registers`, and exits 0 with 0 violations and 40/40 seeded violations caught on both corpora with both checkers. `--self-test` asserts the new source in both directions and under `--moving-only`, so the widening cannot silently stop working. Recorded rather than hidden: the shared `REWRITTEN_LOAD_RE` also names `@perry_class_keys_*`, which `--unrooted-allocas` has always used. It contributes 0 hits in `--stale-registers` over this corpus, so that arm is currently carried by the shared definition rather than exercised by it. Refs #7154.
|
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 (10)
📝 WalkthroughWalkthroughThe change roots call arguments across rest-array construction and same-module call paths. It delays root release until after ChangesGC-safe call argument lowering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CallLowering
participant RootedOperands
participant RestCallHelper
participant RestArrays
CallLowering->>RootedOperands: root lowered arguments
CallLowering->>RestCallHelper: lower rest call arguments
RestCallHelper->>RestArrays: build rooted rest arrays
RestCallHelper-->>CallLowering: return arguments and guard
CallLowering->>CallLowering: restore implicit_this
CallLowering->>RootedOperands: release argument roots
Possibly related issues
Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This was referenced Aug 2, 2026
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 2, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
jdalton
added a commit
to jdalton/perry
that referenced
this pull request
Aug 3, 2026
Four PRs in a row shipped a test file that ran nowhere. PerryTS#7192 and PerryTS#7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; PerryTS#7252 added a third; PerryTS#7270/PerryTS#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs PerryTS#7192, PerryTS#7216, PerryTS#7252, PerryTS#7270, PerryTS#7271.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 3, 2026
Four PRs in a row shipped a test file that ran nowhere. #7192 and #7216 each added a `test_gap_gc_*` stale-root witness and no corpus line; #7252 added a third; #7270/#7271 added two more, caught by the maintainer at merge. Four occurrences of one mistake is a missing gate, not carelessness. An unregistered file is not a failing test, it is no test at all. The PR is green, the reviewer sees a witness in the diff next to a passing run and reads the two together as "covered", and nothing says otherwise because nothing ran. This is CLAUDE.md's fourth hazard in its purest form: the gate runs, its subject never does. Registration checks already existed for two of the three prefixes (`gc_repsel_matrix.sh` auto-detects `test_gap_repsel_*`/`test_gap_specabi_*`, `gc-moving-witnesses.yml` adds `test_gap_gc_*`) and neither could catch the pull request that needed it: both sit behind a 90-minute release build of the compiler, behind a changed-paths relevance filter, and in workflows that are not in branch protection's required contexts. `scripts/check_test_registration.py` is the cheap half of those checks, pulled out to where it can block, and generalised past that one corpus. Pure filesystem and text — no compiler, no Node, ~0.2s — over four mechanisms: gc-repsel-corpus test-files/test_gap_{gc,repsel,specabi}_*.ts -> test-parity/gc_repsel_corpus.txt feature-matrix-probes test-features/probes/**/*.ts -> test-features/feature_matrix.toml compiler-output-workloads benchmarks/compiler_output/fixtures/**/*.ts -> benchmarks/compiler_output/workloads.toml rust-test-modules crates/*/**/tests/**/*.rs below a suite root -> the `mod` declaration in the parent module The last is the Rust analogue and is worth naming: cargo auto-discovers `crates/<c>/tests/<suite>.rs`, but a file one level deeper only compiles if a `mod` names it. Without one rustc never parses it — not dead code, not code, no warning. Built to be able to fail, against all four hazards: 1. no `continue-on-error`, no `|| true`; the step's exit status is the gate. 2. it is a step in `lint`, which is ALREADY a required context. That placement is the point: forgetting to add a new job to branch protection is hazard 2, and it is what left `gc-root-dominance` red and blocking nothing for days. No admin action is needed here because the step that gets forgotten does not exist. 3. `lint`'s concurrency already cancels pull-request runs only. 4. the subject is asserted live. Each mechanism floors its candidate set and FAILS if the glob stops matching, so "0 dark over 0 candidates" cannot print the same verdict as "0 dark over 157". Every run states `checked N files against M registries`. Exclusions are named with reasons rather than counted, because a threshold cannot tell a new dark file from an old one — fix one, add one, tally unchanged. A stale exclusion, one matching no file on disk, is itself a failure, so an excuse cannot outlive the file it excuses. Same for the mirror image: a registry entry whose file is gone fails as a rotted entry. `--self-test` (32 cases, also run in `lint`) plants an unregistered file into each of the four mechanisms over the REAL registries via an in-memory overlay, asserts the gate names it, then removes it and asserts green. It also pins the one false positive found while writing this: `resolve/tests/ declaration_sidecar_tests/compile_package.rs` IS declared, by a `mod` inside an inline `mod … { }` block two levels up, and the first draft condemned it. A gate that cries wolf gets deleted. Verified end to end on disk, not just through the overlay: planted a real unregistered file in each of the four mechanisms, watched the gate go red and name it; registered one and watched it go green; deleted the file leaving the line and watched the rotted-entry arm go red; restored and watched it go green. Today's dark set is empty for all four, so this is green on `main` from the first run and safe in a required context. Three feature probes and two compiler-output fixtures are excluded, each with its reason: four are helper modules imported by a registered test, and `benchmarks/compiler_output/fixtures/raw_numeric_layout_smoke.ts` is registered in a different registry (the `raw_numeric_layouts` target-collector workload in `scripts/run_memory_stability_tests.sh`). Deliberately out of scope: `tests/*.sh|py|ts`, where 143 of 171 files are referenced by nothing in the tree. There is no registry there to diff against, so "unregistered" is not even well defined; that is an archaeology problem (triage each, wire it up or delete it) and inventing a registry for it retroactively would make this gate red on day one for reasons unrelated to the four dark witnesses. `--list` says so out loud rather than leaving the silence. Docs where an author will actually meet the rule: a new `docs/src/testing/test-registration.md`, a bullet in CONTRIBUTING.md's "what goes in a PR", and a rewritten header on each of the three registry files. Refs #7192, #7216, #7252, #7270, #7271.
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.
Authored by @jdalton (#7270), restacked onto
mainwith authorship preserved, plus three small merge-time fixes. #7270 is the PR of record for review discussion; this exists to land it.The work (jdalton's)
The follow-ups #7240/#7252 named and would not ship unmeasured:
extern_func.rs'shas_restarm — two unprotected registers where the non-rest arm had one. The accumulator is the serious one:currentwas a raw*mut ArrayHeaderin a bare SSA register, holding the only reference to every argument pushed so far while the next argument's expression — arbitrary user code — was lowered. A minor landing in that window was free to sweep it, not merely move it.temp_root::rooted_array_begin's doc has named this exact shape as "the shape behind every variadic / spread / rest argument list" since gc: console.log argument temporaries are not precise roots — a precise-roots-only collection drops string-literal args (minimal repro, no evacuation needed) #6951; this path never adopted it.func_ref.rs's same-module arms — all four. fix(gc): root every argument of a cross-module direct call — registry --help 28/30 → 30/30 #7240's regression test needed a two-file fixture precisely because a same-file callee never reachesextern_func.rs; it resolves throughExpr::FuncRef(fid), so the identical defect sat oneelseaway.gc_root_dominance_check.pyin the first place — modelling a string-literal handle load as a heap-value source.Merge-time fixes I added
test-parity/gc_repsel_corpus.txt. They were dark. This is the fourth instance of add-a-witness-forget-the-line (fix(codegen): make every GC root store dominate the collection points after it #7192, fix(gc): bind the catch parameter and the closure this/new.target slots, and snapshot the Object.assign string source #7216, fix(gc): root every argument of a cross-module direct call (#7240, by @jdalton) #7252 are documented in that file directly above). fix(gc): root every argument of a cross-module direct call (#7240, by @jdalton) #7252's was caught only because ci(gc): give the moving-GC matrix a main-line run, and gate the wiring (#7194) #7253 gave the moving-GC matrix a main-line trigger hours earlier — before that it would have sat dark indefinitely. Four occurrences is a process problem, not four mistakes; worth atest_gap_gc_*check at PR time rather than post-merge.cargo fmtviolation inlower_call/mod.rs:308. This would have landed red onmainand pushed the already-failing requiredlintjob further from green.7241-…→7270-…to match the PR-keyed convention.Not verified
CI is the first execution of both new witnesses. I did not build locally — three agents are working this machine. If they go red, that is the tests doing their job on first contact, and it would be the first independent exercise of this rooting fix. A dark test is strictly worse than a red one.
Closes #7270.
Summary by CodeRabbit
Bug Fixes
arguments, and garbage collection.Tests