Skip to content

Commit

Permalink
Auto merge of #10913 - y21:issue10033, r=Manishearth,Centri3
Browse files Browse the repository at this point in the history
[`unnecessary_to_owned`]: check that the adjusted type matches target

Fixes #10033.

Before this change, the lint would assume that removing the `.to_string()` in `f(&x.to_string())` would be ok if x is of some type that implements `Deref<Target = str>` and `f` takes a `&str`.
This turns out to not actually be ok if the `to_string` call is some method that exists on `x` directly, which happens if it implements `Display`/`ToString` itself.

changelog: [`unnecessary_to_owned`]: only lint if the adjusted receiver type actually matches
  • Loading branch information
bors committed Jun 9, 2023
2 parents 384cf37 + dd08494 commit 05de787
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 0 deletions.
5 changes: 5 additions & 0 deletions clippy_lints/src/methods/unnecessary_to_owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ fn check_addr_of_expr(
if let Some(deref_trait_id) = cx.tcx.get_diagnostic_item(sym::Deref);
if implements_trait(cx, receiver_ty, deref_trait_id, &[]);
if cx.get_associated_type(receiver_ty, deref_trait_id, "Target") == Some(target_ty);
// Make sure that it's actually calling the right `.to_string()`, (#10033)
// *or* this is a `Cow::into_owned()` call (which would be the wrong into_owned receiver (str != Cow)
// but that's ok for Cow::into_owned specifically)
if cx.typeck_results().expr_ty_adjusted(receiver).peel_refs() == target_ty
|| is_cow_into_owned(cx, method_name, method_def_id);
then {
if n_receiver_refs > 0 {
span_lint_and_sugg(
Expand Down
33 changes: 33 additions & 0 deletions tests/ui/unnecessary_to_owned.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -474,3 +474,36 @@ mod issue_10021 {
Ok(())
}
}

mod issue_10033 {
#![allow(dead_code)]
use std::{fmt::Display, ops::Deref};

fn _main() {
let f = Foo;

// Not actually unnecessary - this calls `Foo`'s `Display` impl, not `str`'s (even though `Foo` does
// deref to `str`)
foo(&f.to_string());
}

fn foo(s: &str) {
println!("{}", s);
}

struct Foo;

impl Deref for Foo {
type Target = str;

fn deref(&self) -> &Self::Target {
"str"
}
}

impl Display for Foo {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "Foo")
}
}
}
33 changes: 33 additions & 0 deletions tests/ui/unnecessary_to_owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,3 +474,36 @@ mod issue_10021 {
Ok(())
}
}

mod issue_10033 {
#![allow(dead_code)]
use std::{fmt::Display, ops::Deref};

fn _main() {
let f = Foo;

// Not actually unnecessary - this calls `Foo`'s `Display` impl, not `str`'s (even though `Foo` does
// deref to `str`)
foo(&f.to_string());
}

fn foo(s: &str) {
println!("{}", s);
}

struct Foo;

impl Deref for Foo {
type Target = str;

fn deref(&self) -> &Self::Target {
"str"
}
}

impl Display for Foo {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "Foo")
}
}
}

0 comments on commit 05de787

Please sign in to comment.