#346: resolve dot-joined constructor base, residualize w_long_new, name colliding classdefs - #411
Conversation
|
Warning Review limit reached
Next review available in: 35 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 (4)
WalkthroughThis PR normalizes dot-joined constructor qualnames to ChangesType Annotator Fixes
JIT w_long_new Tracing and Alias Registration
Estimated code review effort: 2 (Simple) | ~12 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 bcf6ed6). 1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
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
https://github.com/youknowone/pyre/blob/3f098d88df4871ccdca811357984e6f20d5e4fe5/pyre-object/src/longobject.rs#L273-L274
When a traced long path reaches this dont_look_inside call, the codewriter will residualize the registered w_long_new fnaddr and pass the opaque BigInt value through the normal i/r/f residual-call registers. The argument modeled for these bigint paths is a ref/pointer (ValueType::Ref(None) / *mut BigInt from the jit_bigint_* helpers), but this callee's Rust ABI still expects an owned BigInt aggregate by value, so the residual call will interpret a pointer-sized register as a BigInt and can corrupt or crash on bigint allocation. Please residualize a pointer-shaped wrapper such as w_long_from_raw(*mut BigInt) instead of this by-value function.
ℹ️ 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".
![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/annotator/model.rs`:
- Around line 3167-3188: The Instance and WeakRef union branches duplicate the
same commonbase failure pattern, so extract that logic into a shared helper used
by the union handling around the Instance and WeakRef cases in
annotator/model.rs. Add a helper that takes the two ClassDef refs, the kind
string (“instances”/“weakrefs”), and the lhs/rhs values, then calls
ClassDef::commonbase and returns either the base or a UnionError with the
appropriate message. Update both union arms to use this helper so the error
formatting stays consistent and the duplicated match/format! logic is removed.
🪄 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
Run ID: 4bcdd0f9-610c-497a-8c1d-ec1e58c89ab5
📒 Files selected for processing (4)
majit/majit-translate/src/annotator/bookkeeper.rsmajit/majit-translate/src/annotator/model.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-object/src/longobject.rs
| (SomeValue::WeakRef(a), SomeValue::WeakRef(b)) => { | ||
| let merged_classdef = match (&a.classdef, &b.classdef) { | ||
| (None, _) | (_, None) => Some(None), | ||
| (Some(ca), Some(cb)) => ClassDef::commonbase(ca, cb).map(Some), | ||
| }; | ||
| let Some(merged_classdef) = merged_classdef else { | ||
| return Err(UnionError { | ||
| lhs: s1.clone(), | ||
| rhs: s2.clone(), | ||
| msg: "RPython cannot unify weakrefs with no common base class".into(), | ||
| }); | ||
| }; | ||
| let merged_classdef: Option<Rc<RefCell<ClassDef>>> = | ||
| match (&a.classdef, &b.classdef) { | ||
| (None, _) | (_, None) => None, | ||
| (Some(ca), Some(cb)) => match ClassDef::commonbase(ca, cb) { | ||
| Some(base) => Some(base), | ||
| None => { | ||
| return Err(UnionError { | ||
| lhs: s1.clone(), | ||
| rhs: s2.clone(), | ||
| msg: format!( | ||
| "RPython cannot unify weakrefs with no \ | ||
| common base class: {} ∪ {}", | ||
| ca.borrow().name, | ||
| cb.borrow().name | ||
| ), | ||
| }); | ||
| } | ||
| }, | ||
| }; | ||
| Ok(SomeValue::WeakRef(SomeWeakRef::new(merged_classdef))) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider extracting a shared helper for the Instance/WeakRef commonbase-error pattern.
Both this block and the Instance-union block (lines 2980-3005) repeat the same match (Some(ca), Some(cb)) => match commonbase(...) { Some => ..., None => Err(UnionError{ ... format!(...) }) } shape, differing only in the "instances"/"weakrefs" wording. A small helper (e.g. fn commonbase_or_union_err(ca, cb, kind: &str, s1, s2) -> Result<Rc<RefCell<ClassDef>>, UnionError>) would remove the duplication and keep both error messages in sync going forward.
♻️ Sketch of a shared helper
+fn commonbase_or_err(
+ ca: &Rc<RefCell<ClassDef>>,
+ cb: &Rc<RefCell<ClassDef>>,
+ kind: &str,
+ s1: &SomeValue,
+ s2: &SomeValue,
+) -> Result<Rc<RefCell<ClassDef>>, UnionError> {
+ ClassDef::commonbase(ca, cb).ok_or_else(|| UnionError {
+ lhs: s1.clone(),
+ rhs: s2.clone(),
+ msg: format!(
+ "RPython cannot unify {kind} with no common base class: {} ∪ {}",
+ ca.borrow().name,
+ cb.borrow().name
+ ),
+ })
+}🤖 Prompt for 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.
In `@majit/majit-translate/src/annotator/model.rs` around lines 3167 - 3188, The
Instance and WeakRef union branches duplicate the same commonbase failure
pattern, so extract that logic into a shared helper used by the union handling
around the Instance and WeakRef cases in annotator/model.rs. Add a helper that
takes the two ClassDef refs, the kind string (“instances”/“weakrefs”), and the
lhs/rhs values, then calls ClassDef::commonbase and returns either the base or a
UnionError with the appropriate message. Update both union arms to use this
helper so the error formatting stays consistent and the duplicated match/format!
logic is removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef7be9db7
ℹ️ 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".
| &mut entries, | ||
| "pyre_object::longobject::w_long_new", | ||
| "pyre_object::w_long_new", | ||
| pyre_object::longobject::w_long_new as *const (), |
There was a problem hiding this comment.
When a traced long-overflow/math path residualizes w_long_new, the native residual dispatcher does not call Rust signatures directly: majit-backend/src/call_stub.rs collects only i64/f64 slots and transmutes the fnaddr to an extern "C" fn(i64, ...). This entry publishes fn(BigInt) -> PyObjectRef, whose by-value BigInt parameter is not a single C-ABI slot, so the generated call will enter w_long_new with the wrong ABI and can corrupt or crash. Please register an ABI shim that takes an encoded pointer/slot, or residualize a raw-pointer helper instead.
Useful? React with 👍 / 👎.
The SomeInstance and SomeWeakRef union arms raise UnionError when both sides carry a classdef and ClassDef::commonbase returns None. Append the two classdef names to the message so the skip-classified panic path is diagnosable. The "cannot unify instances with no common base class" / "cannot unify weakrefs with no common base class" phrases are preserved verbatim so the dual-gate skip classifier still matches. Flatten the SomeWeakRef merge to a single Option<classdef> match so its None-classdef success case and commonbase-None failure case read the same as the SomeInstance arm. Assisted-by: Claude
intern_class_by_qualname walks the embedded-header chain (`ob_header: PyObject`) to set a struct class's base, but its `struct_fields` lookup used the raw name. A constructor mints its class under the dot-joined, crate-included qualname (`pyre_object.intobject.W_IntObject`), while `struct_fields` is keyed by the `::` name_path and the bare leaf, so the lookup missed and the box class minted base-less. commonbase then returned None for W_IntObject ∪ W_LongObject / W_FloatObject / W_ObjectObject, raising UnionError at the numeric return-phi. Reduce `.`→`::` for the header-chain registry lookups only; the classdef cache key keeps the raw spelling, so the base is seeded without collapsing the constructor class onto the `::`-spelled field-read class. Assisted-by: Claude
w_long_new delegates through alloc_bigint_stable -> *mut BigInt, so the annotator traced into it leaked the raw BigInt pointee into the return model. At the int boxing tail the slow/overflow arm then unified against the w_int_new fast path as PyObject ∪ BigInt, raising UnionError in mergeinputargs for bigint_result / int_lshift / int_floordiv. Mark w_long_new #[dont_look_inside] so it is modeled by its PyObjectRef signature (a plain GCREF, no discriminant to erase) rather than traced, and add its fnaddr alias-pair bind next to w_str_new. The BigInt by-value argument carries no i64 trampoline, so the bind is path-resolution completeness; the overflow tail was already interp-only slow path. Assisted-by: Claude
Assisted-by: Claude
1 participant
(untracked)
Follow-on slices to #405, reducing two-phase rtyper prepass lift failures by wiring classdef common bases and residualizing the numeric boxing tail. Toward gh#346 (retire the rtyper legacy walker): as classdef unification succeeds and boxing tails residualize, more graphs lift through the two-phase CodeWriter path and
cutover::is_known_unportedshrinks.Commits
UnionErrorarms (SomeInstance,SomeWeakRef) with the two colliding classdef names so the failure identifies the exact pair; the skip substring is preserved.SomeWeakRefmerge flattened to a singleOption<classdef>match.intern_class_by_qualname's header-chain walk resolves the enum-base check and field lookup through a::-normalized spelling while keeping the classdef cache key raw, so a boxed subtype and its base unify via their shared base instead of raising no-common-base.w_long_new#[dont_look_inside](with the fnaddr bind) so the annotator models it by signature (PyObjectRefreturn) instead of tracing into theBigIntboxing tail;alloc_bigint_stable/w_long_from_rawreturn*mut BigInt. Closes thePyObject ∪ BigIntcollision.Effect
Measured over these three slices: no-common-base
cannot unifyoccurrences 106→60, two-phase prepass phaseA failures 323→307.check.py3/3 bit-exact (dynasm / cranelift / wasm, 183/183 each).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes