Auto merge of #12091 - samueltardieu:issue-12068, r=Alexendoo

Add .as_ref() to suggestion to remove .to_string() The case of `.to_owned().split(…)` is treated specially in the `unnecessary_to_owned` lint. Test cases check that it works both for slices and for strings, but they missed a corner case: `x.to_string().split(…)` when `x` implements `AsRef<str>` but not `Deref<Target = str>`. In this case, it is wrong to suggest to remove `.to_string()` without adding `.as_ref()` instead. Fix #12068 changelog: [`unnecessary_to_owned`]: suggest replacing `.to_string()` by `.as_ref()`
rust-lang · Jan 5, 2024 · 2d6c238 · 2d6c238
2 parents ee8bfb7 + e758413
commit 2d6c238
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 14 deletions.
diff --git a/clippy_lints/src/methods/unnecessary_to_owned.rs b/clippy_lints/src/methods/unnecessary_to_owned.rs
@@ -3,13 +3,13 @@ use super::unnecessary_iter_cloned::{self, is_into_iter};
 use clippy_config::msrvs::{self, Msrv};
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_opt;
-use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy, peel_mid_ty_refs};
+use clippy_utils::ty::{get_iterator_item_ty, implements_trait, is_copy, is_type_lang_item, peel_mid_ty_refs};
 use clippy_utils::visitors::find_all_ret_expressions;
 use clippy_utils::{fn_def_id, get_parent_expr, is_diag_item_method, is_diag_trait_item, return_ty};
 use rustc_errors::Applicability;
 use rustc_hir::def::{DefKind, Res};
 use rustc_hir::def_id::DefId;
-use rustc_hir::{BorrowKind, Expr, ExprKind, ItemKind, Node};
+use rustc_hir::{BorrowKind, Expr, ExprKind, ItemKind, LangItem, Node};
 use rustc_hir_typeck::{FnCtxt, Inherited};
 use rustc_infer::infer::TyCtxtInferExt;
 use rustc_lint::LateContext;
@@ -246,6 +246,19 @@ fn check_split_call_arg(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: Symb
         && let Some(receiver_snippet) = snippet_opt(cx, receiver.span)
         && let Some(arg_snippet) = snippet_opt(cx, argument_expr.span)
     {
+        // We may end-up here because of an expression like `x.to_string().split(…)` where the type of `x`
+        // implements `AsRef<str>` but does not implement `Deref<Target = str>`. In this case, we have to
+        // add `.as_ref()` to the suggestion.
+        let as_ref = if is_type_lang_item(cx, cx.typeck_results().expr_ty(expr), LangItem::String)
+            && let Some(deref_trait_id) = cx.tcx.get_diagnostic_item(sym::Deref)
+            && cx.get_associated_type(cx.typeck_results().expr_ty(receiver), deref_trait_id, "Target")
+                != Some(cx.tcx.types.str_)
+        {
+            ".as_ref()"
+        } else {
+            ""
+        };
+
         // The next suggestion may be incorrect because the removal of the `to_owned`-like
         // function could cause the iterator to hold a reference to a resource that is used
         // mutably. See /~https://github.com/rust-lang/rust-clippy/issues/8148.
@@ -255,7 +268,7 @@ fn check_split_call_arg(cx: &LateContext<'_>, expr: &Expr<'_>, method_name: Symb
             parent.span,
             &format!("unnecessary use of `{method_name}`"),
             "use",
-            format!("{receiver_snippet}.split({arg_snippet})"),
+            format!("{receiver_snippet}{as_ref}.split({arg_snippet})"),
             Applicability::MaybeIncorrect,
         );
         return true;

diff --git a/tests/ui/unnecessary_to_owned_on_split.fixed b/tests/ui/unnecessary_to_owned_on_split.fixed
@@ -1,4 +1,18 @@
-#[allow(clippy::single_char_pattern)]
+#![allow(clippy::single_char_pattern)]
+
+struct Issue12068;
+
+impl AsRef<str> for Issue12068 {
+    fn as_ref(&self) -> &str {
+        ""
+    }
+}
+
+impl ToString for Issue12068 {
+    fn to_string(&self) -> String {
+        String::new()
+    }
+}
 
 fn main() {
     let _ = "a".split('a').next().unwrap();
@@ -9,6 +23,8 @@ fn main() {
     //~^ ERROR: unnecessary use of `to_owned`
     let _ = "a".split("a").next().unwrap();
     //~^ ERROR: unnecessary use of `to_owned`
+    let _ = Issue12068.as_ref().split('a').next().unwrap();
+    //~^ ERROR: unnecessary use of `to_string`
 
     let _ = [1].split(|x| *x == 2).next().unwrap();
     //~^ ERROR: unnecessary use of `to_vec`

diff --git a/tests/ui/unnecessary_to_owned_on_split.rs b/tests/ui/unnecessary_to_owned_on_split.rs
@@ -1,4 +1,18 @@
-#[allow(clippy::single_char_pattern)]
+#![allow(clippy::single_char_pattern)]
+
+struct Issue12068;
+
+impl AsRef<str> for Issue12068 {
+    fn as_ref(&self) -> &str {
+        ""
+    }
+}
+
+impl ToString for Issue12068 {
+    fn to_string(&self) -> String {
+        String::new()
+    }
+}
 
 fn main() {
     let _ = "a".to_string().split('a').next().unwrap();
@@ -9,6 +23,8 @@ fn main() {
     //~^ ERROR: unnecessary use of `to_owned`
     let _ = "a".to_owned().split("a").next().unwrap();
     //~^ ERROR: unnecessary use of `to_owned`
+    let _ = Issue12068.to_string().split('a').next().unwrap();
+    //~^ ERROR: unnecessary use of `to_string`
 
     let _ = [1].to_vec().split(|x| *x == 2).next().unwrap();
     //~^ ERROR: unnecessary use of `to_vec`

diff --git a/tests/ui/unnecessary_to_owned_on_split.stderr b/tests/ui/unnecessary_to_owned_on_split.stderr
@@ -1,5 +1,5 @@
 error: unnecessary use of `to_string`
-  --> $DIR/unnecessary_to_owned_on_split.rs:4:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:18:13
    |
 LL |     let _ = "a".to_string().split('a').next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"a".split('a')`
@@ -8,46 +8,52 @@ LL |     let _ = "a".to_string().split('a').next().unwrap();
    = help: to override `-D warnings` add `#[allow(clippy::unnecessary_to_owned)]`
 
 error: unnecessary use of `to_string`
-  --> $DIR/unnecessary_to_owned_on_split.rs:6:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:20:13
    |
 LL |     let _ = "a".to_string().split("a").next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"a".split("a")`
 
 error: unnecessary use of `to_owned`
-  --> $DIR/unnecessary_to_owned_on_split.rs:8:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:22:13
    |
 LL |     let _ = "a".to_owned().split('a').next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"a".split('a')`
 
 error: unnecessary use of `to_owned`
-  --> $DIR/unnecessary_to_owned_on_split.rs:10:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:24:13
    |
 LL |     let _ = "a".to_owned().split("a").next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"a".split("a")`
 
+error: unnecessary use of `to_string`
+  --> $DIR/unnecessary_to_owned_on_split.rs:26:13
+   |
+LL |     let _ = Issue12068.to_string().split('a').next().unwrap();
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `Issue12068.as_ref().split('a')`
+
 error: unnecessary use of `to_vec`
-  --> $DIR/unnecessary_to_owned_on_split.rs:13:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:29:13
    |
 LL |     let _ = [1].to_vec().split(|x| *x == 2).next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[1].split(|x| *x == 2)`
 
 error: unnecessary use of `to_vec`
-  --> $DIR/unnecessary_to_owned_on_split.rs:15:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:31:13
    |
 LL |     let _ = [1].to_vec().split(|x| *x == 2).next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[1].split(|x| *x == 2)`
 
 error: unnecessary use of `to_owned`
-  --> $DIR/unnecessary_to_owned_on_split.rs:17:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:33:13
    |
 LL |     let _ = [1].to_owned().split(|x| *x == 2).next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[1].split(|x| *x == 2)`
 
 error: unnecessary use of `to_owned`
-  --> $DIR/unnecessary_to_owned_on_split.rs:19:13
+  --> $DIR/unnecessary_to_owned_on_split.rs:35:13
    |
 LL |     let _ = [1].to_owned().split(|x| *x == 2).next().unwrap();
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `[1].split(|x| *x == 2)`
 
-error: aborting due to 8 previous errors
+error: aborting due to 9 previous errors