native ExceptionGroup - #6358
Conversation
Sorry, something went wrong.
|
Caution Review failedThe pull request is closed. WalkthroughImplements ExceptionGroup natively in Rust (crates/vm/src/exception_group.rs), removes the Python _py_exceptiongroup.py, wires the new type into the VM and stdlib exports, and adjusts VM init/control flow and exceptions re-exports accordingly. All public ExceptionGroup behavior is provided from Rust. Changes
Sequence DiagramsequenceDiagram
participant User
participant VM as VirtualMachine
participant EG as exception_group.rs
participant Matcher as ConditionMatcher
participant Validator as Validation Logic
Note over User,VM: Construct ExceptionGroup(message, exceptions)
User->>VM: call ExceptionGroup(...)
VM->>EG: slot_new(cls, args)
EG->>Validator: validate arg types, sequence, non-empty
Validator-->>EG: validation result / errors
EG->>Validator: validate items are BaseException, nesting constraints
Validator-->>EG: items OK / raise TypeError/ValueError
EG->>EG: convert nested groups when allowed
EG-->>VM: return new ExceptionGroup instance
Note over User,VM: Operate: derive / subgroup / split
User->>VM: call derive/subgroup/split(condition)
VM->>EG: method call
EG->>Matcher: get_condition_matcher(condition)
Matcher-->>EG: matcher object
EG->>EG: traverse exceptions recursively, call Matcher.check
alt matched
EG->>EG: collect matches, call derive_and_copy_attributes
else unmatched
EG->>EG: collect remainder
end
EG-->>VM: return new group(s) or tuple
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between cb878a0 and ba9c8e2. 📒 Files selected for processing (1)
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)crates/vm/src/exception_group.rs (1)📜 Review details314-351: Consider handling potential iteration failures more gracefully in repr.
The slot_repr uses ArgIterable::try_from_object and iterates, which could fail if get_arg(1) returns an unexpected type. While this shouldn't happen in normal operation, consider if this could be made more defensive.
The current implementation is acceptable since exceptions should always have a tuple at arg[1] after proper construction. The fallback to "[]" when get_arg(1) returns None is appropriate.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between bb4e30a and 4d23c3d.
⛔ Files ignored due to path filters (2)- Lib/test/test_exception_group.py is excluded by !Lib/**
- Lib/test/test_socket.py is excluded by !Lib/**
- crates/vm/src/exception_group.rs (1 hunks)
- crates/vm/src/exceptions.rs (5 hunks)
- crates/vm/src/lib.rs (1 hunks)
- crates/vm/src/stdlib/builtins.rs (2 hunks)
- crates/vm/src/vm/mod.rs (2 hunks)
📄 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/vm/src/vm/mod.rs
- crates/vm/src/stdlib/builtins.rs
- crates/vm/src/lib.rs
- crates/vm/src/exceptions.rs
- crates/vm/src/exception_group.rs
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:
- crates/vm/src/vm/mod.rs
- crates/vm/src/exceptions.rs
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 **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
- crates/vm/src/exceptions.rs
crates/vm/src/exception_group.rs (1)crates/vm/src/lib.rs (1)
- exception_group (35-40)
crates/vm/src/exception_group.rs (1)crates/vm/src/exceptions.rs (1)
- exception_group (35-40)
crates/vm/src/exception_group.rs (2)crates/vm/src/exception_group.rs (1)
- exception_group (35-40)
- derive (174-185)
crates/vm/src/exceptions.rs (10)⏰ 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). (1)
- args (568-570)
- args (574-574)
- class (45-47)
- new (547-555)
- new (1081-1083)
- obj (221-221)
- obj (225-225)
- obj (395-395)
- obj (403-403)
- try_from_object (394-410)
- GitHub Check: Run rust tests (windows-latest)
crates/vm/src/lib.rs (1)crates/vm/src/vm/mod.rs (2)58-58: LGTM!
The module declaration is correctly placed and follows the existing pattern of private internal modules. The alphabetical ordering is maintained.
crates/vm/src/stdlib/builtins.rs (2)376-380: LGTM!
The underscore prefix correctly suppresses unused variable warnings when the encodings feature is disabled. The conditional usage is appropriate.
397-398: LGTM!
Clean removal of Python fallback initialization. The comment clearly documents that ExceptionGroup is now natively implemented in Rust.
crates/vm/src/exceptions.rs (4)1073-1074: LGTM!
The dynamic ExceptionGroup creation is correctly sourced from the new exception_group module. The comment provides useful context about the multiple inheritance requirement.
1116-1116: LGTM!
The ExceptionGroup export now correctly uses the dynamically created type that inherits from both BaseExceptionGroup and Exception.
crates/vm/src/exception_group.rs (15)25-26: LGTM!
The re-export of exception_group provides a clean public API through the exceptions module.
450-453: LGTM!
Adding #[derive(Debug)] to ExceptionZoo improves debuggability. The removal of the exception_group field is consistent with the shift to dynamic type creation.
1222-1222: LGTM!
The import cleanup correctly removes PyType which is no longer needed after the type definitions were moved to the exception_group module.
1232-1234: LGTM!
Re-exporting PyBaseExceptionGroup from the dedicated module maintains API compatibility while centralizing the implementation.
1-12: LGTM!
Clean module documentation and well-organized imports. The separation of concerns is appropriate.
14-33: LGTM!
The dynamic type creation correctly establishes multiple inheritance from BaseExceptionGroup and Exception, which is required for Python 3.11+ ExceptionGroup semantics. The use of expect() is acceptable here as this is initialization-time code where failure should be fatal.
62-70: LGTM!
Proper validation requiring exactly 2 positional arguments, matching CPython's BaseExceptionGroup.__new__() behavior.
81-98: Good validation, but consider consolidating error messages.
The validation correctly rejects sets, frozensets, and None before attempting sequence conversion. The error handling is robust.
107-122: LGTM!
Correct validation ensuring all items are BaseException instances and tracking whether any non-Exception items exist for auto-conversion logic.
124-160: LGTM!
The class selection logic correctly handles:
- ExceptionGroup - rejects BaseExceptions that aren't Exceptions
- BaseExceptionGroup - auto-converts to ExceptionGroup when all are Exceptions
- User-defined subclasses - enforces nesting constraints based on inheritance
This matches CPython 3.11+ behavior.
162-171: The no-op __init__ is intentional and correct.
The comment explains the rationale well. Since __new__ already sets up the args tuple correctly, __init__ should be a no-op to allow subclasses flexibility.
187-232: LGTM!
The subgroup implementation correctly:
- Returns self if the entire group matches
- Recursively processes nested groups
- Tracks modifications to avoid unnecessary allocations
- Returns None for empty matches
- Copies metadata via derive_and_copy_attributes
234-291: LGTM!
The split implementation correctly partitions exceptions into matching and non-matching groups, handling nested groups recursively. The tuple return format (match, rest) matches CPython.
293-312: LGTM!
Clean __str__ implementation with proper pluralization of "sub-exception(s)".
354-369: LGTM!
Helper functions are clean and appropriately scoped within the types module.
371-428: LGTM!
The ConditionMatcher enum and get_condition_matcher function properly handle all three condition types:
- Single exception type
- Tuple of exception types
- Callable predicate
The check method correctly implements the matching logic for each variant.
430-468: LGTM!
The derive_and_copy_attributes function properly:
- Creates a new group via derive
- Validates the return type
- Copies __traceback__, __context__, __cause__
- Copies __notes__ as a new list (avoiding shared mutation)
This preserves exception metadata through subgroup/split operations as required by PEP 654.
173-185: Use the instance's actual class for derive instead of always BaseExceptionGroup.
The derive method always creates a BaseExceptionGroup, but CPython's behavior (per PEP 654) is to use the derived class's constructor. This ensures that subclasses of BaseExceptionGroup preserve their type when subgroup() and split() perform partitioning operations.
Consider:
fn derive( zelf: PyRef<PyBaseException>, excs: PyObjectRef, vm: &VirtualMachine, ) -> PyResult { let message = zelf.get_arg(0).unwrap_or_else(|| vm.ctx.new_str("").into()); + let cls = zelf.class().to_owned(); vm.invoke_exception( - vm.ctx.exceptions.base_exception_group.to_owned(), + cls, vec![message, excs], ) .map(|e| e.into()) }
35-40: Verify that Context::genesis() is safe to call during static initialization and that this pattern aligns with RustPython's VM initialization best practices.
The static_cell! macro itself is a safe pattern for runtime-initialized statics. However, ensure that Context::genesis() has no dependencies on VM state that might not be available during early static initialization, and confirm this approach is the intended pattern for exception type registration rather than initializing types during Interpreter::with_init.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)crates/vm/src/vm/mod.rs (1)📜 Review detailscrates/vm/src/exception_group.rs (1)376-395: LGTM with a minor naming suggestion.
The logic correctly handles the case where encodings feature is enabled but path_list is empty, providing helpful guidance. However, the underscore prefix in _expect_stdlib typically indicates an unused variable in Rust conventions, yet it's actually used on line 380. Consider keeping the original name expect_stdlib to avoid confusion.
- let _expect_stdlib = + let expect_stdlib = cfg!(feature = "freeze-stdlib") || !self.state.settings.path_list.is_empty(); #[cfg(feature = "encodings")] - if _expect_stdlib { + if expect_stdlib {389-407: Consider explicit empty tuple validation.
When an empty tuple is passed, it falls through to the callable check and eventually errors. While the behavior is correct, an explicit early check with a clearer error message would improve debuggability.
// If it's a tuple of types if let Some(tuple) = condition.downcast_ref::<PyTuple>() { + if tuple.is_empty() { + return Err(vm.new_type_error( + "expected a function, exception type or tuple of exception types", + )); + } let mut types = Vec::new(); for item in tuple.iter() {
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 4d23c3d and cb878a0.
⛔ Files ignored due to path filters (2)- Lib/test/test_exception_group.py is excluded by !Lib/**
- Lib/test/test_socket.py is excluded by !Lib/**
- crates/vm/Lib/python_builtins/_py_exceptiongroup.py (0 hunks)
- crates/vm/src/exception_group.rs (1 hunks)
- crates/vm/src/exceptions.rs (5 hunks)
- crates/vm/src/lib.rs (1 hunks)
- crates/vm/src/stdlib/builtins.rs (2 hunks)
- crates/vm/src/vm/mod.rs (1 hunks)
- crates/vm/Lib/python_builtins/_py_exceptiongroup.py
- crates/vm/src/lib.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/vm/src/stdlib/builtins.rs
- crates/vm/src/vm/mod.rs
- crates/vm/src/exceptions.rs
- crates/vm/src/exception_group.rs
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 **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
- crates/vm/src/exceptions.rs
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:
- crates/vm/src/exceptions.rs
crates/vm/src/exception_group.rs (1)crates/vm/src/exceptions.rs (1)
- exception_group (35-40)
crates/vm/src/exception_group.rs (2)crates/vm/src/exception_group.rs (1)
- exception_group (35-40)
- derive (174-185)
crates/vm/src/exceptions.rs (10)⏰ 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). (9)
- args (568-570)
- args (574-574)
- class (45-47)
- new (547-555)
- new (1081-1083)
- obj (221-221)
- obj (225-225)
- obj (395-395)
- obj (403-403)
- try_from_object (394-410)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
crates/vm/src/stdlib/builtins.rs (1)crates/vm/src/exceptions.rs (3)1073-1116: LGTM!
The dynamic ExceptionGroup creation and export correctly integrates with the new native implementation. The comment accurately describes the multiple inheritance setup.
crates/vm/src/exception_group.rs (5)25-26: LGTM!
The re-export correctly exposes the exception_group function from the new module, maintaining API accessibility.
450-523: LGTM!
The removal of exception_group field from ExceptionZoo is consistent with the shift to dynamic ExceptionGroup creation. The struct retains base_exception_group which is still needed as a base class.
1232-1234: LGTM!
The re-export of PyBaseExceptionGroup from the dedicated exception_group module maintains API compatibility while consolidating exception group logic.
62-160: LGTM!
The slot_new implementation correctly validates all inputs and implements the CPython 3.11+ semantics for auto-conversion between BaseExceptionGroup and ExceptionGroup based on the exception types contained.
430-468: LGTM!
The derive_and_copy_attributes helper correctly creates a new group and copies all relevant metadata (traceback, context, cause, notes). The notes list is properly cloned to avoid sharing mutable state.
293-350: LGTM!
The __str__ and __repr__ implementations correctly format the exception group output, matching CPython's behavior of displaying exceptions in list format despite internal tuple storage.
1-13: Well-structured module.
The module documentation and imports are clean. The implementation correctly uses the RustPython macro system (pyexception, pymethod, pyslot) for implementing Python functionality in Rust, as per coding guidelines.
35-40: [rewritten comment]
[classification tag]
Sorry, something went wrong.
| } | ||
|
|
||
| #[pymethod(name = "__str__")] | ||
| fn str(zelf: PyRef<PyBaseException>, vm: &VirtualMachine) -> PyResult<String> { |
There was a problem hiding this comment.
Can't we call this __str__?
Sorry, something went wrong.
There was a problem hiding this comment.
great!
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.