test(codegen): match the alloca TYPE, not its whole def text (#7675 follow-up) - #7681
Conversation
An `align` suffix would have emptied `temp_root_slots` and made every `assert_no_temp_rooting` vacuous again — the exact failure this PR removes. Matched through one shared `root_slots::alloca_type` in all three places that compared whole def text, with a regression test each that fails against the old compare. Also routes the accumulator write-back check through `slot_holding` so it tolerates the same boxing step its neighbours do, and drops a redundant load-specific pass in `zero_seeded_slots` that re-inserted what the generic `, ptr %s` scan already caught. Raised by review on #7675. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
|
Warning Review limit reached
Next review available in: 35 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Audit — merging as v0.5.1386This is the fix that keeps #7675 from re-creating the defect it removed, so it should not have been left on overnight — my merge of #7675 landed while you were mid-fix, which is what stranded it. Sabotage-verified independently: reverting The failure mode was worth landing on its own: Your two declines are both right, and the first is the important one: broadening Two process findings I'm keeping, and both are mine to own:
Gates: 22/22 lint, fmt clean, |
Follow-up to #7675, which was squash-merged while these review fixes were being applied. Three findings from the review on that PR; the first is the same defect class #7675 exists to remove, so it should not sit unlanded.
The
alignsuffix would have made every negative gate vacuous againtesting::temp_slots::temp_root_slotsfiltered slots by comparing the alloca's whole def text:One
align 8away from matching nothing. And the failure mode is the silent one: the filter empties the result,temp_root_slotsreturns[], and everyassert_no_temp_rootingin the tree passes for a program that roots — exactly the vacuity #7503 was opened about. The positives would have gone red, so the risk was bounded today; the negatives would have lost their meaning without warning.Now matched by alloca type, through one shared
root_slots::alloca_type, applied in all three places that were comparing whole def text:temp_slots::temp_root_slots(the silent one),root_slots::classify(loud — an unmatched type hits the panic arm — but it would have panicked on a spelling that is perfectly valid), androot_slots::value_slot_barriers.Sabotage, run. With
alloca_typetemporarily replaced by the old exact-text compare, both new tests fail and nothing else does:Both tests assert their own substitution actually applied (
assert_ne!(aligned, MIXED)), so neither can pass by testing an unmodified fixture.The write-back check was the one clause not using the shared derivation rule
temp_root_coverage's accumulator test required the store-back value to equal the push result verbatim, while every neighbouring clause goes throughslot_holding, which deliberately tolerates one boxing step — and a raw allocation result is NaN-boxed before it reaches a slot. A lowering that boxed the push result would have failed a test whose contract still held. Nowslot_holding(&ir, &push).as_deref() == Some(slot.as_str()).A redundant pass that read as coverage it did not add
zero_seeded_slotshad a load-specific second pass over threeload …, ptrprefixes. Verified redundant: the generic, ptr %sscan above it already catches%d = load i64, ptr %sand both other spellings. Removed, with a comment stating why there is deliberately no load-specific pass.Declined, with reasons
derives_fromto follow every operand. It backs the positive clauses ("this consumer's operand WAS re-read from the slot"), so a breadth-first walk makes that claim easier to satisfy and could let a genuinely unrooted operand pass. The current narrow walk errs toward a loud false alarm. On this of all subjects, trading a false alarm for a possible silent pass is the wrong direction; if it ever fires in practice the fix is to name the operand index at the call site.entry_opts/module_with_init. Fair, but there are already ~6 copies acrosscrates/perry-codegen/tests/. Deduplicating 2 of 6 does not remove the "add every newCompileOptionsfield twice" hazard, and doing all 6 is a repo-wide refactor.Verification
cargo fmt --all -- --checkclean.cargo test -p perry-codegen --lib778 passed (776 + the two new regression tests);--test temp_root_operand_temporaries19/19,--test scalar_replaced_slot_roots11/11,--test shadow_slot_hygiene12/12../scripts/check_file_size.shOK.Test-only: every changed file is a
#[cfg(test)]/feature = "testing"module, so the shipped compiler is unchanged.