Implement more except* - #6545
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughReimplements except* support across compiler and VM: new SetExcInfo bytecode and frame handling, multi-block codegen for except*/reraise, plus VM exception-group validation, projection, and reraise helpers. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Codegen as Codegen (compile.rs)
participant BC as Bytecode (bytecode.rs)
participant Frame as VM Frame (frame.rs)
participant Exc as Exceptions (exceptions.rs)
rect rgb(235, 245, 255)
Note over Codegen,BC: Compile-time except* lowering
Codegen->>Codegen: Analyze except* patterns (validate no break/continue/return)
Codegen->>BC: Emit multi-block flow, PrepReraiseStar, SetExcInfo
end
rect rgb(235, 255, 235)
Note over Frame,Exc: Runtime matching and reraise
Frame->>Frame: Execute handler entry (SET_EXC_INFO reads TOS)
Frame->>Exc: Call exception_group_match / prep_reraise_star
Exc->>Exc: Validate types, split/project, collect leaf ids
Exc-->>Frame: Return matched/unmatched (exception or ExceptionGroup)
Frame->>Frame: Route to matched handler, else, finally, or reraise path
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
| When you modify bytecode instructions, a full clean is required: | ||
|
|
||
| ```bash | ||
| rm -rf target/debug/build/rustpython-* && find . | grep -E "(__pycache__|\.pyc$)" | xargs rm -rf |
There was a problem hiding this comment.
| rm -rf target/debug/build/rustpython-* && find . | grep -E "(__pycache__|\.pyc$)" | xargs rm -rf | |
| rm -r target/debug/build/rustpython-* && find . | grep -E "(__pycache__|\.pyc$)" | xargs rm -r |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)crates/vm/src/exceptions.rs (1)2538-2604: prep_reraise_star leaf‑ID projection matches the design; update the derive() comment
The revamped prep_reraise_star looks robust:
- Handles the non‑group “naked exception” case early, returning the first non‑None entry.
- Classifies items in excs into “raised” vs “reraised” by walking exception‑group leaves and comparing object IDs, which is a stable way to track identity through split() / restructuring.
- Uses exception_group_projection + split_by_leaf_ids to project reraised leaves back onto the original group’s shape, and then combines that result with any newly raised exceptions, returning either a single exception or a new ExceptionGroup as appropriate.
- All paths handle None safely and short‑circuit when there’s nothing to reraise.
One small inconsistency is the comment in split_by_leaf_ids:
// Reconstruct using derive() to preserve the exception group typePer the existing design in exception_group::types::PyBaseExceptionGroup::derive, the helper intentionally always constructs a BaseExceptionGroup rather than preserving the original subclass type, so this comment is misleading for future readers. Consider rephrasing to something like “reconstruct using derive() to preserve the structure (but not necessarily the concrete group subclass).”
Based on learnings, derive() is intentionally not type‑preserving.
Also applies to: 2606-2712
crates/vm/src/exceptions.rs (1)📜 Review detailscrates/vm/src/frame.rs (1)2436-2468: except match-type validation and split() contract look sound*
The new check_except_star_type_valid helper and its use in exception_group_match correctly enforce:
- match types must derive from BaseException, including tuple elements.
- ExceptionGroup/BaseExceptionGroup are rejected for except* in favor of plain except.
The added runtime checks around exc_value.split(match_type) (type must be tuple, length at least 2) are defensive and prevent misbehaving custom groups from corrupting control flow. Only minor nit: the error message says “2‑tuple” but the code only rejects tuples with length < 2; not wrong in practice since builtin ExceptionGroup.split always returns length‑2, but could be tightened if you want the contract to be exact.
Also applies to: 2484-2486, 2514-2519
4-5: VM handling of SetExcInfo correctly updates the active exception
The new arm in execute_instruction:
- Peeks at TOS without popping and, when it is a PyBaseException, sets it as the current exception via vm.set_exception(Some(exc.to_owned())).
- No‑ops cleanly when TOS is not an exception, which keeps the VM stable even if codegen misfires.
Behavior is correct for except* handlers that want bare raise to re‑raise the matched exception. If you’d like stricter debugging, you could add a debug_assert!(exc.downcast_ref::<PyBaseException>().is_some()) around these sites to catch invalid SetExcInfo emissions during development, but it’s not required for correctness.
Also applies to: 1358-1365
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between b704f42 and ea4e001.
📒 Files selected for processing (6)- .github/copilot-instructions.md
- crates/codegen/src/compile.rs
- crates/codegen/src/error.rs
- crates/compiler-core/src/bytecode.rs
- crates/vm/src/exceptions.rs
- crates/vm/src/frame.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/codegen/src/error.rs
- crates/codegen/src/compile.rs
- crates/vm/src/frame.rs
- crates/compiler-core/src/bytecode.rs
- crates/vm/src/exceptions.rs
Learnt from: youknowone Repo: RustPython/RustPython PR: 6358 File: crates/vm/src/exception_group.rs:173-185 Timestamp: 2025-12-09T08:46:58.660Z Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Run `cargo test --workspace --exclude rustpython_wasm` to execute Rust unit tests
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/test/**/*.py : Only acceptable modifications to test files are: (1) Removing `unittest.expectedFailure` decorators and upper TODO comments when tests actually pass, (2) Adding `unittest.expectedFailure` decorators when tests cannot be fixed
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to files in the `Lib/` directory
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/test/**/*.py : NEVER comment out or delete test code lines except for removing `unittest.expectedFailure` decorators and upper TODO comments
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Use `cargo run -- script.py` instead of the standard `python` command to run and test Python code with RustPython
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to **/*.py : In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
- .github/copilot-instructions.md
Learnt from: youknowone Repo: RustPython/RustPython PR: 6358 File: crates/vm/src/exception_group.rs:173-185 Timestamp: 2025-12-09T08:46:58.660Z Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
- crates/codegen/src/compile.rs
- crates/vm/src/frame.rs
- crates/vm/src/exceptions.rs
crates/stdlib/src/openssl.rs (1)crates/compiler-core/src/bytecode.rs (1)
- exc (3740-3740)
crates/vm/src/exceptions.rs (2)crates/vm/src/exceptions.rs (1)
- w (1730-1730)
- w (1731-1731)
crates/vm/src/exception_group.rs (1)🔇 Additional comments (7)
- exception_group (35-40)
.github/copilot-instructions.md (1)crates/codegen/src/error.rs (1)76-82: Clean-build instructions for bytecode edits are clear and consistent
The added section gives a precise, reproducible clean command for bytecode changes and fits well with the surrounding Lib/ guidance. No issues from a tooling or safety perspective.
crates/compiler-core/src/bytecode.rs (1)91-92: New BreakContinueReturnInExceptStar error variant is wired correctly
The enum variant and its Display arm are added consistently with existing error types, with a clear diagnostic string that will surface nicely at compile time. No structural or behavioral issues.
Also applies to: 153-158
crates/codegen/src/compile.rs (4)868-871: SetExcInfo opcode is cleanly integrated into the bytecode model
- Adding SetExcInfo at the end of Instruction and updating LAST_INSTRUCTION keeps the marshaling range consistent.
- stack_effect returns 0, matching the “peek‑only” semantics.
- Disassembly uses SET_EXC_INFO, which aligns with the other mnemonics.
No additional wiring issues spotted.
Also applies to: 875-875, 1749-1750, 1982-1983
1583-1617: LGTM: Break statement validation for except blocks.*
The control flow correctly validates that break cannot occur inside an ExceptionGroupHandler block before reaching a loop. The Result<Option<BlockIdx>, ()> approach cleanly handles the three cases: found loop, no loop, and forbidden context.
1620-1657: LGTM: Continue statement validation mirrors break handling.
The implementation correctly mirrors the break validation logic, using fb_block (loop start) for continue vs fb_exit (loop end) for break.
1665-1676: LGTM: Return statement check in except blocks.*
Unlike break/continue which need to find a loop first, return is unconditionally forbidden anywhere within an ExceptionGroupHandler, and this implementation correctly checks for any such handler in the fblock stack.
2156-2426: The except* implementation correctly handles exception group matching, handler execution, exception accumulation, and final reraise. All ListAppend index calculations are correct and properly target the exception results list based on the documented stack states.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)crates/codegen/src/compile.rs (2)📜 Review details1582-1619: Break inside except now correctly rejected while preserving nested-loop behavior*
The new fblock scan for break:
- Correctly targets the innermost WhileLoop/ForLoop when present, even inside an except* handler with inner loops.
- Emits InvalidBreak when no loop exists, preserving existing semantics.
- Emits BreakContinueReturnInExceptStar when an ExceptionGroupHandler is encountered before any loop, which matches the CPython restriction for top‑level break in an except* suite.
This looks right and integrates cleanly with the existing FBlockType usage. If you like, the found_loop pattern here and in Continue could be factored into a small helper to avoid duplication, but that’s optional.
1622-1657: Continue inside except is validated symmetrically to break*
The continue handling mirrors the new break logic:
- Uses the same reverse fblock scan to find the innermost loop block.
- Yields InvalidContinue outside any loop.
- Produces BreakContinueReturnInExceptStar when the first relevant fblock is an ExceptionGroupHandler, while still allowing continue in loops nested inside the handler.
The symmetry with the break implementation keeps behavior predictable and should match CPython’s constraints on except* blocks.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between ea4e001 and d8cda04.
⛔ Files ignored due to path filters (1)- Lib/test/test_except_star.py is excluded by !Lib/**
- .github/copilot-instructions.md
- crates/codegen/src/compile.rs
- crates/codegen/src/error.rs
- crates/compiler-core/src/bytecode.rs
- crates/vm/src/exceptions.rs
- crates/vm/src/frame.rs
- crates/codegen/src/error.rs
- crates/vm/src/frame.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/compiler-core/src/bytecode.rs
- crates/codegen/src/compile.rs
- crates/vm/src/exceptions.rs
Learnt from: youknowone Repo: RustPython/RustPython PR: 6358 File: crates/vm/src/exception_group.rs:173-185 Timestamp: 2025-12-09T08:46:58.660Z Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Run `cargo test --workspace --exclude rustpython_wasm` to execute Rust unit tests
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/test/**/*.py : Only acceptable modifications to test files are: (1) Removing `unittest.expectedFailure` decorators and upper TODO comments when tests actually pass, (2) Adding `unittest.expectedFailure` decorators when tests cannot be fixed
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to files in the `Lib/` directory
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/test/**/*.py : NEVER comment out or delete test code lines except for removing `unittest.expectedFailure` decorators and upper TODO comments
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Use `cargo run -- script.py` instead of the standard `python` command to run and test Python code with RustPython
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to **/*.py : In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
- .github/copilot-instructions.md
Learnt from: youknowone Repo: RustPython/RustPython PR: 6358 File: crates/vm/src/exception_group.rs:173-185 Timestamp: 2025-12-09T08:46:58.660Z Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
- crates/codegen/src/compile.rs
- crates/vm/src/exceptions.rs
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
crates/vm/src/exceptions.rs (10).github/copilot-instructions.md (1)2436-2468: LGTM!
The validation logic correctly enforces except* semantics: match types must be BaseException subclasses but cannot be ExceptionGroup/BaseExceptionGroup. Error messages are clear and match Python's behavior.
2484-2485: Good addition of validation.
The early validation ensures that except* handlers reject ExceptionGroup types, which correctly implements Python 3.11+ semantics.
2513-2519: Defensive type checking added.
This validation ensures that user-defined split() methods return the correct type, catching errors early with a clear message.
2548-2559: Correct handling of edge cases.
The early return for empty lists and special handling for naked (non-group) exceptions correctly implements the except* semantics where only one except* clause can execute for a naked exception.
2569-2575: Good use of leaf ID comparison.
Using is_exception_from_orig to distinguish raised from reraised exceptions is a robust approach that relies on object identity rather than mutable metadata.
2582-2603: Correct projection and combination logic.
The projection preserves the original nested structure for reraised exceptions, and the combination logic correctly handles single vs. multiple exceptions, matching Python's except* behavior.
2606-2624: LGTM!
The leaf ID comparison approach correctly identifies reraised exceptions by checking if all of the exception's leaf IDs are present in the original group, relying on the fact that split() preserves leaf exception identity.
2626-2650: LGTM!
The recursive collection correctly identifies leaf exceptions by checking if an exception is not an exception group, and safely traverses nested structures using if-let chains.
2652-2672: LGTM!
The projection function correctly collects leaf IDs from the keep list and delegates to split_by_leaf_ids for the actual recursive splitting, maintaining a clean separation of concerns.
2674-2712: LGTM!
The recursive splitting correctly preserves the nested structure using the derive() method. Per the learnings, derive() intentionally creates BaseExceptionGroup instances rather than preserving the original exception class type, which is a deliberate design decision.
Based on learnings, this design differs from CPython but is intentional in RustPython.
crates/compiler-core/src/bytecode.rs (1)76-82: Clean-build instructions look good and match prior suggested workflow
The new “Clean Build” snippet is accurate for forcing a rebuild after bytecode instruction changes (wiping rustpython build artifacts and Python bytecode). No issues from a tooling or safety perspective; this should help avoid stale-instruction failures.
crates/codegen/src/compile.rs (2)868-876: SetExcInfo opcode is wired consistently (enum, marshaling, stack effect, disassembly)
The new SetExcInfo instruction is correctly integrated:
- Added as the last Instruction variant and LAST_INSTRUCTION updated, keeping TryFrom<u8>/marshaling valid.
- stack_effect returns 0, matching the “does not pop the value” behavior.
- Disassembler prints SET_EXC_INFO, consistent with other op names.
No issues spotted here.
Also applies to: 1749-1750, 1982-1983
1665-1676: Return is correctly disallowed anywhere within an except handler frame*
The added check for FBlockType::ExceptionGroupHandler before emitting a return:
- Ensures return is rejected anywhere in the dynamic extent of an except* handler (including inside nested loops), which aligns with CPython’s semantics.
- Still permits return in the try body, else, finally, and in nested functions defined inside an except* since those have their own CodeInfo/fblock stacks.
This is a good, minimal way to enforce the restriction using the existing fblock mechanism.
2156-2423: except compilation flow matches CPython’s multi-block design and maintains stack invariants*
The reworked compile_try_star_except looks internally consistent and closely follows CPython’s strategy:
Stack protocol:
- At handler entry, [exc] → [orig, list, rest] via BUILD_LIST(0) and COPY(2), giving a stable orig and mutable rest plus an aggregation list.
- For each handler, CHECK_EG_MATCH transforms [orig, list, rest, type] into [orig, list, new_rest, match], and all subsequent paths (no match, normal handler completion, and handler raising) end with [orig, list, new_rest], ready for the next handler.
- After the last handler, ListAppend { i: 0 } appends the final rest to the list, and PrepReraiseStar(orig, list) is invoked with exactly two arguments, returning a single result.
Handler semantics:
- Unparenthesized tuple types and except* without a type are rejected with clear SyntaxErrors during codegen.
- SetExcInfo is issued before running the handler body so bare raise re-raises the correct match.
- Handlers are wrapped in an inner SetupExcept with a dedicated handler_except_block:
- Normal completion: no new exception; match is dropped and None is appended with ListAppend { i: 1 }.
- Raised in handler: the raised exception is appended with ListAppend { i: 2 } and match is dropped.
- Name binding (as name) is cleaned up in both normal and exceptional paths by resetting to None and deleting the variable, matching standard except behavior.
- FBlockType::ExceptionGroupHandler is pushed only around the handler body and popped immediately afterward, giving break/continue/return the necessary context while not leaking into surrounding code.
Reraise / no‑reraise paths:
- The result is not None test uses identity (is not None) via IsOp(Invert::Yes), and if true branches to a Raise { kind: Raise } with result still on the stack—effectively raise result.
- When result is None, result is popped and the original try* exception is cleaned up with PopException, after which (if a finally exists) the code performs PopBlock + EnterFinally and jumps to a shared end_block.
Else / finally integration:
- The try-else path is unchanged structurally: on success, SetupExcept is popped, else_block is executed, and for a present finally we again use PopBlock + EnterFinally before jumping to end_block.
- end_block serves as a common join point for “no re‑raise” and “else” paths; finally_block is chained after it and runs finalbody followed by EndFinally.
Given how subtle these flows are, especially with nested except*, finally, and different ways of raising (bare raise, raise e, new exceptions from handlers), it’s worth making sure tests cover:
- try/except* with and without else/finally,
- handlers that both handle and re-raise parts of an exception group,
- bare raise inside except*,
- and combinations of multiple except* handlers.
Structurally, though, this implementation looks sound and matches the expected stack behavior.
Sorry, something went wrong.
| When you modify bytecode instructions, a full clean is required: | ||
|
|
||
| ```bash | ||
| rm -r target/debug/build/rustpython-* && find . | grep -E "(__pycache__|\.pyc$)" | xargs rm -r |
There was a problem hiding this comment.
Deleting only the pyc is enough
Sorry, something went wrong.
| YieldValue, | ||
| /// Set the current exception to TOS (for except* handlers). | ||
| /// Does not pop the value. | ||
| SetExcInfo, |
There was a problem hiding this comment.
Move to the proper place please:)
Should be between SetAdd and SetFunctionAttribute
Sorry, something went wrong.
There was a problem hiding this comment.
I fixed other parts, but let's keep this here until we have to really break bytecode next time. Please move it to proper place when you are going to remove some instructions.
This is a sort of tiny effort not to break bytecode compatibility without a good reason
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3).github/copilot-instructions.md (1)📜 Review detailscrates/codegen/src/compile.rs (1)76-82: Consider a more robust shell command for clean builds.
The current pipeline has a few potential issues:
🔎 Proposed safer command
- xargs rm -r will error if no .pyc files are found
- find . | grep is less efficient than using find's built-in filtering
- The -r flag is unnecessary when removing individual .pyc files
-rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r +rm -rf target/debug/build/rustpython-* && find . -name "*.pyc" -type f -deleteThis version:
- Uses find -delete to avoid the xargs pipeline entirely
- Adds -type f to ensure only files are matched
- Uses -rf on the first command to avoid errors if directories don't exist
- Is more efficient and handles edge cases gracefully
crates/vm/src/exceptions.rs (1)2236-2246: Clarify comment - the identity check is for "is None", not "is not None".
The comment on line 2236 states "Check if match is not None" but the code performs:
emit!(self, Instruction::IsOp(bytecode::Invert::No)); // is None? emit!(self, Instruction::PopJumpIfTrue { target: no_match_block });This checks if match is None (using Invert::No which means is, not is not) and jumps to no_match_block when the result is True. The logic is correct, but the comment is misleading.
Suggested fix:
-// Check if match is not None (use identity check, not truthiness) +// Check if match is None and jump to no_match_block (use identity check, not truthiness)2537-2604: Consider adding more inline comments for the projection algorithm.
The reimplemented logic correctly handles the complex cases (naked exceptions, raised vs reraised, projection onto original structure), but the algorithm is intricate. Adding a few more inline comments explaining the high-level strategy would improve maintainability.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between d8cda04 and 9dda34c.
⛔ Files ignored due to path filters (1)- Lib/test/test_except_star.py is excluded by !Lib/**
- .github/copilot-instructions.md
- crates/codegen/src/compile.rs
- crates/codegen/src/error.rs
- crates/compiler-core/src/bytecode.rs
- crates/vm/src/exceptions.rs
- crates/vm/src/frame.rs
- crates/compiler-core/src/bytecode.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
Files:
- crates/codegen/src/error.rs
- crates/codegen/src/compile.rs
- crates/vm/src/frame.rs
- crates/vm/src/exceptions.rs
Learnt from: youknowone Repo: RustPython/RustPython PR: 6358 File: crates/vm/src/exception_group.rs:173-185 Timestamp: 2025-12-09T08:46:58.660Z Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Run `cargo test --workspace --exclude rustpython_wasm` to execute Rust unit tests
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to files in the `Lib/` directory
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/test/**/*.py : Only acceptable modifications to test files are: (1) Removing `unittest.expectedFailure` decorators and upper TODO comments when tests actually pass, (2) Adding `unittest.expectedFailure` decorators when tests cannot be fixed
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/test/**/*.py : NEVER comment out or delete test code lines except for removing `unittest.expectedFailure` decorators and upper TODO comments
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Use `cargo run -- script.py` instead of the standard `python` command to run and test Python code with RustPython
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to **/*.py : In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Applied to files:
- .github/copilot-instructions.md
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-11-29T12:17:28.606Z Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
- .github/copilot-instructions.md
Learnt from: youknowone Repo: RustPython/RustPython PR: 6358 File: crates/vm/src/exception_group.rs:173-185 Timestamp: 2025-12-09T08:46:58.660Z Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
- crates/vm/src/frame.rs
- crates/vm/src/exceptions.rs
crates/stdlib/src/openssl.rs (1)crates/vm/src/exceptions.rs (1)
- exc (3740-3740)
crates/vm/src/exception_group.rs (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- exception_group (35-40)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
crates/codegen/src/error.rs (1)crates/codegen/src/compile.rs (4)91-92: LGTM! Clear error handling for except* control flow restrictions.
The new error variant and its display message correctly capture the semantic restriction that break, continue, and return statements cannot appear in except* blocks. The implementation follows established patterns in the codebase.
Also applies to: 153-158
crates/vm/src/exceptions.rs (4)1583-1618: LGTM - Break statement validation in except blocks implemented correctly.*
The logic properly scans the fblock stack to detect if a break statement occurs inside an except* handler before reaching a loop, and raises the appropriate BreakContinueReturnInExceptStar error as required by PEP 654.
1620-1657: LGTM - Continue statement validation mirrors break handling.
The implementation correctly prevents continue statements inside except* blocks using the same pattern as break validation.
1665-1676: LGTM - Return statement validation completes the except control flow restrictions.*
Correctly prevents return inside except* handlers, completing the PEP 654 requirement that break/continue/return are not allowed in exception group handlers.
2156-2426: Comprehensive except implementation looks solid.*
The rewritten compile_try_star_except method correctly implements PEP 654 exception group handling with:
- Proper stack layout management ([orig, list, rest])
- SetExcInfo instruction to support bare raise in handlers
- ExceptionGroupHandler fblock to prevent break/continue/return
- PrepReraiseStar intrinsic for exception group reconstruction
- Correct multi-block control flow for matched, non-matched, and reraise paths
The stack manipulation and ListAppend index calculations are correct, and the cleanup logic properly handles both normal and exceptional exits.
2436-2468: Validation logic looks correct.
The function properly enforces except* semantics by rejecting ExceptionGroup types and ensuring BaseException subclasses. Error messages are clear.
As per coding guidelines, ensure cargo fmt and cargo clippy have been run on this file.
2484-2519: Good defensive type checking for split() result.
The added validation ensures that exception groups return a properly-typed tuple from their split() method, with clear error messages when they don't. This prevents downstream type errors.
2652-2712: Projection logic correctly uses derive() for structure preservation.
The recursive splitting and reconstruction properly preserves the nested exception group structure. The comment at line 2709 correctly notes that subclass type may not be preserved—this aligns with the design decision documented in the learnings about derive() behavior.
Based on learnings, the derive() method intentionally creates BaseExceptionGroup instances, which is the expected behavior here.
2643-2648: No issues found. The chained if let syntax is fully compatible with the project's MSRV of Rust 1.89.0 (well above the required 1.53.0).
Sorry, something went wrong.
| bytecode::Instruction::SetExcInfo => { | ||
| // Set the current exception to TOS (for except* handlers) | ||
| // This updates sys.exc_info() so bare 'raise' will reraise the matched exception | ||
| let exc = self.top_value(); | ||
| if let Some(exc) = exc.downcast_ref::<PyBaseException>() { | ||
| vm.set_exception(Some(exc.to_owned())); | ||
| } | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
# Search for similar instruction handlers with type validation patterns
rg -n -A 5 -B 2 'bytecode::Instruction::(ListAppend|SetAdd|MapAdd)' crates/vm/src/frame.rs | head -60Repository: RustPython/RustPython
Length of output: 1251
🏁 Script executed:
#!/bin/bash
# Also search for SetExcInfo usage and context
rg -n -B 5 -A 10 'SetExcInfo' crates/vm/src/Repository: RustPython/RustPython
Length of output: 1256
🏁 Script executed:
#!/bin/bash
# Search for except* handler context to understand if SetExcInfo should always receive PyBaseException
rg -n -A 3 -B 3 'except\*' crates/vm/src/ --type py --type rs | head -80Repository: RustPython/RustPython
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for what generates SetExcInfo bytecode instruction
rg -n 'SetExcInfo' crates/vm/src/ -A 3 -B 3Repository: RustPython/RustPython
Length of output: 603
🏁 Script executed:
#!/bin/bash
# Check if there's any compiler/codegen logic that produces SetExcInfo
rg -n 'SetExcInfo' crates/compiler/Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look at the pattern - check if downcast_ref with if-let is used elsewhere for similar operations
rg -n 'downcast_ref.*if let' crates/vm/src/frame.rs | head -20Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for instructions that use safe downcast with if let (like SetExcInfo)
rg -n 'if let Some.*downcast_ref' crates/vm/src/frame.rs -B 3 -A 3Repository: RustPython/RustPython
Length of output: 945
🏁 Script executed:
#!/bin/bash
# Search for bytecode generation of SetExcInfo in compiler module (broader search)
rg -n 'SetExcInfo' crates/ --type-list | head -20Repository: RustPython/RustPython
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Look for where SetExcInfo bytecode instruction is defined
rg -n 'SetExcInfo' crates/vm/src/bytecode.rs -B 2 -A 2Repository: RustPython/RustPython
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check compiler for ExceptionGroup handling which might generate SetExcInfo
rg -n 'except\*|ExceptionGroup' crates/compiler/src/Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Let's find the bytecode definition file
fd -name "*.rs" crates/ | xargs grep -l 'enum Instruction' | head -5Repository: RustPython/RustPython
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Check how JumpIfNotExcMatch (which uses similar safe downcast pattern) is generated
rg -n 'JumpIfNotExcMatch' crates/compiler/src/ -B 3 -A 3Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# See if there are comments or tests explaining SetExcInfo behavior
rg -n 'SetExcInfo' crates/ -B 5 -A 5Repository: RustPython/RustPython
Length of output: 4926
🏁 Script executed:
#!/bin/bash
# Look at the except* handler compilation context more closely
rg -n 'except\*|handler_except_block' crates/codegen/src/compile.rs -B 5 -A 10 | head -100Repository: RustPython/RustPython
Length of output: 4266
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for SetExcInfo
rg -n 'SetExcInfo' crates/wasm-lib/tests/ crates/*/tests/Repository: RustPython/RustPython
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Look at what comes before SetExcInfo in the stack - is it guaranteed to be BaseException?
rg -n 'emit.*SetExcInfo' crates/codegen/src/compile.rs -B 20 -A 5Repository: RustPython/RustPython
Length of output: 1293
🏁 Script executed:
#!/bin/bash
# Look at how CheckEgMatch is implemented - what does it guarantee?
rg -n 'CheckEgMatch' crates/vm/src/frame.rs -B 3 -A 15 | head -50Repository: RustPython/RustPython
Length of output: 1046
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about why safe downcast is used for exception handling
rg -n 'downcast_ref.*PyBaseException' crates/vm/src/frame.rs -B 5 -A 5Repository: RustPython/RustPython
Length of output: 663
🏁 Script executed:
#!/bin/bash
# See if SetExcInfo ever handles cases where the value is NOT a BaseException
rg -n 'CheckEgMatch\|SetExcInfo' crates/codegen/src/compile.rs -B 2 -A 2 | head -40Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the exception_group_match function to see what it guarantees
rg -n 'fn exception_group_match' crates/vm/src/ -A 30Repository: RustPython/RustPython
Length of output: 2448
🏁 Script executed:
#!/bin/bash
# Look at exception_group.rs derive method mentioned in learnings to understand exception handling design
rg -n 'fn derive' crates/vm/src/exception_group.rs -B 3 -A 30 | head -80Repository: RustPython/RustPython
Length of output: 2532
Consider consistency with similar bytecode handlers for exception type validation.
SetExcInfo receives guaranteed non-None exception values from CheckEgMatch (the None case is eliminated at lines 2238-2245 in compile.rs). However, it uses a safe downcast_ref with silent failure, which differs from similar stack-based instructions:
- ListAppend/SetAdd/MapAdd use unsafe downcast_unchecked_ref() with "SAFETY: trust compiler" comments
- Other exception instructions like JumpIfNotExcMatch also use the safe downcast pattern
Either document why exception handlers intentionally use defensive safe downcast (as opposed to trusting the compiler like other instructions), or standardize on downcast_unchecked_ref() for consistency.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.