-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Add suggestion to .to_owned()
used on Cow
when borrowing
#144925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
r? @davidtwco rustbot has assigned @davidtwco. Use |
This PR modifies |
&& adtdef.did() == cow | ||
{ | ||
if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(return_span) { | ||
if let Some(pos) = snippet.rfind(".to_owned") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not happy about detecting the use of .to_owned
by way of string comparison, but since this check is in the MIR I don't know how to do it better. Constructive criticism would be greatly appreciated!
a89614d
to
3403c4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some test adjustments
We have a bit of a gap in our test coverage, so I'm trying to keep a close eye on new tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd personally move this test to suggestions/
or borrowck/
and about name... I think name is fine but we can do better right? So i'd suggest something like cow-into-owned-suggestion.rs
@@ -0,0 +1,7 @@ | |||
// issue #144792 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use full links and special comment //! Regression test for: https://github.com/rust-lang/rust/issues/144792
just because it will be easy to follow with just copy past link
// issue #144792 | ||
|
||
fn main() { | ||
_ = std::env::var_os("RUST_LOG").map_or("warn".into(), |x| x.to_string_lossy().to_owned()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be fine but I don't like it's env dependable, can we have more interesting test like below and with more coverage
// More robust - doesn't depend on environment
fn test_cow_suggestion() -> String {
let os_string = std::ffi::OsString::from("test");
os_string.to_string_lossy().to_owned()
}
// Test multiple Cow scenarios
fn test_cow_from_str() -> String {
let s = "hello";
let cow = std::borrow::Cow::from(s);
cow.to_owned() // Should suggest into_owned()
}
// Test with different Cow types
fn test_cow_bytes() -> Vec<u8> {
let bytes = b"hello";
let cow = std::borrow::Cow::from(&bytes[..]);
cow.to_owned() // Should suggest into_owned()
}
Fixes #144792
Adds a suggestion when using
.to_borrow()
onCow
where.into_borrow()
should be used, and makes a test that explicitly checks if it is shown.