crates/capi/src/util.rs (1)

232-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix 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 value

Simplify null pointer check.

Since try_as_str(vm)? naturally handles null pointers by returning Option::None, you can simplify this block using if let instead 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.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/capi/src/util.rs (1)

232-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Fix 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 value

Simplify null pointer check.

Since try_as_str(vm)? naturally handles null pointers by returning Option::None, you can simplify this block using if let instead 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.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

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 -S

Repository: 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 in PyDescr_NewMember. The extra Ok(...) adds a nested Result and breaks the with_vm conversion 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 PyResult rather than panicking.

Calling .expect("...") on try_as_str(vm) triggers a Rust panic if the input is not valid UTF-8. Since we are inside a with_vm closure, 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.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/capi/src/unicodeobject.rs (1)

74-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Simplify null-pointer handling by leveraging try_as_str.

The try_as_str(vm) method natively handles null pointers by returning Ok(None). You can remove the manual is_null() branches and simplify these blocks using unwrap_or and map, matching the clean pattern already established in codecs.rs and warnings.rs.

• crates/capi/src/unicodeobject.rs#L74-L84: Replace the manual is_null() blocks for encoding and errors with unwrap_or and map.
• crates/capi/src/unicodeobject.rs#L177-L187: Replace the identical manual is_null() blocks with the same simplified pattern.

♻️ Proposed refactor

Apply this pattern to both locations:

-        let encoding = if encoding.is_null() {
-            "utf-8"
-        } else {
-            encoding.try_as_str(vm)?.unwrap()
-        };
-        let errors = if errors.is_null() {
-            None
-        } else {
-            let errors = errors.try_as_str(vm)?.unwrap();
-            Some(vm.ctx.new_utf8_str(errors))
-        };
+        let encoding = encoding.try_as_str(vm)?.unwrap_or("utf-8");
+        let errors = errors.try_as_str(vm)?.map(|e| vm.ctx.new_utf8_str(e));

🤖 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/unicodeobject.rs` around lines 74 - 84, In both
encoding/error handling blocks in crates/capi/src/unicodeobject.rs (lines 74-84
and 177-187), replace the manual is_null branches with try_as_str(vm), using
unwrap_or for the UTF-8 encoding default and map to construct the optional error
string. Apply the same simplified pattern at both sites.

crates/capi/src/import.rs (1)

52-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use try_as_str_opt for optional C strings.

Instead of manually checking for null and calling try_as_str, you can use the newly introduced try_as_str_opt, which is designed to handle null pointers gracefully by returning None.

♻️ Proposed refactor

-        if !pathname.is_null() {
-            let pathname = unsafe { pathname.try_as_str(vm) }?;
+        if let Some(pathname) = unsafe { pathname.try_as_str_opt(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 - 55, Update the pathname
conversion in the import logic to use try_as_str_opt instead of manually
checking pathname for null and calling try_as_str. Preserve setting the
"__file__" attribute only when the optional conversion returns Some, while
propagating conversion errors as before.

crates/capi/src/listobject.rs (1)

78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use assume_borrowed() for uniform pointer dereferencing.

To fully align with the PR's objective of standardizing raw pointer access via FfiPtrExt, consider replacing the remaining unsafe { &*item } dereferences with unsafe { item.assume_borrowed() }.

♻️ Proposed refactor for `PyList_Append` and other functions

For PyList_Append:

-        let item = unsafe { &*item }.to_owned();
+        let item = unsafe { item.assume_borrowed() }.to_owned();

Apply similar updates to PyList_Insert and PyList_SetSlice further down in the file:

// In PyList_Insert:
-        let item = unsafe { &*item }.to_owned();
+        let item = unsafe { item.assume_borrowed() }.to_owned();

// In PyList_SetSlice:
-        let items: Vec<PyObjectRef> = unsafe { &*itemlist }.try_to_value(vm)?;
+        let items: Vec<PyObjectRef> = unsafe { itemlist.assume_borrowed() }.try_to_value(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/listobject.rs` at line 78, Replace remaining raw pointer
dereferences in PyList_Append, PyList_Insert, and PyList_SetSlice with
FfiPtrExt::assume_borrowed(). Preserve the existing ownership conversions and
try_to_value behavior while standardizing all item/list pointer access through
the extension method.

crates/capi/src/floatobject.rs (1)

53-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Simplify null-checking and borrowing with assume_borrowed_or_opt.

Since FfiPtrExt provides assume_borrowed_or_opt(), you can streamline the null check and the borrow into a single step, removing the need to construct a NonNull manually.

♻️ Proposed refactor

-        let obj = NonNull::new(obj)
-            .ok_or_else(|| vm.new_type_error("float() argument must be a string or a number"))?;
-        let obj = unsafe { obj.as_ptr().assume_borrowed() }.to_owned();
+        let obj = unsafe { obj.assume_borrowed_or_opt() }
+            .ok_or_else(|| vm.new_type_error("float() argument must be a string or a number"))?
+            .to_owned();

🤖 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/floatobject.rs` around lines 53 - 55, Update the object
conversion flow in the float handling code to use
FfiPtrExt::assume_borrowed_or_opt() instead of constructing NonNull with
NonNull::new and then calling assume_borrowed(). Preserve the existing
type-error result when the pointer is null, and continue producing an owned
object for valid pointers.