style: restore rustfmt on linker.rs, red on main since #7355 - #7361
Merged
Conversation
`cargo fmt --all -- --check` exits 1 on main HEAD (7428437). One file: `crates/perry-codegen/src/linker.rs`, where `rs4gc_funclet_refusal`'s needle array crossed the width that makes rustfmt break it vertically. `lint` is a REQUIRED context. A required check that is red on main does not block anything -- it means every subsequent merge is a bypass, which is the hazard CLAUDE.md lists and that this repo has already paid for once (lint sat red across three nightlies on 16 oversized files, and every merge in that window went in unchecked). No behavior change: whitespace only. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe linker changes reformat an array literal and collapse a multi-line ChangesLinker formatting cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
) `scripts/check_file_size.sh` exits 1 on main HEAD: `crates/perry-codegen/src/expr/index_set.rs` is 2035 lines against a 2000 cap. It crossed in #7342. That script runs inside the `lint` job, which is a REQUIRED context -- so this is the second independent way `lint` was red on main today (the first was rustfmt on linker.rs, #7361). A required check that is red on main blocks nothing; it means every merge is a bypass. The split follows the recipe in the script's own failure message: extract a topical group into a sibling module. `lower_inline_dyn_typed_array_set` and its `emit_inline_ta_int_store` helper are the guarded inline typed-array store for a type-erased receiver -- one coherent unit, moved verbatim to `index_set_typed_array.rs`. index_set.rs drops to 1749 lines, leaving real headroom rather than landing one line under the cap. Mechanical move: the two functions are byte-identical, only the imports they need travelled with them and `lower_inline_dyn_typed_array_set` became `pub(super)` so its one caller can still reach it. cargo test -p perry-codegen --lib: 609 passed. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…#7369) * fix(lint): split index_set.rs, over the 2000-line cap since #7342 (#7366) `scripts/check_file_size.sh` exits 1 on main HEAD: `crates/perry-codegen/src/expr/index_set.rs` is 2035 lines against a 2000 cap. It crossed in #7342. That script runs inside the `lint` job, which is a REQUIRED context -- so this is the second independent way `lint` was red on main today (the first was rustfmt on linker.rs, #7361). A required check that is red on main blocks nothing; it means every merge is a bypass. The split follows the recipe in the script's own failure message: extract a topical group into a sibling module. `lower_inline_dyn_typed_array_set` and its `emit_inline_ta_int_store` helper are the guarded inline typed-array store for a type-erased receiver -- one coherent unit, moved verbatim to `index_set_typed_array.rs`. index_set.rs drops to 1749 lines, leaving real headroom rather than landing one line under the cap. Mechanical move: the two functions are byte-identical, only the imports they need travelled with them and `lower_inline_dyn_typed_array_set` became `pub(super)` so its one caller can still reach it. cargo test -p perry-codegen --lib: 609 passed. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * docs(plan): both statepoint adoption gates are closed; record the platform matrix (#7367) The plan still said statepoints were aarch64-only (#7321), that the matrix "therefore runs on macos-14", that `statepoints-refuse-x86` pinned the refusal, and it spelled the knob `PERRY_STATEPOINTS` four times. None of that is true now, and this document is what the adoption decision gets made from. What actually changed: - x86-64 is unblocked. `_Unwind_GetGR(ctx, 7)` does segfault and cannot be fixed as stated -- libgcc tracks only the columns CFI restores and RSP is derived, not tracked. #7349 stopped asking for it and derives the SP-relative base from `_Unwind_GetCFA`, with a per-arch return-address adjustment (x86-64 `call` pushes one, aarch64 `bl` does not). x86-64 Linux is a first-class arm. - Windows works via RtlVirtualUnwind (#7355), the one walker with no Itanium unwinder beneath it. - aarch64+ELF is now covered too (#7360) -- the only shape where LLVM spells 32-bit stack-map fields `.word`. - One mechanism, not two: PERRY_STATEPOINTS and the plain-map bridge are deleted, so the kill-policy line about "a mode that still exists" no longer applies to this pair. - The gate proves something now. Until today the Unix arms reported 7 frames and ZERO locations -- they would have passed with a walker that visited nothing. #7359's deep-collect probe took them to 221 locations. - watchOS/visionOS are not blocked by Perry: they build on stable without `dyn-eval`, and fail three crates away in psm's Mach-O guard. So the remaining adoption gate is `llvm-inprocess` becoming a default cargo feature, plus sequencing step 2 (root density) -- adopting today would regress binary size on root-dense code. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * gc: admit two provably-leaf helpers, and measure that it buys nothing js_gc_register_global_root was the most frequent non-leaf callee in the probe suite (148 call sites) and is provably GC-leaf: its whole body is runtime_write_barrier_root_heap_word -- which js_write_barrier_root_heap_word, already CannotCollect, wraps in one line -- plus a TLS Vec::push. The "malloc count threshold" trigger does not apply to that push: the counter is MALLOC_STATE.objects.len(), a registry of Perry GC objects, and the #[global_allocator] is plain mimalloc/System with no GC hook. js_typed_feedback_maybe_dump_trace joins its already-admitted family siblings. Measured A/B on the same tree, and the result is a null: probe safepoints roots total bytes __text 06_string_retention 105 -> 100 27=27 0 -4 B 09_try_catch_roots 343 -> 339 259=259 0 -4 B 11_collect_at_depth 120 -> 117 36=36 0 -4 B Root counts are IDENTICAL. The 40 safepoints removed across the suite were all rootless, and a rootless safepoint costs essentially nothing -- which is what docs/engine-plan.md already says: "the axis is not 'statepoints are bigger', it is 'roots are bigger'". Recording it as evidence: the safepoint-count lever is not the binary-size lever, so sequencing step 2 must attack live-root SETS. Two tests come with it. One pins the wrapper's classification to the barrier it wraps. The other pins js_nanbox_string OUT of the allowlist: at 120 call sites it is the obvious next candidate and reads as pure bit manipulation, but its null guard calls js_string_from_bytes to allocate an empty string. Probe suite 11/11 byte-identical under forced evacuation + verification. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…7368) * fix(codegen): statepoint report counted zero safepoints since #7348 #7348 deleted the explicit bridge and with it the only callers of note_statepoint and note_skipped -- they lived in the bridge, which counted safepoints as it emitted them. The methods survived with no callers, so statepoints, relocations, max_live_roots, skipped_non_safepoints, live_roots_histogram and both by-callee maps went structurally zero in production. A real compile printed "0 statepoints emitted" while its binary carried 120. Counting at IR-emission time cannot work any more, and that is the lesson: Perry no longer decides which calls become safepoints -- RewriteStatepointsForGC does, inside LLVM. The only honest source is the compact-map rewrite, which already parses the assembly LLVM emitted and computed these exact numbers before dropping them into log::debug!. The report reads from there now: 120 safepoints across 6 function(s) in 1 module(s) 36 live roots recorded, 0.30 per safepoint An absent measurement no longer renders as a measured zero: gc_map.modules == 0 means "never reported", the text report says UNAVAILABLE rather than printing zeros, and JSON carries gc_map separately from totals so a consumer can tell them apart. schema_version -> 2. The CI gate now asserts the counts, not just the label. --only-backend rs4gc passed throughout the regression -- the label was right, the numbers were fiction. It now also requires records > 0 and roots > 0; verified against a synthetic report with the #7348 shape, where the label check still reports 9 functions green while the count checks exit 1. Second round of dead counters here (#7362 removed four that never had a writer at all). The new test documents why the first invariant missed this one: every_rendered_counter_has_a_writer called the mutators itself, so "has a writer" passed while "is written" was false. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF * gc: admit two provably-leaf helpers, and measure that it buys nothing (#7369) * fix(lint): split index_set.rs, over the 2000-line cap since #7342 (#7366) `scripts/check_file_size.sh` exits 1 on main HEAD: `crates/perry-codegen/src/expr/index_set.rs` is 2035 lines against a 2000 cap. It crossed in #7342. That script runs inside the `lint` job, which is a REQUIRED context -- so this is the second independent way `lint` was red on main today (the first was rustfmt on linker.rs, #7361). A required check that is red on main blocks nothing; it means every merge is a bypass. The split follows the recipe in the script's own failure message: extract a topical group into a sibling module. `lower_inline_dyn_typed_array_set` and its `emit_inline_ta_int_store` helper are the guarded inline typed-array store for a type-erased receiver -- one coherent unit, moved verbatim to `index_set_typed_array.rs`. index_set.rs drops to 1749 lines, leaving real headroom rather than landing one line under the cap. Mechanical move: the two functions are byte-identical, only the imports they need travelled with them and `lower_inline_dyn_typed_array_set` became `pub(super)` so its one caller can still reach it. cargo test -p perry-codegen --lib: 609 passed. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * docs(plan): both statepoint adoption gates are closed; record the platform matrix (#7367) The plan still said statepoints were aarch64-only (#7321), that the matrix "therefore runs on macos-14", that `statepoints-refuse-x86` pinned the refusal, and it spelled the knob `PERRY_STATEPOINTS` four times. None of that is true now, and this document is what the adoption decision gets made from. What actually changed: - x86-64 is unblocked. `_Unwind_GetGR(ctx, 7)` does segfault and cannot be fixed as stated -- libgcc tracks only the columns CFI restores and RSP is derived, not tracked. #7349 stopped asking for it and derives the SP-relative base from `_Unwind_GetCFA`, with a per-arch return-address adjustment (x86-64 `call` pushes one, aarch64 `bl` does not). x86-64 Linux is a first-class arm. - Windows works via RtlVirtualUnwind (#7355), the one walker with no Itanium unwinder beneath it. - aarch64+ELF is now covered too (#7360) -- the only shape where LLVM spells 32-bit stack-map fields `.word`. - One mechanism, not two: PERRY_STATEPOINTS and the plain-map bridge are deleted, so the kill-policy line about "a mode that still exists" no longer applies to this pair. - The gate proves something now. Until today the Unix arms reported 7 frames and ZERO locations -- they would have passed with a walker that visited nothing. #7359's deep-collect probe took them to 221 locations. - watchOS/visionOS are not blocked by Perry: they build on stable without `dyn-eval`, and fail three crates away in psm's Mach-O guard. So the remaining adoption gate is `llvm-inprocess` becoming a default cargo feature, plus sequencing step 2 (root density) -- adopting today would regress binary size on root-dense code. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF Co-authored-by: Ralph Küpper <ralph@skelpo.com> * gc: admit two provably-leaf helpers, and measure that it buys nothing js_gc_register_global_root was the most frequent non-leaf callee in the probe suite (148 call sites) and is provably GC-leaf: its whole body is runtime_write_barrier_root_heap_word -- which js_write_barrier_root_heap_word, already CannotCollect, wraps in one line -- plus a TLS Vec::push. The "malloc count threshold" trigger does not apply to that push: the counter is MALLOC_STATE.objects.len(), a registry of Perry GC objects, and the #[global_allocator] is plain mimalloc/System with no GC hook. js_typed_feedback_maybe_dump_trace joins its already-admitted family siblings. Measured A/B on the same tree, and the result is a null: probe safepoints roots total bytes __text 06_string_retention 105 -> 100 27=27 0 -4 B 09_try_catch_roots 343 -> 339 259=259 0 -4 B 11_collect_at_depth 120 -> 117 36=36 0 -4 B Root counts are IDENTICAL. The 40 safepoints removed across the suite were all rootless, and a rootless safepoint costs essentially nothing -- which is what docs/engine-plan.md already says: "the axis is not 'statepoints are bigger', it is 'roots are bigger'". Recording it as evidence: the safepoint-count lever is not the binary-size lever, so sequencing step 2 must attack live-root SETS. Two tests come with it. One pins the wrapper's classification to the barrier it wraps. The other pins js_nanbox_string OUT of the allowlist: at 120 call sites it is the obvious next candidate and reads as pure bit manipulation, but its null guard calls js_string_from_bytes to allocate an empty string. Probe suite 11/11 byte-identical under forced evacuation + verification. Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com> * fix(ci): report assertion pinned to a probe Windows cannot compile Three review fixes on #7368. The report assertion ran on 09_try_catch_roots, which contains four `try` blocks. RS4GC cannot rewrite WinEH funclet pads, so linker.rs's rs4gc_funclet_refusal rejects that probe on windows-msvc -- the probe loop above tolerates it by grepping the compile log for "funclet", but this step did not. A gate pinned to a probe that cannot compile on one arm fails for a reason unrelated to its subject. The portable assertion now uses 11_collect_at_depth (no `try`, compiles on all four arms); 09_try_catch_roots keeps its own non-Windows step so the try-specific coverage that justified deleting the bridge is not lost. The gc_map doc claimed records/roots would be ABSENT when unmeasured. They are plain u64 fields on a plain derive and always serialise; `modules` is the sentinel. Fixed to describe what the code actually does -- the same class of comment-vs-code drift this PR exists to clean up. The "map never reported" guard fired for any --require-*/--print, including fields that live in `totals` and are counted at IR-emission time whether or not the rewrite ran. Now scoped to map-backed fields: --require-positive textual_calls is answered from its measured value (verified exit 0) while --require-positive records still fails on an unreported map (exit 1). Claude-Session: https://claude.ai/code/session_01EaD6yNwoinzdW1JbYNkMMF --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
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
1 participant
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.
cargo fmt --all -- --checkexits 1 on main HEAD (7428437e4). One file —crates/perry-codegen/src/linker.rs, wherers4gc_funclet_refusal's needle array crossed the width at which rustfmt breaks it vertically. It came in with the #7355 Windows-walker merge.lintis a required context. A required check that is red onmaindoesn't block anything — it means every subsequent merge is a bypass. That is the hazard CLAUDE.md lists, and this repo has already paid for it once: lint sat red across three nightlies on 16 oversized files, and every merge in that window went in unchecked.Whitespace only, no behavior change. Worth merging ahead of anything else in flight so the gate is load-bearing again.
Summary by CodeRabbit