Add more error functions to the c-api#8248
Conversation
bschoenmaeckers
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe C-API now exposes VM-backed exception state operations, platform error constructors, Unicode error accessors, traceback handling, signal utilities, and FFI error conversion. ChangesException and error C-API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CAPI
participant VM
participant Exception
CAPI->>VM: fetch, restore, or update exception state
VM->>Exception: normalize traceback and exception values
Exception-->>VM: exception attributes and triple
VM-->>CAPI: return pointers or FFI error sentinel
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/capi/src/pyerrors/errno.rs (1)
18-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce duplication by delegating to the object-based helper.
PyErr_SetFromErrnoWithFilenamere-implements the errno read,EINTRhandling, message construction, downcast, and call logic that already exists inPyErr_SetFromErrnoWithFilenameObjects. As in CPython, this can decode the filename and then delegate toPyErr_SetFromErrnoWithFilenameObject, keeping the errno/message logic in one place.🤖 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 `@crates/capi/src/pyerrors/errno.rs` around lines 18 - 56, Refactor PyErr_SetFromErrnoWithFilename to only decode the optional C filename, convert it to a Python object, and delegate to PyErr_SetFromErrnoWithFilenameObject. Remove its duplicated errno retrieval, EINTR handling, message construction, exception downcast, and call logic so the object-based helper remains the single implementation.
🤖 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 `@crates/capi/src/pyerrors/windows.rs`:
- Around line 69-84: In the Windows error conversion branch, avoid leaking the
decoded filename created by decode_fsdefault_and_size. Update the
Some(Ok(filename)) handling to keep the PyObjectRef alive locally and pass its
borrowed raw pointer to set_windows_error, rather than calling into_raw();
preserve the existing error and None handling.
---
Nitpick comments:
In `@crates/capi/src/pyerrors/errno.rs`:
- Around line 18-56: Refactor PyErr_SetFromErrnoWithFilename to only decode the
optional C filename, convert it to a Python object, and delegate to
PyErr_SetFromErrnoWithFilenameObject. Remove its duplicated errno retrieval,
EINTR handling, message construction, exception downcast, and call logic so the
object-based helper remains the single implementation.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 05c2152b-5010-4ab6-a5ca-c0afe9842760
📒 Files selected for processing (5)
crates/capi/src/pyerrors.rscrates/capi/src/pyerrors/errno.rscrates/capi/src/pyerrors/unicode.rscrates/capi/src/pyerrors/windows.rscrates/capi/src/util.rs
8f6e3ec to
29f68a9
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/capi/src/pyerrors/errno.rs (1)
23-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
EINTRbefore decoding the filename. Match thePyErr_SetFromErrnoWithFilenameObjectsordering here: checkEINTRimmediately after readingerrno, beforedecode_fsdefault_and_size(...). Otherwise a decode failure can bypassvm.check_signals()and return the wrong exception.🤖 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 `@crates/capi/src/pyerrors/errno.rs` around lines 23 - 34, Move the errno == libc::EINTR handling immediately after retrieving errno in the PyErr_SetFromErrnoWithFilenameObjects flow, before any filename null check or decode_fsdefault_and_size call. Preserve vm.check_signals()? behavior, then decode the filename only after signal handling completes.
🤖 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.
Outside diff comments:
In `@crates/capi/src/pyerrors/errno.rs`:
- Around line 23-34: Move the errno == libc::EINTR handling immediately after
retrieving errno in the PyErr_SetFromErrnoWithFilenameObjects flow, before any
filename null check or decode_fsdefault_and_size call. Preserve
vm.check_signals()? behavior, then decode the filename only after signal
handling completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 415416af-d06c-4cb0-bcad-3eb97d1d62a2
📒 Files selected for processing (2)
crates/capi/src/pyerrors/errno.rscrates/capi/src/pyerrors/windows.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/capi/src/pyerrors/windows.rs
1fdf316 to
dbfcbc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@crates/capi/src/pyerrors.rs`:
- Around line 545-555: Update the PyErr_SetExcInfo handling around exc_val and
vm.set_exception so Py_None is treated the same as NULL, producing an empty
handled-exception state instead of being downcast as PyBaseException. Preserve
the existing TypeError for non-None values that are not exception instances, and
keep valid exception restoration unchanged.
- Around line 481-486: Update PyErr_GetHandledException and PyErr_GetExcInfo to
use topmost_exception() instead of current_exception(), so both APIs find the
oldest active handled exception past empty stack slots. Preserve
PyErr_GetExcInfo’s existing behavior of writing NULL values when no exception is
active; apply the changes at crates/capi/src/pyerrors.rs lines 481-486 and
507-535.
- Around line 490-497: Update PyErr_SetHandledException in
crates/capi/src/pyerrors.rs lines 490-497 to retain the incoming exception
rather than consume the raw pointer, while clearing handled-exception state for
NULL or Py_None. In the corresponding handled-exception setter at
crates/capi/src/pyerrors.rs lines 545-555, continue stealing type and traceback,
but treat value == NULL or Py_None as an empty handled state instead of
downcasting it.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 16ddb047-c75b-4bd8-b995-5381c46e5117
📒 Files selected for processing (5)
crates/capi/src/pyerrors.rscrates/capi/src/pyerrors/errno.rscrates/capi/src/pyerrors/unicode.rscrates/capi/src/pyerrors/windows.rscrates/capi/src/util.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/capi/src/util.rs
- crates/capi/src/pyerrors/windows.rs
- crates/capi/src/pyerrors/errno.rs
- crates/capi/src/pyerrors/unicode.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/capi/src/pyerrors.rs (1)
508-535: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFix memory leaks when out-pointers are
NULL.When
ptype,pvalue, orptracebackis passed asNULL, the correspondingPyObjectRefreferences obtained fromvm.split_exception()are converted to raw pointers viainto_raw().as_ptr()but never written out or decremented. This causes the objects to leak because their reference counts are never decremented.Keep the references as
Option<PyObjectRef>until they are actually written to the non-NULLdestination pointers. This ensures that any discarded references automatically decrement their reference counts when the options are dropped at the end of the block.🐛 Proposed fix
- let (ty, value, tb) = vm.topmost_exception().map_or_else( - || { - ( - core::ptr::null_mut(), - core::ptr::null_mut(), - core::ptr::null_mut(), - ) - }, - |exc| { - let (ty, value, tb) = vm.split_exception(exc); - let tb = if vm.is_none(&tb) { - core::ptr::null_mut() - } else { - tb.into_raw().as_ptr() - }; - (ty.into_raw().as_ptr(), value.into_raw().as_ptr(), tb) - }, - ); - - if let Some(ptype) = NonNull::new(ptype) { - unsafe { ptype.write(ty) }; - } - if let Some(pvalue) = NonNull::new(pvalue) { - unsafe { pvalue.write(value) }; - } - if let Some(ptraceback) = NonNull::new(ptraceback) { - unsafe { ptraceback.write(tb) }; - } + let (ty, value, tb) = vm.topmost_exception().map_or_else( + || (None, None, None), + |exc| { + let (ty, value, tb) = vm.split_exception(exc); + let tb = if vm.is_none(&tb) { None } else { Some(tb) }; + (Some(ty), Some(value), tb) + }, + ); + + if let Some(ptype) = NonNull::new(ptype) { + unsafe { ptype.write(ty.map_or(core::ptr::null_mut(), |x| x.into_raw().as_ptr())) }; + } + if let Some(pvalue) = NonNull::new(pvalue) { + unsafe { pvalue.write(value.map_or(core::ptr::null_mut(), |x| x.into_raw().as_ptr())) }; + } + if let Some(ptraceback) = NonNull::new(ptraceback) { + unsafe { ptraceback.write(tb.map_or(core::ptr::null_mut(), |x| x.into_raw().as_ptr())) }; + }Note: Please ensure
PyErr_Fetch(around line 416) is also updated if it uses the same pattern of callinginto_raw()before checking forNULLpointers, as this would result in identical memory leaks.🤖 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 `@crates/capi/src/pyerrors.rs` around lines 508 - 535, Update the exception-fetching logic around vm.split_exception and PyErr_Fetch to retain each extracted PyObjectRef as an Option until its corresponding out-pointer is validated; only call into_raw when ptype, pvalue, or ptraceback is non-NULL, allowing unconsumed references to drop and decrement their counts.
🤖 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.
Outside diff comments:
In `@crates/capi/src/pyerrors.rs`:
- Around line 508-535: Update the exception-fetching logic around
vm.split_exception and PyErr_Fetch to retain each extracted PyObjectRef as an
Option until its corresponding out-pointer is validated; only call into_raw when
ptype, pvalue, or ptraceback is non-NULL, allowing unconsumed references to drop
and decrement their counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 366486ed-8a81-4a52-b219-d5ab3beef433
📒 Files selected for processing (2)
crates/capi/src/pyerrors.rscrates/vm/src/vm/mod.rs
122fd5a to
46f2c23
Compare
| let exc_type = unsafe { &*exc }.try_downcast_ref::<PyType>(vm)?; | ||
| let msg = if errno == 0 { | ||
| vm.ctx.new_str("Error").into() | ||
| } else { | ||
| let msg = rustpython_vm::host_env::errno::strerror_string(errno) | ||
| .unwrap_or_else(|| "Error".to_owned()); | ||
| vm.ctx.new_str(msg).into() | ||
| }; | ||
|
|
||
| let args = if let Some(filename) = filename { | ||
| vec![errno.to_pyobject(vm), msg, filename] | ||
| } else { | ||
| vec![errno.to_pyobject(vm), msg] | ||
| }; | ||
| let err = exc_type.as_object().call(args, vm)?; |
There was a problem hiding this comment.
is this logic ok to live in capi crate? it seems like RustPython must have this mechanism inside.
|
|
||
| let args = if !vm.is_none(&filename2) { | ||
| vec![ | ||
| errno.to_pyobject(vm), | ||
| msg, | ||
| filename, | ||
| 0.to_pyobject(vm), | ||
| filename2, | ||
| ] | ||
| } else if !vm.is_none(&filename) { | ||
| vec![errno.to_pyobject(vm), msg, filename] | ||
| } else { | ||
| vec![errno.to_pyobject(vm), msg] | ||
| }; | ||
|
|
||
| let err = exc_type.as_object().call(args, vm)?; |
Add windows error functions Implement unicode error functions Add errno functions to c-api Spell Review Refactor windows errors Review
46f2c23 to
26be396
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
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/vm/vm_new.rs (1)
946-948: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winPrevent VM panics by safely propagating Python exceptions.
invoke_exceptionexecutes Python-level constructors (__new__/__init__), which can fail at runtime (e.g., due to aMemoryErrororRecursionError). Using.expect()here will cause a Rust panic and crash the interpreter.Since
invoke_exceptionreturnsResult<PyBaseExceptionRef, PyBaseExceptionRef>, you can safely fall back to the thrown exception using.unwrap_or_else(|err| err). This gracefully propagates the failure into the VM's normal error-handling flow.🔒️ Proposed fix
let exc = self.invoke_exception(stop_iteration_error, args); - exc.expect("StopIteration is a BaseException Subclass.") + exc.unwrap_or_else(|err| err)🤖 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 `@crates/vm/src/vm/vm_new.rs` around lines 946 - 948, Update the StopIteration handling in the VM method containing invoke_exception to avoid expect-induced panics: replace the infallible assertion with Result fallback logic that returns the thrown PyBaseExceptionRef when construction fails, preserving normal VM exception propagation.
🤖 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.
Outside diff comments:
In `@crates/vm/src/vm/vm_new.rs`:
- Around line 946-948: Update the StopIteration handling in the VM method
containing invoke_exception to avoid expect-induced panics: replace the
infallible assertion with Result fallback logic that returns the thrown
PyBaseExceptionRef when construction fails, preserving normal VM exception
propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: cd5b44b5-9780-449b-a66d-75b7422af5b1
📒 Files selected for processing (7)
crates/capi/src/pyerrors.rscrates/capi/src/pyerrors/errno.rscrates/capi/src/pyerrors/unicode.rscrates/capi/src/pyerrors/windows.rscrates/capi/src/util.rscrates/vm/src/vm/mod.rscrates/vm/src/vm/vm_new.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/vm/src/vm/mod.rs
- crates/capi/src/util.rs
- crates/capi/src/pyerrors/windows.rs
- crates/capi/src/pyerrors/unicode.rs
- crates/capi/src/pyerrors.rs
72defcc to
bfcf173
Compare
bfcf173 to
14c6a0a
Compare
2 participants
Summary by CodeRabbit
exc_info, and interrupt/signal helpers with fatal-error support.errnoand Windows error codes, with optional filename context.