Skip to content

Add mapping c-api functions#8003

Merged
youknowone merged 1 commit into
RustPython:mainfrom
bschoenmaeckers:c-api-mapping
Jun 2, 2026
Merged

Add mapping c-api functions#8003
youknowone merged 1 commit into
RustPython:mainfrom
bschoenmaeckers:c-api-mapping

Conversation

@bschoenmaeckers
Copy link
Copy Markdown
Contributor

@bschoenmaeckers bschoenmaeckers commented Jun 1, 2026

Used AI to implement the mapping protocol c-api.

Summary by CodeRabbit

  • New Features
    • Added mapping protocol support to the C API so native code can query mapping size and retrieve keys, values, and items as Python objects, improving interoperability with native extensions and tooling.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 43d75432-ddb4-4e97-a455-a9697e170767

📥 Commits

Reviewing files that changed from the base of the PR and between 5bc9850 and a7c74a9.

📒 Files selected for processing (2)
  • crates/capi/src/abstract_.rs
  • crates/capi/src/abstract_/mapping.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/capi/src/abstract_.rs
  • crates/capi/src/abstract_/mapping.rs

📝 Walkthrough

Walkthrough

Adds a new mapping submodule and four C-API FFI functions (PyMapping_Size, PyMapping_Keys, PyMapping_Values, PyMapping_Items) that call into the VM to obtain mapping size or return new Python lists of keys/values/items; includes a disabled pyo3 test.

Changes

Mapping Protocol FFI Surface

Layer / File(s) Summary
Mapping protocol FFI functions and module wiring
crates/capi/src/abstract_.rs, crates/capi/src/abstract_/mapping.rs
New mapping module declared and re-exported from abstract_.rs. Four #[no_mangle] unsafe extern "C" functions (PyMapping_Size, PyMapping_Keys, PyMapping_Values, PyMapping_Items) each run inside with_vm, dereference the input PyObject, call try_mapping(...) to obtain the operation result, and return either an isize length or a new *mut PyObject list.
Mapping protocol validation tests
crates/capi/src/abstract_/mapping.rs
Disabled (#[cfg(false)]) test module constructs a PyDict, validates that len(), keys(), values(), and items() return correct sizes, and confirms each item is a tuple.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • youknowone
  • ShaharNaveh

Poem

🐇 I hopped through VM and mapping light,
Four tiny bridges to C took flight,
Keys, values, items in tidy rows,
Size counted true as the rabbit knows,
A little rustle of FFI delight.

🚥 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 'Add mapping c-api functions' directly and clearly describes the main change: adding C-API functions for the mapping protocol.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% 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.

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

🧹 Nitpick comments (1)
crates/capi/src/abstract_/mapping.rs (1)

10-38: 💤 Low value

Consider extracting shared logic into a helper to reduce duplication.

All three functions follow the same pattern: get mapping, call method, iterate to list. Per coding guidelines, when branches differ only in a method call but share common logic, extracting the common parts improves maintainability.

♻️ Possible helper extraction
+fn mapping_to_list(
+    obj: *mut PyObject,
+    get_items: impl FnOnce(&rustpython_vm::protocol::PyMapping, &rustpython_vm::VirtualMachine) -> rustpython_vm::PyResult<rustpython_vm::PyObjectRef>,
+) -> *mut PyObject {
+    with_vm(|vm| {
+        let obj = unsafe { &*obj };
+        let items = get_items(&obj.try_mapping(vm)?, vm)?;
+        let iter = items.get_iter(vm)?;
+        Ok(vm.ctx.new_list(iter.try_to_value(vm)?))
+    })
+}
+
 #[unsafe(no_mangle)]
 pub unsafe extern "C" fn PyMapping_Keys(obj: *mut PyObject) -> *mut PyObject {
-    with_vm(|vm| {
-        let obj = unsafe { &*obj };
-        let keys = obj.try_mapping(vm)?.keys(vm)?;
-        let iter = keys.get_iter(vm)?;
-        Ok(vm.ctx.new_list(iter.try_to_value(vm)?))
-    })
+    mapping_to_list(obj, |m, vm| m.keys(vm))
 }

Apply similarly to PyMapping_Values and PyMapping_Items.

🤖 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/abstract_/mapping.rs` around lines 10 - 38, Extract the
repeated logic in PyMapping_Keys, PyMapping_Values, and PyMapping_Items into a
small helper (e.g., fn map_to_list(obj: *mut PyObject, f: impl FnOnce(&dyn
Mapping, &VirtualMachine) -> PyResult<PyObjectRef>) -> *mut PyObject or a helper
taking an enum/closure) that: calls with_vm, dereferences obj, calls
obj.try_mapping(vm)?, invokes the specific mapping method (keys/values/items)
via the passed closure or selector, gets the iterator via get_iter(vm)?,
converts it with try_to_value(vm) and returns vm.ctx.new_list(...); then replace
the bodies of PyMapping_Keys, PyMapping_Values and PyMapping_Items to call this
helper with the appropriate mapping method selector so the only differing part
is the method passed in.
🤖 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/abstract_/mapping.rs`:
- Around line 40-66: Change the test module attribute from the disabled
#[cfg(false)] to #[cfg(test)] (or #[cfg(test)] plus #[ignore] if you want it
compiled but skipped), and update the test function size_keys_values_items to
call the RustPython C-API wrappers instead of only exercising PyO3: keep
Python::attach and creation of the PyDict/PyMapping, then invoke the C-API
functions (e.g., PyMapping_Size, PyMapping_Keys, PyMapping_Values,
PyMapping_Items or their crate wrapper functions) against the mapping and assert
their return values/lengths and that items are tuples, so the test actually
validates the C-API implementation rather than only PyO3 helpers.

---

Nitpick comments:
In `@crates/capi/src/abstract_/mapping.rs`:
- Around line 10-38: Extract the repeated logic in PyMapping_Keys,
PyMapping_Values, and PyMapping_Items into a small helper (e.g., fn
map_to_list(obj: *mut PyObject, f: impl FnOnce(&dyn Mapping, &VirtualMachine) ->
PyResult<PyObjectRef>) -> *mut PyObject or a helper taking an enum/closure)
that: calls with_vm, dereferences obj, calls obj.try_mapping(vm)?, invokes the
specific mapping method (keys/values/items) via the passed closure or selector,
gets the iterator via get_iter(vm)?, converts it with try_to_value(vm) and
returns vm.ctx.new_list(...); then replace the bodies of PyMapping_Keys,
PyMapping_Values and PyMapping_Items to call this helper with the appropriate
mapping method selector so the only differing part is the method passed in.
🪄 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: 1d45ea1b-0816-4cf2-9aa0-8956af0e0836

📥 Commits

Reviewing files that changed from the base of the PR and between 1ce37bf and 5cb4197.

📒 Files selected for processing (2)
  • crates/capi/src/abstract_.rs
  • crates/capi/src/abstract_/mapping.rs

Comment thread crates/capi/src/abstract_/mapping.rs
@youknowone youknowone enabled auto-merge (squash) June 1, 2026 15:37
auto-merge was automatically disabled June 1, 2026 16:43

Head branch was pushed to by a user without write access

Copy link
Copy Markdown
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

👍

@youknowone youknowone merged commit 74b4707 into RustPython:main Jun 2, 2026
26 checks passed
@bschoenmaeckers bschoenmaeckers deleted the c-api-mapping branch June 2, 2026 07:24
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