#131: uniform Result<T, PyError> exception-link lowering (exceptiontransform.py parity) - #313
Conversation
|
Warning Review limit reached
Next review available in: 10 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 (36)
WalkthroughThis PR adds a bool::then-to-Option short-circuit rewrite pass in majit-translate, removes the allowlist-based scoping for Result<T, PyError> exception lowering in favor of a type-gated approach, seeds annotator return-variable bindings before rtyping, reconciles void call result types, and removes phi all-equal collapse. Separately, it migrates numerous pyre-interpreter attribute writes to direct dict-value calls with propagated errors, and adds JIT dont_look_inside tracing boundaries across pyre-interpreter, pyre-jit, and pyre-object. Changesmajit-translate compiler changes
Estimated code review effort: 4 (Complex) | ~60 minutes pyre-interpreter dict writes and JIT tracing boundaries
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant MirLowering as Lowering::lower_call
participant BoolThenRecognizer as recognize_bool_then_site
participant BoolThenModule as rewire_bool_then_call_sites
participant BlockA
participant ThenArm as then_bb
participant ElseArm as else_bb
participant BlockB
MirLowering->>BoolThenRecognizer: detect bool::then(cond, closure)
BoolThenRecognizer-->>MirLowering: BoolThenSite
MirLowering->>BoolThenModule: bool_then_sites at finish()
BoolThenModule->>BlockA: validate then-call shape
BoolThenModule->>ThenArm: emit call_once + Option::Some
BoolThenModule->>ElseArm: emit Option::None
BoolThenModule->>BlockA: replace call with branch on cond
ThenArm->>BlockB: goto with Some(payload)
ElseArm->>BlockB: goto with None
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 e86c757). 1. Regressions to PyPy parity introduced by this patch
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
Here are some automated review suggestions for this pull request.
Reviewed commit: 650272596e
ℹ️ 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 effective_result_ty = | ||
| self.effective_call_result_ty(target, op.result.as_ref(), result_ty); |
There was a problem hiding this comment.
When a transformed Result<(), PyError> callee is a registered graph, guess_call_kind classifies it as Regular (or recursive) and the branch above still calls handle_regular_call(..., result_ty, ...), so it emits/stamps an inline_call_r from the front-end Ref unit shell even though the callee jitcode is later widened to a void return. This new reconciliation only runs in the residual branch, so any inlined foo()?/discarded-unit call to a Result<(), PyError> helper still has a caller/callee result-kind mismatch and can read a non-existent/garbage ref slot; apply the same effective result type before regular/recursive call lowering too.
Useful? React with 👍 / 👎.
3310350 to
e8b97e6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8b97e6562
ℹ️ 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".
| eprintln!( | ||
| "[result-exc] {} function(s) declined the exception-link \ | ||
| lowering (fail-safe → residual); shape-coverage gap:{detail}", | ||
| regressions.len() | ||
| ))); | ||
| ); |
There was a problem hiding this comment.
This branch now only logs and then returns Ok even though regressions contains every skipped function whose error is not the known uninitialised-local gap; the skipped bucket is filled from all lower_fun_decl_with_static_addrs failures, not just fail-safe result-exception declines. When an unrelated new lowering error appears, its graph is silently omitted from the SemanticProgram and calls fall back to residual handling, masking translation coverage/perf regressions that this gate previously stopped.
Useful? React with 👍 / 👎.
| if cur != from_block { | ||
| for op in &block.operations { | ||
| if op_operand_vars(&op.kind).iter().any(|o| o == &v) { | ||
| return Err(format!( | ||
| "{}: Result shell alias is read by an operation in \ | ||
| block {cur} on the forwarding path — not a pure forward", | ||
| graph.name | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
This verifier now accepts forwarding blocks that contain operations as long as those operations do not read the Result shell, but lower_result_exc_returns rewrites an Err shell by calling set_raise_values in the producer block, which replaces the producer's normal exit. In a Result callee where an Err value is threaded through a cleanup/logging/counter block before returning, those accepted downstream operations are skipped and the JIT raises earlier than the interpreter; keep the old pure-forwarder requirement for Err returns or emit the raise at the original return edge.
Useful? React with 👍 / 👎.
| if args.len() == 1 && self.is_string_as_bytes_identity(®) { | ||
| self.local_var[dest_local] = Some(args[0].clone()); |
There was a problem hiding this comment.
This aliases every String/str/Wtf8::as_bytes() result to the original string object. For translated code that indexes or casts the byte slice, such as pyre-interpreter/src/type_methods.rs using as_bytes()[0] == b'0', the adapter lowers the later ArrayRead to getitem on StringRepr, whose rstr path returns a Char rather than the u8/Signed value the Rust source expects. That can either fail rtyping or give character semantics to byte arithmetic; gate this identity to known len/equality consumers or lower a real byte-slice representation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Type the rewrap conversion for the annotator
When catch_and_rewrap handles a custom match consumer, this synthetic Method call is resolved statically by the codewriter, but the flowspace adapter still lowers CallTarget::Method for annotation as getattr(args[0], "from_exc_object") followed by simple_call. Here args[0] is the caught W_BaseException, and this patch only adds an annotator override for PyError.to_exc_object, so rewrapped sites have no from_exc_object attribute to annotate and will block/skip instead of preserving the custom match path; add a matching annotator special case or emit a target the annotator can resolve.
ℹ️ 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 !well_formed_return || !forwards_ok { | ||
| continue; |
There was a problem hiding this comment.
When one real Result<T, PyError> return has any extra use of the shell before it reaches returnblock (for example the temporary is passed to a helper/logging path and then returned) and another return in the same callee rewrites cleanly, this continue leaves that shell-return path unlowered while rewritten > 0 makes the callee succeed as transformed. Callers are then rewired to receive T/exception instead of a Result, so the skipped path can return a Result object where the caller expects the unwrapped payload; only skip ctors after proving they do not reach returnblock, otherwise decline/fail the whole callee.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73eb77b454
ℹ️ 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".
| kind: OpKind::Call { | ||
| target: CallTarget::method("call_once", Some(site.call_once_owner.clone())), | ||
| args: vec![env_in_then, unit], | ||
| result_ty: site.payload_ty.clone(), |
There was a problem hiding this comment.
When the lifted call is cond.then(|| ()) (Option<()>), site.payload_ty is derived through tyref_to_value_type, which intentionally treats unit in operand position as Ref(None), but the synthesized call_once here is a function call whose unit return should be Void like the ordinary lower_call is_unit_type special case. This makes the new diamond emit a ref-returning call_once and then build Some from that non-existent ref slot, so unit-returning closures can produce a bogus payload/register instead of the payload-less Some(()) shape; special-case unit payloads before stamping the call result and Some::__pos_0 write.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 126151dbd8
ℹ️ 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".
| graph.name | ||
| )); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
When a Result shell has an extra use and a conditional exit but one arm still forwards it to returnblock, well_formed_return is false so this continue runs before the reachability guard below. Fresh evidence is this earlier conditional-exit skip: in a shape like let r = Ok(x); if cond { log(&r) }; r, another return in the same callee can still be rewritten, leaving this path returning a materialized Result after callers were rewired to expect the unwrapped payload/exception. Treat non-well-formed conditional shells that reach returnblock as a whole-callee decline instead of a consumed intermediate.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-interpreter/src/module/sys/vm.rs (1)
57-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiscarded
setdictvalueboolean acrosssys.namespaceinitialization.Every
setdictvaluecall inbuild_frame_stub_chain,make_traceback_frame_stub, andregister_module(e.g. Lines 86-106, 131-144, 530-596, 618-686) discards the returnedbool. This is currently safe becausesys_namespace_type()explicitly setshasdict = true(Lines 19-28), but there's no defensive check tying the two together — a future change tosys_namespace_type(or reuse of these helpers with a different type) would silently drop sys-module fields instead of failing loudly.🛡️ Example defensive pattern
-crate::baseobjspace::setdictvalue(stub, "f_code", pycode); +debug_assert!( + crate::baseobjspace::setdictvalue(stub, "f_code", pycode), + "sys.namespace stub must have a dict slot" +);Also applies to: 259-1018
🤖 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 `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 57 - 150, The sys.namespace initialization code is ignoring the boolean result from setdictvalue in build_frame_stub_chain, make_traceback_frame_stub, and register_module, which can silently drop fields if the object stops supporting dict storage. Update these helpers to check the return value of each setdictvalue call and surface an explicit failure path instead of assuming success, using the existing symbols build_frame_stub_chain, make_traceback_frame_stub, register_module, and sys_namespace_type to keep the behavior tied to the namespace type contract.majit/majit-translate/src/front/mir.rs (1)
862-883: 📐 Maintainability & Code Quality | 🔵 TrivialCoverage gate now degrades silently — consider an opt-in strict mode for CI.
The regression bucket switched from build-failure to an unconditional
eprintln+ continue, so a genuinely unrelated new lowering decline degrades silently to a residual call (correctness is preserved; only JIT coverage/perf regresses). The comment already delegates the safety net tocheck.py. As defense-in-depth, consider gating a hard failure behind an opt-in env var mirroring the existingPYRE_MIR_FRAMESTATE_STRICTpattern, so CI can fail loud on new regressions while production keeps degrading.🤖 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 862 - 883, The regression handling in mir.rs now only logs with eprintln! and continues, so add an opt-in strict failure mode for CI while keeping the current fail-safe fallback for production. Update the regressions block in the MIR coverage path to check a dedicated environment variable, mirroring the existing PYRE_MIR_FRAMESTATE_STRICT pattern, and when enabled, turn the unrecognised MIR shape regression into a hard error instead of degrading silently to residual. Keep the current diagnostics and continue behavior unchanged when the strict flag is absent.
🤖 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 `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 7276-7282: The `seek` path is still updating the `__file_pos__`
slot through a separate `setattr_str(...)` write instead of the shared
`file_set_pos` helper. Update `seek` to call `file_set_pos` for setting the file
position so the `__file_pos__` state uses one consistent code path and
semantics, matching the newly documented store behavior in `file_set_pos`.
---
Outside diff comments:
In `@majit/majit-translate/src/front/mir.rs`:
- Around line 862-883: The regression handling in mir.rs now only logs with
eprintln! and continues, so add an opt-in strict failure mode for CI while
keeping the current fail-safe fallback for production. Update the regressions
block in the MIR coverage path to check a dedicated environment variable,
mirroring the existing PYRE_MIR_FRAMESTATE_STRICT pattern, and when enabled,
turn the unrecognised MIR shape regression into a hard error instead of
degrading silently to residual. Keep the current diagnostics and continue
behavior unchanged when the strict flag is absent.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 57-150: The sys.namespace initialization code is ignoring the
boolean result from setdictvalue in build_frame_stub_chain,
make_traceback_frame_stub, and register_module, which can silently drop fields
if the object stops supporting dict storage. Update these helpers to check the
return value of each setdictvalue call and surface an explicit failure path
instead of assuming success, using the existing symbols build_frame_stub_chain,
make_traceback_frame_stub, register_module, and sys_namespace_type to keep the
behavior tied to the namespace type contract.
🪄 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: f3ec32b4-b097-46d0-aacd-8bd7f882edec
📒 Files selected for processing (36)
majit/charon-corpus/corpus.ullbcmajit/charon-corpus/src/lib.rsmajit/majit-charon-reader/tests/corpus.rsmajit/majit-translate/src/annotator/annrpython.rsmajit/majit-translate/src/annotator/unaryop.rsmajit/majit-translate/src/codewriter/call.rsmajit/majit-translate/src/codewriter/jtransform.rsmajit/majit-translate/src/front/bool_then.rsmajit/majit-translate/src/front/mir.rsmajit/majit-translate/src/front/mod.rsmajit/majit-translate/src/front/result_exc.rsmajit/majit-translate/src/lib.rsmajit/majit-translate/src/model.rsmajit/majit-translate/src/translator/rtyper/cutover.rsmajit/majit-translate/tests/test_mir_frontend.rspyre/pyre-interpreter/src/_structseq.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/jit_fnaddr.rspyre/pyre-interpreter/src/module/_collections/mod.rspyre/pyre-interpreter/src/module/_csv/mod.rspyre/pyre-interpreter/src/module/_weakref/interp__weakref.rspyre/pyre-interpreter/src/module/pyexpat/mod.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/pyopcode.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/gc_interp.rspyre/pyre-object/src/gc_roots.rspyre/pyre-object/src/longobject.rs
| fn file_set_pos(self_obj: PyObjectRef, pos: usize) { | ||
| let _ = crate::baseobjspace::setattr_str(self_obj, "__file_pos__", w_int_new(pos as i64)); | ||
| // Private storage slot on a fresh hasdict file wrapper (no custom | ||
| // `__setattr__`, `__file_pos__` is not a descriptor), so the write is | ||
| // the infallible instance-dict store `W_Root.setdictvalue` | ||
| // (baseobjspace.py:51) that `setattr_str` would itself reach. | ||
| crate::baseobjspace::setdictvalue(self_obj, "__file_pos__", w_int_new(pos as i64)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Reuse file_set_pos in seek for consistency.
seek's handling of the same __file_pos__ slot (line ~7209, unchanged) still writes via setattr_str(...) and discards the result with let _ =, duplicating and diverging from this newly-documented "infallible instance-dict store" pattern. Consider routing seek through file_set_pos so both writers share one code path/semantics.
♻️ Proposed consolidation
- if args.len() >= 2 {
- let _ = crate::baseobjspace::setattr_str(args[0], "__file_pos__", args[1]);
- }
+ if let Some(&pos) = args.get(1) {
+ if let Ok(v) = unsafe { crate::pyre_object::w_int_get_value_checked(pos) } {
+ file_set_pos(args[0], v as usize);
+ }
+ }🤖 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 `@pyre/pyre-interpreter/src/builtins.rs` around lines 7276 - 7282, The `seek`
path is still updating the `__file_pos__` slot through a separate
`setattr_str(...)` write instead of the shared `file_set_pos` helper. Update
`seek` to call `file_set_pos` for setting the file position so the
`__file_pos__` state uses one consistent code path and semantics, matching the
newly documented store behavior in `file_set_pos`.
is_string_as_bytes_identity recognizes as_bytes on a string-family impl owner (String/str/Wtf8/Wtf8Buf) and binds the call destination to the receiver, mirroring the as_str/as_ref/borrow identity, so the byte view lowers as the string value instead of an as_bytes getattr the rtyper cannot route on the classdef-less string receiver. Assisted-by: Claude
… machinery Mark 18 pyre_jit functions #[majit_macros::dont_look_inside]: the driver / merge-point / run path (maybe_compile_and_run, jit_merge_point_hook, bound_reached, execute_assembler, build_jit_state, handle_fail, handle_jit_outcome, driver_pair, call_depth, debug_first_arg_int, jit_suppressed_by_unsupported_frame, make_green_key), the deopt / blackhole resume path (decode_and_restore_guard_failure, decode_exit_layout_values, resume_in_blackhole_from_exit_layout), and the bridge-compile path (trace_and_compile_from_bridge, bridge_source_identity_from_descr, take_ca_exception). find_all_graphs pulled these in as look-inside candidates, but they read JIT-internal TLS / state / statics with no registry-resolvable accessor and failed the two-phase prepass annotate. look_inside_graph now returns false for them so callers emit residual Calls instead of inlining. Removes the macro's #[inline(never)]. Takes effect after LLBC re-extraction. Prepass phaseA 496->474 distinct (the 18 graphs plus transitive callees reachable only through them); phaseB unchanged at 0. check.py dynasm 158/158, cranelift 158/158. Assisted-by: Claude
…d-opcode slow-path handlers Mark descr_not_settable_error, raiseattrerror (baseobjspace) and execute_unsupported (pyopcode) #[majit_macros::dont_look_inside]. These are AttributeError construction (read-only descriptor / attribute miss) and the unsupported-opcode handler. Residualizing them drops them plus their transitive callees attribute_error_with_context and OpcodeStepExecutor::unsupported from the codewriter candidate set. Measured against a fresh no-marks pyre-interpreter extraction: phaseA distinct 502 -> 497 (-5), zero added fails. check.py dynasm 158/158 + cranelift 158/158. Assisted-by: Claude
…redecessor merges
simplify_phis collapsed an all-equal phi column for any arity, including a
single-predecessor block's one-element column (a one-element slice is
trivially all-equal). Upstream simplify.py:561 is safe because cleanup_graph
runs join_blocks first, folding every single-predecessor block into its
predecessor. The charon-MIR front-end makes every Call a block terminator and
threads the result as the next block's inputarg, so single-predecessor
non-empty blocks persist here — a call block must keep its single exit, so
join_blocks cannot fold them. Collapsing such a one-element column unions the
inputarg with a Variable defined in the predecessor and renames the body
reference across the block boundary, leaving the flowspace adapter an operand
defined in no reachable predecessor ("undefined operand").
Restrict the all-equal collapse to genuine multi-predecessor merges
(new_args.len() > 1). A single-predecessor block's duplicate slots are still
folded by the unique_phis equivalence, keeping the surviving inputarg.
PREPASS phaseA failures 495 -> 473; undefined-operand 54 -> 28.
check.py dynasm 165/165 + cranelift 165/165.
Assisted-by: Claude
…o_SSI repair The all-equal collapse (simplify.py:561-563) removes a merge block's inputarg and renames body references to a value defined in a predecessor, producing cross-block references. RPython repairs these with the next all_passes entry, SSA_to_SSI (backendopt/ssa.py:135-196), which re-threads each used-but-undefined variable as a fresh inputarg. remove_identical_vars_SSA runs here on the crate::model front-end graph, which has no SSA_to_SSI port (the ssa_to_ssi port operates on crate::flowspace::model, a distinct IR), so the collapse left body references the flowspace adapter rejects as undefined operands. prune_dead_phis already drops dead inputargs, so every surviving column is used and SSA_to_SSI would re-thread each collapsed column; skipping the collapse yields the same graph with the column left threaded. The duplicate-column dedup (simplify.py:565-568) is the unique_phis branch, which is unaffected. Removes the all_equal/isspecialvar helpers and the earlier new_args.len() > 1 guard this subsumes. [PREPASS phaseA fail] undefined-operand distinct 28 -> 0; phaseA distinct 473 -> 472. check.py dynasm+cranelift 165/165. Assisted-by: Claude
collapse_pos0_read is the only fallible mutation in the ?-diamond caller rewrite; with two or more continue-arm payload positions it can mutate one target before erring on a later one, leaving a partially-rewritten graph. Decline that shape up front, mirroring iter_next::rewire_one_next_site. Assisted-by: Claude
A `SomeInstance(PyError)` receiver reaching `getattr(recv, "to_exc_object")`
Blocked: PyError's synthesized class dict carries no inherent-impl methods,
so `s_getattr` returns Impossible. The result_exc ?-lowering mints
`to_exc_object` as a `Method{receiver_root: "PyError"}` residual that the
codewriter already resolves via `for_impl_method` to the real conversion
graph. Surface it in the SomeInstance.getattr path as a residual builtin
method (mirroring the `is_null` ptr-method arm) typed `PyObjectRef`, so the
annotator agrees with the codewriter's real fnaddr conversion.
phaseA prepass distinct failures 472 -> 459 (26 to_exc_object blocks clear).
Assisted-by: Claude
The exception-link lowering is gated on the RESULT_EXC_LOWERING_SCOPE allowlist plus the execute_* wrapper family; RPython's exceptiontransform (transform_completely, exceptiontransform.py:212) instead transforms every graph uniformly with a shape-agnostic per-op check. Add a default-off switch (PYRE_RESULT_EXC_UNIFORM=1) that makes in_result_exc_scope answer true for every callee, so both gate sites (mir.rs callee rule + caller diamond record) fall back to the structural tyref_is_result_of_pyerror type filter alone — the no-allowlist model. Under the flag the shape-specific rewrite declines (fail-safe → residual, no miscompile) every caller/callee shape it does not recognise; gate the MIR-lowering coverage-regression guard so the experiment reports those declines and proceeds instead of failing the build. The production gate (flag off) is unchanged: byte-identical lowering, phaseA 449, the guard still fails loud on any unrecognised error. Measured Stage-3 shape-coverage gap (flag on): 192 function(s) decline the exception-link rewrite and 350 cachedgraph lifts hit unregistered callees (bigint::to_i64/to_f64, bool::then, slice methods, __dyn_call, …) — the allowlist drop is coupled to the unregistered-callee registration lever. Assisted-by: Claude
… ctors and generalize the return-forwarding check lower_result_exc_returns iterates every Result::Ok/Err ctor and lowers each into a value return / raise, failing the whole callee on any ctor whose value does not pure-forward to returnblock. Under PYRE_RESULT_EXC_UNIFORM, a ctor whose value is consumed inside the graph (an inlined callee's return that the graph then matches on or passes to a call) is now skipped, leaving it materialized, rather than failing the callee; only ctors that flow to returnblock are lowered. verify_forwards_to_returnblock dispatches under the gate to a new verify_forwards_to_returnblock_general: a worklist over (block, alias) states that accepts forwarding chains whose intermediate blocks carry unrelated operations and conditional exits, as long as the tracked value is never read by an operation nor used as an exitswitch operand. The flag-off path is unchanged. With PYRE_RESULT_EXC_UNIFORM=1 the prepass declines drop from 192 to 26. Assisted-by: Claude
…carrying shapes Convert the `let _ = <Result>` discard idiom in the functions that declined the result_exc uniform exception-link lowering: - propagate via `?` where the enclosing fn returns Result: file_flush_dirty, file_method_close, textio_configure, make_exc_type add_note closure, init_file_wrapper __exit__ closure, normalize_exception, build_class_inner prepared-dict setitem. - infallible instance-dict store via setdictvalue (promoted to pub(crate)) for fresh builtin-object construction: wrap_trace_frame, pyexpat init/init_parser_slots/make_namespace, sys build_frame_stub_chain, make_traceback_frame_stub, make_std_stream, register_module. - explicit best-effort catch where a failure must not mask the original result: set_name add_note, _csv reader_next finally-reset, build_class scaffolding delitem (KeyError-only). - _structseq new_instance_with_extra setdict: .expect on the infallible fresh-dict install. Assisted-by: Claude
…callee `rewrite_op_direct_call`'s residual arm resolved the call's result kind from `front::mir`'s `result_ty`, which types a `Result<(), PyError>` callee's result `Ref` (every aggregate, including the unit `()`, lowers to `Ref`). `getcalldescr` then hard-fails the call's `RESULT` against the callee's declared `void` `FUNC.RESULT` (call.py:230 `RESULT == FUNC.RESULT`). Add `CallControl::declared_result_type_for_target` — the callee's post-`?` projected `RESULT`, the same value getcalldescr derives as `expected_result` — and `Transformer::effective_call_result_ty`, which substitutes `void` for the call's result when the callee declares `void` and the front resolved `Ref`. The residual_call then emits a `_v` opname with no result slot (`emit_call_result_arg` writes no slot for kind `v`), the caller-side mirror of the returnblock widen in `finalize_rewritten_graph_to_jitcode`. Assisted-by: Claude
Drop the allowlist-gated exception-link lowering for the whole-program transform (exceptiontransform.py:212 transform_completely): - Delete RESULT_EXC_LOWERING_SCOPE, in_execute_wrapper_family, result_exc_uniform_gate, in_result_exc_scope, and the PYRE_RESULT_EXC_UNIFORM env switch. The callee-side type gate tyref_is_result_of_pyerror is the sole scope filter. - mir.rs: result_exc_callee and the caller-capture guard drop the in_result_exc_scope conjunct. - mir.rs lowering-coverage gate reports an unrecognised shape and proceeds (fail-safe residual) instead of failing the build. - lower_result_exc_returns and the rewrap forward check use verify_forwards_to_returnblock_general unconditionally; delete the strict verify_forwards_to_returnblock. Assisted-by: Claude
…ches on its own discriminant
A Result-ctor block whose conditional exit is driven by the ctor's own
`__discriminant` read is a consumed-intermediate `match` on the freshly
built shell (the `__new__` wrapper's `match { Ok(obj) } { Ok(o) => o,
Err(e) => return Err(e) }`), not a return shell.
lower_result_exc_returns now computes `well_formed_return` before the
conditional-exit check and skips such a block — leaving the shell
materialised for the local `match` — instead of declining the whole
callee. The hard decline is kept only for a well-formed return shell that
has a conditional exit, where an `Err` rewrite's set_raise_values →
set_goto would discard the other arm.
Clears the 8 `__pyre_wrap___new__` conditional-exit declines (_random,
struct, __pypy__::interp_buffer, _pickle::pickler+unpickler,
select::interp_kevent+kqueue+select); whole-program result-exc declines
14 → 6.
Assisted-by: Claude
…rding setattr_str file_set_pos (__file_pos__), deque_class::store (__data__), and deque_class::modified (__state__) write a private non-descriptor slot on a hasdict instance with no custom __setattr__, where setattr_str's terminal path is the instance-dict write setdictvalue; call setdictvalue directly so the helper no longer discards a Result. function_setdict stored `func.__dict__ = value` through setattr_str(obj, "__dict__", value) — a literal "__dict__" dict entry with the result discarded — instead of replacing the function's dict. Route it through baseobjspace::setdict (wholesale dict replacement, raising TypeError on a non-dict per Function.setdict) and propagate; function_setdict and setdict now return Result<(), PyError>. Whole-program result-exc declines 6 -> 2. Assisted-by: Claude
`sync_python_sys_path` replaced a discarded `let _ = setattr_str(sys_mod, "path", ..)` with the infallible direct dict store the `setattr_str` module branch reaches — `w_module_get_w_dict` + `w_dict_setitem_str` — dropping the discarded `Result`. Assisted-by: Claude
The two-phase rtyper prepass drives annotation per subject and defers `RPythonAnnotator::complete()`, so complete()'s return-var seeding tail (annrpython.py:258-261) never runs. A graph all of whose reachable paths end at the exceptblock leaves the returnblock unreachable and its return var unannotated; Phase B's `setconcretetype(graph.getreturnvar())` then panics on the missing binding. Extract the per-graph seeding from `complete()` into `force_return_var_annotation`; add `seed_all_annotated_return_vars` over the annotated graphs; call it at the two-phase barrier after `assign_inheritance_ids`, alongside the other stand-ins for the deferred `complete()`. Unbound return vars bind to `s_ImpossibleValue`. phaseB return-var "no binding" panics: 172 -> 13. The other graphs now proceed past the return var and fail rtype with a caught error instead of a panic. check.py 166/166 dynasm + 166/166 cranelift. Assisted-by: Claude
…ect two degrade-to-residual comments result_exc: split the intervening-block forward check by shell kind. The Ok rewrite only edits the producer exit link args, so the payload still threads through an intervening op-bearing block to returnblock — keep the generalized verify_forwards_to_returnblock_general check. The Err rewrite calls set_raise_values, which replaces the producer block exit with a jump to exceptblock and bypasses any intervening block, dropping its operations and raising earlier than the interpreter. Require the strict forwards_to_returnblock (empty unconditional intervening blocks only) for is_err shells; otherwise decline to a residual call. mir: correct the coverage-gate comment to state that a non-tracked lowering failure degrades to a residual call (matching exceptiontransform.py:212) rather than failing the build, and that the regressions bucket covers every non-tracked skip. Extend the as_bytes-identity-alias comment to record that the alias is sound only for len/equality/iteration; a scalar index yields a Char, which currently fails rtype and fail-safe residualizes. Assisted-by: Claude
…nstance-dict set accessors pin_root reads the thread-local SHADOW_STACK, dereference reads the weakref w_obj_weak slot (@jit.dont_look_inside in interp__weakref.py:168), and _obj_setdict writes the per-instance INSTANCE_DICT side table — each through a closure with no extractable graph, so as look-inside candidates the three subjects failed the prepass lift on an unregistered static (SHADOW_STACK / ATTR_W_OBJ_WEAK / INSTANCE_DICT) FunctionPath. Mark each #[majit_macros::dont_look_inside] so callers emit a residual Call, and bind their fnaddrs in jit_trace_fnaddrs (pin_root twins shadow_stack_len, dereference twins proxy_type). pin_root drops its #[inline]; the macro forces #[inline(never)] for a stable residual symbol. Assisted-by: Claude
Add `bool_then_closure` (`c.then(|| x + 1)`) exercising the `core::bool::<Impl>::then` opaque combinator with a `FnOnce` closure that captures an enclosing value by reference. Charon extracts the closure's `call_once` as a transparent inherent method of the closure type. Re-extract the checked-in corpus.ullbc. Assisted-by: Claude
core::bool::<Impl>::then is a foreign combinator with an Opaque body, so
its caller emitted a residual `then` call — an unregistered callee the
rtyper prepass Skips. Recognize `bool::then(cond, closure_env)` during MIR
lowering and rewrite it, in a post-pass, into the branch its semantics
imply: `if cond { Some(closure()) } else { None }`. The branch is
mandatory — the closure must not run on the false arm — so a single-block
always-compute encoding is unsound. The closure body reaches the graph as
the closure type's transparent `call_once` inherent method; the then arm
calls it directly.
- front/bool_then.rs: BoolThenSite + rewire_bool_then_call_sites post-pass,
modeled on front::iter_next; fail-safe (a structural mismatch leaves the
residual call, keeping the prepass Skip).
- front/mir.rs: recognize and record sites during lower_call, resolving the
Option ctor owners, the closure call_once owner, and the payload type;
drive the post-pass after the next-diamond rewrite.
- test_mir_frontend: bool_then_closure lifts to the diamond (call_once +
bool branch + Some/None).
core::bool::<Impl>::then prepass phaseA failures: 26 -> 0, no new
unregistered callee. check.py dynasm 139/139 + cranelift 139/139.
Assisted-by: Claude
…he gc-enabled / bigint-type-id atomic reads py_repr reads the const bool CAN_BE_TAGGED and is_array/memoryview_backing_slice read the static PyType ARRAY_TYPE — both surface as opaque global-read FunctionPaths the prepass cannot resolve. Register CAN_BE_TAGGED in jit_static_int_values (bake the build-time value) and ARRAY_TYPE in jit_static_pytype_addrs (bake the singleton address). enabled reads (and lazily initialises) the gc_interp STATE atomic and bigint_gc_type_id reads the init-assigned BIGINT_GC_TYPE_ID atomic — neither is a build-time constant, so mark both #[majit_macros::dont_look_inside] so callers emit a residual Call and bind their fnaddrs in jit_trace_fnaddrs. bigint_gc_type_id becomes pub for the cross-crate fnptr; both drop #[inline] for the macro's #[inline(never)]. Assisted-by: Claude
…ebase `loads_fixture_corpus` asserted 6 `charon_corpus::` local fns; the `bool_then_closure` fixture (9c3463d) added the closure fn plus its `<Impl>::call_once` inherent method, so `iter_local_fns` now counts 9. `prune_dead_phis_collapses_single_source_phi_with_reader` (from #274, merged via the rebase) asserted the all-equal single-source phi collapse. `remove_duplicate_inputargs` on this branch omits that collapse (documented PRE-EXISTING-ADAPTATION: no `SSA_to_SSI` repair on `crate::model`, so the collapse would strand a cross-block reader as an undefined operand). Rename and invert the test to pin the retained-live-phi shape. Assisted-by: Claude
…aches returnblock `lower_result_exc_returns` skipped a Result ctor it could not lower cleanly (`!well_formed_return || !forwards_ok`) unconditionally. A ctor that is a genuine return shell — its value reaches `returnblock` on some path — but is not a pure forward (an extra shell use, an intervening read, a non-pure `Err` forward) was thereby left materialised while another return in the same callee rewrote cleanly. `rewritten > 0` then reported the callee transformed and rewired its callers to receive the unwrapped `T`/exception, yet the skipped path still returned a `Result` object. Guard the skip with `shell_reaches_returnblock` (a purity-agnostic forward reachability check): only leave a ctor materialised when its value never reaches `returnblock` (a consumed intermediate); otherwise decline the whole callee (fail-safe → residual call) instead of emitting the partial rewrite. Assisted-by: Claude
…ssage The `is_known_lowering_gap` doc comment claimed that an unrecognised lowering failure "fails the whole-program build"; the code instead degrades every un-lowerable function to a residual call (the coverage gate only warns), matching `exceptiontransform.py:212` `transform_completely`, which transforms every graph and leaves an un-rewritable one to the residual-call ABI. Rewrite the doc comment to describe that fail-safe behaviour and cite the upstream analog. The regressions-bucket eprintln labelled every non-tracked skip "[result-exc] declined the exception-link lowering", but the bucket also holds non-result-exc shapes (e.g. `init_file_wrapper_type::<Impl>::call`, a call block exit that does not carry the tracked value). Relabel to "[mir-coverage] ... unrecognised MIR shape degraded to residual". Assisted-by: Claude
`effective_call_result_ty` runs only in the Residual call-dispatch arm. Applying it to the Regular / Recursive (inlined) arms retypes an inlined `Result<(), PyError>` callee's unit `()` result to `Void`, but that result is a live `Ref`-carried block inputarg / link arg: regalloc `lookup_coloring` (assembler.rs:2403) rejects an uncoloured `Void` carried variable and the build-script worker panics (`eval_loop_jit`). Record on both inlined arms why the reconciliation is Residual-only (there `residual_call_v` genuinely drops the result slot). Assisted-by: Claude
…_reaches_returnblock The conditional-exit branch in lower_result_exc_returns skipped a non-well-formed Result shell via `continue` without checking whether the shell reaches returnblock. A shell that reaches returnblock on a conditional arm, left materialised while another return in the same callee rewrites cleanly, keeps rewritten > 0: the callee is reported transformed and its callers are rewired to the unwrapped T/exception, yet that path still returns a Result object. Add the shell_reaches_returnblock guard (mirroring the unconditional non-well-formed path) to decline the whole callee to a residual call. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a0af068f7
ℹ️ 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".
| graph.block_mut(then_bb).operations.push(SpaceOperation { | ||
| result: Some(payload.clone()), | ||
| kind: OpKind::Call { | ||
| target: CallTarget::method("call_once", Some(site.call_once_owner.clone())), | ||
| args: vec![env_in_then, unit], | ||
| result_ty: site.payload_ty.clone(), | ||
| }, |
There was a problem hiding this comment.
When the closure passed to bool::then itself returns Result<T, PyError> (for example cond.then(|| fallible())), that call_once callee is still transformed by the uniform Result-exception pass, but this synthesized call site is emitted after rewire_result_exc_call_sites, so it never gets the catch-and-rewrap handling needed for a non-? consumer. In that scenario the true arm wraps a bare T or raises on Err instead of producing Some(Ok(T)) / Some(Err(e)), so either decline Result payloads here or synthesize the same rewrap shape around the call_once.
Useful? React with 👍 / 👎.
1 participant
#131
Summary
Ports RPython's shape-agnostic exception transform (
rpython/translator/exceptiontransform.py) to pyre's JIT front-end, replacing the shape-specific?-diamond matcher inmajit-translate/src/front/result_exc.rs. The JIT erases the materialisedResult<T, PyError>shell that Charon lowers from Rust source, turning it into bare-Treturns with out-of-band exception propagation (ExitSwitch::LastException) — matching the residual-call ABI the runtime already uses.The end state is
transform_completelyparity: everyResult<T, PyError>callee is transformed (the callee-side type gatetyref_is_result_of_pyerroris the only filter), with no per-callee allowlist. Shapes the rewrite does not yet recognise degrade fail-safe to a residual call (no miscompile), exactly asexceptiontransform.py:212leaves an un-rewritable graph to the residual-call ABI.What changed
103b32223c): deleted theRESULT_EXC_LOWERING_SCOPEallowlist,in_execute_wrapper_family, thePYRE_RESULT_EXC_UNIFORMenv gate, and the strict pure-forward verifier. The exception-link lowering is now unconditional.151a8ab659): reconcile a direct call's RESULT against a void-declared callee (the caller-side mirror of the callee-side unit-return widening), so aResult<(), PyError>call site whose aggregatefront::mirtypesRefno longer panics against aVoid-declared callee (call.py:220parity).bc1c240ce5,e4a4f0ac88): aResultctor consumed locally — including a ctor block whose conditional exit switches on the ctor's own__discriminant(the__new__wrapper'smatch { Ok(obj) } { Ok(o)=>o, Err(e)=>.. }) — is left materialised as an ordinary ADT rather than declining the whole callee.1e9cfeca55,650272596e): replacedlet _ = <fallible call>discards (a non-RPython idiom) with the parity-correct shape — infalliblesetdictvaluefor private-slot stores, or?-propagation where the error must surface (function_setdictalso fixes a latent__dict__-key-store bug).to_exc_objectresidual annotation, SSA phi-collapse restrictions pendingSSA_to_SSI,dont_look_insideon JIT-driver / slow-path machinery, and aString/str/Wtf8 as_bytesreceiver alias.Result
Whole-program result-exc declines reduced to 2 (
sync_python_sys_path, the_io.TextIOWrapper.__exit__discard-Ok closure), both fail-safe to residual.Validation
python3 pyre/check.py: dynasm 166/166 + cranelift 166/166 (2/2 backends, zero regression).Remaining (toward deleting the legacy walker — declines → 0)
sync_python_sys_path: needs a direct module-dict infallible store (setdictvalueis a no-op for modules; thesetattr_strmodule branch usesw_module_get_w_dict)._io.TextIOWrapper.__exit__: a legitimate discard-Ok needing the general call-site forward-substitution rule.SSA_to_SSIport for cross-block annotation threading.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
bool.then(...)closures into short-circuit branching, improving closure-based control flow handling.Result<_, PyError>call paths during translation.Bug Fixes
void-style call results to prevent mismatches in generated code.Tests
bool.thenlowering behavior.