Remove unnecessary to_{owned,string}() calls#7367
Merged
youknowone merged 1 commit intoRustPython:mainfrom Mar 6, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR systematically refactors error message construction across ~50 files by removing unnecessary Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/ctypes/simple.rs (1)
316-327:⚠️ Potential issue | 🟠 MajorAccept
bytearrayhere or stop advertising it.
from_param()for"c"still rejectsbytearray, even thoughset_primitive()and this error message both treat it as valid. That makes FFI coercion stricter than normal construction for the same ctypes type.Suggested fix
Some("c") => { if let Some(bytes) = value.downcast_ref::<PyBytes>() && bytes.len() == 1 { return create_simple_with_value("c", &value); } + if let Some(bytes) = value.downcast_ref::<PyByteArray>() + && bytes.borrow_buf().len() == 1 + { + return create_simple_with_value("c", &value); + } if let Ok(int_val) = value.try_int(vm) && int_val.as_bigint().to_u8().is_some() { return create_simple_with_value("c", &value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/ctypes/simple.rs` around lines 316 - 327, The from_param branch handling the "c" ctypes case rejects bytearray even though set_primitive and the error message treat it as valid; update the check in the "c" arm (in the from_param function) to accept PyByteArray in addition to PyBytes (or else change the error text to not mention bytearray) so that create_simple_with_value("c", &value) is returned for one-byte bytearray values as well; modify the downcast_ref check to allow &PyBytes or &PyByteArray (or adjust the logic that looks at bytes.len() to handle both types) and keep the integer-path checks and error path unchanged.
🧹 Nitpick comments (3)
crates/vm/src/stdlib/winreg.rs (2)
1109-1124: Consider removing.to_string()here for consistency.These error messages still use
.to_string()on string literals, which could be removed to match the rest of the refactoring in this file.Suggested diff
if required_size == 0 { - return Err(vm.new_os_error("ExpandEnvironmentStringsW failed".to_string())); + return Err(vm.new_os_error("ExpandEnvironmentStringsW failed")); } ... if r == 0 { - return Err(vm.new_os_error("ExpandEnvironmentStringsW failed".to_string())); + return Err(vm.new_os_error("ExpandEnvironmentStringsW failed")); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/winreg.rs` around lines 1109 - 1124, The error handling uses vm.new_os_error("ExpandEnvironmentStringsW failed".to_string()) twice; remove the unnecessary .to_string() and pass string literals (or &str) consistently to vm.new_os_error in both places (the check after required_size == 0 and the check after calling ExpandEnvironmentStringsW) so the calls match the rest of the refactor around vm.new_os_error and ExpandEnvironmentStringsW usage.
160-170: Consider extending this refactoring to remaining.to_owned()calls.These methods still call
HKEY_ERR_MSG.to_owned(). For consistency with the PR's goal of removing unnecessary allocations, consider removing these as well:Suggested diff
fn unary_fail(vm: &VirtualMachine) -> PyResult { - Err(vm.new_type_error(HKEY_ERR_MSG.to_owned())) + Err(vm.new_type_error(HKEY_ERR_MSG)) } fn binary_fail(vm: &VirtualMachine) -> PyResult { - Err(vm.new_type_error(HKEY_ERR_MSG.to_owned())) + Err(vm.new_type_error(HKEY_ERR_MSG)) } fn ternary_fail(vm: &VirtualMachine) -> PyResult { - Err(vm.new_type_error(HKEY_ERR_MSG.to_owned())) + Err(vm.new_type_error(HKEY_ERR_MSG)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/stdlib/winreg.rs` around lines 160 - 170, The three helper functions unary_fail, binary_fail, and ternary_fail still call HKEY_ERR_MSG.to_owned(); update each to pass HKEY_ERR_MSG directly to vm.new_type_error (remove the .to_owned() allocation) so the refactor eliminating unnecessary allocations is applied consistently across these functions.crates/stdlib/src/socket.rs (1)
2211-2220: Extract the shared timeout validation.
settimeout()andsetdefaulttimeout()now duplicate the same NaN/range checks and exact exception text. A small helper would keep those Python-visible errors from drifting apart the next time one path changes.As per coding guidelines "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code".
Also applies to: 3388-3397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/socket.rs` around lines 2211 - 2220, The NaN/range checks for timeout are duplicated in settimeout and setdefaulttimeout (and also at the block around lines 3388-3397); extract a small helper function like validate_timeout(vm: &VirtualMachine, timeout_obj: impl IntoFloat?) -> Result<Option<f64>, ValueError> (or an appropriately typed fn) that converts the input to f64, checks is_nan, <0.0, and is_finite, and returns Some(f) or an Err(vm.new_value_error(...)) with the exact existing messages; then replace the duplicated blocks in settimeout, setdefaulttimeout, and the other occurrence to call this helper, preserving the same error texts and behavior, and re-use vm.new_value_error for error creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/stdlib/src/overlapped.rs`:
- Around line 1858-1859: In BindLocal, the branch that validates the address
family currently raises vm.new_value_error("expected tuple of length 2 or 4")
which is misleading for unsupported families; update that error to report an
unsupported-family error (e.g., vm.new_value_error with a message like
"unsupported address family: {family}" or similar that includes the actual
family value) so callers get the correct exception when the family is invalid
rather than a tuple-length message; locate the check in the BindLocal
implementation and replace the error string returned by vm.new_value_error
accordingly.
In `@crates/vm/src/stdlib/ctypes/array.rs`:
- Around line 801-803: The error message passed to vm.new_type_error contains a
literal "{}" placeholder; change it to include the actual offending type by
building a formatted string with format!() and passing that to
vm.new_type_error. Locate the return Err(vm.new_type_error("bytes or integer
address expected instead of {}")) call and replace it with something like
vm.new_type_error(&format!("bytes or integer address expected instead of {}",
actual_type)), where actual_type is obtained from the runtime value (e.g.,
value.type_name() or value.class().name() / similar accessor available in this
context) so the real type is shown.
In `@crates/vm/src/stdlib/winsound.rs`:
- Line 90: The PR changed several Python-visible error string literals in
winsound.rs; restore the exact original literal texts (do not alter wording,
punctuation, or capitalization) and only remove ownership conversion calls like
.to_owned() or .to_string(); update occurrences that call
vm.new_runtime_error(...) in winsound.rs (specifically the places currently at
the return Err(vm.new_runtime_error(...)) lines and the other occurrences
referenced: the calls around the error messages at lines noted in the review) so
the message strings match the originals verbatim while eliminating only the
unnecessary conversion method calls.
---
Outside diff comments:
In `@crates/vm/src/stdlib/ctypes/simple.rs`:
- Around line 316-327: The from_param branch handling the "c" ctypes case
rejects bytearray even though set_primitive and the error message treat it as
valid; update the check in the "c" arm (in the from_param function) to accept
PyByteArray in addition to PyBytes (or else change the error text to not mention
bytearray) so that create_simple_with_value("c", &value) is returned for
one-byte bytearray values as well; modify the downcast_ref check to allow
&PyBytes or &PyByteArray (or adjust the logic that looks at bytes.len() to
handle both types) and keep the integer-path checks and error path unchanged.
---
Nitpick comments:
In `@crates/stdlib/src/socket.rs`:
- Around line 2211-2220: The NaN/range checks for timeout are duplicated in
settimeout and setdefaulttimeout (and also at the block around lines 3388-3397);
extract a small helper function like validate_timeout(vm: &VirtualMachine,
timeout_obj: impl IntoFloat?) -> Result<Option<f64>, ValueError> (or an
appropriately typed fn) that converts the input to f64, checks is_nan, <0.0, and
is_finite, and returns Some(f) or an Err(vm.new_value_error(...)) with the exact
existing messages; then replace the duplicated blocks in settimeout,
setdefaulttimeout, and the other occurrence to call this helper, preserving the
same error texts and behavior, and re-use vm.new_value_error for error creation.
In `@crates/vm/src/stdlib/winreg.rs`:
- Around line 1109-1124: The error handling uses
vm.new_os_error("ExpandEnvironmentStringsW failed".to_string()) twice; remove
the unnecessary .to_string() and pass string literals (or &str) consistently to
vm.new_os_error in both places (the check after required_size == 0 and the check
after calling ExpandEnvironmentStringsW) so the calls match the rest of the
refactor around vm.new_os_error and ExpandEnvironmentStringsW usage.
- Around line 160-170: The three helper functions unary_fail, binary_fail, and
ternary_fail still call HKEY_ERR_MSG.to_owned(); update each to pass
HKEY_ERR_MSG directly to vm.new_type_error (remove the .to_owned() allocation)
so the refactor eliminating unnecessary allocations is applied consistently
across these functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: aa30f9ce-6cd0-46af-b575-c22e5c79ceaf
📒 Files selected for processing (71)
crates/stdlib/src/_asyncio.rscrates/stdlib/src/_remote_debugging.rscrates/stdlib/src/_sqlite3.rscrates/stdlib/src/faulthandler.rscrates/stdlib/src/hashlib.rscrates/stdlib/src/math.rscrates/stdlib/src/mmap.rscrates/stdlib/src/multiprocessing.rscrates/stdlib/src/openssl.rscrates/stdlib/src/overlapped.rscrates/stdlib/src/socket.rscrates/stdlib/src/ssl.rscrates/vm/src/builtins/bytearray.rscrates/vm/src/builtins/code.rscrates/vm/src/builtins/complex.rscrates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/frame.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/genericalias.rscrates/vm/src/builtins/memory.rscrates/vm/src/builtins/module.rscrates/vm/src/builtins/object.rscrates/vm/src/builtins/property.rscrates/vm/src/builtins/singletons.rscrates/vm/src/builtins/traceback.rscrates/vm/src/builtins/type.rscrates/vm/src/builtins/weakref.rscrates/vm/src/bytes_inner.rscrates/vm/src/convert/try_from.rscrates/vm/src/coroutine.rscrates/vm/src/exception_group.rscrates/vm/src/exceptions.rscrates/vm/src/frame.rscrates/vm/src/import.rscrates/vm/src/stdlib/_abc.rscrates/vm/src/stdlib/_winapi.rscrates/vm/src/stdlib/_wmi.rscrates/vm/src/stdlib/ast.rscrates/vm/src/stdlib/ast/expression.rscrates/vm/src/stdlib/ast/pattern.rscrates/vm/src/stdlib/ast/python.rscrates/vm/src/stdlib/ast/statement.rscrates/vm/src/stdlib/ast/string.rscrates/vm/src/stdlib/ast/validate.rscrates/vm/src/stdlib/builtins.rscrates/vm/src/stdlib/codecs.rscrates/vm/src/stdlib/ctypes.rscrates/vm/src/stdlib/ctypes/array.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/stdlib/ctypes/function.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/stdlib/ctypes/simple.rscrates/vm/src/stdlib/ctypes/structure.rscrates/vm/src/stdlib/ctypes/union.rscrates/vm/src/stdlib/functools.rscrates/vm/src/stdlib/gc.rscrates/vm/src/stdlib/io.rscrates/vm/src/stdlib/nt.rscrates/vm/src/stdlib/os.rscrates/vm/src/stdlib/posix.rscrates/vm/src/stdlib/signal.rscrates/vm/src/stdlib/sys.rscrates/vm/src/stdlib/thread.rscrates/vm/src/stdlib/time.rscrates/vm/src/stdlib/typing.rscrates/vm/src/stdlib/warnings.rscrates/vm/src/stdlib/winreg.rscrates/vm/src/stdlib/winsound.rscrates/vm/src/types/structseq.rscrates/vm/src/vm/python_run.rscrates/vm/src/warn.rs
Comment on lines
1858
to
+1859
| } else { | ||
| return Err(vm.new_value_error("expected tuple of length 2 or 4".to_owned())); | ||
| return Err(vm.new_value_error("expected tuple of length 2 or 4")); |
Contributor
There was a problem hiding this comment.
Fix the unsupported-family error in BindLocal.
This branch validates family, but the message still says "expected tuple of length 2 or 4". Unsupported families will surface a misleading exception.
🩹 Suggested fix
- return Err(vm.new_value_error("expected tuple of length 2 or 4"));
+ return Err(vm.new_value_error("family must be AF_INET or AF_INET6"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/stdlib/src/overlapped.rs` around lines 1858 - 1859, In BindLocal, the
branch that validates the address family currently raises
vm.new_value_error("expected tuple of length 2 or 4") which is misleading for
unsupported families; update that error to report an unsupported-family error
(e.g., vm.new_value_error with a message like "unsupported address family:
{family}" or similar that includes the actual family value) so callers get the
correct exception when the family is invalid rather than a tuple-length message;
locate the check in the BindLocal implementation and replace the error string
returned by vm.new_value_error accordingly.
Comment on lines
+801
to
+803
| return Err( | ||
| vm.new_type_error("bytes or integer address expected instead of {}") | ||
| ); |
Contributor
There was a problem hiding this comment.
Malformed error message contains literal {}.
The error string has an unformatted {} placeholder that will be displayed literally to users. It should either use format!() to include the actual type, or remove the placeholder entirely.
Proposed fix
- return Err(
- vm.new_type_error("bytes or integer address expected instead of {}")
- );
+ return Err(vm.new_type_error(format!(
+ "bytes or integer address expected instead of {}",
+ value.class().name()
+ )));📝 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.
Suggested change
| return Err( | |
| vm.new_type_error("bytes or integer address expected instead of {}") | |
| ); | |
| return Err(vm.new_type_error(format!( | |
| "bytes or integer address expected instead of {}", | |
| value.class().name() | |
| ))); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/ctypes/array.rs` around lines 801 - 803, The error
message passed to vm.new_type_error contains a literal "{}" placeholder; change
it to include the actual offending type by building a formatted string with
format!() and passing that to vm.new_type_error. Locate the return
Err(vm.new_type_error("bytes or integer address expected instead of {}")) call
and replace it with something like vm.new_type_error(&format!("bytes or integer
address expected instead of {}", actual_type)), where actual_type is obtained
from the runtime value (e.g., value.type_name() or value.class().name() /
similar accessor available in this context) so the real type is shown.
Contributor
There was a problem hiding this comment.
@ShaharNaveh, thank you! 🐇 The unformatted {} in the error message would have been confusing for users. The fix with format!("bytes or integer address expected instead of {}", value.class().name()) will give them the actual type name instead of a literal {}.
🧠 Learnings used
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.
Learnt from: youknowone
Repo: RustPython/RustPython PR: 7333
File: crates/vm/src/datastack.rs:77-79
Timestamp: 2026-03-04T11:07:41.882Z
Learning: In RustPython's `crates/vm/src/datastack.rs`, the fast-path alignment formula `(size + ALIGN - 1) & !(ALIGN - 1)` in `DataStack::push()` and `has_space()` intentionally omits checked arithmetic because the `size` argument is already validated upstream: `LocalsPlus::new_on_datastack()` in `crates/vm/src/frame.rs` computes `byte_size` via `checked_add` + `checked_mul` with `expect()` panics before calling `vm.datastack_push(byte_size)`. Overflow protection is enforced at the caller level, not inside the allocator fast path.
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7181
File: crates/stdlib/src/csv.rs:20-20
Timestamp: 2026-02-17T07:54:36.200Z
Learning: In RustPython, ensure that modules using Python sequence-like types bring in extension methods by importing the trait (for example, 'use rustpython_vm::sliceable::SliceableSequenceOp;'). Even if the trait name isn't directly referenced in code, its methods (such as len()) become in-scope on types like PyStr. Add this import in modules that rely on those extension methods to avoid compilation errors.
| let ok = unsafe { super::win32::PlaySoundW(core::ptr::null(), 0, flags) }; | ||
| if ok == 0 { | ||
| return Err(vm.new_runtime_error("Failed to play sound".to_owned())); | ||
| return Err(vm.new_runtime_error("Failed to play sound")); |
Contributor
There was a problem hiding this comment.
Keep the exception text unchanged in this cleanup.
Lines 90, 97, 105, 139, 156, 162, 178, and 183 tweak Python-visible error messages instead of only removing ownership conversions. That makes this PR behavior-changing and can break compatibility tests or callers that assert on exact text. Please preserve the existing literals verbatim and only drop the .to_owned() / .to_string() calls.
Also applies to: 97-97, 105-105, 139-139, 156-156, 162-162, 178-178, 183-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/stdlib/winsound.rs` at line 90, The PR changed several
Python-visible error string literals in winsound.rs; restore the exact original
literal texts (do not alter wording, punctuation, or capitalization) and only
remove ownership conversion calls like .to_owned() or .to_string(); update
occurrences that call vm.new_runtime_error(...) in winsound.rs (specifically the
places currently at the return Err(vm.new_runtime_error(...)) lines and the
other occurrences referenced: the calls around the error messages at lines noted
in the review) so the message strings match the originals verbatim while
eliminating only the unnecessary conversion method calls.
youknowone
pushed a commit
that referenced
this pull request
Mar 7, 2026
* Remove unnecessary `to_{owned,string}()` calls (#7367)
* Fix error message in ctype
youknowone
pushed a commit
to youknowone/RustPython
that referenced
this pull request
Mar 8, 2026
youknowone
pushed a commit
to youknowone/RustPython
that referenced
this pull request
Mar 8, 2026
* Remove unnecessary `to_{owned,string}()` calls (RustPython#7367)
* Fix error message in ctype
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Used the same code from #5836
Summary by CodeRabbit
Note: This release contains only internal code improvements with no visible changes to user-facing functionality, features, or behavior.