Skip to content

Use PyO3 main & enable c-api tests#8000

Open
bschoenmaeckers wants to merge 7 commits into
RustPython:mainfrom
bschoenmaeckers:c-api-misc-tests
Open

Use PyO3 main & enable c-api tests#8000
bschoenmaeckers wants to merge 7 commits into
RustPython:mainfrom
bschoenmaeckers:c-api-misc-tests

Conversation

@bschoenmaeckers
Copy link
Copy Markdown
Contributor

@bschoenmaeckers bschoenmaeckers commented Jun 1, 2026

Summary by CodeRabbit

  • New Features

    • Added multiple C-API bindings for module/import, dict/object, refcount, exceptions, version info, and numeric/index operations.
  • Tests

    • Re-enabled and expanded many unit tests across the codebase to improve coverage.
  • Chores

    • Switched PyO3 dependencies to repository sources and changed the configured Python backend.
  • Bug Fixes

    • Improved power builtin modulus handling and FFI return handling for object-typed results.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds many C-API exports and VM helpers, enables previously-disabled test modules, consolidates imports, tweaks builtin pow and ctypes return handling, and switches PyO3/pyo3-ffi to the PyO3 Git source with the workspace set to use RustPython.

Changes

C-API Surface and Test Expansion

Layer / File(s) Summary
Build configuration and workspace dependency
Cargo.toml, crates/capi/pyo3-rustpython.config
PyO3 and pyo3-ffi switched to the PyO3 Git repository; workspace pyo3 dependency updated; implementation set to RustPython.
Module helpers and import entrypoint
crates/capi/src/lib.rs, crates/capi/src/moduleobject.rs, crates/capi/src/import.rs
Expose moduleobject submodule; define PyModule_Check/PyModule_CheckExact; add PyModule_GetNameObject; implement PyImport_AddModuleRef to lookup or create modules in sys.modules.
Object protocol, dict, refcount, exceptions, lifecycle, VM helpers
crates/capi/src/abstract_.rs, crates/capi/src/dictobject.rs, crates/capi/src/object.rs, crates/capi/src/refcount.rs, crates/capi/src/pyerrors.rs, crates/capi/src/pylifecycle.rs, crates/vm/src/builtins/object.rs, crates/vm/src/stdlib/builtins.rs, crates/vm/src/stdlib/_ctypes/function.rs
New C-API exports: PyNumber_Index, PyDict_SetDefaultRef, PyObject_GenericGetDict, PyObject_GenericSetDict, Py_NewRef, Py_REFCNT, PyException_SetCause, PyException_SetTraceback, Py_Version, Py_GetVersion; object_get_dict and object_generic_set_dict made public; pow modulus handling adjusted; ctypes convert_raw_result handles "O" return.
Test enablement, import consolidation, CI update
crates/capi/src/*, .github/workflows/ci.yaml
Many mod tests sections changed from #[cfg(false)] to #[cfg(test)] and test functions were renamed; assorted import consolidations; CI snippets_cpython build enables capi feature.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

"🐰 I hopped through crates and tests, with patches bold and spry,
New C-API doors I gently nudge, and tests wake up to try.
PyO3 now flows from Git's clear stream, the versions hum and send,
RustPython's bindings grew some roots — new helpers to extend. 🥕"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the two main objectives of the changeset: switching to PyO3 main branch and enabling C-API tests throughout the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 93.15% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/ctypes
[ ] test: cpython/Lib/test/test_ctypes (TODO: 7)
[x] test: cpython/Lib/test/test_stable_abi_ctypes.py

dependencies:

  • ctypes

dependent tests: (34 tests)

  • ctypes: test_android test_buffer test_bytes test_code test_codecs test_ctypes test_genericalias test_io test_ntpath test_os test_ssl test_venv
    • platform: test__locale test__osx_support test_asyncio test_baseexception test_builtin test_cmath test_fcntl test_math test_mimetypes test_platform test_posix test_regrtest test_shutil test_socket test_strptime test_sysconfig test_time test_winreg test_wsgiref
      • pydoc: test_enum test_pydoc
    • webbrowser: test_webbrowser

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
Cargo.toml (1)

122-122: ⚡ Quick win

Pin PyO3 git rev in Cargo.toml

Cargo.lock already resolves all PyO3 crates (pyo3, pyo3-ffi, pyo3-build-config, pyo3-macros, pyo3-macros-backend) to the same commit (de5816ada27bfa38cb3176d91bd6c93e206dcb3f), but Cargo.toml still uses bare git URLs for both pyo3 and pyo3-ffi. Add rev = "de5816ada27bfa38cb3176d91bd6c93e206dcb3f" to both entries to make the manifest explicitly reproducible.

🤖 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 `@Cargo.toml` at line 122, Update the Cargo.toml git dependencies for pyo3 and
pyo3-ffi to pin them to the resolved commit: add rev =
"de5816ada27bfa38cb3176d91bd6c93e206dcb3f" to the pyo3 = { git =
"https://github.com/PyO3/pyo3" } and pyo3-ffi = { git =
"https://github.com/PyO3/pyo3" } entries so the manifest explicitly matches the
commit resolved in Cargo.lock.
🤖 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/import.rs`:
- Around line 24-31: The current logic collapses all sys_modules.get_item
failures into creating a new module; instead, explicitly handle each case: call
sys_modules.get_item(name, vm) and if it returns Err, propagate or return that
error rather than creating a module; if it returns Ok(Some(value)) but
value.downcast_ref::<PyModule>() is None, return a type/mismatch error instead
of overwriting sys.modules; only when get_item returns Ok(None) should you call
vm.new_module(name, vm.ctx.new_dict(), None) and sys_modules.set_item(name,
module.clone().into(), vm)?; update the block around sys_modules.get_item,
downcast_ref::<PyModule>(), vm.new_module, and sys_modules.set_item accordingly.
- Around line 16-18: The extern "C" function PyImport_AddModuleRef currently
calls CStr::from_ptr(name).to_str().expect(...), which can panic on invalid
UTF-8; change it to return an FfiResult error instead of panicking by replacing
the expect with a to_str().map_err(|_| vm.new_system_error("Name is not valid
UTF-8"))? expression (using the existing with_vm/FfiResult flow) so invalid
UTF-8 is converted into a Python SystemError via vm.new_system_error rather than
causing a panic.

In `@crates/capi/src/object.rs`:
- Around line 205-227: PyObject_GenericGetDict and PyObject_GenericSetDict
incorrectly call obj.get_attr / obj.set_attr which go through attribute
machinery and allow __getattribute__/descriptors to intercept; instead wire
these C-API functions to the VM helpers that bypass attribute hooks. Replace the
calls in PyObject_GenericGetDict and PyObject_GenericSetDict to use the direct
helpers (object_get_dict and object_generic_set_dict from
crates/vm/src/builtins/object.rs) that read/write the underlying instance dict
(obj.dict()/obj.instance_dict()) without routing through get_attr/set_attr so
the real instance dict is returned/updated.

In `@crates/capi/src/pyerrors.rs`:
- Around line 303-308: PyException_SetCause currently clones the borrowed raw
pointer (using to_owned()) which violates CPython’s reference-stealing semantics
and leaks/increfs; change it to take ownership of the incoming raw pointer for
`cause` (use the equivalent of a from_raw/from_owned conversion on the raw `*mut
PyObject`) so the caller’s reference is consumed, map NULL to Python None as
before for clearing, and then pass that owned PyBaseException into
`exc.set___cause__` instead of cloning; ensure no extra incref remains and the
raw pointer is not used after ownership is transferred.

In `@crates/capi/src/pylifecycle.rs`:
- Around line 61-63: The function Py_GetVersion currently returns String::as_ptr
which may lack a terminating NUL; change the static LazyLock to hold a CString
(e.g. static VERSION: LazyLock<CString> = LazyLock::new(||
CString::new(format!("{MAJOR}.{MINOR}.{MICRO}")).unwrap());) and return
VERSION.as_ptr() as *const c_char so callers get a stable, NUL-terminated C
string from Py_GetVersion. Ensure you import std::ffi::CString and keep the
LazyLock so the CString lives for the program duration.

---

Nitpick comments:
In `@Cargo.toml`:
- Line 122: Update the Cargo.toml git dependencies for pyo3 and pyo3-ffi to pin
them to the resolved commit: add rev =
"de5816ada27bfa38cb3176d91bd6c93e206dcb3f" to the pyo3 = { git =
"https://github.com/PyO3/pyo3" } and pyo3-ffi = { git =
"https://github.com/PyO3/pyo3" } entries so the manifest explicitly matches the
commit resolved in Cargo.lock.
🪄 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: 4e562741-82e6-4a0e-ad6f-0b276776160f

📥 Commits

Reviewing files that changed from the base of the PR and between 1ce37bf and 43529c0.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • Lib/test/test_ctypes/test_python_api.py is excluded by !Lib/**
📒 Files selected for processing (22)
  • Cargo.toml
  • crates/capi/pyo3-rustpython.config
  • crates/capi/src/abstract_.rs
  • crates/capi/src/bytesobject.rs
  • crates/capi/src/ceval.rs
  • crates/capi/src/complexobject.rs
  • crates/capi/src/dictobject.rs
  • crates/capi/src/floatobject.rs
  • crates/capi/src/import.rs
  • crates/capi/src/lib.rs
  • crates/capi/src/listobject.rs
  • crates/capi/src/longobject.rs
  • crates/capi/src/methodobject.rs
  • crates/capi/src/moduleobject.rs
  • crates/capi/src/object.rs
  • crates/capi/src/pycapsule.rs
  • crates/capi/src/pyerrors.rs
  • crates/capi/src/pylifecycle.rs
  • crates/capi/src/refcount.rs
  • crates/capi/src/tupleobject.rs
  • crates/capi/src/unicodeobject.rs
  • crates/vm/src/stdlib/builtins.rs

Comment thread crates/capi/src/import.rs Outdated
Comment thread crates/capi/src/import.rs Outdated
Comment thread crates/capi/src/object.rs
Comment thread crates/capi/src/pyerrors.rs
Comment thread crates/capi/src/pylifecycle.rs Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/vm/src/stdlib/_ctypes/function.rs`:
- Around line 1435-1455: The code incorrectly maps a NULL py_object to
Some(None) and also leaks a refcount by using ManuallyDrop + clone; update the
convert_raw_result branch that handles restype_type "_type_" == "O" so that when
ptr == 0 you raise a ValueError("PyObject is NULL") on the vm and return None
(propagating an error), and when ptr != 0 construct the PyObjectRef by moving
ownership with PyObjectRef::from_raw(NonNull::new_unchecked(ptr as *mut
PyObject)) and return Some(obj) directly (do NOT wrap in ManuallyDrop or clone),
referencing the restype_type check, RawResult variants, and
PyObjectRef::from_raw to locate and fix the code.
🪄 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: bd14ff21-a37e-48fe-bff9-21046b4badbd

📥 Commits

Reviewing files that changed from the base of the PR and between 2149dcb and 3f5cfe3.

📒 Files selected for processing (2)
  • .github/workflows/ci.yaml
  • crates/vm/src/stdlib/_ctypes/function.rs

Comment thread crates/vm/src/stdlib/_ctypes/function.rs
@bschoenmaeckers
Copy link
Copy Markdown
Contributor Author

CI failures seem unrelated?

Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYSM for all the work on this!

just a minor request:)

Comment thread .github/workflows/ci.yaml
Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYSM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants