#346: per-instantiation Option/Result classdefs + compute_mro residualization - #429
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (7)
WalkthroughThis PR threads per-instantiation type suffixes through enum and tuple owner name construction in MIR lowering, closure-select, and map_or rewriting so distinct ChangesEnum/Tuple Instantiation Suffix Threading
MRO Tracing Opacity Boundaries
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 1dfca47). 1. 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
|
![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: a4f458290b
ℹ️ 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".
| if reg.is_enum_base(&lookup) { | ||
| let stripped = majit_ir::descr::strip_generic_args(&lookup); | ||
| let bare_leaf = stripped.rsplit("::").next().unwrap_or(&stripped); | ||
| if bare_leaf != lookup.as_str() && !seen.contains(bare_leaf) { |
There was a problem hiding this comment.
When the prologue visits a qualified enum root such as core::option::Option before the bare alias, this branch returns Option as its base even though canonical_struct_name("Option") resolves back to the same core::option::Option cache key. The mint loop below does not de-duplicate chain entries, so it first inserts the bare-root class and then overwrites the same key with a second class whose base is the phantom first one, making enum ClassDef identity and inheritance numbering depend on HashMap iteration order. Please compare canonical keys (or restrict this to generic/dotted spellings) before adding the bare-leaf base.
Useful? React with 👍 / 👎.
…ion combinators Subclass a generic enum instantiation (`Option<X>`, `Result<T,E>`) and a collapsed non-suffixed spelling of the same enum (`core.option.Option`) under the bare enum leaf in intern_class_by_qualname, so two sibling instantiations and a collapsed value share a base and commonbase to it at a phi merge. Suffix the tagged-pair enum root (emit_tagged_pair_aggregate, checked_neg / usize::try_from) with the destination `Option<X>`/`Result<X,E>`'s `<X>` via tyref_enum_instantiation_suffix, so a runtime-discriminant construction mints the same per-instantiation root a static `Some(..)`/`Ok(..)` mints. Suffix the enum root in recognize_bool_then_site / recognize_map_or_site / recognize_closure_select_site from the site's dest/recv `Option<X>`. Suffix the synthesized closure-`Args` tuple `(payload,)` in option_map_or and option_closure_select with option_payload_tuple_suffix, which wraps the receiver Option's payload node in a Tuple and routes it through tyref_tuple_suffix — the same renderer the extracted call_once reads `.0` under at resolve_place — so the `__pos_0` write and read key under one classdef. A niladic closure's `()` tuple stays bare Tuple. Census: cannot unify with no common base 52->10 (Option family 42->0, PyObject union FrameDebugData 4->0). dynasm/cranelift check.py 183/183. Assisted-by: Claude
…t_look_inside `compute_mro` (C3 linearization) starts with `vec![w_type]`, which lowers to the foreign non-scalar `box_assume_init_into_vec_unsafe` alloc intrinsic that has no annotator graph; the self-recursive C3 walk therefore blocked every numeric/unary and lookup-family graph funneling through the cold uncached `lookup_where` branch. Annotate `compute_mro` and the scalar-Option boundary `lookup_in_type_where_uncached` `#[dont_look_inside]` so the walk residualizes, mirroring MRO computation opaque / MRO iteration traced (compute_C3_mro behind `__init__` dont_look_inside, typeobject.py:199/1687). Bind the residual fnaddrs for compute_mro, its wrapper compute_default_mro, and lookup_in_type_where_uncached (siblings of the note_alloc / try_gc_charge residuals). Correct the union-fallback skip comment in cutover.rs: upstream RAISES for the pairs reaching it (pair(SomeObject,SomeObject).union binaryop.py:90-93, pair(SomePtr,SomeObject) llannotation.py:118-120), so the skip marks a pyre producer divergence (a boxed pointer lifted as SomePtr where RPython carries SomeInstance), not a missing union handler. Census: cat2 compute_at_fixpoint 165->138. dynasm/cranelift check.py 183/183. Assisted-by: Claude
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e169c1ae33
ℹ️ 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".
| let option_owner = format!( | ||
| "{}{}", | ||
| td.item_meta.name_path(), | ||
| tyref_enum_instantiation_suffix(recv_ty, self.llbc) | ||
| ); |
There was a problem hiding this comment.
For Option<T>::map and and_then where the closure returns U/Option<U> with U != T, this suffix is derived from the receiver type and then reused both to read opt.__pos_0 and to construct the result Some/None in rewire_one_closure_select_site. That makes map write the U payload into the Option<T>::Some class and makes and_then build the None arm as Option<T>, while the actual result type is Option<U>. The pass now needs separate receiver owners from recv_ty and result owners from dest_ty for the combinators that return an Option.
Useful? React with 👍 / 👎.
…lization (#429) * #346: per-instantiation enum root + closure-Args Tuple suffix for Option combinators Subclass a generic enum instantiation (`Option<X>`, `Result<T,E>`) and a collapsed non-suffixed spelling of the same enum (`core.option.Option`) under the bare enum leaf in intern_class_by_qualname, so two sibling instantiations and a collapsed value share a base and commonbase to it at a phi merge. Suffix the tagged-pair enum root (emit_tagged_pair_aggregate, checked_neg / usize::try_from) with the destination `Option<X>`/`Result<X,E>`'s `<X>` via tyref_enum_instantiation_suffix, so a runtime-discriminant construction mints the same per-instantiation root a static `Some(..)`/`Ok(..)` mints. Suffix the enum root in recognize_bool_then_site / recognize_map_or_site / recognize_closure_select_site from the site's dest/recv `Option<X>`. Suffix the synthesized closure-`Args` tuple `(payload,)` in option_map_or and option_closure_select with option_payload_tuple_suffix, which wraps the receiver Option's payload node in a Tuple and routes it through tyref_tuple_suffix — the same renderer the extracted call_once reads `.0` under at resolve_place — so the `__pos_0` write and read key under one classdef. A niladic closure's `()` tuple stays bare Tuple. Census: cannot unify with no common base 52->10 (Option family 42->0, PyObject union FrameDebugData 4->0). dynasm/cranelift check.py 183/183. Assisted-by: Claude * #346: residualize compute_mro / lookup_in_type_where_uncached via dont_look_inside `compute_mro` (C3 linearization) starts with `vec![w_type]`, which lowers to the foreign non-scalar `box_assume_init_into_vec_unsafe` alloc intrinsic that has no annotator graph; the self-recursive C3 walk therefore blocked every numeric/unary and lookup-family graph funneling through the cold uncached `lookup_where` branch. Annotate `compute_mro` and the scalar-Option boundary `lookup_in_type_where_uncached` `#[dont_look_inside]` so the walk residualizes, mirroring MRO computation opaque / MRO iteration traced (compute_C3_mro behind `__init__` dont_look_inside, typeobject.py:199/1687). Bind the residual fnaddrs for compute_mro, its wrapper compute_default_mro, and lookup_in_type_where_uncached (siblings of the note_alloc / try_gc_charge residuals). Correct the union-fallback skip comment in cutover.rs: upstream RAISES for the pairs reaching it (pair(SomeObject,SomeObject).union binaryop.py:90-93, pair(SomePtr,SomeObject) llannotation.py:118-120), so the skip marks a pyre producer divergence (a boxed pointer lifted as SomePtr where RPython carries SomeInstance), not a missing union handler. Census: cat2 compute_at_fixpoint 165->138. dynasm/cranelift check.py 183/183. Assisted-by: Claude * #346: cargo fmt jit_fnaddr lookup_in_type_where_uncached binding Assisted-by: Claude
1 participant
Two follow-on #346 slices toward retiring the rtyper legacy walker: per-instantiation classdef specialization for the
Option/Resultcombinator front passes, and residualizing thecompute_mroC3 walk so the numeric/unary and lookup families stop failing the two-phase prepass on the foreignvec!alloc intrinsic.Commits
per-instantiation enum root + closure-Args Tuple suffix for Option combinators
A generic enum instantiation (
Option<X>,Result<T,E>) and a collapsed non-suffixed spelling of the same enum (core.option.Option) were interned as distinct classdefs with no shared base, socommonbasereturnedNoneat a phi merge. Separately, thebool::then/map_or/map/and_then/unwrap_or_elsefront passes minted a bareTuplefor the closureArgs, whose__pos_0unioned every closure payload in the program onto one classdef (PyObject ∪ FrameDebugData).intern_class_by_qualname(discriminant-only root, no payload attr to conflict on).emit_tagged_pair_aggregate, thechecked_neg/usize::try_fromruntime-discriminant lowering) with the destinationOption<X>/Result<X,E>'s<X>.recognize_bool_then_site/recognize_map_or_site/recognize_closure_select_sitefrom the site's dest/recvOption<X>; suffix the synthesized closure-Argstuple inoption_map_or/option_closure_selectviaoption_payload_tuple_suffix, which routes the receiver Option's payload node throughtyref_tuple_suffix— the same renderer the extractedcall_oncereads.0under atresolve_place— so the__pos_0write and read key under one classdef. A niladic closure's()tuple stays bareTuple.Effect:
cannot unify instances with no common base class52→10 (theOption<…> ∪ core.option.Optionfamily andPyObject ∪ FrameDebugDatago to zero).residualize compute_mro / lookup_in_type_where_uncached via dont_look_inside
compute_mro(C3 linearization) starts withvec![w_type], which lowers to the foreign non-scalarbox_assume_init_into_vec_unsafealloc intrinsic that has no annotator graph; the self-recursive C3 walk blocked every numeric/unary and lookup-family graph funneling through the cold uncachedlookup_wherebranch.compute_mroand the scalar-Option boundarylookup_in_type_where_uncached#[dont_look_inside]so the walk residualizes — MRO computation opaque, MRO iteration traced (compute_C3_mrobehind__init__dont_look_inside, typeobject.py:199/1687).compute_mro, its wrappercompute_default_mro, andlookup_in_type_where_uncached(siblings of thenote_alloc/try_gc_chargeresiduals).cutover.rs: upstream RAISES for the pairs reaching it (pair(SomeObject,SomeObject).unionbinaryop.py:90-93,pair(SomePtr,SomeObject)llannotation.py:118-120), so the skip marks a pyre producer divergence (a boxed pointer lifted asSomePtrwhere RPython carriesSomeInstance), not a missing union handler.Effect:
compute_at_fixpointprepass failures 165→138.Verification
check.pydynasm 183/183 + cranelift 183/183 (the JIT paths that consume the new classdef owners / residual fnaddrs). The four wasm-backendcheck.pyfailures (inline_callee_constructs_object,inlined_helper_mutation,inlined_mutation_before_abort,sre_pattern_methods) reproduce identically on the base commit — pre-existing, independent of these changes.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes