front/rtyper: PyType-static ConstPyTypeAddr fold, rlist ListRepr foundation, sign-changing int casts, classdef-less pointer-method routing - #184
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds full ChangesJIT lowering pipeline, MIR call special-cases, and annotator union fix
Sequence Diagram(s)sequenceDiagram
participant MIR as MIR lower_call
participant ExtRegistry as ExtRegistryEntry
participant RBuiltin as rtype_we_are_jitted
participant FlowAdapter as flowspace_adapter
participant JTransform as jtransform rewrite_op_direct_call
participant ConstFold as replace_we_are_jitted
MIR->>MIR: is_jit_promote(x) → select hint_promote FunctionPath
FlowAdapter->>FlowAdapter: translate_op hint_promote → same_as(arg, result)
ExtRegistry->>ExtRegistry: lookup_host_object("we_are_jitted") → WeAreJitted
ExtRegistry->>RBuiltin: specialize_call → rtype_we_are_jitted emits SpecTag constant
ExtRegistry->>ExtRegistry: compute_annotation → SomeValue::Bool
JTransform->>JTransform: we_are_jitted call → ConstBool(true)
ConstFold->>ConstFold: _we_are_jitted symbolic → Bool(false) / LowLevelType::Bool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
youknowone
changed the title
0f4b27d to
42b7907
Compare
![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: 42b7907077
ℹ️ 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".
| .alloc_value_var_with_type(crate::model::ConcreteType::Unknown); | ||
| self.graph.block_mut(bb_id).operations.push(SpaceOperation { | ||
| result: Some(res.clone()), | ||
| kind: cast_pointer_marker_op(root, args[0].clone()), |
There was a problem hiding this comment.
When this arm sees FrameBox::deref(_mut) (and similarly Rc/Arc), args[0] is the wrapper value, not the pointee. The repo has hot call paths that create a FrameBox and then call PyFrame methods through Deref (for example new_frame.locals_w_mut() / run() in pyre-interpreter/src/function.rs); lowering those calls to cast_pointer(PyFrame, args[0]) skips the wrapper's ptr field (and for Rc/Arc, the control-block/data offset), so subsequent PyFrame field reads are based on the wrapper representation rather than the actual frame pointer. Please restrict this shortcut to truly transparent pointer values or read the inner pointer before casting.
Useful? React with 👍 / 👎.
42b7907 to
7e02ad8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e02ad8e20
ℹ️ 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".
| /// materialises it as a solid `_ptr` to the repr's `object_type` at | ||
| /// the host address, so `ptr::eq` / `is` against `ob_type` resolve | ||
| /// through `pair(InstanceRepr, InstanceRepr)`. | ||
| PyTypeAddr(i64), |
There was a problem hiding this comment.
When this new constant is introduced, ConstValue::PartialEq below does not add a PyTypeAddr arm, so even ConstValue::PyTypeAddr(x) == ConstValue::PyTypeAddr(x) falls through to _ => false while Hash does hash the address. Any path that sees two folded reads of the same PyType static as constants (for example constant maps/sets or frame-state equality during joins) will treat them as distinct and violates the Eq contract for this enum; please add the address comparison alongside the other value variants.
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
majit/majit-translate/src/translator/rtyper/rclass.rs (1)
345-355:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve null-address falsiness for
PyTypeAddr.Line 353 makes every
PyTypeAddrtruthy, but the flowspaceConstValuetruthiness usesaddr != 0. Keep this helper aligned soPyTypeAddr(0)cannot be classified as true if it reachesconst_truthy.🐛 Proposed fix
| ConstValue::LowLevelType(_) | ConstValue::LLPtr(_) | ConstValue::LLAddress(_) | ConstValue::AddressOffset(_) - | ConstValue::PyTypeAddr(_) | ConstValue::SpecTag(_) | ConstValue::HostObject(_) => true, + ConstValue::PyTypeAddr(addr) => *addr != 0,🤖 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/translator/rtyper/rclass.rs` around lines 345 - 355, The `PyTypeAddr` variant is being unconditionally classified as truthy in this match expression, but it should instead be treated like other address-bearing variants that need explicit null-address checking (where address 0 is falsy). Remove `ConstValue::PyTypeAddr(_)` from the pattern match block in the match statement (currently at line 353) that returns true, so that `PyTypeAddr` values can be properly evaluated based on whether their address is non-zero, aligning with the flowspace `ConstValue` truthiness semantics which uses `addr != 0`.majit/majit-translate/src/translator/rtyper/cutover.rs (1)
715-726: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider adding the new
noneify() not supportedpattern to the documentation table.The table documents all known-unported patterns, but the newly added
noneify() not supportedpattern (line 966) is missing. For maintainability, add a row such as:| `noneify() not supported` | Front-end typed null pointer lowering (Option<*T> → SomePtr). |🤖 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/translator/rtyper/cutover.rs` around lines 715 - 726, The documentation table in the file is missing an entry for the newly added noneify() not supported pattern (which appears at line 966). Add a new row to the markdown table that documents this pattern using the suggested format with the substring noneify() not supported and a description explaining it relates to front-end typed null pointer lowering for Option<*T> → SomePtr conversions. Place this new row in the appropriate position within the table to maintain logical grouping with other patterns.
🤖 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/bookkeeper.rs`:
- Around line 2591-2610: The ConstValue::PyTypeAddr(_) match arm in the
immutablevalue function lacks a direct regression test to verify its behavior.
Add a focused unit test that calls immutablevalue with a PyTypeAddr constant
value and asserts that it returns SomeValue::Instance with the const_box field
correctly populated with the original constant. The test should cover both the
success path (where project_pyre_field_type returns an Instance) and ideally the
error path (where it returns something else).
In `@majit/majit-translate/src/flowspace/model.rs`:
- Line 2260: The PyTypeAddr variant's Hash implementation hashes the i64
address, but the PartialEq implementation lacks a PyTypeAddr arm, causing two
PyTypeAddr instances with the same address to compare unequal and breaking
HashMap lookup and dedup for PyType keys. Add a PyTypeAddr arm to the PartialEq
implementation (around lines 2283-2312) that compares the inner i64 addresses
for equality, ensuring consistency between Hash and PartialEq behavior. Also
verify that any other related equality comparisons (around lines 2401-2401) are
updated if necessary to maintain this consistency throughout the codebase.
In `@majit/majit-translate/src/front/mir.rs`:
- Around line 3839-3849: The deref_cast_root function currently accepts any
deref or deref_mut implementation without validating the receiver type, which
could lead to incorrect pointer reinterpretation for custom Deref
implementations. Add an allowlist check within deref_cast_root to verify that
the receiver type (the ADT implementing Deref) is one of the known thin-pointer
wrappers: Box, Rc, Arc, or FrameBox. If the receiver type is not in this
allowlist, the function should return None, leaving other deref calls to be
handled through the normal call path.
In `@majit/majit-translate/src/translator/rtyper/cutover.rs`:
- Around line 954-966: Add a test case to validate that the new "noneify() not
supported" pattern is correctly identified as a known unported condition. Create
a new test function (similar in structure to existing tests like
`known_unported_classifies_indirect_call_adapter_invariant`) that calls the
`is_known_unported` function with a message containing "noneify() not supported"
and asserts that it returns true. This test should verify that the pattern added
to the skip condition in the comment block is properly handled by the unported
classification logic.
In `@majit/majit-translate/src/translator/rtyper/flowspace_adapter.rs`:
- Around line 1167-1174: The hint_promote operation now lowers to a non-raising
same_as flowspace op, but the op_canraise function still treats the
corresponding OpKind::Call as raising via the general Call handling path. Update
the op_canraise logic to recognize hint_promote calls and correctly classify
them as non-raising to match the actual lowering behavior, preventing spurious
exception edges from being added for a marker that produces no raising flowspace
operation.
---
Outside diff comments:
In `@majit/majit-translate/src/translator/rtyper/cutover.rs`:
- Around line 715-726: The documentation table in the file is missing an entry
for the newly added noneify() not supported pattern (which appears at line 966).
Add a new row to the markdown table that documents this pattern using the
suggested format with the substring noneify() not supported and a description
explaining it relates to front-end typed null pointer lowering for Option<*T> →
SomePtr conversions. Place this new row in the appropriate position within the
table to maintain logical grouping with other patterns.
In `@majit/majit-translate/src/translator/rtyper/rclass.rs`:
- Around line 345-355: The `PyTypeAddr` variant is being unconditionally
classified as truthy in this match expression, but it should instead be treated
like other address-bearing variants that need explicit null-address checking
(where address 0 is falsy). Remove `ConstValue::PyTypeAddr(_)` from the pattern
match block in the match statement (currently at line 353) that returns true, so
that `PyTypeAddr` values can be properly evaluated based on whether their
address is non-zero, aligning with the flowspace `ConstValue` truthiness
semantics which uses `addr != 0`.
🪄 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: f0047713-0c2c-4165-b7ba-579c8d15eb2d
📒 Files selected for processing (23)
majit/majit-translate/src/annotator/bookkeeper.rsmajit/majit-translate/src/annotator/builtin.rsmajit/majit-translate/src/annotator/model.rsmajit/majit-translate/src/flowspace/model.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/src/front/result_exc.rsmajit/majit-translate/src/inline.rsmajit/majit-translate/src/jit_codewriter/assembler.rsmajit/majit-translate/src/jit_codewriter/call.rsmajit/majit-translate/src/jit_codewriter/flatten.rsmajit/majit-translate/src/jit_codewriter/jtransform.rsmajit/majit-translate/src/model.rsmajit/majit-translate/src/translator/rtyper/cutover.rsmajit/majit-translate/src/translator/rtyper/flowspace_adapter.rsmajit/majit-translate/src/translator/rtyper/legacy_annotator.rsmajit/majit-translate/src/translator/rtyper/legacy_resolve.rsmajit/majit-translate/src/translator/rtyper/mod.rsmajit/majit-translate/src/translator/rtyper/pairtype.rsmajit/majit-translate/src/translator/rtyper/rclass.rsmajit/majit-translate/src/translator/rtyper/rlist.rsmajit/majit-translate/src/translator/rtyper/rmodel.rsmajit/majit-translate/src/translator/transform.rspyre/pyre-interpreter/src/jit_fnaddr.rs
| ConstValue::PyTypeAddr(_) => { | ||
| // A prebuilt `*const PyType` static: annotate as the same | ||
| // `SomeInstance(PyType)` an `ob_type` field read carries | ||
| // (`project_pyre_field_type("PyType")`), keeping the host | ||
| // address as `const_box` so `InstanceRepr.convert_const` | ||
| // materialises the matching `_ptr` at rtyper time. Both | ||
| // sides then share the `Ptr(GcStruct pyobject::PyType)` | ||
| // repr and `ptr::eq`/`is` resolve through | ||
| // `pair(InstanceRepr, InstanceRepr)`. | ||
| match self.project_pyre_field_type("PyType") { | ||
| SomeValue::Instance(mut inst) => { | ||
| inst.base.const_box = Some(Constant::new(x.clone())); | ||
| Ok(SomeValue::Instance(inst)) | ||
| } | ||
| other => Err(AnnotatorError::new(format!( | ||
| "immutablevalue(PyTypeAddr): PyType did not resolve to a \ | ||
| registered classdef instance: {other:?}" | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a direct regression test for immutablevalue(ConstValue::PyTypeAddr(_)).
This new arm is core to the PyTypeAddr pipeline; please add a focused unit test that asserts it returns SomeValue::Instance and preserves the original constant in const_box.
🤖 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/bookkeeper.rs` around lines 2591 - 2610,
The ConstValue::PyTypeAddr(_) match arm in the immutablevalue function lacks a
direct regression test to verify its behavior. Add a focused unit test that
calls immutablevalue with a PyTypeAddr constant value and asserts that it
returns SomeValue::Instance with the const_box field correctly populated with
the original constant. The test should cover both the success path (where
project_pyre_field_type returns an Instance) and ideally the error path (where
it returns something else).
| /// materialises it as a solid `_ptr` to the repr's `object_type` at | ||
| /// the host address, so `ptr::eq` / `is` against `ob_type` resolve | ||
| /// through `pair(InstanceRepr, InstanceRepr)`. | ||
| PyTypeAddr(i64), |
There was a problem hiding this comment.
Make PyTypeAddr equality match its address hash.
Hash now hashes the address, but PartialEq has no PyTypeAddr arm, so two constants for the same folded static address compare unequal. That breaks Constant equality and HashMap<ConstValue, ...> lookup/dedup for PyType keys.
🐛 Proposed fix
(ConstValue::LLAddress(a), ConstValue::LLAddress(b)) => a == b,
+ (ConstValue::PyTypeAddr(a), ConstValue::PyTypeAddr(b)) => a == b,
(ConstValue::HostObject(a), ConstValue::HostObject(b)) => a == b,Also applies to: 2283-2312, 2401-2401
🤖 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/flowspace/model.rs` at line 2260, The PyTypeAddr
variant's Hash implementation hashes the i64 address, but the PartialEq
implementation lacks a PyTypeAddr arm, causing two PyTypeAddr instances with the
same address to compare unequal and breaking HashMap lookup and dedup for PyType
keys. Add a PyTypeAddr arm to the PartialEq implementation (around lines
2283-2312) that compares the inner i64 addresses for equality, ensuring
consistency between Hash and PartialEq behavior. Also verify that any other
related equality comparisons (around lines 2401-2401) are updated if necessary
to maintain this consistency throughout the codebase.
| fn deref_cast_root(&self, reg: &RegularCall, dest_ty: &TyRef) -> Option<String> { | ||
| let CallKind::Fun(FunId::Regular { id }) = ®.kind else { | ||
| return None; | ||
| }; | ||
| let fd = self.llbc.fn_by_id(*id)?; | ||
| let np = fd.item_meta.name_path(); | ||
| if !(np.ends_with("::deref") || np.ends_with("::deref_mut")) { | ||
| return None; | ||
| } | ||
| tyref_class_root(dest_ty, self.llbc) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List project Deref/DerefMut implementations for manual classification.
# Expectation: every deref/deref_mut that can return a named ADT and be lowered by MIR must be a transparent pointer wrapper before this broad cast rewrite is safe.
rg -n -C4 'impl\s+.*Deref|impl\s+.*DerefMut|fn\s+deref(_mut)?\s*\(' --type rustRepository: youknowone/pyre
Length of output: 16492
🏁 Script executed:
# Check where deref_cast_root is called/used
rg -n "deref_cast_root" --type rustRepository: youknowone/pyre
Length of output: 315
🏁 Script executed:
# Check the context around the call site mentioned (lines 3274-3288)
sed -n '3270,3295p' majit/majit-translate/src/front/mir.rsRepository: youknowone/pyre
Length of output: 1585
🏁 Script executed:
# Find tests related to deref_cast_root
rg -n "deref" majit/majit-translate/src/front/mir.rs | grep -i testRepository: youknowone/pyre
Length of output: 41
🏁 Script executed:
# Check if there are any test files that exercise this code path
fd test mir.rs --type fRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
# Search for any related issues, TODOs, or comments about deref casting
rg -n -B2 -A2 "cast_pointer|deref" majit/majit-translate/src/front/mir.rs | head -100Repository: youknowone/pyre
Length of output: 5597
Restrict deref casts to known thin-pointer wrappers to prevent future misuse.
deref_cast_root currently matches any ::deref / ::deref_mut returning a named ADT. While the current codebase contains no problematic custom Deref implementations—all existing implementations are transparent wrappers directly forwarding a single field—the predicate does not explicitly verify that the impl's receiver is a thin-pointer wrapper. A future custom Deref<Target = T> impl that returns an interior field or computed value would incorrectly match and cause cast_pointer(T, receiver) to reinterpret the owner as the target type.
Add an allowlist restricting this optimization to known one-word wrappers (Box / Rc / Arc / FrameBox), leaving other derefs on the normal call path.
🤖 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/front/mir.rs` around lines 3839 - 3849, The
deref_cast_root function currently accepts any deref or deref_mut implementation
without validating the receiver type, which could lead to incorrect pointer
reinterpretation for custom Deref implementations. Add an allowlist check within
deref_cast_root to verify that the receiver type (the ADT implementing Deref) is
one of the known thin-pointer wrappers: Box, Rc, Arc, or FrameBox. If the
receiver type is not in this allowlist, the function should return None, leaving
other deref calls to be handled through the normal call path.
7e02ad8 to
95ba8fc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95ba8fce54
ℹ️ 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".
| && (matches!(self.blanket_into_devirt(®), Some(IntoDevirt::Identity)) | ||
| || self.trait_clause_into_string_identity(®, &call.dest.ty)) | ||
| || self.trait_clause_into_string_identity(®, &call.dest.ty) | ||
| || self.is_noop_ptr_cast(®) |
There was a problem hiding this comment.
When the receiver is a raw pointer cast to a registered struct, this new early alias fires before the existing is_ptr_identity_cast block below that emits __pyre_cast_instance for ptr.cast::<W_SRE_Pattern>()-style targets. Those casts are used before field reads such as _sre's w_srepat.cast::<W_SRE_Pattern>().w_groupindex; aliasing the destination to the original PyObjectRef/raw pointer leaves it classdef-less, so the downstream field access loses the target W_SRE_Pattern annotation and falls back/fails instead of being typed as that struct. Please keep registered-struct pointer casts on the narrowing path rather than handling them in this generic identity branch.
Useful? React with 👍 / 👎.
union(SomeInstance(Some(ca)), SomeInstance(Some(cb))) returned a UnionError when commonbase(ca, cb) was None. The rooted RPython hierarchy never reaches that arm (every class roots at object); pyre's flat Rust W_-structs carry no synthesized root, so correct code (normalize_slice, w_range_getitem merging differently-narrowed PyObjectRefs at a join) hit it. Widen to SomeInstance(None) — the same annotation the (None, _) arm already produces — modelling the runtime PyObjectRef. Replace union_distinct_classdef_instances_errors with union_instances_with_no_common_base_widens_to_object. Closes the 4 "UnionError in mergeinputargs" dual-gate skips (w_module_dict_object_storage lifts fully; normalize_slice, w_range_getitem, w_module_dict_getitem_str advance to other walls). Census 627->626, divergence 0. check.py 55/55 both backends. Assisted-by: Claude
`<*const T>::cast_mut` / `<*mut T>::cast_const` / `cast` change only const/mut or the pointee type, which the JIT does not model (`Ref` / `RawPtr` lower to a same-Variable alias). Add `is_noop_ptr_cast` and route these one-arg calls through the existing identity-passthrough path in `lower_call`, binding the destination local to the pointer argument instead of emitting a `Call` to an unregistered raw-pointer method. check.py 55/55 dynasm + cranelift; census skips 584->577, div 0; the `core::ptr::const_ptr::<Impl>::cast_mut` lift failures are closed. Assisted-by: Claude
Charon records the initializer of `core`'s `f64::INFINITY` (`1.0_f64 / 0.0_f64`) as an `Opaque` body, so `const_eval_global` finds no init to evaluate and the `Global` read falls through to an unresolvable `FunctionPath` Call. Add `primitive_float_const`, consulted in the same `or_else` chain as `static_addr_op` / `const_eval_global`, to emit the IEEE-754 value as a `ConstFloat` — the same by-value op an inline float literal lowers to. Closes the 11 `core::f64::<Impl>::INFINITY` not-registered skips; `descroperation::as_float` and its dependent graphs now lift past the constant read to their next annotator wall. Assisted-by: Claude
`core::f64::<Impl>::is_nan` has an Opaque body, so the callsite skips
as an unregistered FunctionPath. `is_nan` is `value != value`
(`rfloat.isnan`); lower a 1-arg `is_nan` call to `BinOp { op: "ne" }`
with both operands the receiver. The float operand makes the rtyper
select `float_ne`, which carries no `n(x, x) => 0` reflexive fold
(that fold is gated to `int_eq`/`int_ne` in intbounds), so the
NaN-only truth value is preserved.
Closes the 3 `core::f64::<Impl>::is_nan` not-registered skips;
`descroperation::float_pow_raw` lifts past the NaN test.
Assisted-by: Claude
The blanket `impl<I: Iterator> IntoIterator for I` (`core::iter::traits::collect::<Impl>::into_iter`) returns the receiver unchanged, but its body is an unregistered callee, so a `for` desugar's `into_iter` callsite skips. Recognise the exact blanket path and bind the destination local to the argument, the same identity alias used for reflexive `into` / no-op pointer casts. Container `IntoIterator` impls live under other module paths and are unaffected. Closes the 10 `core::iter::traits::collect::<Impl>::into_iter` not-registered skips. Assisted-by: Claude
…path front::mir rewrites a `majit_metainterp::jit::promote(x)` callsite to the single-segment `hint_promote` marker so the residual `OpKind::Call` reaches `jtransform::rewrite_op_hint`, which emits `[-live-, <kind>_guard_value(x)]` (jit_codewriter/jtransform.py:608-614). The rtyper lowers the marker to `same_as(arg)` for the dual-gate type projection, mirroring the existing `hint_promote_or_string` handling. Getting past the promote wall lets `function::getcode` lift to a `_ptr ∪ NoneType` merge whose `noneify()` raises UnionError (the default `SomeObject.noneify`, annotator/model.py:121, which `SomePtr` does not override). Classify that message as known-unported, alongside the sibling `cannot unify instances` mergeinputargs UnionError. Assisted-by: Claude
A thin-pointer `Deref::deref` / `DerefMut::deref_mut` (`Box<T>` / `Rc<T>` / `Arc<T>` / the workspace `FrameBox`) is one pointer word, so `*p` is a typed pointer reinterpret of the pointee address. When the dereferenced `&T` resolves to a named-ADT struct root, lower the call to the `cast_pointer(T, p)` downcast marker instead of the ordinary method shape, so the result annotates as `SomeInstance(T)` regardless of the receiver's classdef-less annotation (ann_cast_pointer, lltype.py:970-974) — the same lowering `obj as *const W_Foo` already takes. `deref_cast_root` matches the `deref` / `deref_mut` leaf and resolves the dereferenced type's class root through `tyref_class_root`; slice / `str` derefs resolve no struct root and keep their ordinary lowering. Dual-gate skip census 592 -> 587 (8 struct-target derefs lowered). Assisted-by: Claude
The `noneify() not supported` skip in `is_known_unported` documents that the raise it catches is parity-correct: RPython's default `noneify` raises (model.py:121-122), `SomePtr` defines no override, and `pair(SomePtr, SomeObject).union` raises (llannotation.py:119-120). The divergence is solely the `front::mir` null-pointer typing — a typed null is `lltype.nullptr(T)` -> `SomePtr` upstream (pair(SomePtr, SomePtr).union, llannotation.py:94-98), not `SomeNone`. Fix the comment's `lltype.py` citation to `llannotation.py` and state the convergence (lower a typed null pointer at the producer; removing the skip without it only hard-breaks on the correct raise). Assisted-by: Claude
…t noneify skip deref_cast_root resolves the Deref impl's Self-type leaf via the impl owner ADT (deref_impl_owner_leaf) and subtracts only Rc/Arc, whose word points at a refcount header rather than the pointee. Box/FrameBox and single-field transparent wrappers (UnsafeCell payload) keep the cast_pointer rewrite; an unresolved owner keeps it too. Replaces the registry-keyed Box/FrameBox allowlist, which dropped every conversion because impl_method_owner_for_fundecl does not name plain Deref impls. cutover: add the noneify() not supported row to the dual-gate skip-reason table and a known_unported test for the pattern. Assisted-by: Claude
Register `we_are_jitted` as a value-level `ExtRegistryEntry` (rlib/jit.py:396) so `findbltintyper` resolves a `specialize_call` (`rbuiltin::rtype_we_are_jitted`, jit.py:404-406) that emits the `_we_are_jitted` symbolic (`WE_ARE_JITTED_TAG_ID`) at the result repr's lltype. The real path previously raised "don't know about built-in function" on the call, which the dual gate re-panicked instead of classifying as a graceful skip. The annotation stays served by the `BUILTIN_ANALYZERS` entry (`immutablevalue_hostobject` returns before the extregistry fall-through), so the new entry is rtyper-only and its `compute_annotation` fails closed. In the JIT codewriter, `rewrite_op_direct_call` folds the `majit_metainterp::jit::we_are_jitted` call to `ConstBool(true)` (jtransform.py:1636-1639), so the tracer records a green branch condition rather than a residual call plus guard on the runtime JIT-mode flag. `replace_we_are_jitted` emits a Bool replacement to match the Bool-typed symbol carrier (pyre's `we_are_jitted() -> bool`). Assisted-by: Claude
95ba8fc to
01992ee
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01992eebb3
ℹ️ 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".
| .alloc_value_var_with_type(crate::model::ConcreteType::Unknown); | ||
| self.graph.block_mut(bb_id).operations.push(SpaceOperation { | ||
| result: Some(res.clone()), | ||
| kind: cast_pointer_marker_op(root, args[0].clone()), |
There was a problem hiding this comment.
When this path handles FrameBox::deref(_mut), the current IR argument is still the FrameBox wrapper value, not the inner PyFrame pointer: FrameBox is defined as { ptr: *mut PyFrame } and its deref implementation reads self.ptr (pyre/pyre-interpreter/src/pyframe.rs). Fresh evidence in this version is that deref_cast_root now filters only Rc/Arc, so FrameBox still reaches this cast_pointer(PyFrame, args[0]); hot call paths such as new_frame.locals_w_mut() then type subsequent field reads as PyFrame while using the wrapper/handle representation instead of the pointee. Please read the ptr field (or otherwise prove the receiver was already lowered to that field) before emitting the cast.
Useful? React with 👍 / 👎.
01992ee to
4c8051a
Compare
…e non-raising `is_noop_ptr_cast` no longer matches `<ptr>::cast`, only the const/mut-only `cast_mut` / `cast_const`. A `ptr.cast::<T>()` callsite now falls through to `is_ptr_identity_cast`, which narrows a registered-struct target to `__pyre_cast_instance` instead of aliasing the destination classdef-less. `op_canraise`: classify the `hint_promote` / `hint_promote_or_string` markers non-raising before the general `Call` arm — `translate_op` lowers them to a non-raising `same_as`. Assisted-by: Claude
4c8051a to
394205e
Compare
1 participant
Summary
#131 / task #40 work: grow the set of interpreter graphs the orthodox
RPythonAnnotator+RPythonTyperreal-path can close, one censuscategory at a time, shrinking dual-gate Skips toward deleting the legacy
walker (
legacy_annotator.rs/legacy_resolve.rs, the #131 endgame).Rebased onto
main(#165 Result-of-PyError lowering).Production codegen for still-skipped graphs is unchanged — they keep
falling back to the legacy walker; only the dual-gate real-path lifts
further.
Front-end (
majit/majit-translate/src/front)annotator: widen a no-common-base instance union to the classdef-lesstop instead of raising
UnionError.ConstPyTypeAddrtyped asInstanceRepr,so
is/eqresolve throughpair(InstanceRepr, InstanceRepr).cast_mut/cast_const/cast) to their argument (the JIT does not model pointer reprs).usize as i64/i64 as usize)through
rarithmetic.intmask/r_uint, closing theint ∪ r_uintsignedness
UnionErroron Vec-field index/length merges.core::ptr::const_ptr::<Impl>::is_nullthrough the classdef-lesspointer-method path (same
ptr_method_is_nullanalyzer + null compareas
mut_ptr::is_null).result_exc: cover the newConstPyTypeAddrvariant in thefail-closed
op_operand_varsmatch.Rtyper (
majit/majit-translate/src/translator/rtyper)rpython/rtyper/rlist.pyListRepr/FixedSizeListReprfoundation, closing all 7
SomeList.rtyper_makereprskips(
w_tuple_len/w_set_len/items_block_capacityand otherVec-field readers now specialize).
Verification
pyre/check.py56/57 on both backends (dynasm + cranelift); the solefailure is the pre-existing, unrelated
synth/sre_pattern_methods(
_sre.MAGIChost-stdlib mismatch atimport re).Self-review
AI-assisted (Claude Code). Per the contribution note, the parity review
is run in a separate session from the one that generated the code; the
/parity to upstream/main(and the gpt-5.5 prompt) output will beattached below after that independent pass.
Prompt & Model
Model:
Prompt:
Answer
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements