native ExceptionGroup #6358
native ExceptionGroup #6358
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughImplements ExceptionGroup natively in Rust (crates/vm/src/exception_group.rs), removes the Python 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)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 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. Comment |
578a2c8 to
4d23c3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/exception_group.rs (1)
314-351: Consider handling potential iteration failures more gracefully in repr.The
slot_reprusesArgIterable::try_from_objectand iterates, which could fail ifget_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
"[]"whenget_arg(1)returns None is appropriate.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_exception_group.pyis excluded by!Lib/**Lib/test/test_socket.pyis excluded by!Lib/**
📒 Files selected for processing (5)
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)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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.rscrates/vm/src/stdlib/builtins.rscrates/vm/src/lib.rscrates/vm/src/exceptions.rscrates/vm/src/exception_group.rs
🧠 Learnings (2)
📚 Learning: 2025-11-29T12:17:28.606Z
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.rscrates/vm/src/exceptions.rs
📚 Learning: 2025-11-29T12:17:28.606Z
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
🧬 Code graph analysis (4)
crates/vm/src/stdlib/builtins.rs (1)
crates/vm/src/exception_group.rs (1)
exception_group(35-40)
crates/vm/src/lib.rs (1)
crates/vm/src/exception_group.rs (1)
exception_group(35-40)
crates/vm/src/exceptions.rs (1)
crates/vm/src/exception_group.rs (2)
exception_group(35-40)derive(174-185)
crates/vm/src/exception_group.rs (1)
crates/vm/src/exceptions.rs (10)
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)
⏰ 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)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (24)
crates/vm/src/lib.rs (1)
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/vm/mod.rs (2)
376-380: LGTM!The underscore prefix correctly suppresses unused variable warnings when the
encodingsfeature 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/stdlib/builtins.rs (2)
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
BaseExceptionGroupandException.crates/vm/src/exceptions.rs (4)
25-26: LGTM!The re-export of
exception_groupprovides a clean public API through the exceptions module.
450-453: LGTM!Adding
#[derive(Debug)]toExceptionZooimproves debuggability. The removal of theexception_groupfield is consistent with the shift to dynamic type creation.
1222-1222: LGTM!The import cleanup correctly removes
PyTypewhich is no longer needed after the type definitions were moved to the exception_group module.
1232-1234: LGTM!Re-exporting
PyBaseExceptionGroupfrom the dedicated module maintains API compatibility while centralizing the implementation.crates/vm/src/exception_group.rs (15)
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
BaseExceptionGroupandException, which is required for Python 3.11+ExceptionGroupsemantics. The use ofexpect()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
BaseExceptioninstances and tracking whether any non-Exceptionitems exist for auto-conversion logic.
124-160: LGTM!The class selection logic correctly handles:
ExceptionGroup- rejects BaseExceptions that aren't ExceptionsBaseExceptionGroup- 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
subgroupimplementation 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
splitimplementation 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
ConditionMatcherenum andget_condition_matcherfunction properly handle all three condition types:
- Single exception type
- Tuple of exception types
- Callable predicate
The
checkmethod correctly implements the matching logic for each variant.
430-468: LGTM!The
derive_and_copy_attributesfunction 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
derivemethod always creates aBaseExceptionGroup, but CPython's behavior (per PEP 654) is to use the derived class's constructor. This ensures that subclasses ofBaseExceptionGrouppreserve their type whensubgroup()andsplit()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 thatContext::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 thatContext::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 duringInterpreter::with_init.
4d23c3d to
ed59eba
Compare
ed59eba to
cb878a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/vm/src/vm/mod.rs (1)
376-395: LGTM with a minor naming suggestion.The logic correctly handles the case where
encodingsfeature is enabled butpath_listis empty, providing helpful guidance. However, the underscore prefix in_expect_stdlibtypically indicates an unused variable in Rust conventions, yet it's actually used on line 380. Consider keeping the original nameexpect_stdlibto 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 {crates/vm/src/exception_group.rs (1)
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() {
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_exception_group.pyis excluded by!Lib/**Lib/test/test_socket.pyis excluded by!Lib/**
📒 Files selected for processing (6)
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)
💤 Files with no reviewable changes (1)
- crates/vm/Lib/python_builtins/_py_exceptiongroup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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.rscrates/vm/src/vm/mod.rscrates/vm/src/exceptions.rscrates/vm/src/exception_group.rs
🧠 Learnings (2)
📚 Learning: 2025-11-29T12:17:28.606Z
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
📚 Learning: 2025-11-29T12:17:28.606Z
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
🧬 Code graph analysis (3)
crates/vm/src/stdlib/builtins.rs (1)
crates/vm/src/exception_group.rs (1)
exception_group(35-40)
crates/vm/src/exceptions.rs (1)
crates/vm/src/exception_group.rs (2)
exception_group(35-40)derive(174-185)
crates/vm/src/exception_group.rs (1)
crates/vm/src/exceptions.rs (10)
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)
⏰ 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)
- 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)
🔇 Additional comments (9)
crates/vm/src/stdlib/builtins.rs (1)
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/exceptions.rs (3)
25-26: LGTM!The re-export correctly exposes the
exception_groupfunction from the new module, maintaining API accessibility.
450-523: LGTM!The removal of
exception_groupfield fromExceptionZoois consistent with the shift to dynamic ExceptionGroup creation. The struct retainsbase_exception_groupwhich is still needed as a base class.
1232-1234: LGTM!The re-export of
PyBaseExceptionGroupfrom the dedicatedexception_groupmodule maintains API compatibility while consolidating exception group logic.crates/vm/src/exception_group.rs (5)
62-160: LGTM!The
slot_newimplementation correctly validates all inputs and implements the CPython 3.11+ semantics for auto-conversion betweenBaseExceptionGroupandExceptionGroupbased on the exception types contained.
430-468: LGTM!The
derive_and_copy_attributeshelper 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]
crates/vm/src/exception_group.rs
Outdated
| } | ||
|
|
||
| #[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__?
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.