Mirrors/rust
1
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2026-04-26 13:01:27 +03:00
Code Issues Packages Projects Releases Wiki Activity

Rollup merge of #154197 - yuk1ty:fix-redundant-clone-error2, r=adwinwhite

Browse Source 
Avoid redundant clone suggestions in borrowck diagnostics

Fixes rust-lang/rust#153886

Removed redundant `.clone()` suggestions.

I found that there are two patterns to handle this issue while I was implementing:

- Should suggest only UFCS
- Should suggest only simple `.clone()`

For the target issue, we can just remove the UFCS (`<Option<String> as Clone>::clone(&selection.1)`) side.

However, for the `BorrowedContentSource::OverloadedDeref` pattern like `Rc<Vec<i32>>`, for instance the `borrowck-move-out-of-overloaded-auto-deref.rs` test case, I think we need to employ the UFCS way. The actual test case is:

```rust
//@ run-rustfix
use std::rc::Rc;

pub fn main() {
    let _x = Rc::new(vec![1, 2]).into_iter();
    //~^ ERROR [E0507]
}
```

And another error will be shown if we simply use the simple `.clone()` pattern. Like:

```rust
use std::rc::Rc;

pub fn main() {
    let _x = Rc::new(vec![1, 2]).clone().into_iter();
}
```

then we will get

```
error[E0507]: cannot move out of an `Rc`
   --> src/main.rs:5:14
    |
  5 |     let _x = Rc::new(vec![1, 2]).clone().into_iter();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ----------- value moved due to this method call
    |              |
    |              move occurs because value has type `Vec<i32>`, which does not implement the `Copy` trait
    |
note: `into_iter` takes ownership of the receiver `self`, which moves value
   --> /playground/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/collect.rs:310:18
    |
310 |     fn into_iter(self) -> Self::IntoIter;
    |                  ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
  5 -     let _x = Rc::new(vec![1, 2]).clone().into_iter();
  5 +     let _x = <Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter();
    |

For more information about this error, try `rustc --explain E0507`.
```

[Rust Playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=7e767bed3f1c573c03642f20f454ed03)

In this case, `Rc::clone` only increments the reference count and returns a new `Rc<Vec<i32>>`; it does not grant ownership of the inner `Vec<i32>`. As a result, calling into_iter() attempts to move the `Vec<i32>`, leading to the same E0507 error again.

On the other hand, in UFCS form:

```
<Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter()
```

This explicitly calls `<Vec<i32> as Clone>::clone`, and the argument `&Rc<Vec<i32>>` is treated as `&Vec<i32>` via Rc’s `Deref` implementation. As a result, the `Vec<i32>` itself is cloned, yielding an owned `Vec<i32>`, which allows `into_iter()` to succeed, if my understanding is correct.

I addressed the issue as far as I could find the edge cases but please advice me if I'm overlooking something.
This commit is contained in:
Jacob Pratt
2026-04-25 01:21:51 -04:00
committed by GitHub
parent d3eddcff9e 61ff157bd4
commit 0af72af1b8
15 changed files with 249 additions and 95 deletions
Download Patch File Download Diff File Expand all files Collapse all files
compiler/rustc_borrowck/src/diagnostics/mod.rs
+89 -42
View File
@@ -602,8 +602,14 @@ pub(super) fn borrowed_content_source(
BorrowedContentSource::DerefRawPointer
} else if base_ty.is_mutable_ptr() {
BorrowedContentSource::DerefMutableRef
} else {
} else if base_ty.is_ref() {
BorrowedContentSource::DerefSharedRef
} else {
// Custom type implementing `Deref` (e.g. `MyBox<T>`, `Rc<T>`, `Arc<T>`)
// that wasn't detected via the MIR init trace above. This can happen
// when the deref base is initialized by a regular statement rather than
// a `TerminatorKind::Call` with `CallSource::OverloadedOperator`.
BorrowedContentSource::OverloadedDeref(base_ty)
}
}
@@ -1002,6 +1008,14 @@ struct CapturedMessageOpt {
maybe_reinitialized_locations_is_empty: bool,
}
/// Tracks whether [`MirBorrowckCtxt::explain_captures`] emitted a clone
/// suggestion, so callers can avoid emitting redundant suggestions downstream.
#[derive(Copy, Clone, PartialEq, Eq)]
pub(super) enum CloneSuggestion {
Emitted,
NotEmitted,
}
impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
/// Finds the spans associated to a move or copy of move_place at location.
pub(super) fn move_spans(
@@ -1226,7 +1240,7 @@ fn explain_captures(
move_spans: UseSpans<'tcx>,
moved_place: Place<'tcx>,
msg_opt: CapturedMessageOpt,
) {
) -> CloneSuggestion {
let CapturedMessageOpt {
is_partial_move: is_partial,
is_loop_message,
@@ -1235,6 +1249,7 @@ fn explain_captures(
has_suggest_reborrow,
maybe_reinitialized_locations_is_empty,
} = msg_opt;
let mut suggested_cloning = false;
if let UseSpans::FnSelfUse { var_span, fn_call_span, fn_span, kind } = move_spans {
let place_name = self
.describe_place(moved_place.as_ref())
@@ -1461,10 +1476,26 @@ fn explain_captures(
has_sugg = true;
}
if let Some(clone_trait) = tcx.lang_items().clone_trait() {
let sugg = if moved_place
// Check whether the deref is from a custom Deref impl
// (e.g. Rc, Box) or a built-in reference deref.
// For built-in derefs with Clone fully satisfied, we skip
// the UFCS suggestion here and let `suggest_cloning`
// downstream emit a simpler `.clone()` suggestion instead.
let has_overloaded_deref =
moved_place.iter_projections().any(|(place, elem)| {
matches!(elem, ProjectionElem::Deref)
&& matches!(
self.borrowed_content_source(place),
BorrowedContentSource::OverloadedDeref(_)
| BorrowedContentSource::OverloadedIndex(_)
)
});
let has_deref = moved_place
.iter_projections()
.any(|(_, elem)| matches!(elem, ProjectionElem::Deref))
{
.any(|(_, elem)| matches!(elem, ProjectionElem::Deref));
let sugg = if has_deref {
let (start, end) = if let Some(expr) = self.find_expr(move_span)
&& let Some(_) = self.clone_on_reference(expr)
&& let hir::ExprKind::MethodCall(_, rcvr, _, _) = expr.kind
@@ -1490,43 +1521,58 @@ fn explain_captures(
self.infcx.param_env,
) && !has_sugg
{
let msg = match &errors[..] {
[] => "you can `clone` the value and consume it, but this \
might not be your desired behavior"
.to_string(),
[error] => {
format!(
"you could `clone` the value and consume it, if the \
`{}` trait bound could be satisfied",
error.obligation.predicate,
)
}
_ => {
format!(
"you could `clone` the value and consume it, if the \
following trait bounds could be satisfied: {}",
listify(&errors, |e: &FulfillmentError<'tcx>| format!(
"`{}`",
e.obligation.predicate
))
.unwrap(),
)
}
};
err.multipart_suggestion(msg, sugg, Applicability::MaybeIncorrect);
for error in errors {
if let FulfillmentErrorCode::Select(
SelectionError::Unimplemented,
) = error.code
&& let ty::PredicateKind::Clause(ty::ClauseKind::Trait(
pred,
)) = error.obligation.predicate.kind().skip_binder()
{
self.infcx.err_ctxt().suggest_derive(
&error.obligation,
err,
error.obligation.predicate.kind().rebind(pred),
);
let skip_for_simple_clone =
has_deref && !has_overloaded_deref && errors.is_empty();
if !skip_for_simple_clone {
let msg = match &errors[..] {
[] => "you can `clone` the value and consume it, but \
this might not be your desired behavior"
.to_string(),
[error] => {
format!(
"you could `clone` the value and consume it, if \
the `{}` trait bound could be satisfied",
error.obligation.predicate,
)
}
_ => {
format!(
"you could `clone` the value and consume it, if \
the following trait bounds could be satisfied: \
{}",
listify(
&errors,
|e: &FulfillmentError<'tcx>| format!(
"`{}`",
e.obligation.predicate
)
)
.unwrap(),
)
}
};
err.multipart_suggestion(
msg,
sugg,
Applicability::MaybeIncorrect,
);
suggested_cloning = errors.is_empty();
for error in errors {
if let FulfillmentErrorCode::Select(
SelectionError::Unimplemented,
) = error.code
&& let ty::PredicateKind::Clause(ty::ClauseKind::Trait(
pred,
)) = error.obligation.predicate.kind().skip_binder()
{
self.infcx.err_ctxt().suggest_derive(
&error.obligation,
err,
error.obligation.predicate.kind().rebind(pred),
);
}
}
}
}
@@ -1558,6 +1604,7 @@ fn explain_captures(
})
}
}
if suggested_cloning { CloneSuggestion::Emitted } else { CloneSuggestion::NotEmitted }
}
/// Skip over locals that begin with an underscore or have no name
compiler/rustc_borrowck/src/diagnostics/move_errors.rs
+108 -23
View File
@@ -14,7 +14,9 @@
use tracing::debug;
use crate::MirBorrowckCtxt;
use crate::diagnostics::{CapturedMessageOpt, DescribePlaceOpt, UseSpans};
use crate::diagnostics::{
BorrowedContentSource, CapturedMessageOpt, CloneSuggestion, DescribePlaceOpt, UseSpans,
};
use crate::prefixes::PrefixSet;
#[derive(Debug)]
@@ -269,14 +271,19 @@ fn report(&mut self, error: GroupedMoveError<'tcx>) {
.span_delayed_bug(span, "Type may implement copy, but there is no other error.");
return;
}
let mut has_clone_suggestion = CloneSuggestion::NotEmitted;
let mut err = match kind {
&IllegalMoveOriginKind::BorrowedContent { target_place } => self
.report_cannot_move_from_borrowed_content(
&IllegalMoveOriginKind::BorrowedContent { target_place } => {
let (diag, clone_sugg) = self.report_cannot_move_from_borrowed_content(
original_path,
target_place,
span,
use_spans,
),
);
has_clone_suggestion = clone_sugg;
diag
}
&IllegalMoveOriginKind::InteriorOfTypeWithDestructor { container_ty: ty } => {
self.cannot_move_out_of_interior_of_drop(span, ty)
}
@@ -285,7 +292,7 @@ fn report(&mut self, error: GroupedMoveError<'tcx>) {
}
};
self.add_move_hints(error, &mut err, span);
self.add_move_hints(error, &mut err, span, has_clone_suggestion);
self.buffer_error(err);
}
@@ -426,7 +433,7 @@ fn report_cannot_move_from_borrowed_content(
deref_target_place: Place<'tcx>,
span: Span,
use_spans: Option<UseSpans<'tcx>>,
) -> Diag<'infcx> {
) -> (Diag<'infcx>, CloneSuggestion) {
let tcx = self.infcx.tcx;
// Inspect the type of the content behind the
// borrow to provide feedback about why this
@@ -447,8 +454,8 @@ fn report_cannot_move_from_borrowed_content(
let decl = &self.body.local_decls[local];
let local_name = self.local_name(local).map(|sym| format!("`{sym}`"));
if decl.is_ref_for_guard() {
return self
.cannot_move_out_of(
return (
self.cannot_move_out_of(
span,
&format!(
"{} in pattern guard",
@@ -458,9 +465,14 @@ fn report_cannot_move_from_borrowed_content(
.with_note(
"variables bound in patterns cannot be moved from \
until after the end of the pattern guard",
);
),
CloneSuggestion::NotEmitted,
);
} else if decl.is_ref_to_static() {
return self.report_cannot_move_from_static(move_place, span);
return (
self.report_cannot_move_from_static(move_place, span),
CloneSuggestion::NotEmitted,
);
}
}
@@ -539,10 +551,12 @@ fn report_cannot_move_from_borrowed_content(
has_suggest_reborrow: false,
maybe_reinitialized_locations_is_empty: true,
};
if let Some(use_spans) = use_spans {
self.explain_captures(&mut err, span, span, use_spans, move_place, msg_opt);
}
err
let suggested_cloning = if let Some(use_spans) = use_spans {
self.explain_captures(&mut err, span, span, use_spans, move_place, msg_opt)
} else {
CloneSuggestion::NotEmitted
};
(err, suggested_cloning)
}
fn report_closure_move_error(
@@ -676,7 +690,49 @@ fn closure_clause_kind(
}
}
fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) {
/// Suggest cloning via UFCS when a move occurs through a custom `Deref` impl.
///
/// A simple `.clone()` on a type like `MyBox<Vec<i32>>` would clone the wrapper,
/// not the inner `Vec<i32>`. Instead, we suggest `<Vec<i32> as Clone>::clone(&val)`.
fn suggest_cloning_through_overloaded_deref(
&self,
err: &mut Diag<'_>,
ty: Ty<'tcx>,
span: Span,
) -> CloneSuggestion {
let tcx = self.infcx.tcx;
let Some(clone_trait) = tcx.lang_items().clone_trait() else {
return CloneSuggestion::NotEmitted;
};
let Some(errors) =
self.infcx.type_implements_trait_shallow(clone_trait, ty, self.infcx.param_env)
else {
return CloneSuggestion::NotEmitted;
};
if !errors.is_empty() {
return CloneSuggestion::NotEmitted;
}
let sugg = vec![
(span.shrink_to_lo(), format!("<{ty} as Clone>::clone(&")),
(span.shrink_to_hi(), ")".to_string()),
];
err.multipart_suggestion(
"you can `clone` the value and consume it, but this might not be \
your desired behavior",
sugg,
Applicability::MaybeIncorrect,
);
CloneSuggestion::Emitted
}
fn add_move_hints(
&self,
error: GroupedMoveError<'tcx>,
err: &mut Diag<'_>,
span: Span,
has_clone_suggestion: CloneSuggestion,
) {
match error {
GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
self.add_borrow_suggestions(err, span, !binds_to.is_empty());
@@ -719,14 +775,43 @@ fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span
None => "value".to_string(),
};
if let Some(expr) = self.find_expr(use_span) {
self.suggest_cloning(
err,
original_path.as_ref(),
place_ty,
expr,
Some(use_spans),
);
if has_clone_suggestion == CloneSuggestion::NotEmitted {
// Check if the move is directly through a custom Deref impl
// (e.g. `*my_box` where MyBox implements Deref).
// A simple `.clone()` would clone the wrapper type rather than
// the inner value, so we need a UFCS suggestion instead.
//
// However, if there are further projections after the deref
// (e.g. `(*rc).field`), the value accessed is already the inner
// type and a simple `.clone()` works correctly.
let needs_ufcs = original_path.projection.last()
== Some(&ProjectionElem::Deref)
&& original_path.iter_projections().any(|(place, elem)| {
matches!(elem, ProjectionElem::Deref)
&& matches!(
self.borrowed_content_source(place),
BorrowedContentSource::OverloadedDeref(_)
| BorrowedContentSource::OverloadedIndex(_)
)
});
let emitted_ufcs = if needs_ufcs {
self.suggest_cloning_through_overloaded_deref(err, place_ty, use_span)
} else {
CloneSuggestion::NotEmitted
};
if emitted_ufcs == CloneSuggestion::NotEmitted {
if let Some(expr) = self.find_expr(use_span) {
self.suggest_cloning(
err,
original_path.as_ref(),
place_ty,
expr,
Some(use_spans),
);
}
}
}
if let Some(upvar_field) = self
tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.fixed
+1 -1
View File
@@ -2,6 +2,6 @@
use std::rc::Rc;
pub fn main() {
let _x = <Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2]).clone()).into_iter();
let _x = <Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter();
//~^ ERROR [E0507]
}
tests/ui/borrowck/borrowck-move-out-of-overloaded-auto-deref.stderr
-4
View File
@@ -12,10 +12,6 @@ help: you can `clone` the value and consume it, but this might not be your desir
|
LL | let _x = <Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter();
| ++++++++++++++++++++++++++++ +
help: consider cloning the value if the performance cost is acceptable
|
LL | let _x = Rc::new(vec![1, 2]).clone().into_iter();
| ++++++++
error: aborting due to 1 previous error
tests/ui/borrowck/clone-span-on-try-operator.fixed
+1 -1
View File
@@ -7,5 +7,5 @@ impl Foo {
}
fn main() {
let foo = &Foo;
<Foo as Clone>::clone(&foo.clone()).foo(); //~ ERROR cannot move out
foo.clone().foo(); //~ ERROR cannot move out
}
tests/ui/borrowck/clone-span-on-try-operator.stderr
-4
View File
@@ -11,10 +11,6 @@ note: `Foo::foo` takes ownership of the receiver `self`, which moves `*foo`
|
LL | fn foo(self) {}
| ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
|
LL | <Foo as Clone>::clone(&(*foo)).foo();
| +++++++++++++++++++++++ +
help: consider cloning the value if the performance cost is acceptable
|
LL - (*foo).foo();
tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.fixed
+1 -1
View File
@@ -9,7 +9,7 @@ fn call<F>(f: F) where F : Fn() {
fn main() {
let y = vec![format!("World")];
call(|| {
<Vec<String> as Clone>::clone(&y.clone()).into_iter();
y.clone().into_iter();
//~^ ERROR cannot move out of `y`, a captured variable in an `Fn` closure
});
}
tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr
-4
View File
@@ -17,10 +17,6 @@ LL | fn call<F>(f: F) where F : Fn() {
| ^^^^
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 | <Vec<String> as Clone>::clone(&y).into_iter();
| +++++++++++++++++++++++++++++++ +
help: consider cloning the value if the performance cost is acceptable
|
LL | y.clone().into_iter();
tests/ui/moves/suggest-clone.fixed
+1 -1
View File
@@ -7,5 +7,5 @@ impl Foo {
}
fn main() {
let foo = &Foo;
<Foo as Clone>::clone(&foo.clone()).foo(); //~ ERROR cannot move out
foo.clone().foo(); //~ ERROR cannot move out
}
tests/ui/moves/suggest-clone.stderr
-4
View File
@@ -11,10 +11,6 @@ note: `Foo::foo` takes ownership of the receiver `self`, which moves `*foo`
|
LL | fn foo(self) {}
| ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
|
LL | <Foo as Clone>::clone(&foo).foo();
| +++++++++++++++++++++++ +
help: consider cloning the value if the performance cost is acceptable
|
LL | foo.clone().foo();
tests/ui/suggestions/option-content-move.fixed
+2 -2
View File
@@ -7,7 +7,7 @@ impl LipogramCorpora {
pub fn validate_all(&mut self) -> Result<(), char> {
for selection in &self.selections {
if selection.1.is_some() {
if <Option<String> as Clone>::clone(&selection.1.clone()).as_mut().as_ref().unwrap().contains(selection.0) {
if selection.1.clone().as_mut().as_ref().unwrap().contains(selection.0) {
//~^ ERROR cannot move out of `selection.1`
return Err(selection.0);
}
@@ -25,7 +25,7 @@ impl LipogramCorpora2 {
pub fn validate_all(&mut self) -> Result<(), char> {
for selection in &self.selections {
if selection.1.is_ok() {
if <Result<String, String> as Clone>::clone(&selection.1.clone()).as_mut().as_ref().unwrap().contains(selection.0) {
if selection.1.clone().as_mut().as_ref().unwrap().contains(selection.0) {
//~^ ERROR cannot move out of `selection.1`
return Err(selection.0);
}
tests/ui/suggestions/option-content-move.stderr
-8
View File
@@ -16,10 +16,6 @@ help: consider calling `.as_mut()` to mutably borrow the value's contents
|
LL | if selection.1.as_mut().unwrap().contains(selection.0) {
| +++++++++
help: you can `clone` the value and consume it, but this might not be your desired behavior
|
LL | if <Option<String> as Clone>::clone(&selection.1).unwrap().contains(selection.0) {
| ++++++++++++++++++++++++++++++++++ +
help: consider cloning the value if the performance cost is acceptable
|
LL | if selection.1.clone().unwrap().contains(selection.0) {
@@ -43,10 +39,6 @@ help: consider calling `.as_mut()` to mutably borrow the value's contents
|
LL | if selection.1.as_mut().unwrap().contains(selection.0) {
| +++++++++
help: you can `clone` the value and consume it, but this might not be your desired behavior
|
LL | if <Result<String, String> as Clone>::clone(&selection.1).unwrap().contains(selection.0) {
| ++++++++++++++++++++++++++++++++++++++++++ +
help: consider cloning the value if the performance cost is acceptable
|
LL | if selection.1.clone().unwrap().contains(selection.0) {
tests/ui/suggestions/ufcs-for-deref-inner-clone.fixed
+16
View File
@@ -0,0 +1,16 @@
//@ run-rustfix
use std::ops::Deref;
struct MyBox<T>(T);
impl<T> Deref for MyBox<T> {
type Target = T;
fn deref(&self) -> &Self::Target {
&self.0
}
}
pub fn main() {
let _x = <Vec<i32> as Clone>::clone(&MyBox(vec![1, 2])).into_iter();
//~^ ERROR cannot move out of dereference of `MyBox<Vec<i32>>`
}
tests/ui/suggestions/ufcs-for-deref-inner-clone.rs
+16
View File
@@ -0,0 +1,16 @@
//@ run-rustfix
use std::ops::Deref;
struct MyBox<T>(T);
impl<T> Deref for MyBox<T> {
type Target = T;
fn deref(&self) -> &Self::Target {
&self.0
}
}
pub fn main() {
let _x = MyBox(vec![1, 2]).into_iter();
//~^ ERROR cannot move out of dereference of `MyBox<Vec<i32>>`
}
tests/ui/suggestions/ufcs-for-deref-inner-clone.stderr
+14
View File
@@ -0,0 +1,14 @@
error[E0507]: cannot move out of dereference of `MyBox<Vec<i32>>`
--> $DIR/ufcs-for-deref-inner-clone.rs:14:14
|
LL | let _x = MyBox(vec![1, 2]).into_iter();
| ^^^^^^^^^^^^^^^^^ move occurs because value has type `Vec<i32>`, which does not implement the `Copy` trait
|
help: you can `clone` the value and consume it, but this might not be your desired behavior
|
LL | let _x = <Vec<i32> as Clone>::clone(&MyBox(vec![1, 2])).into_iter();
| ++++++++++++++++++++++++++++ +
error: aborting due to 1 previous error
For more information about this error, try `rustc --explain E0507`.