Remove useless &PyRef patterns by youknowone · Pull Request #6455 · RustPython/RustPython

• Create PR with unit tests
• Post copyable unit tests in a comment

Reviewing files that changed from the base of the PR and between da6b45c and 905c706.

• crates/stdlib/src/openssl.rs (2 hunks)
• crates/stdlib/src/scproxy.rs (2 hunks)
• crates/stdlib/src/ssl/compat.rs (3 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/union.rs (1 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/import.rs (2 hunks)
• crates/vm/src/scope.rs (1 hunks)
• crates/vm/src/suggestion.rs (3 hunks)
• crates/vm/src/vm/mod.rs (3 hunks)
• crates/wasm/src/convert.rs (2 hunks)
• crates/wasm/src/js_module.rs (1 hunks)

• crates/stdlib/src/openssl.rs
• crates/vm/src/vm/mod.rs
• crates/wasm/src/convert.rs
• crates/vm/src/builtins/genericalias.rs
• crates/vm/src/suggestion.rs
• crates/stdlib/src/scproxy.rs
• crates/vm/src/scope.rs

• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Check the WASM package and demo
• GitHub Check: Check Rust code with clippy
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run tests under miri

crates/stdlib/src/ssl/compat.rs (3)

26-29: LGTM! Import updates align with the signature changes below.

The addition of PyBaseException and Py imports properly supports the type signature changes from &PyBaseExceptionRef to &Py<PyBaseException>.

---

987-989: LGTM! Correct simplification of reference type.

Changing from &PyBaseExceptionRef (i.e., &PyRef<PyBaseException>) to &Py<PyBaseException> removes unnecessary indirection since PyRef<T> derefs to Py<T>. The function body using fast_isinstance remains valid as it's available on Py<T>.

---

1537-1556: LGTM! Consistent with the pattern applied throughout this PR.

The signature change to &Py<PyBaseException> is correct. All method calls (fast_isinstance, as_object, get_attr, try_int, try_to_primitive) are properly available on Py<PyBaseException>.

crates/vm/src/import.rs (2)

3-10: LGTM! Import reorganization is clean and correct.

The imports are properly updated to include Py and PyBaseException from exceptions::types, supporting the signature change in remove_importlib_frames.

---

208-218: LGTM! Signature change is correct and body remains valid.

The change from &PyBaseExceptionRef to &Py<PyBaseException> properly simplifies the reference type. All method calls (fast_isinstance, __traceback__, set_traceback_typed) work correctly on Py<PyBaseException>.

crates/vm/src/exceptions.rs (6)

79-86: LGTM! Entry point for exception writing correctly updated.

The signature change to &Py<PyBaseException> is appropriate and cascades consistently to the internal helper methods.

---

88-129: LGTM! Recursive exception writing with proper type handling.

The parameter change is correct. The get_id() and recursive calls work properly with &Py<PyBaseException>. The &cause_or_context reference on line 121 correctly passes a reference to the inner Py<PyBaseException>.

---

132-170: LGTM! Inner exception writing logic remains valid.

All method accesses (.traceback.read(), .args(), .class(), .as_object()) are properly available on Py<PyBaseException>.

---

177-183: LGTM! SyntaxError formatting with updated types.

Both exc: &Py<PyBaseException> and exc_type: &Py<PyType> parameter types are correct. The &Py<PyType> change aligns with the broader pattern of this PR.

---

370-385: LGTM! Traceback entry writing correctly uses &Py<PyTraceback>.

The change from &PyTracebackRef to &Py<PyTraceback> is consistent with the PR pattern. Field accesses (.frame, .lineno) work correctly on the Py<PyTraceback> type.

---

1053-1064: LGTM! SerializeException struct and constructor updated consistently.

The struct field and new() parameter now use &'s Py<PyBaseException> instead of &'s PyBaseExceptionRef. This removes unnecessary indirection while maintaining the same functionality. The SerializeExceptionOwned variant (line 1067-1070) correctly retains PyBaseExceptionRef since it owns the exception rather than borrowing it.

crates/wasm/src/js_module.rs (1)

615-615: LGTM! Type signature change aligns with PR objective.

The type passing for vm.ctx.exceptions.exception_type is correct—it's a &'static Py<PyType> that satisfies the &Py<PyType> parameter of new_simple_heap. The Rust compiler enforces type correctness at compile time, and this change maintains type safety.

crates/vm/src/coroutine.rs (2)

2-2: LGTM! Imports correctly support the type system change.

The addition of Py and PyBaseException imports aligns with the shift to owned/wrapper types and supports the updated is_gen_exit signature.

Also applies to: 5-5

---

211-213: Type mismatch: function signature incompatible with current call site.

The signature change to &Py<PyBaseException> introduces a compilation error. At line 168, the error type e from PyResult is PyBaseExceptionRef (alias for PyRef<PyBaseException>), not Py<PyBaseException>. These are incompatible types. Either update PyResult to use Py<PyBaseException> as its error type, or revert the function signature to accept &PyBaseExceptionRef.

⛔ Skipped due to learnings

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.

crates/vm/src/exception_group.rs (3)

355-429: LGTM! Clean type refactoring.

The signature changes from &PyObjectRef/&PyRef<T> to &PyObject/&Py<T> are consistent with the PR's goal. The shift from clone() to to_owned() at lines 412 and 424 correctly expresses the intent of converting borrowed references to owned references. All downcast_ref usages remain appropriate for borrowed types.

---

431-469: LGTM! Signature change preserves all functionality.

The parameter type change from &PyRef<PyBaseException> to &Py<PyBaseException> aligns with the PR objective. All attribute access patterns (__traceback__(), __context__(), __cause__(), as_object()) work correctly with the new type, and the metadata propagation logic remains intact.

---

355-469: Run cargo clippy to verify no lints are introduced.

Per the coding guidelines, clippy must be run on all Rust changes before completion. While the code appears structurally sound with proper error handling and memory management, the clippy check must be executed to ensure no warnings are introduced by these helper functions.

crates/vm/src/builtins/function/jit.rs (2)

3-5: LGTM! Import addition aligns with signature refactor.

The addition of PyDict to the imports is necessary for the updated function signature on line 47. Keeping PyDictRef ensures compatibility with existing call sites.

---

47-47: Type coercion from &PyDictRef to &Py<PyDict> is correct. The signature change is valid because PyDictRef is a type alias for PyRef<PyDict>, and PyRef<T> implements Deref with Target = Py<T>. This allows &dict (where dict is PyDictRef) to automatically coerce to &Py<PyDict> at call sites (lines 98, 102, 121) via Rust's deref coercion mechanism. The pattern is sound and type-safe.

crates/vm/src/builtins/dict.rs (2)

756-756: LGTM! Trait signature updated consistently.

The return type change from &PyDictRef to &Py<PyDict> in the DictView trait aligns with the PR's objective to shift from borrowed refs to owned/wrapper types. The implementation in the dict_view! macro (line 788) correctly reflects this change.

---

1145-1145: No action needed. The type compatibility is correct.

The change from .clone() to .to_owned() is valid. When .to_owned() is called on &Py<PyDict>, it returns PyRef<PyDict> (alias PyDictRef) via the impl<T> ToOwned for Py<T> trait, which directly matches the existing impl From<PyDictRef> for PyMappingProxy. No type mismatch exists.

crates/vm/src/builtins/union.rs (1)

45-47: Review comment is based on incorrect assumptions and should be revised.

The method signature does not involve a const removal—no const qualifier appears on this method. The return type works through automatic Deref coercion: PyRef<T> implements Deref with target Py<T>, so &self.args (which is &PyRef<PyTuple>) automatically coerces to &Py<PyTuple>. This is a transparent implementation mechanism, not a breaking API change requiring downstream verification. The call sites in the codebase are already compatible.

Likely an incorrect or invalid review comment.

crates/vm/src/codecs.rs (1)

55-57: Method as_tuple() has no usages in the codebase; verification request cannot be completed.

The method returns &Py<PyTuple> and is technically correct due to Deref coercion from PyRef<PyTuple>, but it is dead code with zero call sites. The return type annotation returns a reference to an owned type, not an owned type itself, which conflicts with the stated goal of standardizing on owned Py<T> types. This method should either be removed if unused or its purpose clarified if it's part of a planned public API.

Likely an incorrect or invalid review comment.