When possible, suggest cloning the result of a call instead of an argument
``` error[E0505]: cannot move out of `a` because it is borrowed --> $DIR/variance-issue-20533.rs:28:14 | LL | let a = AffineU32(1); | - binding `a` declared here LL | let x = foo(&a); | -- borrow of `a` occurs here LL | drop(a); | ^ move out of `a` occurs here LL | drop(x); | - borrow later used here | help: consider cloning the value if the performance cost is acceptable | LL | let x = foo(&a).clone(); | ++++++++ ```
This commit is contained in:
parent
7f7f6792f1
commit
a1a3abb08f
6 changed files with 157 additions and 19 deletions
|
@ -476,7 +476,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||||
} else if self.suggest_hoisting_call_outside_loop(err, expr) {
|
} else if self.suggest_hoisting_call_outside_loop(err, expr) {
|
||||||
// The place where the the type moves would be misleading to suggest clone.
|
// The place where the the type moves would be misleading to suggest clone.
|
||||||
// #121466
|
// #121466
|
||||||
self.suggest_cloning(err, ty, expr);
|
self.suggest_cloning(err, ty, expr, None);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if let Some(pat) = finder.pat {
|
if let Some(pat) = finder.pat {
|
||||||
|
@ -987,7 +987,78 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||||
can_suggest_clone
|
can_suggest_clone
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(crate) fn suggest_cloning(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>) {
|
pub(crate) fn suggest_cloning(
|
||||||
|
&self,
|
||||||
|
err: &mut Diag<'_>,
|
||||||
|
ty: Ty<'tcx>,
|
||||||
|
expr: &hir::Expr<'_>,
|
||||||
|
other_expr: Option<&hir::Expr<'_>>,
|
||||||
|
) {
|
||||||
|
'outer: {
|
||||||
|
if let ty::Ref(..) = ty.kind() {
|
||||||
|
// We check for either `let binding = foo(expr, other_expr);` or
|
||||||
|
// `foo(expr, other_expr);` and if so we don't suggest an incorrect
|
||||||
|
// `foo(expr, other_expr).clone()`
|
||||||
|
if let Some(other_expr) = other_expr
|
||||||
|
&& let Some(parent_let) =
|
||||||
|
self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
|
||||||
|
if let (hir_id, hir::Node::Local(_) | hir::Node::Stmt(_)) = n {
|
||||||
|
Some(hir_id)
|
||||||
|
} else {
|
||||||
|
None
|
||||||
|
}
|
||||||
|
})
|
||||||
|
&& let Some(other_parent_let) =
|
||||||
|
self.infcx.tcx.hir().parent_iter(other_expr.hir_id).find_map(|n| {
|
||||||
|
if let (hir_id, hir::Node::Local(_) | hir::Node::Stmt(_)) = n {
|
||||||
|
Some(hir_id)
|
||||||
|
} else {
|
||||||
|
None
|
||||||
|
}
|
||||||
|
})
|
||||||
|
&& parent_let == other_parent_let
|
||||||
|
{
|
||||||
|
// Explicitly check that we don't have `foo(&*expr, other_expr)`, as cloning the
|
||||||
|
// result of `foo(...)` won't help.
|
||||||
|
break 'outer;
|
||||||
|
}
|
||||||
|
|
||||||
|
// We're suggesting `.clone()` on an borrowed value. See if the expression we have
|
||||||
|
// is an argument to a function or method call, and try to suggest cloning the
|
||||||
|
// *result* of the call, instead of the argument. This is closest to what people
|
||||||
|
// would actually be looking for in most cases, with maybe the exception of things
|
||||||
|
// like `fn(T) -> T`, but even then it is reasonable.
|
||||||
|
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
|
||||||
|
let mut prev = expr;
|
||||||
|
while let hir::Node::Expr(parent) = self.infcx.tcx.parent_hir_node(prev.hir_id) {
|
||||||
|
if let hir::ExprKind::Call(..) | hir::ExprKind::MethodCall(..) = parent.kind
|
||||||
|
&& let Some(call_ty) = typeck_results.node_type_opt(parent.hir_id)
|
||||||
|
&& let call_ty = call_ty.peel_refs()
|
||||||
|
&& (!call_ty
|
||||||
|
.walk()
|
||||||
|
.any(|t| matches!(t.unpack(), ty::GenericArgKind::Lifetime(_)))
|
||||||
|
|| if let ty::Alias(ty::Projection, _) = call_ty.kind() {
|
||||||
|
// FIXME: this isn't quite right with lifetimes on assoc types,
|
||||||
|
// but ignore for now. We will only suggest cloning if
|
||||||
|
// `<Ty as Trait>::Assoc: Clone`, which should keep false positives
|
||||||
|
// down to a managable ammount.
|
||||||
|
true
|
||||||
|
} else {
|
||||||
|
false
|
||||||
|
})
|
||||||
|
&& let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
|
||||||
|
&& self
|
||||||
|
.infcx
|
||||||
|
.type_implements_trait(clone_trait_def, [call_ty], self.param_env)
|
||||||
|
.must_apply_modulo_regions()
|
||||||
|
&& self.suggest_cloning_inner(err, call_ty, parent)
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
prev = parent;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
let ty = ty.peel_refs();
|
let ty = ty.peel_refs();
|
||||||
if let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
|
if let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
|
||||||
&& self
|
&& self
|
||||||
|
@ -1014,12 +1085,17 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fn suggest_cloning_inner(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>) {
|
fn suggest_cloning_inner(
|
||||||
|
&self,
|
||||||
|
err: &mut Diag<'_>,
|
||||||
|
ty: Ty<'tcx>,
|
||||||
|
expr: &hir::Expr<'_>,
|
||||||
|
) -> bool {
|
||||||
let tcx = self.infcx.tcx;
|
let tcx = self.infcx.tcx;
|
||||||
if let Some(_) = self.clone_on_reference(expr) {
|
if let Some(_) = self.clone_on_reference(expr) {
|
||||||
// Avoid redundant clone suggestion already suggested in `explain_captures`.
|
// Avoid redundant clone suggestion already suggested in `explain_captures`.
|
||||||
// See `tests/ui/moves/needs-clone-through-deref.rs`
|
// See `tests/ui/moves/needs-clone-through-deref.rs`
|
||||||
return;
|
return false;
|
||||||
}
|
}
|
||||||
// Try to find predicates on *generic params* that would allow copying `ty`
|
// Try to find predicates on *generic params* that would allow copying `ty`
|
||||||
let suggestion =
|
let suggestion =
|
||||||
|
@ -1036,7 +1112,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||||
if let hir::ExprKind::AddrOf(_, hir::Mutability::Mut, _) = inner_expr.kind {
|
if let hir::ExprKind::AddrOf(_, hir::Mutability::Mut, _) = inner_expr.kind {
|
||||||
// We assume that `&mut` refs are desired for their side-effects, so cloning the
|
// We assume that `&mut` refs are desired for their side-effects, so cloning the
|
||||||
// value wouldn't do what the user wanted.
|
// value wouldn't do what the user wanted.
|
||||||
return;
|
return false;
|
||||||
}
|
}
|
||||||
inner_expr = inner;
|
inner_expr = inner;
|
||||||
}
|
}
|
||||||
|
@ -1059,6 +1135,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||||
"consider cloning the value if the performance cost is acceptable"
|
"consider cloning the value if the performance cost is acceptable"
|
||||||
};
|
};
|
||||||
err.multipart_suggestion_verbose(msg, sugg, Applicability::MachineApplicable);
|
err.multipart_suggestion_verbose(msg, sugg, Applicability::MachineApplicable);
|
||||||
|
true
|
||||||
}
|
}
|
||||||
|
|
||||||
fn suggest_adding_bounds(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, def_id: DefId, span: Span) {
|
fn suggest_adding_bounds(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, def_id: DefId, span: Span) {
|
||||||
|
@ -1167,7 +1244,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
|
||||||
if let Some(expr) = self.find_expr(borrow_span)
|
if let Some(expr) = self.find_expr(borrow_span)
|
||||||
&& let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
|
&& let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
|
||||||
{
|
{
|
||||||
self.suggest_cloning(&mut err, ty, expr);
|
self.suggest_cloning(&mut err, ty, expr, self.find_expr(span));
|
||||||
}
|
}
|
||||||
self.buffer_error(err);
|
self.buffer_error(err);
|
||||||
}
|
}
|
||||||
|
|
|
@ -435,7 +435,9 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
|
||||||
|
|
||||||
fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) {
|
fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) {
|
||||||
match error {
|
match error {
|
||||||
GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
|
GroupedMoveError::MovesFromPlace {
|
||||||
|
mut binds_to, move_from, span: other_span, ..
|
||||||
|
} => {
|
||||||
self.add_borrow_suggestions(err, span);
|
self.add_borrow_suggestions(err, span);
|
||||||
if binds_to.is_empty() {
|
if binds_to.is_empty() {
|
||||||
let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
|
let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
|
||||||
|
@ -445,7 +447,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
|
||||||
};
|
};
|
||||||
|
|
||||||
if let Some(expr) = self.find_expr(span) {
|
if let Some(expr) = self.find_expr(span) {
|
||||||
self.suggest_cloning(err, place_ty, expr);
|
self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span));
|
||||||
}
|
}
|
||||||
|
|
||||||
err.subdiagnostic(
|
err.subdiagnostic(
|
||||||
|
@ -472,15 +474,15 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
|
||||||
}
|
}
|
||||||
// No binding. Nothing to suggest.
|
// No binding. Nothing to suggest.
|
||||||
GroupedMoveError::OtherIllegalMove { ref original_path, use_spans, .. } => {
|
GroupedMoveError::OtherIllegalMove { ref original_path, use_spans, .. } => {
|
||||||
let span = use_spans.var_or_use();
|
let use_span = use_spans.var_or_use();
|
||||||
let place_ty = original_path.ty(self.body, self.infcx.tcx).ty;
|
let place_ty = original_path.ty(self.body, self.infcx.tcx).ty;
|
||||||
let place_desc = match self.describe_place(original_path.as_ref()) {
|
let place_desc = match self.describe_place(original_path.as_ref()) {
|
||||||
Some(desc) => format!("`{desc}`"),
|
Some(desc) => format!("`{desc}`"),
|
||||||
None => "value".to_string(),
|
None => "value".to_string(),
|
||||||
};
|
};
|
||||||
|
|
||||||
if let Some(expr) = self.find_expr(span) {
|
if let Some(expr) = self.find_expr(use_span) {
|
||||||
self.suggest_cloning(err, place_ty, expr);
|
self.suggest_cloning(err, place_ty, expr, self.find_expr(span));
|
||||||
}
|
}
|
||||||
|
|
||||||
err.subdiagnostic(
|
err.subdiagnostic(
|
||||||
|
@ -489,7 +491,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
|
||||||
is_partial_move: false,
|
is_partial_move: false,
|
||||||
ty: place_ty,
|
ty: place_ty,
|
||||||
place: &place_desc,
|
place: &place_desc,
|
||||||
span,
|
span: use_span,
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
|
|
||||||
|
@ -593,7 +595,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
|
||||||
let place_desc = &format!("`{}`", self.local_names[*local].unwrap());
|
let place_desc = &format!("`{}`", self.local_names[*local].unwrap());
|
||||||
|
|
||||||
if let Some(expr) = self.find_expr(binding_span) {
|
if let Some(expr) = self.find_expr(binding_span) {
|
||||||
self.suggest_cloning(err, bind_to.ty, expr);
|
self.suggest_cloning(err, bind_to.ty, expr, None);
|
||||||
}
|
}
|
||||||
|
|
||||||
err.subdiagnostic(
|
err.subdiagnostic(
|
||||||
|
|
|
@ -3,10 +3,10 @@
|
||||||
// fn body, causing this (invalid) code to be accepted.
|
// fn body, causing this (invalid) code to be accepted.
|
||||||
|
|
||||||
pub trait Foo<'a> {
|
pub trait Foo<'a> {
|
||||||
type Bar;
|
type Bar: Clone;
|
||||||
}
|
}
|
||||||
|
|
||||||
impl<'a, T:'a> Foo<'a> for T {
|
impl<'a, T: 'a> Foo<'a> for T {
|
||||||
type Bar = &'a T;
|
type Bar = &'a T;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
@ -10,6 +10,11 @@ LL | drop(x);
|
||||||
| ^ move out of `x` occurs here
|
| ^ move out of `x` occurs here
|
||||||
LL | return f(y);
|
LL | return f(y);
|
||||||
| - borrow later used here
|
| - borrow later used here
|
||||||
|
|
|
||||||
|
help: consider cloning the value if the performance cost is acceptable
|
||||||
|
|
|
||||||
|
LL | 's: loop { y = denormalise(&x).clone(); break }
|
||||||
|
| ++++++++
|
||||||
|
|
||||||
error: aborting due to 1 previous error
|
error: aborting due to 1 previous error
|
||||||
|
|
||||||
|
|
|
@ -19,8 +19,15 @@ fn baz<'a, T>(_x: &'a T) -> Baked<'a> {
|
||||||
Baked(PhantomData)
|
Baked(PhantomData)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn bat(x: &AffineU32) -> &u32 {
|
||||||
|
&x.0
|
||||||
|
}
|
||||||
|
|
||||||
struct AffineU32(u32);
|
struct AffineU32(u32);
|
||||||
|
|
||||||
|
#[derive(Clone)]
|
||||||
|
struct ClonableAffineU32(u32);
|
||||||
|
|
||||||
fn main() {
|
fn main() {
|
||||||
{
|
{
|
||||||
let a = AffineU32(1);
|
let a = AffineU32(1);
|
||||||
|
@ -40,4 +47,16 @@ fn main() {
|
||||||
drop(a); //~ ERROR cannot move out of `a`
|
drop(a); //~ ERROR cannot move out of `a`
|
||||||
drop(x);
|
drop(x);
|
||||||
}
|
}
|
||||||
|
{
|
||||||
|
let a = AffineU32(1);
|
||||||
|
let x = bat(&a);
|
||||||
|
drop(a); //~ ERROR cannot move out of `a`
|
||||||
|
drop(x);
|
||||||
|
}
|
||||||
|
{
|
||||||
|
let a = ClonableAffineU32(1);
|
||||||
|
let x = foo(&a);
|
||||||
|
drop(a); //~ ERROR cannot move out of `a`
|
||||||
|
drop(x);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
@ -1,5 +1,5 @@
|
||||||
error[E0505]: cannot move out of `a` because it is borrowed
|
error[E0505]: cannot move out of `a` because it is borrowed
|
||||||
--> $DIR/variance-issue-20533.rs:28:14
|
--> $DIR/variance-issue-20533.rs:35:14
|
||||||
|
|
|
|
||||||
LL | let a = AffineU32(1);
|
LL | let a = AffineU32(1);
|
||||||
| - binding `a` declared here
|
| - binding `a` declared here
|
||||||
|
@ -11,7 +11,7 @@ LL | drop(x);
|
||||||
| - borrow later used here
|
| - borrow later used here
|
||||||
|
|
||||||
error[E0505]: cannot move out of `a` because it is borrowed
|
error[E0505]: cannot move out of `a` because it is borrowed
|
||||||
--> $DIR/variance-issue-20533.rs:34:14
|
--> $DIR/variance-issue-20533.rs:41:14
|
||||||
|
|
|
|
||||||
LL | let a = AffineU32(1);
|
LL | let a = AffineU32(1);
|
||||||
| - binding `a` declared here
|
| - binding `a` declared here
|
||||||
|
@ -23,7 +23,7 @@ LL | drop(x);
|
||||||
| - borrow later used here
|
| - borrow later used here
|
||||||
|
|
||||||
error[E0505]: cannot move out of `a` because it is borrowed
|
error[E0505]: cannot move out of `a` because it is borrowed
|
||||||
--> $DIR/variance-issue-20533.rs:40:14
|
--> $DIR/variance-issue-20533.rs:47:14
|
||||||
|
|
|
|
||||||
LL | let a = AffineU32(1);
|
LL | let a = AffineU32(1);
|
||||||
| - binding `a` declared here
|
| - binding `a` declared here
|
||||||
|
@ -34,6 +34,41 @@ LL | drop(a);
|
||||||
LL | drop(x);
|
LL | drop(x);
|
||||||
| - borrow later used here
|
| - borrow later used here
|
||||||
|
|
||||||
error: aborting due to 3 previous errors
|
error[E0505]: cannot move out of `a` because it is borrowed
|
||||||
|
--> $DIR/variance-issue-20533.rs:53:14
|
||||||
|
|
|
||||||
|
LL | let a = AffineU32(1);
|
||||||
|
| - binding `a` declared here
|
||||||
|
LL | let x = bat(&a);
|
||||||
|
| -- borrow of `a` occurs here
|
||||||
|
LL | drop(a);
|
||||||
|
| ^ move out of `a` occurs here
|
||||||
|
LL | drop(x);
|
||||||
|
| - borrow later used here
|
||||||
|
|
|
||||||
|
help: consider cloning the value if the performance cost is acceptable
|
||||||
|
|
|
||||||
|
LL | let x = bat(&a).clone();
|
||||||
|
| ++++++++
|
||||||
|
|
||||||
|
error[E0505]: cannot move out of `a` because it is borrowed
|
||||||
|
--> $DIR/variance-issue-20533.rs:59:14
|
||||||
|
|
|
||||||
|
LL | let a = ClonableAffineU32(1);
|
||||||
|
| - binding `a` declared here
|
||||||
|
LL | let x = foo(&a);
|
||||||
|
| -- borrow of `a` occurs here
|
||||||
|
LL | drop(a);
|
||||||
|
| ^ move out of `a` occurs here
|
||||||
|
LL | drop(x);
|
||||||
|
| - borrow later used here
|
||||||
|
|
|
||||||
|
help: consider cloning the value if the performance cost is acceptable
|
||||||
|
|
|
||||||
|
LL - let x = foo(&a);
|
||||||
|
LL + let x = foo(a.clone());
|
||||||
|
|
|
||||||
|
|
||||||
|
error: aborting due to 5 previous errors
|
||||||
|
|
||||||
For more information about this error, try `rustc --explain E0505`.
|
For more information about this error, try `rustc --explain E0505`.
|
||||||
|
|
Loading…
Add table
Add a link
Reference in a new issue