Skip to content

Add more mapping functions to c-api#8169

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

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

Conversation

@bschoenmaeckers

@bschoenmaeckers bschoenmaeckers commented Jun 25, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features
    • Extended the C API with additional mapping helpers: mapping checks, getting/setting items by UTF-8 string keys, and computing mapping length.
    • Added optional lookup functions that return a not-found result (instead of raising) when a key is missing.
    • Added key-existence checks with *WithError variants for precise failure handling, including defined behavior for invalid UTF-8 input.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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: 66c089b5-3beb-4651-9ead-33cbebd09f90

📥 Commits

Reviewing files that changed from the base of the PR and between 3b5e313 and 94cee7f.

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

Walkthrough

Adds exported mapping C-API wrappers for check, length, lookup, key-existence, and string-key assignment. String-key paths decode C strings as UTF-8, convert invalid keys into VM ValueErrors, and map KeyError to not-found results in the optional and existence variants.

Changes

Mapping C-API additions

Layer / File(s) Summary
Check and length wrappers
crates/capi/src/abstract_/mapping.rs		 Adds PyMapping_Check and PyMapping_Length, plus the core::ffi imports needed for the new exported wrappers.
Lookup, existence, and assignment wrappers
crates/capi/src/abstract_/mapping.rs		 Adds string-key item access, optional item retrieval, key-existence checks, and string-key assignment, including UTF-8 decoding and KeyError handling.

Estimated review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • RustPython/RustPython#8003: Adds earlier PyMapping_* C-API surface in the same file; this PR extends that mapping API with additional wrappers.

Suggested reviewers

  • youknowone

Poem

A rabbit found a mapping key 🐇
and nibbled UTF-8 with glee.
When KeyError hopped out of sight,
the wrapper said, “not found is right.”
Then check and length went bounding free.

🚥 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 matches the main change by describing the addition of more mapping functions to the C API.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% 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.

coderabbitai[bot]
coderabbitai Bot reviewed Jun 25, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

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/capi/src/abstract_/mapping.rs`:
- Around line 137-140: PyMapping_HasKeyString currently propagates invalid UTF-8
as a ValueError via the CStr::from_ptr(...).to_str() conversion in mapping.rs,
which violates the C API contract. Update the PyMapping_HasKeyString path to
treat UTF-8 decode failures as a silent miss by returning Ok(false) immediately
instead of mapping the error into vm.new_value_error, while keeping the existing
obj.get_item(key, vm).is_ok() behavior for valid keys.
🪄 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: b25ba3c3-1271-4bac-aca3-25878e574cad

📥 Commits

Reviewing files that changed from the base of the PR and between 870ad2d and 3b5e313.

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

Comment thread crates/capi/src/abstract_/mapping.rs Outdated
ShaharNaveh
ShaharNaveh approved these changes Jun 25, 2026
View reviewed changes

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀

@youknowone youknowone merged commit c876709 into RustPython:main Jun 25, 2026
26 checks passed
@bschoenmaeckers bschoenmaeckers deleted the c-api-more-mapping branch June 25, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@youknowone youknowone youknowone approved these changes
@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bschoenmaeckers @youknowone @ShaharNaveh