Skip to content

exceptions: PEP 654 except* + reraise/derive/split overridability + implicit __context__ chaining - #625

Merged
youknowone merged 7 commits into
mainfrom
perf-loop
Jul 18, 2026
Merged

youknowone merged 7 commits into
mainfrom
perf-loop

Conversation

@youknowone

@youknowone youknowone commented Jul 17, 2026 •

Copy link
Copy Markdown
Collaborator

Implements PEP 654 except* and hardens exception-chaining and object-construction semantics. Stacked on perf-loop.

Commits

  • except* opcode handling — CHECK_EG_MATCH interp handler (+ validate_check_eg_match_class, naked-exc wrap) and PREP_RERAISE_STAR (IntrinsicFunction2::PrepReraiseStar) porting _prep_reraise_star/_collect_eg_leafs/_exception_group_projection/_is_same_exception_metadata; JIT declines frames carrying either opcode.
  • reraise traceback identity — bare raise (RAISE_VARARGS 0) sets attach_tb=false (RaiseWithExplicitTraceback), cleared one-shot in handle_exception so outer frames still record their own entry; exception_group_same_metadata treats a raw-null meta slot as equal to the None singleton.
  • derive overridability — _derive_and_copy_attrs constructs via the overridable self.derive(excs) and validates the result is a BaseExceptionGroup; default derive builds BaseExceptionGroup(message, excs); __class_getitem__ installed on the group type.
  • split overridability — CHECK_EG_MATCH partial match routes through the overridable self.split(cond) and validates a 2-tuple return.
  • implicit context chaining — any exception raised while another is handled records the active one as __context__ (not only explicit raise); the __context__ write is unified behind chain_context, which breaks context-chain cycles before stamping the slot.
  • object surplus-argument rejection — object.__new__/object.__init__ no longer ignore extra arguments: a type overriding neither slot raises T() takes no arguments, and forwarding excess args to the base slots raises the matching takes exactly one argument TypeError.
  • review fixes — PREP_RERAISE_STAR builds the merged result through BaseExceptionGroup (a mix carrying a bare BaseException stays a BaseExceptionGroup instead of raising TypeError); CHECK_EG_MATCH and the class condition match on the exception type (exception_match) rather than isinstance, so a custom metaclass __instancecheck__ cannot catch/select unrelated leaves.

Verification

  • check.py --backend dynasm,cranelift: 224/224 both backends.
  • cargo test -p pyre-interpreter (dynasm,host_env): 376/0.
  • CPython corpus via caret-avoiding runner: test_except_star 60/60, test_exception_group 50/52 (remaining 2: a deliberate PyPy-faithful .exceptions list-vs-tuple repr, and a test_repr_raises case that fails on stock CPython 3.14.2 too).
  • All review-flagged behaviors probed against CPython 3.14: the two fixed items diverged from CPython (now match); the rest were already CPython-faithful.

Summary by CodeRabbit

  • New Features

    • Added support for Python ExceptionGroup and except* matching, including partial matches and nested exception groups.
    • Improved exception chaining, traceback handling, metadata preservation, and re-raising behavior.
    • Added validation for invalid except* targets and clearer argument errors for object.__new__ and object.__init__.
  • Benchmarks

    • Added coverage demonstrating exception-group matching and re-raise scenarios.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Adds except* and ExceptionGroup support across builtins, interpreter evaluation, opcode dispatch, and JIT handling. It also adds a benchmark covering matching, nested groups, re-raising, invalid targets, and object argument validation.

Changes

Exception-star execution

Layer / File(s) Summary
Exception-group matching and re-raising
pyre/pyre-interpreter/src/builtins.rs
Adds identity-based matching, overridable splitting and derivation, metadata preservation, group projection, and starred re-raise preparation.
Interpreter exception-star control flow
pyre/pyre-interpreter/src/error.rs, pyre/pyre-interpreter/src/eval.rs
Centralizes exception context chaining, validates except* targets, computes group matches, and preserves traceback behavior.
Opcode and JIT integration
pyre/pyre-interpreter/src/pyopcode.rs, pyre/pyre-jit-trace/src/liveness.rs, pyre/pyre-jit/src/eval.rs, pyre/pyre-jit/src/jit/codewriter.rs
Dispatches exception-group instructions, updates stack effects, and routes unsupported shapes through the interpreter.
Exception-star benchmark scenarios
pyre/bench/synth/except_star.py
Exercises matching, nested remainder groups, re-raised groups, and group-target handling.
Object constructor argument validation
pyre/pyre-interpreter/src/typedef.rs
Validates surplus arguments to object.__new__ and object.__init__ based on inherited slots.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Python as except* bytecode
  participant Evaluator as Interpreter evaluator
  participant Groups as ExceptionGroup builtins
  participant JIT as JIT shape filter
  Python->>Evaluator: execute CheckEgMatch
  Evaluator->>Groups: split exception group
  Groups-->>Evaluator: matching and remaining groups
  Evaluator-->>Python: push matching and rest
  Python->>Evaluator: prepare starred re-raise
  Evaluator->>Groups: project and rebuild group
  JIT->>Python: detect unsupported exception-star shape
  JIT-->>Evaluator: execute in plain interpreter
Loading

Possibly related PRs

  • youknowone/pyre#593: Modifies the interpreter exception-handling and traceback pipeline touched by this change.
  • youknowone/pyre#615: Modifies the BaseExceptionGroup and ExceptionGroup machinery used by this change.

Suggested labels: z-ca-2026

Suggested reviewers: lifthrasiir, fregataa

Poem

I’m a rabbit with groups in my burrow tonight,
Matching each leaf with a hop left or right.
Contexts now chain, and star handlers sing,
While JIT paths step back from the spring.
Re-raised groups return neat as can be!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: PEP 654 except* support plus overridable reraising/derivation and implicit context chaining.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch perf-loop

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 22cb898).
Updated: 2026-07-18T04:39:03.119Z

Files in the reviewed diff
pyre/bench/synth/except_star.py
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/error.rs
pyre/pyre-interpreter/src/eval.rs
pyre/pyre-interpreter/src/pyopcode.rs
pyre/pyre-interpreter/src/typedef.rs
pyre/pyre-jit-trace/src/liveness.rs
pyre/pyre-jit/src/eval.rs
pyre/pyre-jit/src/jit/codewriter.rs

1. Regressions to PyPy parity introduced by this patch

  • pyre/pyre-interpreter/src/builtins.rs:5095 ↔ pypy/module/exceptions/app_group.py:123-127 — subgroup/split conditions changed from isinstance(exc, condition) to exception_match(type(exc), condition). This drops metaclass __instancecheck__ behavior that PyPy deliberately honors.

2. Other mismatches introduced by this patch

  • pyre/pyre-interpreter/src/builtins.rs:5257-5273 ↔ pypy/interpreter/pyopcode.py:2157-2159 — overridden split() results are restricted to tuples and accept tuples longer than two items. PyPy uses unpackiterable(..., 2): it accepts iterable results and requires exactly two values.

  • pyre/pyre-interpreter/src/builtins.rs:5154-5158 ↔ pypy/module/exceptions/app_group.py:95-107 — Pyre adds a mandatory BaseExceptionGroup result check for overridden derive(). PyPy does not validate that return type before copying attributes, so custom derive behavior and its resulting exception differ.

  • pyre/pyre-interpreter/src/builtins.rs:5364-5365 ↔ pypy/module/exceptions/app_group.py:151-158 — projection bypasses the overridable eg.split(...) method by calling the internal splitter directly. PyPy calls eg.split(lambda ...); subclasses overriding split observe different behavior.

  • pyre/pyre-interpreter/src/eval.rs:1127-1137 ↔ pypy/interpreter/pyopcode.py:2131-2142 — except-star target validation uses override-aware issubclass, whereas PyPy uses issubtype_w. A __subclasscheck__ result can therefore incorrectly make Pyre reject an otherwise valid target as an exception group.

  • pyre/pyre-interpreter/src/error.rs:290-291 ↔ pypy/interpreter/error.py:432-434,478-508 — _break_context_cycle failures are discarded and Pyre still writes __context__; PyPy propagates the failure through context recording rather than silently continuing.

  • pyre/pyre-interpreter/src/eval.rs:1224 ↔ pypy/interpreter/pyopcode.py:75-95 — context chaining runs even when attach_tb is false. PyPy’s RaiseWithExplicitTraceback branch bypasses record_context; this can stamp a fresh context during an explicit-traceback re-raise.

3. Pre-existing mismatches (already present before this patch)

  • pyre/pyre-interpreter/src/builtins.rs:5022-5031 ↔ pypy/module/exceptions/app_group.py:7-10 — Pyre accepts any object exposing __len__ and __getitem__ as an exception sequence; PyPy requires collections.abc.Sequence unless it is already a list or tuple.

  • pyre/pyre-interpreter/src/builtins.rs:5132-5136 ↔ pypy/module/exceptions/app_group.py:97-103 — Pyre suppresses every error while reading __notes__ or converting it with fixedview; PyPy only ignores TypeError from list(self.__notes__) and otherwise propagates lookup/conversion errors.

4. Structural adaptations

  • pyre/pyre-interpreter/src/builtins.rs:5247-5249 ↔ pypy/interpreter/pyopcode.py:2153-2155 — Pyre constructs the naked-exception wrapper through BaseExceptionGroup, allowing constructor promotion or preservation for non-Exception leaves. PyPy’s 3.11-era path explicitly calls ExceptionGroup; this is a Python-version/CPython-compatibility adaptation.

  • pyre/pyre-interpreter/src/builtins.rs:5337-5346 ↔ pypy/module/exceptions/app_group.py:145-156 — raw object addresses implement PyPy’s identity_dict leaf membership. This is a Rust object-identity representation adaptation.

  • pyre/pyre-interpreter/src/pyopcode.rs:1463-1468 ↔ pypy/interpreter/pyopcode.py:1873-1879 — Pyre handles re-raise preparation as CPython-compatible CALL_INTRINSIC_2(PrepReraiseStar), while PyPy uses its dedicated PREP_RERAISE_STAR opcode.

  • pyre/pyre-jit/src/eval.rs:4423-4430 ↔ pypy/interpreter/pyopcode.py:1853-1879 — frames containing exception-group matching or re-raise preparation are intentionally interpreted rather than traced. This is a temporary Rust JIT coverage limitation, not an interpreter semantic divergence.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f90dea6a80

ℹ️ 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".

Comment thread pyre/pyre-interpreter/src/builtins.rs Outdated
return Ok((group, pyre_object::w_none()));
}
if crate::baseobjspace::isinstance(w_exc, base_group)? {
return exception_group_split_inner(w_exc, &ExceptionGroupCondition::Class(w_type));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Dispatch CHECK_EG_MATCH through split()

When w_exc is a BaseExceptionGroup subclass whose split() is overridden, this partial-match path bypasses the method and directly walks the internal fields. CPython/PyPy dispatch w_eg.split(w_typ) for this opcode, so a subclass whose split() raises or returns (None, self) will still be split and enter the except* handler here instead of observing the subclass behavior.

Useful? React with 👍 / 👎.

Comment on lines +5253 to +5255
if !match (left_notes, right_notes) {
(Some(left), Some(right)) => std::ptr::eq(left, right),
(None, None) => true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not classify copied notes as a new raise

For an original ExceptionGroup that has __notes__, the subgroups produced by split() get a copied notes list, so this pointer comparison marks a bare raise from an except* handler as a newly raised exception rather than a reraised part of orig. If another except* clause raises a new exception, PREP_RERAISE_STAR will then emit the reraised subgroup in source-list order instead of projecting it from orig and appending it after the new exceptions, changing the resulting group shape/order.

Useful? React with 👍 / 👎.

…RERAISE_STAR)

Add the CHECK_EG_MATCH opcode: split the current exception group by the
except* match type, wrapping a naked matching exception in a group, and
reject catching a BaseExceptionGroup subclass with except*. Add the
PREP_RERAISE_STAR intrinsic (CALL_INTRINSIC_2): recombine the per-handler
leftover exceptions with the unhandled rest via identity-based group
projection. The match/projection/reraise algorithms are ported from
module/exceptions/app_group.py, reusing the ExceptionGroup split helpers.

Decline JIT tracing of frames containing CHECK_EG_MATCH or the
PREP_RERAISE_STAR intrinsic (run them in the plain interpreter).

Add a synthetic parity bench exercising except* over constructed groups.

Assisted-by: Claude
RAISE_VARARGS(0) (bare `raise`) now re-raises the active exception with
attach_tb=false, so the frame performing the reraise does not stamp a
fresh traceback entry and the exception's w_traceback identity is
preserved (mirroring the RERAISE opcode).

handle_exception clears attach_tb after the current frame's record/trace
decision, so an exception that finds no handler and propagates to the
caller records the outer frame's traceback entry instead of dropping it.

exception_group_same_metadata compares __traceback__/__cause__/__context__
treating an unset (raw null) slot and the None singleton as equal, matching
the attribute-layer normalization _is_same_exception_metadata relies on.

Fixes except* reraise reconstruction losing the group message and the
outer-frame traceback entry being dropped on reraise propagation.

Assisted-by: Claude
…erive

_derive_and_copy_attrs now constructs the sub-result via the (overridable)
`derive` method instead of a hardcoded constructor call, so a subclass that
overrides `derive` controls reconstruction (e.g. threading extra constructor
args). The result is validated to be a BaseExceptionGroup instance, raising
TypeError("derive must return an instance of BaseExceptionGroup") otherwise.

The default `derive` constructs a BaseExceptionGroup (promoted to
ExceptionGroup when every leaf is an Exception) rather than `type(self)`, so a
subclass with extra constructor args that does not override `derive` falls
back to the base class in split/subgroup instead of failing on the unmatched
constructor signature.

Install `__class_getitem__` (generic_alias_class_getitem) on the exception
group type so `ExceptionGroup[...]` subscription yields a generic alias.

Assisted-by: Claude
@youknowone youknowone changed the title exceptions: implement except* opcode handling (CHECK_EG_MATCH + PREP_RERAISE_STAR) exceptions: except* (CHECK_EG_MATCH + PREP_RERAISE_STAR) + reraise traceback identity + derive-override Jul 18, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7b81cad234

ℹ️ 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".

Comment thread pyre/pyre-interpreter/src/builtins.rs Outdated
w_type: PyObjectRef,
) -> Result<(PyObjectRef, PyObjectRef), crate::PyError> {
let base_group = lookup_exc_class("BaseExceptionGroup").unwrap();
if crate::baseobjspace::isinstance(w_exc, w_type)? {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use exception_match for except target tests*

When the except* target is an exception class with a custom metaclass __instancecheck__, this general isinstance path can run user code and report a match for an exception whose actual type is not a subclass. The normal except handler in this file uses check_exc_match_against/exception_match, and CHECK_EG_MATCH should use the same exception-matching semantics; otherwise except* E can catch and wrap unrelated naked exceptions, and the split path below can select unrelated leaves from an exception group.

Useful? React with 👍 / 👎.

Comment thread pyre/pyre-interpreter/src/builtins.rs Outdated
Comment on lines +5380 to +5383
let exception_group = lookup_exc_class("ExceptionGroup").unwrap();
let message = unsafe { pyre_object::w_str_new("") };
let list = pyre_object::w_list_new(raised);
exception_group_new(&[exception_group, message, list])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Build BaseExceptionGroup for mixed except results*

When multiple except* handlers produce more than one outgoing exception and any one is a non-Exception BaseException (for example one handler raises KeyboardInterrupt while another reraises a remainder), this hardcodes ExceptionGroup. exception_group_new rejects non-Exception leaves for ExceptionGroup, so PREP_RERAISE_STAR raises TypeError instead of returning a BaseExceptionGroup('', [...]); constructing through BaseExceptionGroup here would still promote to ExceptionGroup when all leaves are regular Exceptions.

Useful? React with 👍 / 👎.

exception_group_match now performs the partial-match branch by calling the
(overridable) `split` method on the group and validating the result, instead
of calling the internal split helper directly. A subclass that overrides
`split` is honored; a return value that is not a tuple raises
TypeError("split must return a tuple, not X"), and a tuple shorter than 2
raises TypeError("split must return a 2-tuple, got tuple of size N"). Tuples
longer than 2 are accepted for backwards compatibility, using the first two
elements as (match, rest).

Assisted-by: Claude
Any exception raised while another is being handled now records the
active exception as its __context__, not only an explicit `raise`.
handle_exception calls the shared chain_context helper after
materializing the exception object, so builtin/operator-raised
exceptions carry __context__ like CPython/PyPy.

Unify the __context__ write behind chain_context, which runs
_break_context_cycle before stamping the slot. attach_raise_cause
previously set __context__ without breaking cycles, so a re-raise that
formed a context cycle (raise of an already-handled exception) left the
back-link intact.

Assisted-by: Claude
@youknowone youknowone changed the title exceptions: except* (CHECK_EG_MATCH + PREP_RERAISE_STAR) + reraise traceback identity + derive-override exceptions: PEP 654 except* + reraise/derive/split overridability + implicit __context__ chaining Jul 18, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c5f20dd4d2

ℹ️ 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".

Comment on lines +5361 to +5362
let (matching, _) =
exception_group_split_inner(w_group, &ExceptionGroupCondition::Identity(addresses))?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Dispatch reraised projection through split()

When the original group is a BaseExceptionGroup subclass whose split() override handles callable predicates, PREP_RERAISE_STAR still bypasses that override here by walking the internal tree with exception_group_split_inner. The PyPy helper this ports (_exception_group_projection) dispatches eg.split(lambda element: ...), so a bare raise from an except* handler can silently reconstruct and re-raise a subgroup instead of observing the subclass's split() result or exception.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/builtins.rs (1)

5086-5097: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not retain leaf identity as raw addresses across reconstruction.

exception_group_projection stores PyObjectRef addresses as usize, then enters splitting that can call overridable derive methods and trigger a moving collection. Relocated leaves no longer match the stale addresses, silently dropping reraised exceptions from the projection.

Keep the leaves in a GC-rooted, relocation-aware container and compare their refreshed references by identity. This also avoids the current quadratic Vec::contains representation.

As per coding guidelines, maintain strict structural parity and preserve the upstream storage shape and semantics.

Also applies to: 5326-5363

🤖 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 5086 - 5097, The
ExceptionGroupCondition::Identity representation must not store PyObjectRef
values as raw usize addresses across exception_group_projection splitting,
because derive calls may relocate objects. Replace the address Vec with the
upstream GC-rooted, relocation-aware storage shape, and update
ExceptionGroupCondition::matches plus the related projection code around lines
5326–5363 to compare refreshed references by identity while preserving
structural parity and existing semantics.

Source: Coding guidelines

🤖 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 5400-5403: Update the merged exception construction in the
surrounding reraised-remainder helper to look up and pass BaseExceptionGroup
instead of ExceptionGroup. Preserve the existing empty message and raised list
arguments so BaseExceptionGroup can promote to ExceptionGroup only when all
children are ordinary exceptions.

In `@pyre/pyre-interpreter/src/error.rs`:
- Around line 288-291: Update the context-slot check in the exception handling
flow around _break_context_cycle to treat both raw NULL and the Python None
singleton as unset. When either value is present, continue breaking the cycle
and set the active exception as the context, preserving Python exception-chain
semantics.

---

Outside diff comments:
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 5086-5097: The ExceptionGroupCondition::Identity representation
must not store PyObjectRef values as raw usize addresses across
exception_group_projection splitting, because derive calls may relocate objects.
Replace the address Vec with the upstream GC-rooted, relocation-aware storage
shape, and update ExceptionGroupCondition::matches plus the related projection
code around lines 5326–5363 to compare refreshed references by identity while
preserving structural parity and existing semantics.
🪄 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: 979eecfa-1a0d-4c38-9157-fdc76fde772c

📥 Commits

Reviewing files that changed from the base of the PR and between 5bbd3da and c5f20dd.

📒 Files selected for processing (8)
  • pyre/bench/synth/except_star.py
  • pyre/pyre-interpreter/src/builtins.rs
  • pyre/pyre-interpreter/src/error.rs
  • pyre/pyre-interpreter/src/eval.rs
  • pyre/pyre-interpreter/src/pyopcode.rs
  • pyre/pyre-jit-trace/src/liveness.rs
  • pyre/pyre-jit/src/eval.rs
  • pyre/pyre-jit/src/jit/codewriter.rs

Comment thread pyre/pyre-interpreter/src/builtins.rs Resolved
Comment on lines +288 to +291
let existing = unsafe { pyre_object::interp_exceptions::w_exception_get_context(exc) };
if existing.is_null() {
let _ = _break_context_cycle(exc, active);
unsafe { pyre_object::interp_exceptions::w_exception_set_context(exc, active) };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Treat None as an unset context slot.

_break_context_cycle can store the real None singleton, but this condition only accepts raw NULL. Raising that exception again therefore fails to attach the active exception as __context__, producing an incorrect exception chain.

Proposed fix
     let existing = unsafe { pyre_object::interp_exceptions::w_exception_get_context(exc) };
-    if existing.is_null() {
+    if existing.is_null() || unsafe { pyre_object::is_none(existing) } {
         let _ = _break_context_cycle(exc, active);
         unsafe { pyre_object::interp_exceptions::w_exception_set_context(exc, active) };
     }

As per coding guidelines, the interpreter must preserve Python semantics.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let existing = unsafe { pyre_object::interp_exceptions::w_exception_get_context(exc) };
if existing.is_null() {
let _ = _break_context_cycle(exc, active);
unsafe { pyre_object::interp_exceptions::w_exception_set_context(exc, active) };
let existing = unsafe { pyre_object::interp_exceptions::w_exception_get_context(exc) };
if existing.is_null() || unsafe { pyre_object::is_none(existing) } {
let _ = _break_context_cycle(exc, active);
unsafe { pyre_object::interp_exceptions::w_exception_set_context(exc, active) };
🤖 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/error.rs` around lines 288 - 291, Update the
context-slot check in the exception handling flow around _break_context_cycle to
treat both raw NULL and the Python None singleton as unset. When either value is
present, continue breaking the cycle and set the active exception as the
context, preserving Python exception-chain semantics.

Source: Coding guidelines

…lots

object.__new__ and object.__init__ ignored extra positional arguments.
A type that overrides neither slot now raises `T() takes no arguments`
from __new__, matching the object_descr_new/object_descr_init argument
checks: surplus args are accepted only when __new__ or __init__ is
overridden, and forwarding excess args to object.__new__/object.__init__
raises the corresponding "takes exactly one argument" TypeError.

Assisted-by: Claude
…e merge

Address PR review findings on the except* path:

- CHECK_EG_MATCH and the ExceptionGroup class condition now match on the
  exception's type (exception_match) instead of isinstance, so a matcher
  class with a custom metaclass __instancecheck__ cannot catch or select
  unrelated leaves.
- PREP_RERAISE_STAR builds the merged result through BaseExceptionGroup
  instead of ExceptionGroup, so a result carrying a bare BaseException
  (e.g. a reraised KeyboardInterrupt beside a freshly raised Exception)
  stays a BaseExceptionGroup rather than raising TypeError; the
  constructor still promotes to ExceptionGroup when every leaf is an
  Exception.

Assisted-by: Claude

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 13743-13767: Update object_descr_init to explicitly reject an
empty args slice before the existing surplus-argument logic, raising the
Python-compatible TypeError for a missing self argument. Preserve the current
handling for one or more arguments and the inherited __init__/__new__ validation
for surplus arguments.
🪄 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: aee8df01-e874-4450-9f65-46f7f0795c98

📥 Commits

Reviewing files that changed from the base of the PR and between c5f20dd and 22cb898.

📒 Files selected for processing (2)
  • pyre/pyre-interpreter/src/builtins.rs
  • pyre/pyre-interpreter/src/typedef.rs

Comment on lines +13743 to +13767
/// `object.__init__(self)` — no-op base __init__. Surplus arguments are
/// accepted only when __init__ or __new__ is overridden (objectobject.py
/// descr__init__); otherwise the bare object initializer takes none.
fn object_descr_init(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> {
if args.len() > 1 {
let w_obj = args[0];
if let Some(w_type) = crate::typedef::r#type(w_obj) {
let obj = w_object();
let tp_init = unsafe { crate::baseobjspace::lookup_in_type(w_type, "__init__") };
let obj_init = unsafe { crate::baseobjspace::lookup_in_type(obj, "__init__") };
if !same_inherited_slot(tp_init, obj_init) {
return Err(crate::PyError::type_error(
"object.__init__() takes exactly one argument (the instance to initialize)",
));
}
let tp_new = unsafe { crate::baseobjspace::lookup_in_type(w_type, "__new__") };
let obj_new = unsafe { crate::baseobjspace::lookup_in_type(obj, "__new__") };
if same_inherited_slot(tp_new, obj_new) {
let name = unsafe { pyre_object::w_type_get_name(w_type) };
return Err(crate::PyError::type_error(format!(
"{name}.__init__() takes exactly one argument (the instance to initialize)"
)));
}
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Missing args.is_empty() validation.

Unlike object_descr_new, which correctly validates that at least one argument (the type) is provided, object_descr_init lacks an args.is_empty() check. If object.__init__() is called directly with zero arguments, args.len() > 1 evaluates to false, and the function will silently return w_none() instead of raising a TypeError for the missing self argument.

Unless the descriptor dispatch mechanism guarantees that args is never empty for __init__ before this function is invoked, you should add an explicit check to mirror Python's behavior (TypeError: descriptor '__init__' of 'object' object needs an argument).

🛡️ Proposed fix
 fn object_descr_init(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> {
+    if args.is_empty() {
+        return Err(crate::PyError::type_error(
+            "descriptor '__init__' of 'object' object needs an argument",
+        ));
+    }
     if args.len() > 1 {
         let w_obj = args[0];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// `object.__init__(self)` — no-op base __init__. Surplus arguments are
/// accepted only when __init__ or __new__ is overridden (objectobject.py
/// descr__init__); otherwise the bare object initializer takes none.
fn object_descr_init(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> {
if args.len() > 1 {
let w_obj = args[0];
if let Some(w_type) = crate::typedef::r#type(w_obj) {
let obj = w_object();
let tp_init = unsafe { crate::baseobjspace::lookup_in_type(w_type, "__init__") };
let obj_init = unsafe { crate::baseobjspace::lookup_in_type(obj, "__init__") };
if !same_inherited_slot(tp_init, obj_init) {
return Err(crate::PyError::type_error(
"object.__init__() takes exactly one argument (the instance to initialize)",
));
}
let tp_new = unsafe { crate::baseobjspace::lookup_in_type(w_type, "__new__") };
let obj_new = unsafe { crate::baseobjspace::lookup_in_type(obj, "__new__") };
if same_inherited_slot(tp_new, obj_new) {
let name = unsafe { pyre_object::w_type_get_name(w_type) };
return Err(crate::PyError::type_error(format!(
"{name}.__init__() takes exactly one argument (the instance to initialize)"
)));
}
}
}
fn object_descr_init(args: &[PyObjectRef]) -> Result<PyObjectRef, crate::PyError> {
if args.is_empty() {
return Err(crate::PyError::type_error(
"descriptor '__init__' of 'object' object needs an argument",
));
}
if args.len() > 1 {
let w_obj = args[0];
if let Some(w_type) = crate::typedef::r#type(w_obj) {
let obj = w_object();
let tp_init = unsafe { crate::baseobjspace::lookup_in_type(w_type, "__init__") };
let obj_init = unsafe { crate::baseobjspace::lookup_in_type(obj, "__init__") };
if !same_inherited_slot(tp_init, obj_init) {
return Err(crate::PyError::type_error(
"object.__init__() takes exactly one argument (the instance to initialize)",
));
}
let tp_new = unsafe { crate::baseobjspace::lookup_in_type(w_type, "__new__") };
let obj_new = unsafe { crate::baseobjspace::lookup_in_type(obj, "__new__") };
if same_inherited_slot(tp_new, obj_new) {
let name = unsafe { pyre_object::w_type_get_name(w_type) };
return Err(crate::PyError::type_error(format!(
"{name}.__init__() takes exactly one argument (the instance to initialize)"
)));
}
}
}
🤖 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/typedef.rs` around lines 13743 - 13767, Update
object_descr_init to explicitly reject an empty args slice before the existing
surplus-argument logic, raising the Python-compatible TypeError for a missing
self argument. Preserve the current handling for one or more arguments and the
inherited __init__/__new__ validation for surplus arguments.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22cb898301

ℹ️ 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 reraised_group = exception_group_projection(w_orig, &reraised)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid projecting when nothing is reraised

When an except* clause handles an entire BaseExceptionGroup subclass without a bare reraise (for example except* Exception: pass or raising a replacement exception), reraised is empty, but this still calls exception_group_projection. That helper walks w_orig and invokes the subclass's overridable derive() to build the ignored nonmatching side, so a side-effecting or raising derive() can run even though CPython returns None or the newly raised exception without deriving any subgroup; short-circuit the empty-reraised case before projecting.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant