Add suggestion to .to_owned() used on Cow when borrowing #144925
Conversation
| r? @davidtwco rustbot has assigned @davidtwco. Use |
rustbot added S-waiting-on-review | 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.
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!
There was a problem hiding this comment.
There are other diagnostics code here that does similar things, so happy to call it pre-existing until there's a better solution for this part of the compiler
Periodic1911 force-pushed the 144792-cow-diag branch from a89614d to 3403c4f Compare 
There was a problem hiding this comment.
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.
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.
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() }
| } | ||
| | ||
| if let Some(cow) = tcx.get_diagnostic_item(sym::Cow) | ||
| && let ty::Adt(adtdef, _) = return_ty.kind() |
There was a problem hiding this comment.
| && let ty::Adt(adtdef, _) = return_ty.kind() | |
| && let ty::Adt(adt_def, _) = return_ty.kind() |
nit: more common name for variables of AdtDef
| ); | ||
| } | ||
| | ||
| if let Some(cow) = tcx.get_diagnostic_item(sym::Cow) |
There was a problem hiding this comment.
| if let Some(cow) = tcx.get_diagnostic_item(sym::Cow) | |
| if let Some(cow_did) = tcx.get_diagnostic_item(sym::Cow) |
nit: likewise, more typical naming convention
| && 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.
There are other diagnostics code here that does similar things, so happy to call it pre-existing until there's a better solution for this part of the compiler
| // issue #144792 | ||
| | ||
| fn main() { | ||
| _ = std::env::var_os("RUST_LOG").map_or("warn".into(), |x| x.to_string_lossy().to_owned()); |
rustbot added S-waiting-on-author 
| @Periodic1911 any updates on this? thanks |
5 participants
Fixes #144792
Adds a suggestion when using
.to_borrow()onCowwhere.into_borrow()should be used, and makes a test that explicitly checks if it is shown.