Add minimal capi lifecycle support#7648
Conversation
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant C as C client
participant T as Thread (caller)
participant M as MAIN_INTERP
participant I as Interpreter
participant V as ThreadedVirtualMachine
C->>T: Py_Initialize()
T->>M: store Interpreter if empty
M->>I: construct/interact with Interpreter
T->>M: request_vm_from_interpreter()
M->>I: call new_thread() / obtain ThreadedVirtualMachine
I-->>V: return ThreadedVirtualMachine
T->>T: attach_vm_to_thread() (thread-local)
C->>T: PyGILState_Ensure / refcount ops use V
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Sorry, something went wrong.
bschoenmaeckers
force-pushed
the
capi-lifecycle
branch
from
0a01cb4 to
2b4b20f
Compare
April 21, 2026 12:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 31-35: The current Py_InitializeEx function panics when
INITIALIZED.is_completed() is true, but per the CPython C API it must be a no-op
on repeated calls; modify Py_InitializeEx (and any Py_Initialize wrapper) to
return immediately when INITIALIZED.is_completed() is true instead of panicking
so repeated FFI calls do not abort the process, preserving existing
initialization semantics and side-effects only on the first successful
initialization.
- Around line 63-65: Py_FinalizeEx currently returns success but doesn't update
initialization state because the code uses a non-resettable Once, so
Py_IsInitialized() stays true; replace the Once-based mechanism with a
resettable atomic state (e.g., an AtomicBool named INITIALIZED) and update all
lifecycle functions: set INITIALIZED.store(true, Ordering::SeqCst) in
Py_InitializeEx (and any init paths), set INITIALIZED.store(false,
Ordering::SeqCst) in Py_FinalizeEx before returning, and have Py_IsInitialized()
read INITIALIZED.load(Ordering::SeqCst); ensure thread-safe Ordering and update
or remove the Once symbol usage so no code still relies on the old Once
primitive.
In `@crates/capi/src/pystate.rs`:
- Around line 36-39: The exported PyEval_SaveThread currently returns null which
breaks the C API contract and will corrupt callers using
Py_BEGIN_ALLOW_THREADS/PyEval_RestoreThread; either implement proper
SaveThread/RestoreThread semantics or stop exporting it. To fix: in
PyEval_SaveThread implement logic that captures the current thread's
PyThreadState (returning a non-null pointer token representing the saved state)
and ensure PyEval_RestoreThread accepts that token to restore thread state,
using the existing PyThreadState type and thread-local VM utilities in
pystate.rs; if you cannot implement correct semantics now, remove the
#[no_mangle]/pub extern "C" export for PyEval_SaveThread (and any paired
PyEval_RestoreThread export) so the unstable API is not exposed. Ensure the
chosen change keeps the stable ABI consistent with callers expecting a valid
non-null PyThreadState pointer.
- Around line 26-34: PyGILState_Ensure currently always returns 0 and
PyGILState_Release is a no-op; implement proper per-thread recursion/state
tracking so each PyGILState_Ensure returns a handle encoding the prior state (or
push the prior state on a per-thread stack in the same thread-local VM storage)
and PyGILState_Release(_state) pops/reads that handle to restore the exact
previous state (including detaching the VM if the matching Ensure attached it);
update the thread-local VM storage to track recursion depth/stack and use those
symbols (PyGILState_Ensure, PyGILState_Release, the existing thread-local VM
storage) when implementing the push/encode-on-Ensure and restore/pop-on-Release
behavior.
In `@crates/capi/src/refcount.rs`:
- Around line 5-17: The exported refcount functions currently accept raw
pointers but are declared callable from safe Rust; change both exports to be
unsafe extern "C" functions and restore the normal no_mangle attribute (replace
#[unsafe(no_mangle)] with #[no_mangle]) so the non-null/valid-pointer contract
is encoded in the signature; update _Py_DecRef and _Py_IncRef signatures to pub
unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and pub unsafe extern "C" fn
_Py_IncRef(op: *mut PyObject), remove the clippy allow on
not_unsafe_ptr_arg_deref, and adjust the bodies to use NonNull/new or unwrap
patterns consistent with other FFI functions so dereferencing/reconstructing
ownership is only allowed in unsafe code paths.
🪄 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: c1193732-a759-4d95-962e-bf28e2d74df2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
crates/capi/.cargo/config.tomlcrates/capi/Cargo.tomlcrates/capi/pyo3-rustpython.configcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rscrates/capi/src/pystate.rscrates/capi/src/refcount.rs
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/capi/src/refcount.rs (1)
5-17: ⚠️ Potential issue | 🟠 MajorEncode the raw-pointer contract in the function signatures.
These exports still dereference/reconstruct ownership from
opwhile being callable from safe Rust. Make both functionspub unsafe extern "C" fn, add# Safetydocs, and remove theclippy::not_unsafe_ptr_arg_derefsuppressions; keep the C ABI symbol unchanged.Proposed fix
#[unsafe(no_mangle)] -#[allow(clippy::not_unsafe_ptr_arg_deref)] -pub extern "C" fn _Py_DecRef(op: *mut PyObject) { +/// # Safety +/// +/// `op` must be a valid, non-null pointer to a live `PyObject` with an owned reference. +pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) { // By dropping PyObjectRef, we will decrement the reference count. unsafe { PyObjectRef::from_raw(NonNull::new_unchecked(op)) }; } #[unsafe(no_mangle)] -#[allow(clippy::not_unsafe_ptr_arg_deref)] -pub extern "C" fn _Py_IncRef(op: *mut PyObject) { +/// # Safety +/// +/// `op` must be a valid, non-null pointer to a live `PyObject`. +pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject) { // Don't drop the owned value, as we just want to increment the refcount. core::mem::forget(unsafe { (*op).to_owned() }); }Run this read-only check to verify the signatures and lint suppression are removed:
#!/bin/bash # Description: Verify C-ABI refcount exports encode unsafe raw-pointer contracts. rg -n -C3 '(_Py_DecRef|_Py_IncRef|clippy::not_unsafe_ptr_arg_deref|pub\s+extern\s+"C"\s+fn\s+_Py_)' crates/capi/src/refcount.rsAs per coding guidelines,
**/*.rsshould follow Rust best practices for error handling and memory management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/capi/src/refcount.rs` around lines 5 - 17, Change both exports to encode the raw-pointer contract by making the signatures pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject), remove the #[allow(clippy::not_unsafe_ptr_arg_deref)] attributes, and keep the no-mangle C ABI symbol. Add concise # Safety doc comments to each function stating the caller must pass a non-null, properly aligned, valid PyObject pointer and that ownership/refcount semantics apply to operations using PyObjectRef::from_raw and (*op).to_owned(). Ensure the body logic (using PyObjectRef::from_raw in _Py_DecRef and core::mem::forget((*op).to_owned()) in _Py_IncRef) remains the same but is now behind an unsafe function signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/capi/Cargo.toml`:
- Line 16: Update the dependency declaration for rustpython-stdlib to be
optional and feature-gated: change the rustpython-stdlib entry so it includes
optional = true and preserves features = ["threading"] (i.e., make
rustpython-stdlib optional), and verify the existing stdlib feature still lists
"rustpython-stdlib" so enabling the stdlib feature pulls in the optional
dependency; adjust the dependency line accordingly and keep the stdlib feature
target unchanged.
---
Duplicate comments:
In `@crates/capi/src/refcount.rs`:
- Around line 5-17: Change both exports to encode the raw-pointer contract by
making the signatures pub unsafe extern "C" fn _Py_DecRef(op: *mut PyObject) and
pub unsafe extern "C" fn _Py_IncRef(op: *mut PyObject), remove the
#[allow(clippy::not_unsafe_ptr_arg_deref)] attributes, and keep the no-mangle C
ABI symbol. Add concise # Safety doc comments to each function stating the
caller must pass a non-null, properly aligned, valid PyObject pointer and that
ownership/refcount semantics apply to operations using PyObjectRef::from_raw and
(*op).to_owned(). Ensure the body logic (using PyObjectRef::from_raw in
_Py_DecRef and core::mem::forget((*op).to_owned()) in _Py_IncRef) remains the
same but is now behind an unsafe function signature.
🪄 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: 5fa614e4-9121-4b67-abb1-14a0faa61559
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.cspell.dict/cpython.txtcrates/capi/.cargo/config.tomlcrates/capi/Cargo.tomlcrates/capi/pyo3-rustpython.configcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rscrates/capi/src/pystate.rscrates/capi/src/refcount.rs
✅ Files skipped from review due to trivial changes (4)
- .cspell.dict/cpython.txt
- crates/capi/pyo3-rustpython.config
- crates/capi/.cargo/config.toml
- crates/capi/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/capi/src/pystate.rs
- crates/capi/src/pylifecycle.rs
Sorry, something went wrong.
|
Please feel free to push to this branch. |
Sorry, something went wrong.
bschoenmaeckers
force-pushed
the
capi-lifecycle
branch
from
2b4b20f to
63a9c77
Compare
April 21, 2026 13:23
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
good idea to split to small patches 👍
Sorry, something went wrong.
|
@youknowone Can you help me fix clippy? The |
Sorry, something went wrong.
bschoenmaeckers
force-pushed
the
capi-lifecycle
branch
3 times, most recently
from
2691506 to
ce47be6
Compare
April 21, 2026 14:42
Got it to work. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
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/stdlib/_thread.rs (1)
2-9: ⚠️ Potential issue | 🟠 MajorFeature gate mismatch: strictest re-export and definition gates are incompatible with call-site gates.
Several fork-reinit functions are defined with strict
#[cfg(all(unix, feature = "threading", feature = "host_env"))]gates but called under only#[cfg(feature = "threading")]:
after_fork_child(re-exported at line 2, called at posix.rs:798)reset_weakref_locks_after_fork(defined at object/core.rs:497, called at posix.rs:793)string_pool.reinit_after_fork(defined at intern.rs:40, called at posix.rs:842)codec_registry.reinit_after_fork(defined at codecs.rs:162, called at posix.rs:845)A build with
threadingenabled buthost_envdisabled on Unix will fail compilation because these call sites will resolve to functions that don't exist. Either (a) addfeature = "host_env"to each call-site#[cfg]in posix.rs, or (b) relax the definition gates to require onlyall(unix, feature = "threading")(matching the callers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/_thread.rs` around lines 2 - 9, The build fails because some fork-reinit functions are defined with #[cfg(all(unix, feature = "threading", feature = "host_env"))] but are called under only #[cfg(feature = "threading")]; update the cfgs so callers and definitions match: relax the definition gates for after_fork_child, reset_weakref_locks_after_fork, the string_pool.reinit_after_fork method, and codec_registry.reinit_after_fork to use #[cfg(all(unix, feature = "threading"))] (i.e., remove the feature = "host_env" requirement) so the symbols exist when threading is enabled, or alternatively add feature = "host_env" to the callers' cfgs in posix.rs—pick one consistent approach and apply it to all listed symbols.
♻️ Duplicate comments (1)
crates/capi/src/pylifecycle.rs (1)
7-7: ⚠️ Potential issue | 🟠 MajorTrack logical finalization separately from the
OnceLock.
Py_FinalizeEx()reports success but leavesMAIN_INTERPpopulated, soPy_IsInitialized()remains true forever after the firstPy_InitializeEx(). If full teardown is deferred, at least keep a resettable lifecycle flag so callers can observefinalize → uninitialized → initializecorrectly.Minimal state-tracking direction
use crate::pystate::attach_vm_to_thread; use core::ffi::c_int; use rustpython_vm::Interpreter; use rustpython_vm::vm::thread::ThreadedVirtualMachine; -use std::sync::{Mutex, OnceLock}; +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Mutex, OnceLock, +}; static MAIN_INTERP: OnceLock<Mutex<Interpreter>> = OnceLock::new(); +static INITIALIZED: AtomicBool = AtomicBool::new(false); @@ pub extern "C" fn Py_IsInitialized() -> c_int { - MAIN_INTERP.get().is_some() as c_int + INITIALIZED.load(Ordering::SeqCst) as c_int } @@ pub extern "C" fn Py_InitializeEx(_initsigs: c_int) { MAIN_INTERP.get_or_init(|| Interpreter::with_init(Default::default(), |_vm| {}).into()); + INITIALIZED.store(true, Ordering::SeqCst); attach_vm_to_thread(); } @@ pub extern "C" fn Py_FinalizeEx() -> c_int { + INITIALIZED.store(false, Ordering::SeqCst); 0 }Also applies to: 20-21, 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/capi/src/pylifecycle.rs` at line 7, MAIN_INTERP (a OnceLock<Mutex<Interpreter>>) is never cleared so Py_IsInitialized() stays true after Py_FinalizeEx(); introduce a separate resettable lifecycle flag (e.g., an AtomicBool like PY_INITIALIZED) and update it to true in Py_InitializeEx and false in Py_FinalizeEx, then have Py_IsInitialized read that flag instead of the OnceLock; keep MAIN_INTERP as-is (do not rely on dropping it) so callers see a correct initialized/finalized lifecycle without changing the OnceLock usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/stdlib/_imp.rs`:
- Around line 79-96: Update the call sites that invoke the fork helper functions
so their cfg gating matches the exports: wrap the calls to
acquire_imp_lock_for_fork(), after_fork_child_imp_lock_release(),
reinit_imp_lock_after_fork(), and release_imp_lock_after_fork_parent() with
cfg(all(unix, feature = "threading", feature = "host_env")) so they are only
compiled when those functions are exported; locate the call sites where those
four symbols are used and replace the existing cfg checks (e.g., feature =
"threading" or all(unix, feature = "threading")) with the new all(unix, feature
= "threading", feature = "host_env") gate.
In `@crates/vm/src/stdlib/_io.rs`:
- Around line 5-6: The call-site for reinit_std_streams_after_fork in posix.rs
is only guarded by #[cfg(feature = "threading")] but the symbol is exported in
_io.rs under #[cfg(all(unix, feature = "threading", feature = "host_env"))];
change the cfg on the posix.rs invocation to #[cfg(all(unix, feature =
"threading", feature = "host_env"))] so the call is only compiled when the
function is available (search for the reinit_std_streams_after_fork call in
posix.rs to update its cfg).
---
Outside diff comments:
In `@crates/vm/src/stdlib/_thread.rs`:
- Around line 2-9: The build fails because some fork-reinit functions are
defined with #[cfg(all(unix, feature = "threading", feature = "host_env"))] but
are called under only #[cfg(feature = "threading")]; update the cfgs so callers
and definitions match: relax the definition gates for after_fork_child,
reset_weakref_locks_after_fork, the string_pool.reinit_after_fork method, and
codec_registry.reinit_after_fork to use #[cfg(all(unix, feature = "threading"))]
(i.e., remove the feature = "host_env" requirement) so the symbols exist when
threading is enabled, or alternatively add feature = "host_env" to the callers'
cfgs in posix.rs—pick one consistent approach and apply it to all listed
symbols.
---
Duplicate comments:
In `@crates/capi/src/pylifecycle.rs`:
- Line 7: MAIN_INTERP (a OnceLock<Mutex<Interpreter>>) is never cleared so
Py_IsInitialized() stays true after Py_FinalizeEx(); introduce a separate
resettable lifecycle flag (e.g., an AtomicBool like PY_INITIALIZED) and update
it to true in Py_InitializeEx and false in Py_FinalizeEx, then have
Py_IsInitialized read that flag instead of the OnceLock; keep MAIN_INTERP as-is
(do not rely on dropping it) so callers see a correct initialized/finalized
lifecycle without changing the OnceLock usage.
🪄 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: c5ea4489-fdf4-4945-bd15-c2b5f021848c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.cspell.dict/cpython.txtcrates/capi/.cargo/config.tomlcrates/capi/Cargo.tomlcrates/capi/pyo3-rustpython.configcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rscrates/capi/src/pystate.rscrates/capi/src/refcount.rscrates/vm/src/codecs.rscrates/vm/src/intern.rscrates/vm/src/object/core.rscrates/vm/src/stdlib/_imp.rscrates/vm/src/stdlib/_io.rscrates/vm/src/stdlib/_thread.rscrates/vm/src/vm/thread.rs
✅ Files skipped from review due to trivial changes (6)
- crates/vm/src/vm/thread.rs
- crates/capi/.cargo/config.toml
- crates/capi/pyo3-rustpython.config
- crates/capi/src/lib.rs
- crates/capi/src/refcount.rs
- crates/capi/src/pystate.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .cspell.dict/cpython.txt
- crates/capi/Cargo.toml
Sorry, something went wrong.
bschoenmaeckers
force-pushed
the
capi-lifecycle
branch
from
9d0000c to
9346fd0
Compare
April 23, 2026 08:21
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/capi/src/pylifecycle.rs (1)
43-44: ⚠️ Potential issue | 🟠 MajorFinalize should clear lifecycle state.
Py_FinalizeEx()still returns success without clearingMAIN_INTERP, soPy_IsInitialized()cannot become false after finalization and reinitialization cannot work correctly. This matches the previously flagged lifecycle-state issue and should be fixed together with the state tracking change. The Python lifecycle docs specify thatPy_IsInitialized()returns false afterPy_FinalizeEx(): https://docs.python.org/3/c-api/interp-lifecycle.html#c.Py_FinalizeEx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/capi/src/pylifecycle.rs` around lines 43 - 44, Py_FinalizeEx currently returns success without clearing the global interpreter state; update Py_FinalizeEx() to clear the lifecycle state by resetting MAIN_INTERP (the global tracking the main interpreter) to a null/empty state so that Py_IsInitialized() returns false after finalization, and ensure any associated cleanup (e.g., freeing interpreter resources or flags referenced by MAIN_INTERP) runs before returning; locate the function Py_FinalizeEx() and the MAIN_INTERP symbol to implement the state reset and cleanup so reinitialization works correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 29-34: Py_InitializeEx currently holds the MAIN_INTERP mutex
across attach_vm_to_thread(), which can re-lock MAIN_INTERP via
request_vm_from_interpreter() and deadlock; instead, construct the new
Interpreter (using Interpreter::with_init(...)), store it into MAIN_INTERP and
drop the MAIN_INTERP lock before calling attach_vm_to_thread(); specifically, in
Py_InitializeEx use get_main_interpreter()/MAIN_INTERP only to check/set the
interpreter and ensure the mutex guard is released prior to invoking
attach_vm_to_thread()/request_vm_from_interpreter().
- Around line 19-21: The Py_IsInitialized implementation currently returns the
inverse of the Python C-API contract; update the function Py_IsInitialized to
return get_main_interpreter().is_some() as c_int (instead of is_none()) so it
returns non-zero when an interpreter exists; ensure the change is made in the
Py_IsInitialized function that calls get_main_interpreter().
---
Duplicate comments:
In `@crates/capi/src/pylifecycle.rs`:
- Around line 43-44: Py_FinalizeEx currently returns success without clearing
the global interpreter state; update Py_FinalizeEx() to clear the lifecycle
state by resetting MAIN_INTERP (the global tracking the main interpreter) to a
null/empty state so that Py_IsInitialized() returns false after finalization,
and ensure any associated cleanup (e.g., freeing interpreter resources or flags
referenced by MAIN_INTERP) runs before returning; locate the function
Py_FinalizeEx() and the MAIN_INTERP symbol to implement the state reset and
cleanup so reinitialization works correctly.
🪄 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: b3bd9f59-e5ed-4610-9f85-69c32dfcab49
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlbuild.rscrates/capi/Cargo.tomlcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rssrc/lib.rs
✅ Files skipped from review due to trivial changes (1)
- crates/capi/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/capi/src/lib.rs
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/capi/src/lib.rs (1)
1-1: Narrow the missing_safety_doc suppression or document unsafe C API exports.The crate-wide
#![allow(clippy::missing_safety_doc)]at line 1 suppresses safety documentation requirements for all current and future FFI exports in this module. Multiple publicextern "C"functions exist without documented safety contracts. Either add# Safetysections to these functions or narrow the allow to specific stubs that intentionally omit documentation. This aligns with Rust best practices for memory safety and error handling at FFI boundaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/capi/src/lib.rs` at line 1, The crate-wide attribute #![allow(clippy::missing_safety_doc)] at the top suppresses safety docs for all FFI exports; remove or narrow it and either add /// # Safety sections to each public extern "C" function (any pub extern "C" fn in this module) describing caller requirements and UB conditions, or replace the crate-level allow with targeted #[allow(clippy::missing_safety_doc)] on specific stub items that intentionally omit safety docs; update each FFI export (the pub extern "C" functions) with concrete safety contracts or restrict the allow to those exact symbols so future exports require documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/capi/src/lib.rs`:
- Line 14: Fix the doc-comment typo in crates/capi/src/lib.rs: change the word
"of" to "if" in the comment for the main interpreter (the comment starting "Get
main interpreter of this process.") so it reads "Get main interpreter of this
process. Will be None if it has not been initialized yet." and ensure the
corrected sentence is the docstring associated with the relevant function or
item that exposes the main interpreter.
---
Nitpick comments:
In `@crates/capi/src/lib.rs`:
- Line 1: The crate-wide attribute #![allow(clippy::missing_safety_doc)] at the
top suppresses safety docs for all FFI exports; remove or narrow it and either
add /// # Safety sections to each public extern "C" function (any pub extern "C"
fn in this module) describing caller requirements and UB conditions, or replace
the crate-level allow with targeted #[allow(clippy::missing_safety_doc)] on
specific stub items that intentionally omit safety docs; update each FFI export
(the pub extern "C" functions) with concrete safety contracts or restrict the
allow to those exact symbols so future exports require documentation.
🪄 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: 0f942005-1823-4294-a497-442218c274bb
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlbuild.rscrates/capi/Cargo.tomlcrates/capi/src/lib.rscrates/capi/src/pylifecycle.rssrc/lib.rs
✅ Files skipped from review due to trivial changes (1)
- crates/capi/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib.rs
- build.rs
- crates/capi/src/pylifecycle.rs
Sorry, something went wrong.
bschoenmaeckers
force-pushed
the
capi-lifecycle
branch
2 times, most recently
from
9b46a4d to
3eb587b
Compare
April 23, 2026 08:29
|
@youknowone I think I have the right setup now for the interpreter. Please let me know what you think. |
Sorry, something went wrong.
bschoenmaeckers
force-pushed
the
capi-lifecycle
branch
from
4c82636 to
e06327f
Compare
April 24, 2026 07:31
There was a problem hiding this comment.
I think that the current state is good for now, this PR already brings lots of value and if we see this being fragile we can think of a solution, atm it seems like we're trying to find a solution for a problem we haven't encountered yet (talking about #7648 (comment))
Sorry, something went wrong.
disagree. storing same data to multiple storage must be justified by good reason (e.g. cache). this is not acceptable at least without proper proof they don't make problem, and I will try to avoid this design even if there is a proof of that. |
Sorry, something went wrong.
For what it's worth; I agree that it is not ideal but I think the current solution will not create problems. This currently blocks other work so I propose to merge this as is and refactor |
Sorry, something went wrong.
|
I am trying to figure out how to align it correctly. give me a few more days |
Sorry, something went wrong.
|
@bschoenmaeckers @ShaharNaveh This is my trial. could you review it please? |
Sorry, something went wrong.
Overall looks good 👍 |
Sorry, something went wrong.
ShaharNaveh
left a comment
ShaharNaveh
left a comment
There was a problem hiding this comment.
LGTM!
Just need to satisfy cargo-sheer
Sorry, something went wrong.
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
This is generated review. Let's clear them in follow-up patches.
- Blocker: crates/capi/src/pylifecycle.rs:44 is a no-op. It is fine to leave
APIs unimplemented, but exporting a CPython lifecycle symbol while leaving
the interpreter initialized after Py_FinalizeEx() is incorrect behavior, not
just an incomplete feature. - Blocker: crates/capi/src/pystate.rs:40 always returns null. That is unsafe
for real C API users because callers expect a restorable thread state.
Either implement SaveThread/RestoreThread together or do not expose this
symbol yet. - Major: crates/vm/src/vm/thread.rs:175 does not clean up the C API external
thread’s frame/thread slot. The normal _thread path calls crates/vm/src/
stdlib/_thread.rs:578, but the C API release path only drops the owner and
pops VM_STACK. This can leave stale state visible to _current_frames and
stop-the-world tracking. - Major: crates/vm/src/vm/thread.rs:180 drops GILSTATE_VM before popping
crates/vm/src/vm/thread.rs:183. The intent is understandable, but it exposes
a VM that is already being dropped as the current VM. Cleanup should happen
while the VM is still valid, then the stack entry and owner should be
removed in a well-defined order. - Major: The PR needs to explicitly frame this as an opaque ABI3-style subset,
not general CPython ABI compatibility. RustPython’s crates/vm/src/object/
core.rs:1228 is not CPython’s object layout, so extension compatibility must
be limited to code that treats PyObject* opaquely through exported APIs.
Sorry, something went wrong.
youknowone
merged commit 22d4f43
into
RustPython:main
May 5, 2026
|
Thanks! |
Sorry, something went wrong.
Reviewers
3 participants
This add the most basic c api I can think off. This way we can iterate over how we want to handle the interpreter initialization api's. After this, the road is clear to implement most of the ABI3 api's one-by-one.
This also include my basic pyo3 test harness that exposes the c api's in a safe and battle tested way. To run the tests use
cargo test -p rustpython-capi.References:
xref #7562
Summary by CodeRabbit
New Features
Chores
Behavioral Changes