fix(gates): restore addr-class lint, broken on main by #7579 - #7585
Merged
Conversation
#7579 added a bare `as *mut GcHeader` cast in iter_result.rs to stamp the shared keys array copy-on-write. addr_class_inventory.py refuses that outside gc/ and value/addr_class.rs, so `lint` has been red on main and on every open PR since it merged. Rather than add a 127th allowlist entry, move the cast into gc/ behind `mark_shape_shared`. Every grandfathered entry in the allowlist carries the same promise -- 'migrate to a helper in a follow-up' -- and this is that helper for the one thing those sites actually do: set a flag. addr_class::try_read_gc_header cannot serve them, and that is by design rather than an oversight: it returns `&'static GcHeader`, a SHARED reference, precisely so a probe of an untrusted address can never write through it. A flag write needs `*mut`, hence a narrower entry point with a stronger precondition. Also drops one stale addr-class ratchet entry the tool reported as over-counted (field_get_set.rs lone-valid-obj-ptr: baseline 1, found 0).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe runtime adds ChangesShared-shape GC metadata
Estimated code review effort: 2 (Simple) | ~10 minutes 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 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
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.
linthas been red onmain— and therefore on every open PR — since #7579 merged. My fault: I audited that PR againstraw_handle_debt,check_file_sizeandcargo fmt, but notaddr_class_inventory.py, which CLAUDE.md lists as a gate. Found by an agent working an unrelated issue in the same file.The fix, and why not an allowlist entry
The obvious repair is a 127th entry in
scripts/addr_class_allowlist.txt. I didn't take it. Every one of the 126 grandfathered entries carries the same sentence — "migrate toaddr_class::try_read_gc_headerin a follow-up" — so adding another is borrowing against a debt nobody is paying down.The reason none of them has migrated is structural, not neglect:
try_read_gc_headerreturns&'static GcHeader, a shared reference. That is deliberate — it is the safe probe for an address that might be a handle, so it must not be able to write. These call sites all want to set a flag. No amount of follow-up work makes a shared reference serve a*mutwrite; they needed a second, narrower entry point that never existed.So this adds it:
gc::mark_shape_shared, ingc/where the cast is permitted, with the precondition stated — the pointer must come from allocation this thread just performed, never from a NaN-box payload, which is the same discipline the arena walkers are already allowlisted under.iter_result.rscalls it and the cast is gone. The allowlist does not grow, and the other sites now have a real migration target.Also
Drops one stale ratchet entry the tool itself reported as over-counted (
field_get_set.rslone-valid-obj-ptr: baseline says 1, found 0). Same family as #7582 — a suppression that outlived its subject.Validation
addr_class_inventory.pypasses (885 files, 267 allowlisted, 542 sites held by the ratchet).cargo test -p perry-runtime --lib: 1818 passed, 0 failed, includingshared_iter_result_keys_are_marked_copy_on_write, which is the test that guards this exact flag write.raw_handle_debt998 (baseline 998),check_file_size.shclean,cargo fmt --checkclean.CI has a deep backlog and may not report; this is local validation.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores
0.5.1328to0.5.1329.