Tweak .clone() suggestion to work in more cases

estebank · estebank · commit 03c88aaf218f · 2023-12-04T21:54:32.000Z
When going through auto-deref, the `<T as Clone>` impl sometimes needs to be specified for rustc to actually clone the value and not the reference. ``` error[E0507]: cannot move out of dereference of `S` --> $DIR/needs-clone-through-deref.rs:15:18 | LL | for _ in self.clone().into_iter() {} | ^^^^^^^^^^^^ ----------- value moved due to this method call | | | move occurs because value has type `Vec<usize>`, which does not implement the `Copy` trait | note: `into_iter` takes ownership of the receiver `self`, which moves value --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL help: you can `clone` the value and consume it, but this might not be your desired behavior | LL | for _ in <Vec<usize> as Clone>::clone(&self.clone()).into_iter() {} | ++++++++++++++++++++++++++++++ + ``` CC rust-lang#109429.
diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs
@@ -1135,11 +1135,25 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             )
                             && self.infcx.predicate_must_hold_modulo_regions(&o)
                         {
-                            err.span_suggestion_verbose(
-                                move_span.shrink_to_hi(),
+                            let sugg = if moved_place
+                                .iter_projections()
+                                .any(|(_, elem)| matches!(elem, ProjectionElem::Deref))
+                            {
+                                vec![
+                                    // We use the fully-qualified path because `.clone()` can
+                                    // sometimes choose `<&T as Clone>` instead of `<T as Clone>`
+                                    // when going through auto-deref, so this ensures that doesn't
+                                    // happen, causing suggestions for `.clone().clone()`.
+                                    (move_span.shrink_to_lo(), format!("<{ty} as Clone>::clone(&")),
+                                    (move_span.shrink_to_hi(), ")".to_string()),
+                                ]
+                            } else {
+                                vec![(move_span.shrink_to_hi(), ".clone()".to_string())]
+                            };
+                            err.multipart_suggestion_verbose(
                                 "you can `clone` the value and consume it, but this might not be \
                                  your desired behavior",
-                                ".clone()".to_string(),
+                                sugg,
                                 Applicability::MaybeIncorrect,
                             );
                         }
diff --git a/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr b/tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr
@@ -10,8 +10,8 @@ note: `into_iter` takes ownership of the receiver `self`, which moves value
   --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
 help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
-LL |     let _x = Rc::new(vec![1, 2]).clone().into_iter();
-   |                                 ++++++++
+LL |     let _x = <Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter();
+   |              ++++++++++++++++++++++++++++                   +
 
 error: aborting due to 1 previous error
 
diff --git a/tests/ui/borrowck/clone-span-on-try-operator.fixed b/tests/ui/borrowck/clone-span-on-try-operator.fixed
@@ -7,5 +7,5 @@ impl Foo {
 }
 fn main() {
     let foo = &Foo;
-    (*foo).clone().foo(); //~ ERROR cannot move out
+    <Foo as Clone>::clone(&(*foo)).foo(); //~ ERROR cannot move out
 }
diff --git a/tests/ui/borrowck/clone-span-on-try-operator.stderr b/tests/ui/borrowck/clone-span-on-try-operator.stderr
@@ -13,8 +13,8 @@ LL |     fn foo(self) {}
    |            ^^^^
 help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
-LL |     (*foo).clone().foo();
-   |           ++++++++
+LL |     <Foo as Clone>::clone(&(*foo)).foo();
+   |     +++++++++++++++++++++++      +
 
 error: aborting due to 1 previous error
 
diff --git a/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.fixed b/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.fixed
@@ -9,7 +9,7 @@ fn call<F>(f: F) where F : Fn() {
 fn main() {
     let y = vec![format!("World")];
     call(|| {
-        y.clone().into_iter();
+        <Vec<String> as Clone>::clone(&y).into_iter();
         //~^ ERROR cannot move out of `y`, a captured variable in an `Fn` closure
     });
 }
diff --git a/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr b/tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr
@@ -14,8 +14,8 @@ note: `into_iter` takes ownership of the receiver `self`, which moves `y`
   --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
 help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
-LL |         y.clone().into_iter();
-   |          ++++++++
+LL |         <Vec<String> as Clone>::clone(&y).into_iter();
+   |         +++++++++++++++++++++++++++++++ +
 
 error: aborting due to 1 previous error
 
diff --git a/tests/ui/moves/needs-clone-through-deref.fixed b/tests/ui/moves/needs-clone-through-deref.fixed
@@ -0,0 +1,18 @@
+// run-rustfix
+#![allow(dead_code, noop_method_call)]
+use std::ops::Deref;
+struct S(Vec<usize>);
+impl Deref for S {
+    type Target = Vec<usize>;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl S {
+    fn foo(&self) {
+        // `self.clone()` returns `&S`, not `Vec`
+        for _ in <Vec<usize> as Clone>::clone(&self.clone()).into_iter() {} //~ ERROR cannot move out of dereference of `S`
+    }
+}
+fn main() {}
diff --git a/tests/ui/moves/needs-clone-through-deref.rs b/tests/ui/moves/needs-clone-through-deref.rs
@@ -0,0 +1,18 @@
+// run-rustfix
+#![allow(dead_code, noop_method_call)]
+use std::ops::Deref;
+struct S(Vec<usize>);
+impl Deref for S {
+    type Target = Vec<usize>;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl S {
+    fn foo(&self) {
+        // `self.clone()` returns `&S`, not `Vec`
+        for _ in self.clone().into_iter() {} //~ ERROR cannot move out of dereference of `S`
+    }
+}
+fn main() {}
diff --git a/tests/ui/moves/needs-clone-through-deref.stderr b/tests/ui/moves/needs-clone-through-deref.stderr
@@ -0,0 +1,18 @@
+error[E0507]: cannot move out of dereference of `S`
+  --> $DIR/needs-clone-through-deref.rs:15:18
+   |
+LL |         for _ in self.clone().into_iter() {}
+   |                  ^^^^^^^^^^^^ ----------- value moved due to this method call
+   |                  |
+   |                  move occurs because value has type `Vec<usize>`, which does not implement the `Copy` trait
+   |
+note: `into_iter` takes ownership of the receiver `self`, which moves value
+  --> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL
+help: you can `clone` the value and consume it, but this might not be your desired behavior
+   |
+LL |         for _ in <Vec<usize> as Clone>::clone(&self.clone()).into_iter() {}
+   |                  ++++++++++++++++++++++++++++++            +
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0507`.
diff --git a/tests/ui/moves/suggest-clone.fixed b/tests/ui/moves/suggest-clone.fixed
@@ -7,5 +7,5 @@ impl Foo {
 }
 fn main() {
     let foo = &Foo;
-    foo.clone().foo(); //~ ERROR cannot move out
+    <Foo as Clone>::clone(&foo).foo(); //~ ERROR cannot move out
 }
diff --git a/tests/ui/moves/suggest-clone.stderr b/tests/ui/moves/suggest-clone.stderr
@@ -13,8 +13,8 @@ LL |     fn foo(self) {}
    |            ^^^^
 help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
-LL |     foo.clone().foo();
-   |        ++++++++
+LL |     <Foo as Clone>::clone(&foo).foo();
+   |     +++++++++++++++++++++++   +
 
 error: aborting due to 1 previous error
 
diff --git a/tests/ui/suggestions/option-content-move.stderr b/tests/ui/suggestions/option-content-move.stderr
@@ -11,8 +11,8 @@ note: `Option::<T>::unwrap` takes ownership of the receiver `self`, which moves
   --> $SRC_DIR/core/src/option.rs:LL:COL
 help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
-LL |                 if selection.1.clone().unwrap().contains(selection.0) {
-   |                               ++++++++
+LL |                 if <Option<String> as Clone>::clone(&selection.1).unwrap().contains(selection.0) {
+   |                    ++++++++++++++++++++++++++++++++++           +
 
 error[E0507]: cannot move out of `selection.1` which is behind a shared reference
   --> $DIR/option-content-move.rs:27:20
@@ -27,8 +27,8 @@ note: `Result::<T, E>::unwrap` takes ownership of the receiver `self`, which mov
   --> $SRC_DIR/core/src/result.rs:LL:COL
 help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
-LL |                 if selection.1.clone().unwrap().contains(selection.0) {
-   |                               ++++++++
+LL |                 if <Result<String, String> as Clone>::clone(&selection.1).unwrap().contains(selection.0) {
+   |                    ++++++++++++++++++++++++++++++++++++++++++           +
 
 error: aborting due to 2 previous errors
 

Original file line number	Diff line number	Diff line change
@@ -10,8 +10,8 @@ note: `into_iter` takes ownership of the receiver `self`, which moves value
`10`	`10`	`--> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL`
`11`	`11`	help: you can `clone` the value and consume it, but this might not be your desired behavior
`12`	`12`	`\|`
`13`		`-LL \| let _x = Rc::new(vec![1, 2]).clone().into_iter();`
`14`		`- \| ++++++++`
	`13`	`+LL \| let _x = <Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter();`
	`14`	`+ \| ++++++++++++++++++++++++++++ +`
`15`	`15`
`16`	`16`	`error: aborting due to 1 previous error`
`17`	`17`
Original file line number	Diff line number	Diff line change
`@@ -7,5 +7,5 @@ impl Foo {`
`7`	`7`	`}`
`8`	`8`	`fn main() {`
`9`	`9`	`let foo = &Foo;`
`10`		`- (*foo).clone().foo(); //~ ERROR cannot move out`
	`10`	`+ <Foo as Clone>::clone(&(*foo)).foo(); //~ ERROR cannot move out`
`11`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,8 +13,8 @@ LL \| fn foo(self) {}`
`13`	`13`	`\| ^^^^`
`14`	`14`	help: you can `clone` the value and consume it, but this might not be your desired behavior
`15`	`15`	`\|`
`16`		`-LL \| (*foo).clone().foo();`
`17`		`- \| ++++++++`
	`16`	`+LL \| <Foo as Clone>::clone(&(*foo)).foo();`
	`17`	`+ \| +++++++++++++++++++++++ +`
`18`	`18`
`19`	`19`	`error: aborting due to 1 previous error`
`20`	`20`
Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@ fn call<F>(f: F) where F : Fn() {`
`9`	`9`	`fn main() {`
`10`	`10`	`let y = vec![format!("World")];`
`11`	`11`	`call(\|\| {`
`12`		`- y.clone().into_iter();`
	`12`	`+ <Vec<String> as Clone>::clone(&y).into_iter();`
`13`	`13`	//~^ ERROR cannot move out of `y`, a captured variable in an `Fn` closure
`14`	`14`	`});`
`15`	`15`	`}`
Original file line number	Diff line number	Diff line change
@@ -14,8 +14,8 @@ note: `into_iter` takes ownership of the receiver `self`, which moves `y`
`14`	`14`	`--> $SRC_DIR/core/src/iter/traits/collect.rs:LL:COL`
`15`	`15`	help: you can `clone` the value and consume it, but this might not be your desired behavior
`16`	`16`	`\|`
`17`		`-LL \| y.clone().into_iter();`
`18`		`- \| ++++++++`
	`17`	`+LL \| <Vec<String> as Clone>::clone(&y).into_iter();`
	`18`	`+ \| +++++++++++++++++++++++++++++++ +`
`19`	`19`
`20`	`20`	`error: aborting due to 1 previous error`
`21`	`21`