Remove useless &PyRef patterns #6455
Conversation
WalkthroughThis PR shifts many internal APIs to use owned/wrapper types ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-12-09T08:46:58.660ZApplied to files:
📚 Learning: 2025-11-10T06:27:41.954ZApplied to files:
📚 Learning: 2025-11-29T12:17:28.606ZApplied to files:
📚 Learning: 2025-11-29T12:17:28.606ZApplied to files:
🧬 Code graph analysis (3)crates/wasm/src/js_module.rs (1)
crates/vm/src/exception_group.rs (4)
crates/vm/src/builtins/function/jit.rs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (23)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/vm/src/stdlib/ctypes/base.rs (1)
51-77: Duplicate condition detected - second check is redundant.Lines 54-61 check
downcast_exact::<PyBytes>twice with identical logic. This appears to be a copy-paste error.fn set_primitive(_type_: &str, value: &PyObject, vm: &VirtualMachine) -> PyResult { match _type_ { "c" => { if value .to_owned() .downcast_exact::<PyBytes>(vm) .is_ok_and(|v| v.len() == 1) - || value - .to_owned() - .downcast_exact::<PyBytes>(vm) - .is_ok_and(|v| v.len() == 1) || value .to_owned() .downcast_exact::<PyInt>(vm) .map_or(Ok(false), |v| {crates/stdlib/src/openssl.rs (1)
3354-3377: Type incompatibility between function signature and call sites requires fixing.The function signature expects
&Py<PyBaseException>, but call sites at lines 2500 and 2540 pass&PyBaseExceptionRef(returned fromconvert_ssl_error). These types are incompatible and will cause compilation errors. Either update the function signature to accept&PyBaseExceptionRefor convert the exception objects at call sites before passing them.
🧹 Nitpick comments (1)
crates/stdlib/src/scproxy.rs (1)
77-107: Type inconsistency between closure parameter and function return types.The closure on line 77 accepts
result: &Py<PyDict>, while both containing functions (lines 35, 70) declare return typePyResult<Option<PyDictRef>>. Theresultvariable is created viavm.ctx.new_dict()on lines 40 and 75.While the code compiles without errors, the types are inconsistent: the functions claim to return
PyDictRefbut usePy<PyDict>internally. For clarity and maintainability, align the function return types with the actual types being used, either adoptingPy<PyDict>throughout or usingPyDictRefconsistently.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
crates/stdlib/src/csv.rs(6 hunks)crates/stdlib/src/openssl.rs(2 hunks)crates/stdlib/src/scproxy.rs(2 hunks)crates/stdlib/src/ssl.rs(3 hunks)crates/stdlib/src/ssl/compat.rs(3 hunks)crates/vm/src/builtins/builtin_func.rs(1 hunks)crates/vm/src/builtins/dict.rs(5 hunks)crates/vm/src/builtins/function/jit.rs(2 hunks)crates/vm/src/builtins/genericalias.rs(2 hunks)crates/vm/src/builtins/list.rs(1 hunks)crates/vm/src/builtins/property.rs(3 hunks)crates/vm/src/builtins/type.rs(1 hunks)crates/vm/src/builtins/union.rs(1 hunks)crates/vm/src/cformat.rs(5 hunks)crates/vm/src/codecs.rs(1 hunks)crates/vm/src/coroutine.rs(2 hunks)crates/vm/src/exception_group.rs(3 hunks)crates/vm/src/exceptions.rs(6 hunks)crates/vm/src/frame.rs(1 hunks)crates/vm/src/import.rs(2 hunks)crates/vm/src/object/payload.rs(2 hunks)crates/vm/src/scope.rs(1 hunks)crates/vm/src/sequence.rs(2 hunks)crates/vm/src/stdlib/builtins.rs(2 hunks)crates/vm/src/stdlib/collections.rs(1 hunks)crates/vm/src/stdlib/ctypes.rs(2 hunks)crates/vm/src/stdlib/ctypes/base.rs(9 hunks)crates/vm/src/stdlib/ctypes/field.rs(2 hunks)crates/vm/src/stdlib/ctypes/structure.rs(3 hunks)crates/vm/src/stdlib/ctypes/union.rs(2 hunks)crates/vm/src/stdlib/typevar.rs(2 hunks)crates/vm/src/suggestion.rs(3 hunks)crates/vm/src/vm/context.rs(2 hunks)crates/vm/src/vm/mod.rs(3 hunks)crates/vm/src/warn.rs(2 hunks)crates/wasm/src/convert.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/codecs.rscrates/vm/src/sequence.rscrates/vm/src/builtins/list.rscrates/vm/src/builtins/union.rscrates/vm/src/vm/mod.rscrates/vm/src/builtins/type.rscrates/vm/src/frame.rscrates/vm/src/coroutine.rscrates/vm/src/stdlib/collections.rscrates/vm/src/object/payload.rscrates/vm/src/builtins/builtin_func.rscrates/stdlib/src/ssl/compat.rscrates/vm/src/scope.rscrates/vm/src/stdlib/ctypes.rscrates/vm/src/builtins/function/jit.rscrates/vm/src/warn.rscrates/vm/src/builtins/dict.rscrates/vm/src/stdlib/typevar.rscrates/vm/src/builtins/genericalias.rscrates/vm/src/stdlib/builtins.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/suggestion.rscrates/vm/src/exceptions.rscrates/vm/src/import.rscrates/vm/src/stdlib/ctypes/field.rscrates/stdlib/src/csv.rscrates/wasm/src/convert.rscrates/vm/src/builtins/property.rscrates/vm/src/vm/context.rscrates/stdlib/src/openssl.rscrates/vm/src/exception_group.rscrates/vm/src/cformat.rscrates/stdlib/src/scproxy.rscrates/vm/src/stdlib/ctypes/union.rscrates/stdlib/src/ssl.rscrates/vm/src/stdlib/ctypes/structure.rs
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/vm/mod.rscrates/vm/src/coroutine.rscrates/vm/src/suggestion.rscrates/vm/src/exceptions.rscrates/vm/src/import.rscrates/wasm/src/convert.rscrates/stdlib/src/openssl.rscrates/vm/src/exception_group.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
crates/stdlib/src/ssl/compat.rscrates/vm/src/import.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/stdlib/ctypes.rscrates/vm/src/import.rscrates/stdlib/src/csv.rs
📚 Learning: 2025-11-10T06:27:41.954Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.
Applied to files:
crates/vm/src/stdlib/ctypes/base.rscrates/vm/src/exceptions.rscrates/wasm/src/convert.rscrates/vm/src/stdlib/ctypes/union.rscrates/vm/src/stdlib/ctypes/structure.rs
🧬 Code graph analysis (13)
crates/vm/src/sequence.rs (3)
crates/vm/src/builtins/list.rs (2)
do_get(370-372)index(265-278)crates/vm/src/stdlib/collections.rs (2)
do_get(425-427)index(165-187)crates/vm/src/frame.rs (1)
elem(2541-2541)
crates/vm/src/builtins/list.rs (2)
crates/vm/src/sequence.rs (1)
do_get(15-15)crates/vm/src/stdlib/collections.rs (2)
do_get(425-427)index(165-187)
crates/vm/src/builtins/union.rs (3)
crates/vm/src/builtins/genericalias.rs (1)
args(102-102)crates/vm/src/exceptions.rs (2)
args(568-570)args(574-574)crates/wasm/src/convert.rs (1)
args(227-231)
crates/vm/src/vm/mod.rs (1)
crates/vm/src/stdlib/sys.rs (1)
exception(341-343)
crates/vm/src/stdlib/collections.rs (2)
crates/vm/src/builtins/list.rs (2)
do_get(370-372)index(265-278)crates/vm/src/sequence.rs (1)
do_get(15-15)
crates/stdlib/src/ssl/compat.rs (2)
crates/vm/src/builtins/dict.rs (1)
err(82-84)crates/vm/src/stdlib/io.rs (1)
vm(3519-3521)
crates/vm/src/stdlib/ctypes.rs (1)
crates/vm/src/frame.rs (2)
cls(1154-1155)cls(1202-1203)
crates/vm/src/builtins/function/jit.rs (1)
crates/vm/src/builtins/dict.rs (1)
dict(756-756)
crates/vm/src/builtins/dict.rs (1)
crates/vm/src/object/core.rs (1)
dict(693-695)
crates/stdlib/src/csv.rs (5)
crates/vm/src/stdlib/collections.rs (1)
obj(448-449)crates/vm/src/stdlib/ctypes/field.rs (4)
obj(106-106)obj(115-115)obj(200-200)obj(217-217)crates/stdlib/src/sqlite.rs (5)
obj(2090-2090)obj(2091-2091)obj(2943-2943)obj(2948-2948)obj(2951-2951)crates/vm/src/stdlib/builtins.rs (4)
attr(230-230)attr(408-408)attr(429-429)attr(831-831)crates/vm/src/protocol/object.rs (1)
to_owned(381-381)
crates/vm/src/cformat.rs (2)
crates/vm/src/frame.rs (1)
element(596-596)crates/vm/src/stdlib/io.rs (1)
vm(3519-3521)
crates/vm/src/stdlib/ctypes/union.rs (2)
crates/vm/src/stdlib/ctypes/structure.rs (2)
get_field_size(112-139)field_type(277-278)crates/vm/src/stdlib/ctypes.rs (1)
field_type(734-734)
crates/vm/src/stdlib/ctypes/structure.rs (2)
crates/vm/src/stdlib/ctypes/union.rs (1)
get_field_size(93-117)crates/vm/src/stdlib/ctypes.rs (1)
field_type(734-734)
🔇 Additional comments (71)
crates/vm/src/builtins/type.rs (1)
176-189: LGTM! Clean ownership refactoring.The signature and implementation changes correctly refactor from
&PyTypeRefto&Py<PyType>, updating the conversion method fromclone()toto_owned()to match the new type. This aligns with the broader PR objective of systematically removing&PyRefpatterns in favor ofPy<T>ownership semantics.crates/vm/src/stdlib/typevar.rs (2)
47-68: LGTM!The signature change from
&PyObjectRefto&PyObjectis a clean simplification. Callers using&obj_ref(whereobj_ref: PyObjectRef) work seamlessly via auto-deref. The special-case handling forbuiltinsand<...>module names correctly follows CPython behavior.
1008-1022: LGTM!The signature changes from
&PyObjectRefto&PyObjectare consistent with the PR objective. The switch from.clone()to.to_owned()on line 1017 is the correct idiom for converting&PyObjectto an ownedPyObjectReffor assignment into the mutex-guarded field.crates/vm/src/frame.rs (2)
1867-1872: LGTM! Parameter type change aligns with API migration.The parameter type change from
&PyObjectRefto&PyObjectis consistent with the broader refactoring effort described in the PR objectives. Call sites correctly pass references that coerce to&PyObjectthrough Rust's deref coercion.
1876-1876: LGTM! Usingto_owned()is more idiomatic.The change from
clone()toto_owned()is more idiomatic when converting from a reference to an owned value, and correctly maintains the same semantics of creating an owned copy to pass tovm.get_method().crates/vm/src/object/payload.rs (2)
107-117: LGTM! Consistent type refinement for error helper.The parameter type change from
&PyTypeRefto&Py<PyType>aligns with the PR's objective to removePyRefpatterns. The function logic remains unchanged, and the signature is now consistent with the second error helper function.
124-134: LGTM! Parallel type refinement maintains consistency.The parameter type change from
&PyTypeRefto&Py<PyType>mirrors the change in_into_ref_size_error, maintaining consistency between both error helper functions. The error message logic is preserved.crates/vm/src/builtins/genericalias.rs (3)
297-299: LGTM! Signature refactored to use&PyObject.The change from
&PyObjectRefto&PyObjectaligns with the PR's objective to remove useless reference patterns. All call sites (lines 263, 280, 329, 470) correctly pass compatible types from tuple iteration.
301-311: LGTM! Signature refactored to use&PyObject.The parameter type change to
&PyObjectis consistent with the broader refactoring effort. The implementation correctly handles both the presence and absence of the__typing_is_unpacked_typevartuple__attribute, returning the appropriate boolean value in each case.
313-357: LGTM! Signature refactored to use&Py<PyTuple>.The parameter type change from
&PyTupleRefto&Py<PyTuple>removes the type alias in favor of the direct type, aligning with the PR's refactoring goals. The implementation correctly usesas_slice()(line 329) and indexing operations (line 330) that are available on the new type. All call sites (lines 474, 478) pass compatible types.crates/vm/src/scope.rs (1)
37-37: Clarify the refactoring scope for this file's PyDictRef usage.Line 37's commented function signature was updated to
&Py<PyDict>, but active code throughout this file (lines 1, 7, 19, 26) still usesPyDictRef. Given the PR goal to remove &PyRef patterns, either the active code should also be refactored or the comment update is premature. Please clarify the intended state.crates/stdlib/src/scproxy.rs (1)
8-11: Complete the refactoring to usePy<PyDict>consistently.Line 9 imports both
PyDictandPyDictRef, but if this PR aims to replacePyRefpatterns with owned types (Py<PyDict>), verify thatPyDictRefis not used elsewhere in the file. If unused, remove it from the imports.Additionally, run
cargo fmtandcargo clippyto ensure the changes comply with Rust best practices.crates/vm/src/stdlib/ctypes/union.rs (1)
10-10: LGTM! Consistent with the PR's type refactoring objective.The signature change from
&PyObjectRefto&PyObjectaligns with the broader refactor. The implementation is identical toPyCStructType::get_field_sizeinstructure.rs- consider extracting to a shared utility if this pattern continues.Also applies to: 93-117
crates/vm/src/stdlib/ctypes/base.rs (1)
440-445: LGTM! Signature changes are consistent.The
value_to_bytes_endiansignature update from&PyObjectRefto&PyObjectis consistent with the refactor. Theto_owned()call at line 603 is necessary sincetry_to_boolconsumes ownership.Also applies to: 603-607
crates/vm/src/stdlib/ctypes/field.rs (1)
4-4: LGTM! Consistent type refactoring.The
value_to_bytessignature change to&PyObjectis appropriate. Call sites at lines 205 and 222 pass&value(wherevalue: PyObjectRef), which correctly derefs to&PyObject.Also applies to: 155-180
crates/vm/src/stdlib/ctypes/structure.rs (1)
10-10: LGTM! Signature updates align with the refactor.Both
get_field_sizeandget_field_alignnow accept&PyObjectinstead of&PyObjectRef. The call sites at lines 97, 273, and 274 correctly pass references that deref appropriately.Also applies to: 112-139, 142-157
crates/vm/src/stdlib/ctypes.rs (1)
351-358: Signature Simplification is Correct and All Call Sites Are CompatibleThe signature change from
Either<&PyObjectRef, &PyTypeRef>toEither<&PyObject, &PyTypeRef>is verified as correct. The match implementation at lines 355-358 correctly unifies both variants to&PyObject:
Either::A(obj)directly returns the&PyObjectEither::B(typ)callstyp.as_object()to convert&PyTypeRefto&PyObjectAll 13 call sites across
ctypes.rs(line 438) andbase.rs(lines 265, 291, 299, 309, 316, 329, 338, 346, 357, 365) passEither::B(&cls)orEither::B(&type_ref), making them fully compatible with the updated signature. Theas_object()method exists and is used throughout the codebase for type conversions.crates/vm/src/builtins/union.rs (1)
45-47: LGTM!The return type change from
&PyTupleRefto&Py<PyTuple>is correct sincePyRef<T>derefs toPy<T>. This aligns with the PR's goal of simplifying reference patterns while maintaining the same semantics.crates/vm/src/codecs.rs (1)
55-57: LGTM!The return type change from
&PyTupleRefto&Py<PyTuple>is consistent with the PR's refactoring pattern. SincePyTupleRefderefs toPy<PyTuple>, returning a reference to the innerPy<PyTuple>is semantically equivalent.crates/vm/src/sequence.rs (2)
15-15: LGTM!The trait method signature change from
Option<&PyObjectRef>toOption<&PyObject>is correct. The implementations inlist.rsandcollections.rsuse.map(|r| r.as_ref())to properly convert the reference.
79-79: LGTM!Changing from
elem.clone()toelem.to_owned()is correct sinceelemis now&PyObject. Theto_owned()call creates an ownedPyObjectfor use inrich_compare_bool.crates/vm/src/vm/mod.rs (3)
26-26: LGTM!The new import for
PyBaseExceptionis required to support the updated parameter types in the methods below.
730-741: LGTM!The parameter type change from
&PyBaseExceptionRefto&Py<PyBaseException>is consistent with the PR's refactoring pattern. The method body correctly usesexc.as_object()to access the underlying object.
818-832: LGTM!The parameter type change from
&PyBaseExceptionRefto&Py<PyBaseException>aligns with the PR's goal. All usages within the method (.is(),.__context__(),.set___context__()) work correctly withPy<PyBaseException>.crates/vm/src/vm/context.rs (2)
65-69: LGTM!The parameter type change from
&PyTypeRefto&Py<PyType>and the correspondingtyp.to_owned()call are correct. This aligns with the PR's refactoring pattern.
320-320: LGTM!The call site correctly passes
types.str_type(which is&'static Py<PyType>) directly to the updatedConstName::newsignature.crates/vm/src/builtins/list.rs (1)
370-372: LGTM!The
do_getimplementation correctly aligns with the updatedMutObjectSequenceOptrait signature. The.map(|r| r.as_ref())properly converts&PyObjectRefto&PyObject, maintaining consistency with the trait definition incrates/vm/src/sequence.rsand the parallelPyDequeimplementation.crates/vm/src/stdlib/collections.rs (1)
425-427: LGTM!The
do_getimplementation forPyDequeis consistent with thePyListimplementation and correctly satisfies the updatedMutObjectSequenceOptrait signature.crates/wasm/src/convert.rs (1)
35-56: LGTM!The signature change from
&PyBaseExceptionRefto&Py<PyBaseException>aligns with the PR's goal. SincePyBaseExceptionRefisPyRef<PyBaseException>andPyRef<T>derefs toPy<T>, the call site at line 170 (py_err_to_js_err(vm, &err)) will work correctly through Rust's auto-deref coercion.crates/vm/src/warn.rs (1)
41-52: LGTM!The signature change from
&PyObjectRefto&PyObjectis consistent with the PR's refactoring goal. Call sites inget_filter(lines 131, 143) pass&PyObjectRefwhich correctly coerces to&PyObjectthrough Rust's deref coercion.crates/vm/src/stdlib/builtins.rs (1)
263-287: LGTM!The parameter type change from
&PyObjectRefto&PyObjectfor the internalvalidate_globals_dicthelper is consistent with the PR's refactoring approach. The call site at line 291 correctly leverages auto-deref coercion.crates/vm/src/builtins/dict.rs (3)
756-756: LGTM!The return type change from
&PyDictRefto&Py<PyDict>aligns with the PR's ownership semantics refactoring. SincePyDictRefisPyRef<PyDict>andPyRef<T>implementsDeref<Target = Py<T>>, the macro implementation correctly returns&Py<PyDict>through deref coercion.
788-790: LGTM!The macro implementation correctly satisfies the updated trait signature by returning
&self.dict, which coerces from&PyRef<PyDict>to&Py<PyDict>through Rust's auto-deref.
1145-1145: LGTM!The switch from
.clone()to.to_owned()in themappinggetters is appropriate. Sincedict()now returns&Py<PyDict>rather than&PyDictRef, calling.to_owned()correctly produces aPyRef<PyDict>via theToOwnedtrait implementation, maintaining the same behavior.Also applies to: 1209-1209, 1272-1272
crates/vm/src/builtins/builtin_func.rs (1)
54-58: LGTM!The signature change from
Option<&PyObjectRef>toOption<&PyObject>withas_deref()is correct. The removal ofconst fnis necessary sinceas_deref()is not a const function.crates/vm/src/builtins/function/jit.rs (1)
47-67: LGTM!The signature change from
&PyDictRefto&Py<PyDict>aligns with the PR's goal. The call sites at lines 98, 102, and 121 pass&dict(wheredict: PyDictRef), which works correctly via Deref coercion sincePyDictRefderefs toPy<PyDict>.crates/vm/src/cformat.rs (2)
209-224: LGTM!The parameter type change to
Option<&PyObject>is correct. The internal logic remains unchanged, and the function correctly handles the borrowedPyObjectreference.
258-262: Correct adaptation of call sites.The
.map(|v| v.as_ref())conversion fromOption<&PyObjectRef>toOption<&PyObject>is the appropriate way to adapt the iterator elements to the updated function signature.crates/stdlib/src/csv.rs (4)
134-151: LGTM!The signature change to
&PyObjectand the use ofobj.to_owned()for thematch_class!macro are correct. The recursive call at line 136 with&attrworks via Deref coercion.
152-172: LGTM!The parameter type change is consistent with the broader refactoring pattern.
173-194: LGTM!Signature correctly updated to accept
&PyObject.
195-231: LGTM!Both
prase_lineterminator_from_objandprase_quoting_from_objare correctly updated to use&PyObject. Note: the "prase" typo in function names is pre-existing and unrelated to this PR.crates/vm/src/exception_group.rs (5)
355-357: LGTM!The parameter type change to
&PyObjectis correct and aligns with the refactoring goals.
359-370: LGTM!Using
&Py<PyBaseException>instead of&PyRef<PyBaseException>is consistent with the ownership model being established across the codebase.
378-416: LGTM!The
get_condition_matcherfunction correctly uses&PyObjectfor the condition parameter, andcondition.to_owned()appropriately creates an owned reference when storing in theCallablevariant.
418-429: LGTM!The
checkmethod signature change to&PyObjectis correct. Theexc.to_owned()call at line 424 correctly converts the borrowed reference to an owned value for the function call.
431-469: LGTM!The
derive_and_copy_attributesfunction correctly uses&Py<PyBaseException>for the original exception reference. The logic for copying traceback, context, cause, and notes remains functionally equivalent.crates/vm/src/coroutine.rs (2)
2-2: LGTM: Imports correctly updated for type migration.The addition of
PyandPyBaseExceptionimports supports the function signature change inis_gen_exit, aligning with the broader migration from reference types to owned/wrapper types.Also applies to: 5-5
211-213: LGTM: Function signature correctly updated.The change from
&PyBaseExceptionRefto&Py<PyBaseException>is consistent with the PR's objective to migrate from reference types to owned/wrapper types. The function body remains unchanged andfast_isinstanceworks identically with both parameter types.crates/stdlib/src/ssl.rs (3)
1425-1432: LGTM: Parameter type correctly updated.The change from
PyObjectRefto&PyObjectfor theenvironparameter is correct. The function body uses only methods that work with references (get_item,try_into_value), so no additional changes are needed internally.
2103-2113: LGTM: Reference parameter with internal cloning.The change to accept
&PyObjectinstead ofPyObjectRefis correct. The function uses.to_owned()to create owned values when callingtry_from_object, which is necessary since those functions expectPyObjectRef. This cloning pattern is acceptable for these helper functions as they're used during setup/initialization rather than hot paths.
2281-2290: LGTM: Consistent refactoring pattern.The parameter type change from
PyObjectRefto&PyObjectfollows the same pattern asparse_path_arg, using.to_owned()internally when owned values are required. This is consistent with the PR's objective to migrate toward using references instead of owned types at function boundaries.crates/stdlib/src/openssl.rs (1)
59-60: LGTM - Transitional imports for type migration.The addition of
PyBaseExceptionalongsidePyBaseExceptionRefis appropriate for this incremental refactoring. Both types are needed during the migration period.crates/stdlib/src/ssl/compat.rs (3)
26-29: LGTM - Import updates align with type migration.The import changes correctly add
PyBaseExceptionandPywhile maintainingPyBaseExceptionReffor transitional compatibility. This is the expected pattern during incremental refactoring.
987-989: LGTM - Signature change is consistent with PR objectives.The change from
&PyBaseExceptionRefto&Py<PyBaseException>is consistent with the PR's goal of removing reference type patterns. The function body usesfast_isinstancewhich is compatible with the new type.
1537-1556: LGTM - Helper function signature updated correctly.The signature change follows the same pattern as
is_blocking_io_error. The function body's use offast_isinstanceand attribute access methods are compatible withPy<PyBaseException>.crates/vm/src/suggestion.rs (3)
5-8: LGTM - Import updates support type migration.The import changes appropriately add
PyandPyObjectwhile replacingPyBaseExceptionRefwithPyBaseException. This aligns with the broader refactoring effort.
50-76: LGTM - Exception type signature updated correctly.The change from
&PyBaseExceptionRefto&Py<PyBaseException>follows the standard refactoring pattern seen throughout the PR. The function body correctly uses the new type with methods likefast_isinstanceandas_object().get_attr().
15-48: Verify PyObject vs PyObjectRef type change — CONFIRMED CORRECT.The type change from
&PyObjectRefto&PyObjectis correct and all call sites have been properly updated. Since PyObjectRef isRc<PyObject<dyn PyObjectPayload>>,&PyObjectRefcan deref coerce to&PyObject, making the types compatible.All four call sites (lines 55, 62, 67, 72) and the external call in crates/stdlib/src/suggestions.rs pass
&PyObjectRefwhich is compatible with the function signature expecting&PyObject. The function body's use ofdowncast_refworks correctly with this parameter type.crates/vm/src/import.rs (2)
4-9: LGTM - Import reorganization for type migration.The import updates correctly add
PyandPyBaseExceptionwhile reorganizing the builtins imports. The changes support the type signature updates in this file.
208-219: LGTM - Function signature updated consistently.The change from
&PyBaseExceptionRefto&Py<PyBaseException>is consistent with the PR's refactoring pattern. The function body correctly usesfast_isinstanceand traceback methods that are compatible with the new type.crates/vm/src/exceptions.rs (7)
79-86: LGTM! Clean type refactoring for write_exception.The parameter type change from
&PyBaseExceptionRefto&Py<PyBaseException>removes redundant indirection (&PyRef<T>→&Py<T>). The function logic is unchanged.Note: This is a public API change. Ensure all call sites across the codebase have been updated to handle the new signature.
88-129: LGTM! Consistent type refactoring for write_exception_recursive.The parameter type change aligns with the refactoring in
write_exception. Function logic and recursive calls remain unchanged.
132-170: LGTM! Type refactoring for write_exception_inner.The parameter type change is consistent with other exception handling functions. All field accesses and method calls work identically with the new
&Py<PyBaseException>type.
177-279: LGTM! Type refactoring for write_syntaxerror.The parameter type change maintains consistency with other exception writing methods. The complex syntax error formatting logic is unchanged and compatible with the new type.
370-385: LGTM! Type refactoring for write_traceback_entry.Consistent with the exception type changes, this function now uses
&Py<PyTraceback>instead of&PyTracebackRef. All field accesses remain valid.
1054-1064: LGTM! Type refactoring for SerializeException.The struct field and constructor parameter types are updated consistently with the exception handling function changes. The straightforward constructor logic is preserved.
Note: This is a public API change for the serde feature. Ensure downstream serialization code is compatible.
55-77: The type coercion from&PyRef<T>to&Py<T>works correctly through Rust's Deref trait. PyRef derefs to a shared/immutable reference, and the RustPython implementation confirms this withimpl<T> Deref for PyRef<T> { type Target = Py<T>; }, which allows automatic deref coercion of&PyRef<PyBaseException>to&Py<PyBaseException>when passed towrite_exception. The code at lines 55-77 compiles successfully, withwrite_exceptionaccepting&Py<PyBaseException>and receiving the dereferenced exception through standard Rust deref coercion. This pattern is intentional and part of a systematic refactoring.crates/vm/src/builtins/property.rs (3)
312-333: Method parameter type updated correctly.The
format_property_errorsignature now acceptsobj: &PyObjectinstead of&PyObjectRef, consistent with the PR's systematic refactoring. Call sites at lines 61, 111, and 119 pass&objwhereobj: PyObjectRef, relying on automatic deref coercion.
359-363: Closure parameter type aligned with refactoring.The
get_getter_docclosure parameter type changed from&PyObjectRefto&PyObject. The call site at line 370 passes a value fromargs.fget.as_ref(), maintaining type compatibility through deref coercion.
269-274: Type change is sound and correctly implemented.The closure parameter type change from
&PyObjectRefto&PyObjectaligns with the refactoring objectives. Rust's deref coercion automatically converts&PyObjectRefto&PyObjectsince PyObjectRef isRc<PyObject<dyn PyObjectPayload>>and implementsDeref<Target = PyObject>. The closure is invoked with values from.as_ref()onOption<PyObjectRef>at lines 278, 285, and 292, which deref coerces seamlessly. The storage types remain unchanged, and no logic modifications were introduced.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.