Add codecs support to c-api#8267
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a public ChangesC API codec bridge and string conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
youknowone
left a comment
There was a problem hiding this comment.
could you also check if other boilerplates can be also refactored?
| } else { | ||
| unsafe { CStr::from_ptr(encoding) } | ||
| .to_str() | ||
| .map_err(|_| vm.new_system_error("encoding must be valid UTF-8"))? |
There was a problem hiding this comment.
please add a helper for this commonly used error
There was a problem hiding this comment.
I've added a helper. What do you think?
26a957c to
ecb2046
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/capi/src/util.rs (1)
232-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in the exception message.
The message
"argument of must be valid UTF-8"appears to be missing a word or incorrectly includes"of". Consider changing it to"argument must be valid UTF-8".♻️ Proposed fix
impl<'a> CStrExt<'a> for *mut c_char { fn try_as_str(self, vm: &VirtualMachine) -> PyResult<Option<&'a str>> { NonNull::new(self) .map(|ptr| unsafe { CStr::from_ptr(ptr.as_ptr()) }.to_str()) .transpose() - .map_err(|_| vm.new_system_error("argument of must be valid UTF-8")) + .map_err(|_| vm.new_system_error("argument must be valid UTF-8")) } }🤖 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/util.rs` around lines 232 - 239, Update the error message in CStrExt::try_as_str to remove the extraneous “of”, so invalid UTF-8 reports that the argument must be valid UTF-8.crates/capi/src/import.rs (1)
52-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify null pointer check.
Since
try_as_str(vm)?naturally handles null pointers by returningOption::None, you can simplify this block usingif letinstead of explicitly checking for.is_null()and calling.unwrap().♻️ Proposed fix
- if !pathname.is_null() { - let pathname = pathname.try_as_str(vm)?.unwrap(); + if let Some(pathname) = pathname.try_as_str(vm)? { module.set_attr("__file__", vm.ctx.new_str(pathname), vm)?; }🤖 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/import.rs` around lines 52 - 53, Update the pathname handling block to use `if let Some(pathname) = pathname.try_as_str(vm)?` directly, removing the explicit `is_null()` check and `unwrap()` while preserving the existing processing for valid pathnames.
🤖 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/descrobject.rs`:
- Around line 229-234: Update PyDescr_NewMember to return the result of
member.build directly from the with_vm closure, removing the wrapping Ok so the
existing with_vm conversion path receives the expected result type.
In `@crates/capi/src/pyerrors.rs`:
- Around line 211-216: Update the exception-name parsing in the with_vm closure
to propagate the PyResult from try_as_str(vm) with ?, removing the expect that
panics on invalid UTF-8; preserve the existing unwrap and rsplit_once validation
behavior.
---
Nitpick comments:
In `@crates/capi/src/import.rs`:
- Around line 52-53: Update the pathname handling block to use `if let
Some(pathname) = pathname.try_as_str(vm)?` directly, removing the explicit
`is_null()` check and `unwrap()` while preserving the existing processing for
valid pathnames.
In `@crates/capi/src/util.rs`:
- Around line 232-239: Update the error message in CStrExt::try_as_str to remove
the extraneous “of”, so invalid UTF-8 reports that the argument must be valid
UTF-8.
🪄 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: f7f94de3-6e6c-4046-b9cc-1149b6a5215f
📒 Files selected for processing (14)
crates/capi/src/abstract_.rscrates/capi/src/abstract_/mapping.rscrates/capi/src/codecs.rscrates/capi/src/descrobject.rscrates/capi/src/dictobject.rscrates/capi/src/import.rscrates/capi/src/lib.rscrates/capi/src/methodobject.rscrates/capi/src/object.rscrates/capi/src/pycapsule.rscrates/capi/src/pyerrors.rscrates/capi/src/util.rscrates/capi/src/warnings.rscrates/vm/src/sequence.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/capi/src/lib.rs
- crates/capi/src/codecs.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/capi/src/util.rs (1)
232-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in the exception message.
The message
"argument of must be valid UTF-8"appears to be missing a word or incorrectly includes"of". Consider changing it to"argument must be valid UTF-8".♻️ Proposed fix
impl<'a> CStrExt<'a> for *mut c_char { fn try_as_str(self, vm: &VirtualMachine) -> PyResult<Option<&'a str>> { NonNull::new(self) .map(|ptr| unsafe { CStr::from_ptr(ptr.as_ptr()) }.to_str()) .transpose() - .map_err(|_| vm.new_system_error("argument of must be valid UTF-8")) + .map_err(|_| vm.new_system_error("argument must be valid UTF-8")) } }🤖 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/util.rs` around lines 232 - 239, Update the error message in CStrExt::try_as_str to remove the extraneous “of”, so invalid UTF-8 reports that the argument must be valid UTF-8.crates/capi/src/import.rs (1)
52-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify null pointer check.
Since
try_as_str(vm)?naturally handles null pointers by returningOption::None, you can simplify this block usingif letinstead of explicitly checking for.is_null()and calling.unwrap().♻️ Proposed fix
- if !pathname.is_null() { - let pathname = pathname.try_as_str(vm)?.unwrap(); + if let Some(pathname) = pathname.try_as_str(vm)? { module.set_attr("__file__", vm.ctx.new_str(pathname), vm)?; }🤖 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/import.rs` around lines 52 - 53, Update the pathname handling block to use `if let Some(pathname) = pathname.try_as_str(vm)?` directly, removing the explicit `is_null()` check and `unwrap()` while preserving the existing processing for valid pathnames.
🤖 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/descrobject.rs`:
- Around line 229-234: Update PyDescr_NewMember to return the result of
member.build directly from the with_vm closure, removing the wrapping Ok so the
existing with_vm conversion path receives the expected result type.
In `@crates/capi/src/pyerrors.rs`:
- Around line 211-216: Update the exception-name parsing in the with_vm closure
to propagate the PyResult from try_as_str(vm) with ?, removing the expect that
panics on invalid UTF-8; preserve the existing unwrap and rsplit_once validation
behavior.
---
Nitpick comments:
In `@crates/capi/src/import.rs`:
- Around line 52-53: Update the pathname handling block to use `if let
Some(pathname) = pathname.try_as_str(vm)?` directly, removing the explicit
`is_null()` check and `unwrap()` while preserving the existing processing for
valid pathnames.
In `@crates/capi/src/util.rs`:
- Around line 232-239: Update the error message in CStrExt::try_as_str to remove
the extraneous “of”, so invalid UTF-8 reports that the argument must be valid
UTF-8.
🪄 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: f7f94de3-6e6c-4046-b9cc-1149b6a5215f
📒 Files selected for processing (14)
crates/capi/src/abstract_.rscrates/capi/src/abstract_/mapping.rscrates/capi/src/codecs.rscrates/capi/src/descrobject.rscrates/capi/src/dictobject.rscrates/capi/src/import.rscrates/capi/src/lib.rscrates/capi/src/methodobject.rscrates/capi/src/object.rscrates/capi/src/pycapsule.rscrates/capi/src/pyerrors.rscrates/capi/src/util.rscrates/capi/src/warnings.rscrates/vm/src/sequence.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/capi/src/lib.rs
- crates/capi/src/codecs.rs
🛑 Comments failed to post (2)
crates/capi/src/descrobject.rs (1)
229-234: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail printf '\n== descrobject.rs excerpt ==\n' sed -n '200,260p' crates/capi/src/descrobject.rs printf '\n== with_vm definition/usages ==\n' rg -n "fn with_vm|with_vm\(" -S crates | sed -n '1,120p' printf '\n== build() signatures near PyMemberDescriptor ==\n' rg -n "build\(" crates/capi/src/descrobject.rs crates -S | sed -n '1,120p'Repository: RustPython/RustPython
Length of output: 12962
🏁 Script executed:
set -euo pipefail echo '=== descrobject.rs (220-240) ===' sed -n '220,240p' crates/capi/src/descrobject.rs echo echo '=== search with_vm definition ===' rg -n "pub .*with_vm|fn with_vm|with_vm\s*\(" crates -S | sed -n '1,120p' echo echo '=== search build signature for PyMemberDef ===' rg -n "struct PyMemberDef|impl .*PyMemberDef|build\(.*PyTypeObject|PyResult<PyRef<PyMemberDescriptor>>" crates -S | sed -n '1,160p'Repository: RustPython/RustPython
Length of output: 9132
🏁 Script executed:
set -euo pipefail echo '=== crates/vm/src/vm/thread.rs (with_vm) ===' sed -n '740,810p' crates/vm/src/vm/thread.rs echo echo '=== crates/capi/src/descrobject.rs (PyMemberDef build) ===' sed -n '120,155p' crates/capi/src/descrobject.rs echo echo '=== nearby patterns in crates/capi/src ===' rg -n "with_vm\\(\\|.*Ok\\(.*build\\(" crates/capi/src -S | sed -n '1,120p'Repository: RustPython/RustPython
Length of output: 4154
🏁 Script executed:
set -euo pipefail echo '=== crates/capi/src/descrobject.rs (PyGetSetDef build) ===' sed -n '1,80p' crates/capi/src/descrobject.rs echo echo '=== crates/capi/src/descrobject.rs (PyMemberDef build continued) ===' sed -n '80,180p' crates/capi/src/descrobject.rs echo echo '=== functions in descrobject.rs using with_vm + Ok(...) ===' rg -n "with_vm\\(\\|\\s*Ok\\(" crates/capi/src/descrobject.rs -n -SRepository: RustPython/RustPython
Length of output: 7393
🏁 Script executed:
set -euo pipefail echo '=== crates/capi/src/descrobject.rs (1-180) ===' sed -n '1,180p' crates/capi/src/descrobject.rs echo echo '=== other with_vm + Ok(...) patterns in capi ===' rg -n "with_vm\\(\\|\\s*Ok\\(" crates/capi/src -S | sed -n '1,200p'Repository: RustPython/RustPython
Length of output: 7223
🏁 Script executed:
set -euo pipefail echo '=== crates/capi/src/pystate.rs ===' sed -n '1,220p' crates/capi/src/pystate.rs echo echo '=== direct uses of crate::pystate::with_vm in capi ===' rg -n "crate::pystate::with_vm|with_vm\\(" crates/capi/src -S | sed -n '1,220p'Repository: RustPython/RustPython
Length of output: 18885
🏁 Script executed:
set -euo pipefail echo '=== crates/capi/src/util.rs ===' sed -n '1,220p' crates/capi/src/util.rs echo echo '=== FfiResult implementations ===' rg -n "trait FfiResult|impl .*FfiResult" crates/capi/src crates -S | sed -n '1,220p'Repository: RustPython/RustPython
Length of output: 7911
Return
build()directly inPyDescr_NewMember. The extraOk(...)adds a nestedResultand breaks thewith_vmconversion path here.🤖 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/descrobject.rs` around lines 229 - 234, Update PyDescr_NewMember to return the result of member.build directly from the with_vm closure, removing the wrapping Ok so the existing with_vm conversion path receives the expected result type.crates/capi/src/pyerrors.rs (1)
211-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Propagate the
PyResultrather than panicking.Calling
.expect("...")ontry_as_str(vm)triggers a Rust panic if the input is not valid UTF-8. Since we are inside awith_vmclosure, propagating the result via?will correctly translate the failure into a Python exception state.♻️ Proposed fix
let (module, name) = name - .try_as_str(vm) - .expect("Exception name is not valid UTF-8") - .unwrap() + .try_as_str(vm)? + .unwrap() .rsplit_once('.') .expect("Exception name must be of the form 'module.ExceptionName'");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let (module, name) = name .try_as_str(vm)? .unwrap() .rsplit_once('.') .expect("Exception name must be of the form 'module.ExceptionName'");🤖 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/pyerrors.rs` around lines 211 - 216, Update the exception-name parsing in the with_vm closure to propagate the PyResult from try_as_str(vm) with ?, removing the expect that panics on invalid UTF-8; preserve the existing unwrap and rsplit_once validation behavior.
Summary by CodeRabbit