Repository navigation
jit: recognize shared-reference niche Option<&T> with a thin ADT pointee (gh#346) - #998
Conversation
|
Warning Review limit reached
Next review available in: 33 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe MIR folding logic now recognizes bigint and list container item-view adapters in raw-slice operations. Niche detection also supports thin shared references to nominal ADTs while excluding fat and mutable references. Regression tests cover both behaviors. ChangesFolding recognizers
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a49e3ac9d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || self | ||
| .graph | ||
| .name | ||
| .ends_with("listobject::<Impl>::object_items_slice") |
There was a problem hiding this comment.
When an object-strategy list has spare capacity, such as after pop, this fold discards the self.length passed to from_raw_parts in pyre/pyre-object/src/listobject.rs:144-146 and retains only the ItemsBlock header, whose array descriptor reads ItemsBlock.capacity (object_array.rs:100-124). Lowered consumers such as w_list_getitem use items.len() for bounds checking, so an index between the logical length and capacity can be accepted and return a cleared PY_NULL slot instead of reporting out of bounds; preserve the explicit slice length rather than aliasing the entire slice to the backing array.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
| // `getdebug_data() -> Option<&FrameDebugData>`. | ||
| if let Some(shared_pointee) = type_node_shared_ref_pointee(payload, self.llbc) | ||
| && let Some(stripped) = strip_ty_wrappers(shared_pointee, self.llbc) | ||
| && adt_node_def_id(stripped).is_some() |
There was a problem hiding this comment.
An ADT def-id does not imply that its instantiated type is Sized: a concrete current example is rustpython_wtf8::Wtf8, an unsized nominal ADT backed by [u8]. Consequently the live str_slice_args return type Option<&Wtf8> (pyre/pyre-interpreter/src/type_methods.rs:1239-1275) passes this predicate even though its payload is a two-word fat reference whose metadata carries the substring length; folding its construction, discriminant, and __pos_0 access to one pointer discards that metadata and can give bounded startswith/endswith operations the wrong slice extent. Check the resolved layout or Sized bounds rather than treating every ADT node as thin.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@majit/majit-translate/src/front/mir.rs`:
- Around line 11480-11494: In
majit/majit-translate/src/front/mir.rs:11480-11494, update
tyref_is_niche_option_ptr’s shared-reference gate to accept only thin, Sized
nominal ADT pointees, rejecting unsized nominal ADTs and other fat references
before payload aliasing. In majit/majit-translate/src/front/mir.rs:23484-23538,
extend shared_ref_sized_gate_accepts_adt_rejects_slice with an unsized nominal
ADT case, alongside the existing Slice and sized ADT coverage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 063b9f14-6ded-4ae9-b756-6d9b7fe3f478
📒 Files selected for processing (1)
majit/majit-translate/src/front/mir.rs
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit d9e749e). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
7267c4a to
0202483
Compare
|
Both review findings were verified against source + the built LLBC corpus and fixed (2 commits pushed). P2 — P1 — list logical length lost (Codex Verification: — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0202483c51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .and_then(|td| td.layout_for_target(&std::env::var("TARGET").unwrap_or_default())) | ||
| .and_then(|l| l.size) | ||
| .is_some() |
There was a problem hiding this comment.
When lowering the registered pyre_interpreter::pyframe::npure_cellvars, the direct code.cellvars.iter() loop in pyframe.rs:1905-1911 produces Option<&String>. Since String is a sized ADT with a concrete layout, this new arm converts its MIR discriminant into a pointer-null test, but front/iter_next.rs:413-426 only recognizes a literal __discriminant FieldRead; the rewrite therefore declines and leaves the unregistered Iterator::next call, dropping this graph from the prepass/JIT. This is already a current registered route (jit_fnaddr.rs:2439-2444), so either teach iter_next the niche-null-test shape or exclude iterator-next results from this fold.
Useful? React with 👍 / 👎.
tyref_is_niche_option_ptr accepted only Option<&mut T> / Option<NonNull<T>>; a shared Option<&T> over a Sized ADT pointee is equally a one-word null niche, so its Some payload aliases the base pointer instead of an aggregate __pos_0 FieldRead. Fat shared refs (&str/&[T]/&dyn) are excluded by the ADT-def-id gate. Assisted-by: Claude
object_items_slice builds its slice with from_raw_parts(base, self.length) —
the logical list length, which is < capacity for an over-allocated block (e.g.
after pop). Aliasing it to the ItemsBlock header makes items.len() report the
gcarray descr capacity, so w_list_getitem accepts an index in [length, capacity)
and returns a stale slot instead of raising. Drop the two object_items_slice{,_mut}
arms from is_container_items_view_from_raw_parts; rbigint digits{,_mut} stays
(from_raw_parts(base, capacity) — length equals capacity, header alias sound).
Assisted-by: Claude
adt_node_def_id(stripped).is_some() does not imply the pointee is thin: an
unsized nominal newtype (a DST such as rustpython_wtf8::Wtf8 { bytes: [u8] })
is a nominal ADT with a def-id, but &Wtf8 is a two-word fat pointer whose
metadata carries the length. str_slice_args returns Option<&Wtf8> (matched by
startswith/endswith), so folding it to one word dropped the length and gave the
wrong substring extent. Require the pointee's resolved layout size to be
concrete (Some); size==null is the DST signature. Every W_*/FrameDebugData
niche win carries a concrete size, so the fold is unchanged for them.
Assisted-by: Claude
`Option::<&PyObjectRef>::copied()` / `.cloned()` lowered to a residual method call whose classdef-less `Option` receiver the rtyper cannot route, leaving a `getattr(opt, "copied")` wall in every graph reaching it (`args.first().copied()`, `split_builtin_kwargs`'s `positional.first().copied()`). `Option<&T>` and `Option<T>` share the front value model (niche discriminant + one pointer word), and the front materializes a `.first()` payload as the element value, not a slot pointer, so `.copied()` / `.cloned()` reads the value the payload already holds — an identity on the receiver. Bind the destination to the receiver instead, the same alias shape as `is_to_string_identity` / `nonnull_new_identity_alias`. `is_option_copied_identity` gates on the callee leaf being `copied`/`cloned` and both the receiver and destination being `Option`, so a slice/iterator `.copied()` or a `Vec::clone` keeps its ordinary lowering. Assisted-by: Claude
0202483 to
d9e749e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9e749e8d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// unreached: every `.iter()` for-loop that reaches iter_next iterates a | ||
| /// raw `*mut PyObject` / tuple / int element (none a thin nominal ADT), so | ||
| /// the shared arm never fires on a `next()` result — verified by a clean | ||
| /// census (0 `slice::iter::Iter::next` heads) and 370/370 check.py. The |
There was a problem hiding this comment.
This path is already reached: fresh evidence in the current tree is the registered pyre_interpreter::pyframe::npure_cellvars route (pyre/pyre-interpreter/src/jit_fnaddr.rs:2439-2444), whose loops over code.cellvars.iter() and code.varnames.iter() produce Option<&String> (pyframe.rs:1905-1915). Because String is a sized ADT, this fold replaces the discriminant with a null-test, while front/iter_next.rs:413-426 still accepts only a literal __discriminant field read; consequently the Iterator::next rewrite declines and this registered graph falls out of the prepass. Exclude recorded iterator-next results from this fold or teach iter_next the null-test shape before enabling it.
Useful? React with 👍 / 👎.
Summary
Two JIT front-end slices for the gh#346 two-phase-rtyper census campaign (
rtyper-legacy), both stacked on currentmain(#993).1.
jit: fold object_items_slice from_raw_parts to the items-block base(c5324700590)Ports the
object_items_slicefrom_raw_partsitems-block-base fold onto this branch (prior task, previously reviewed).2.
jit: recognize shared-reference niche Option<&T> with a thin ADT pointee(3a49e3ac9df)tyref_is_niche_option_ptrpreviously accepted onlyOption<&mut T>/Option<NonNull<T>>. A sharedOption<&T>over a Sized ADT pointee is equally a one-word null niche (Rust guarantees&Tnon-null, identically to&mut T), so itsSomepayload should alias the base pointer rather than emit an aggregate__pos_0FieldReadthat the annotator cannot resolve. This widening lifts the read-only&selfinterpreter accessors (__pyre_wrap_*getset/dunder wrappers viafrom_obj(...).map(|m| &*m), andgetdebug_data() -> Option<&FrameDebugData>combinator getters) that were blocking the prepass.Fat shared refs (
&str/&[T]/&dyn/&<?Sized>) are excluded by theadt_node_def_idSized gate — their two-word layout has no one-word niche payload, and folding them would read the metadata word. One recognizer edit flips all 12 niche call sites uniformly.Front-only change (
majit/majit-translate/src/front/mir.rs), so no LLBC re-extract.Evidence
1213 → 1187(−26 real drop).__pos_0"Blocked block" heads144 → 5(the 5 residual are out-of-scope genuine two-wordOption::Some).collected 207 source pathsunchanged (no truncation); no new failure symbol class.check.py: dynasm 370/370, cranelift 370/370 — zero correctness/SNAPDIFF mismatch on both backends.cargo test -p majit-translate --libgreen (synthetic Sized-gate test + two#[ignore]real-LLBC anchors: theget_w_localswin and theset__CHUNK_SIZE&mutregression, both pass on the current corpus).Reviewer note (documented, non-blocking)
A whole-branch review flagged that the shared-ref widening makes the
Rvalue::Discriminantfold emit a pointer-null bool switch forOption<&T>, which would defeatfront::iter_next's literal__discriminant-diamond matcher. This is latent and safe-failing: the worst case is a census Skip (graph falls to the legacy walker = correct execution, not a miscompile), and it is currently unreached — every.iter()for-loop reachingiter_nextiterates*mut PyObject/tuple/int (none a thin nominal ADT), confirmed by 0slice::iter::Iter::nextcensus heads and the clean check.py. The two governing doc comments (tyref_is_niche_option_ptrandtype_node_is_mut_ref) are updated to record the collision, its dormancy, and the prescribed follow-up (teachiter_nextthe null-test shape) for the day a thin-ADT.iter()loop appears; the per-slice census is the existing tripwire.🤖 Generated with Claude Code
Summary by CodeRabbit