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/.cargo/config.toml`:
- Around line 1-3: The PYO3 env entries (PYO3_CONFIG_FILE and PYO3_NO_PYTHON)
defined in the local crate .cargo/config.toml won't be picked up for
workspace-root builds; move those two entries into the workspace root
.cargo/config.toml, keep PYO3_CONFIG_FILE pointed at pyo3-rustpython.config (do
not change it to ../pyo3-rustpython.config since relative=true already resolves
to the crate dir), and remove the duplicate/local config so workspace builds
(e.g., builds with the capi feature) inherit the PYO3 settings.
In `@crates/capi/src/object.rs`:
- Around line 32-67: PyType_GetFlags currently initializes flags from
ty.slots.flags.bits() (u64) and then ORs c_ulong constants
(PY_TPFLAGS_LONG_SUBCLASS, etc.), which breaks on Windows because c_ulong is
u32; fix by casting the initial value to c_ulong—i.e., change the initialization
of flags in PyType_GetFlags so it uses ty.slots.flags.bits() as c_ulong before
any |= with the PY_TPFLAGS_* constants so all operations are the same c_ulong
type.
In `@crates/capi/src/pyerrors.rs`:
- Around line 99-105: PyErr_Occurred currently returns the exception instance
(exc.as_object().as_raw()) but must return the exception type (the first arg to
PyErr_Set*), so change the mapper in PyErr_Occurred to return the exception's
type raw pointer instead — i.e., when vm.current_exception() yields exc, return
the raw pointer for exc's class/type (use the VM/exception API such as
exc.class().as_object().as_raw() or the equivalent on your exception type) and
keep the borrowed semantics (no INCREF), preserving the unwrap_or_default
fallback.
- Around line 118-123: In PyErr_SetRaisedException, validate the incoming
pointer before taking ownership: check that exc is not null (use NonNull::new
instead of NonNull::new_unchecked and return an appropriate Err if null), then
construct the PyObjectRef via PyObjectRef::from_raw only after the non-null
check, and replace downcast_unchecked() with a checked downcast (e.g.,
downcast() or the crate's checked variant) so you verify the object is an
exception instance before returning Err(exception).
- Around line 192-236: The extern "C" functions (notably PyErr_NewException,
PyErr_NewExceptionWithDoc, PyErr_PrintEx, PyErr_DisplayException,
PyErr_WriteUnraisable) currently call expect!/panic!/assert!, which will abort
the process on bad input; instead catch invalid inputs, set a Python SystemError
on the VM and return NULL (or return early) per the C-API contract. Replace uses
of expect()/panic!/assert! in PyErr_NewException (e.g.,
CStr::from_ptr(...).to_str().expect(...), rsplit_once().expect(...), panic! for
bad base types, and assert! on dict) with guarded checks that build an error
message, call vm.ctx.exceptions.system_error (or
vm.set_exception(vm.ctx.exceptions.system_error, message)) to set the exception
on the VM, and then return std::ptr::null_mut(); apply the same pattern to
PyErr_NewExceptionWithDoc (delegate should propagate NULL on error) and to
PyErr_PrintEx / PyErr_DisplayException / PyErr_WriteUnraisable so they no longer
panic but set an appropriate SystemError and return gracefully.
In `@crates/capi/src/pylifecycle.rs`:
- Around line 42-53: Py_FinalizeEx() currently returns success without resetting
interpreter state; update Py_FinalizeEx to properly finalize and clear
MAIN_INTERP (drop/free the interpreter handle and set MAIN_INTERP to null/None)
and update whatever initialized flag Py_IsInitialized() checks so it returns 0
after finalize; ensure Py_Finalize() still calls Py_FinalizeEx(), and mirror the
same teardown in Py_FinalizeEx so a subsequent Py_InitializeEx() can create a
fresh interpreter. Reference: Py_FinalizeEx, Py_Finalize, MAIN_INTERP,
Py_IsInitialized, Py_InitializeEx.
In `@crates/capi/src/pystate.rs`:
- Around line 46-49: The PyEval_SaveThread stub must not silently return NULL;
update the body of the extern "C" function PyEval_SaveThread to fail loudly
instead of ptr::null_mut(): add a TODO comment about implementing proper GIL
release, and replace the current return with either an explicit abort (print a
clear message then process::abort()) or call unimplemented!() so callers (and C
extensions expecting a valid PyThreadState pointer) will fail loudly during
development; also ensure the complementary PyEval_RestoreThread handling is
noted in the TODO so the full pair gets implemented against
release_current_thread/attach_current_thread later.
In `@crates/vm/src/codecs.rs`:
- Around line 161-164: The cfg gates are inconsistent:
CodecsRegistry::reinit_after_fork and StringPool::reinit_after_fork are only
defined with #[cfg(all(unix, feature = "threading", feature = "host_env"))] but
posix::reinit_locks_after_fork (posix.rs::reinit_locks_after_fork) is gated with
#[cfg(all(unix, feature = "threading"))], causing missing method errors when
host_env is disabled; update the cfg on posix::reinit_locks_after_fork to
#[cfg(all(unix, feature = "threading", feature = "host_env"))] so its calls to
CodecsRegistry::reinit_after_fork and StringPool::reinit_after_fork compile
consistently.
In `@crates/vm/src/intern.rs`:
- Around line 39-42: The function reinit_locks_after_fork is currently gated
with #[cfg(all(unix, feature = "threading"))] but calls
StringPool::reinit_after_fork and CodecRegistry::reinit_after_fork which are
defined only when #[cfg(all(unix, feature = "threading", feature = "host_env"))]
is set; change the attribute on reinit_locks_after_fork to #[cfg(all(unix,
feature = "threading", feature = "host_env"))] so the compilation gate matches
the methods it calls (locate the reinit_locks_after_fork function and update its
cfg to include feature = "host_env").
In `@crates/vm/src/stdlib/_io.rs`:
- Around line 5-6: The exported function cfg is too restrictive: change the
module-level export so reinit_std_streams_after_fork and reinit_io_locks are
available under #[cfg(all(unix, feature = "threading"))] and move the
#[cfg(feature = "host_env")] guard inside each function body; implement the
host_env-disabled path as a no-op (early return) so calls from posix.rs (which
are under #[cfg(feature = "threading")]) compile when host_env is not enabled,
while keeping the real implementation when host_env is present.
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 2-3: The call site py_os_after_fork_child in posix.rs calls
after_fork_child which is only exported under #[cfg(all(unix, feature =
"threading", feature = "host_env"))]; update the gating around
py_os_after_fork_child (or the specific call) to require the same cfg (e.g.
#[cfg(all(unix, feature = "threading", feature = "host_env"))]) so the call is
only compiled when after_fork_child is available, ensuring the cfg for
py_os_after_fork_child matches the after_fork_child export.
In `@crates/vm/src/vm/thread.rs`:
- Around line 175-190: The release_current_thread function currently drops
GILSTATE_VM before popping the raw pointer from VM_STACK, which can cause
destructors to call with_current_vm and dereference freed memory; fix by
reversing the order in release_current_thread: first remove/pop the VM pointer
from VM_STACK (the vms.borrow_mut().pop().expect(...) call) and only after that
take/drop the GILSTATE_VM (GILSTATE_VM.with(|gilstate_vm|
gilstate_vm.borrow_mut().take())); keep the existing detach_thread call and
other logic unchanged.
---
Outside diff comments:
In `@crates/vm/src/object/core.rs`:
- Around line 477-503: The call site gating for reset_weakref_locks_after_fork
in posix.rs::py_os_after_fork_child must match the function's cfg; change the
cfg on the call to require both feature = "threading" and feature = "host_env"
(i.e. use #[cfg(all(feature = "threading", feature = "host_env"))]) so the call
to crate::object::reset_weakref_locks_after_fork() is only compiled when the
function is present.
In `@crates/vm/src/vm/mod.rs`:
- Around line 2063-2098: The C-API error-indicator must be backed by a separate
per-Vm state, not the exc_info stack used by sys.exc_info()/PUSH_EXC_INFO;
change the methods so they don't read/write exceptions.stack: leave
current_exception/set_exception (and their semantics for the exc_info stack)
as-is, and introduce/replace the C-API facing API with a separate storage (e.g.,
add a new field like raised_exception: RefCell<Option<PyBaseExceptionRef>>) and
implement get_raised_exception / set_raised_exception / take_raised_exception to
operate on that field instead of exceptions.stack; ensure
thread::update_thread_exception continues to reflect the thread-visible error
indicator using the new getter (not topmost_exception), and update any callers
that relied on the old take_raised_exception to use the new C-API methods so
C-level errors do not corrupt the handled-exception stack.
---
Nitpick comments:
In `@build.rs`:
- Around line 5-12: The Windows branch currently lacks any export configuration
when target.as_str() == "windows" and capi_enabled is true; update the build.rs
match to handle that case by either (a) generating a module-definition (.def)
file listing the C-API symbols and emitting a
cargo:rustc-link-arg-bin=rustpython=/DEF:<path> (or rustc-link-arg for the MSVC
linker) or (b) detecting MinGW and emitting the appropriate
-Wl,--export-all-symbols flag; implement one of these in the "windows" arm and
add a brief comment that documents which method you chose and why (reference the
target.as_str() match, the capi_enabled flag, and the println! calls that emit
cargo:rustc-link-arg-bin).
In `@crates/capi/src/lib.rs`:
- Around line 27-33: The code currently calls
init_exception_statics(&Context::genesis().exceptions) which may diverge if
Context becomes per-interpreter; change set_main_interpreter to derive
exceptions from the Interpreter being installed by entering the interpreter
(e.g., use Interpreter::enter or interpreter.enter(|vm| ...)) and call
init_exception_statics(&vm.ctx.exceptions) while still in that closure, then
store the interpreter; ensure the safety comment is updated to reflect that
exception statics are initialized from the interpreter's Context and keep the
assert!(interp.is_none()) and assignment to *interp = Some(interpreter) as
before.
In `@crates/capi/src/object.rs`:
- Around line 70-83: In Py_GetConstantBorrowed replace the panic! on the _
(invalid id) branch with CPython-style behavior: create/raise a SystemError on
the VM/context (e.g. via vm.ctx.new_system_error(...) or the crate's VM
exception-raising helper) and return null_mut() from the extern "C" function;
alternatively, change the function to return an FfiResult that sets the
exception and yields a null pointer on error. Ensure you still return the
borrowed object's as_raw() for valid ids and do not unwind across the FFI
boundary.
In `@crates/capi/src/pystate.rs`:
- Around line 11-13: with_vm currently calls with_current_vm which holds a
RefCell borrow on VM_STACK for the duration of the callback; if the provided
closure f (or anything it transitively calls) tries to mutate VM_STACK (for
example by calling release_current_thread) it will cause a borrow_mut panic —
add a concise safety comment on with_vm explaining that the closure must not
call any function that mutates or re-enters VM_STACK (e.g.,
release_current_thread or other VM stack-mutating APIs) and suggest using a
different API or refactoring to avoid holding the borrow across such calls;
reference the functions with_vm and with_current_vm and the VM_STACK cell in the
comment to help future callers locate the caveat.
In `@crates/capi/src/util.rs`:
- Around line 79-85: The impl FfiResult<isize> for usize currently casts usize
-> isize in into_output which can wrap for values > isize::MAX and collide with
ERR_VALUE; change into_output in the impl for usize to convert safely (e.g., use
isize::try_from(self).unwrap_or(isize::MAX) or otherwise saturate/validate and
return isize::MAX on overflow) and document the chosen behavior, making sure
ERR_VALUE (-1) remains distinct; update the impl block (symbols:
FfiResult<isize> for usize, const ERR_VALUE, fn into_output(self, _vm:
&VirtualMachine)) accordingly.