Skip to content

Commit 913c59b

Browse files
compiler-errorscompiler-errors
committed
Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck
1 parent 2d5a628 commit 913c59b

File tree

45 files changed

+123
-386
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

45 files changed

+123
-386
lines changed

‎compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

+24-99
Original file line numberDiff line numberDiff line change
@@ -570,11 +570,11 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
570570
} = move_spans
571571
&& can_suggest_clone
572572
{
573-
self.suggest_cloning(err, ty, expr, None, Some(move_spans));
573+
self.suggest_cloning(err, ty, expr, Some(move_spans));
574574
} else if self.suggest_hoisting_call_outside_loop(err, expr) && can_suggest_clone {
575575
// The place where the type moves would be misleading to suggest clone.
576576
// #121466
577-
self.suggest_cloning(err, ty, expr, None, Some(move_spans));
577+
self.suggest_cloning(err, ty, expr, Some(move_spans));
578578
}
579579
}
580580

@@ -1236,8 +1236,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
12361236
&self,
12371237
err: &mut Diag<'_>,
12381238
ty: Ty<'tcx>,
1239-
mut expr: &'tcx hir::Expr<'tcx>,
1240-
mut other_expr: Option<&'tcx hir::Expr<'tcx>>,
1239+
expr: &'tcx hir::Expr<'tcx>,
12411240
use_spans: Option<UseSpans<'tcx>>,
12421241
) {
12431242
if let hir::ExprKind::Struct(_, _, Some(_)) = expr.kind {
@@ -1249,97 +1248,6 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
12491248
return;
12501249
}
12511250

1252-
if let Some(some_other_expr) = other_expr
1253-
&& let Some(parent_binop) =
1254-
self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
1255-
if let (hir_id, hir::Node::Expr(e)) = n
1256-
&& let hir::ExprKind::AssignOp(_binop, target, _arg) = e.kind
1257-
&& target.hir_id == expr.hir_id
1258-
{
1259-
Some(hir_id)
1260-
} else {
1261-
None
1262-
}
1263-
})
1264-
&& let Some(other_parent_binop) =
1265-
self.infcx.tcx.hir().parent_iter(some_other_expr.hir_id).find_map(|n| {
1266-
if let (hir_id, hir::Node::Expr(expr)) = n
1267-
&& let hir::ExprKind::AssignOp(..) = expr.kind
1268-
{
1269-
Some(hir_id)
1270-
} else {
1271-
None
1272-
}
1273-
})
1274-
&& parent_binop == other_parent_binop
1275-
{
1276-
// Explicitly look for `expr += other_expr;` and avoid suggesting
1277-
// `expr.clone() += other_expr;`, instead suggesting `expr += other_expr.clone();`.
1278-
other_expr = Some(expr);
1279-
expr = some_other_expr;
1280-
}
1281-
'outer: {
1282-
if let ty::Ref(..) = ty.kind() {
1283-
// We check for either `let binding = foo(expr, other_expr);` or
1284-
// `foo(expr, other_expr);` and if so we don't suggest an incorrect
1285-
// `foo(expr, other_expr).clone()`
1286-
if let Some(other_expr) = other_expr
1287-
&& let Some(parent_let) =
1288-
self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
1289-
if let (hir_id, hir::Node::LetStmt(_) | hir::Node::Stmt(_)) = n {
1290-
Some(hir_id)
1291-
} else {
1292-
None
1293-
}
1294-
})
1295-
&& let Some(other_parent_let) =
1296-
self.infcx.tcx.hir().parent_iter(other_expr.hir_id).find_map(|n| {
1297-
if let (hir_id, hir::Node::LetStmt(_) | hir::Node::Stmt(_)) = n {
1298-
Some(hir_id)
1299-
} else {
1300-
None
1301-
}
1302-
})
1303-
&& parent_let == other_parent_let
1304-
{
1305-
// Explicitly check that we don't have `foo(&*expr, other_expr)`, as cloning the
1306-
// result of `foo(...)` won't help.
1307-
break 'outer;
1308-
}
1309-
1310-
// We're suggesting `.clone()` on an borrowed value. See if the expression we have
1311-
// is an argument to a function or method call, and try to suggest cloning the
1312-
// *result* of the call, instead of the argument. This is closest to what people
1313-
// would actually be looking for in most cases, with maybe the exception of things
1314-
// like `fn(T) -> T`, but even then it is reasonable.
1315-
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
1316-
let mut prev = expr;
1317-
while let hir::Node::Expr(parent) = self.infcx.tcx.parent_hir_node(prev.hir_id) {
1318-
if let hir::ExprKind::Call(..) | hir::ExprKind::MethodCall(..) = parent.kind
1319-
&& let Some(call_ty) = typeck_results.node_type_opt(parent.hir_id)
1320-
&& let call_ty = call_ty.peel_refs()
1321-
&& (!call_ty
1322-
.walk()
1323-
.any(|t| matches!(t.unpack(), ty::GenericArgKind::Lifetime(_)))
1324-
|| if let ty::Alias(ty::Projection, _) = call_ty.kind() {
1325-
// FIXME: this isn't quite right with lifetimes on assoc types,
1326-
// but ignore for now. We will only suggest cloning if
1327-
// `<Ty as Trait>::Assoc: Clone`, which should keep false positives
1328-
// down to a managable ammount.
1329-
true
1330-
} else {
1331-
false
1332-
})
1333-
&& self.implements_clone(call_ty)
1334-
&& self.suggest_cloning_inner(err, call_ty, parent)
1335-
{
1336-
return;
1337-
}
1338-
prev = parent;
1339-
}
1340-
}
1341-
}
1342-
let ty = ty.peel_refs();
13431251
if self.implements_clone(ty) {
13441252
self.suggest_cloning_inner(err, ty, expr);
13451253
} else if let ty::Adt(def, args) = ty.kind()
@@ -1606,10 +1514,27 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
16061514
);
16071515
self.suggest_copy_for_type_in_cloned_ref(&mut err, place);
16081516
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
1609-
if let Some(expr) = self.find_expr(borrow_span)
1610-
&& let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
1611-
{
1612-
self.suggest_cloning(&mut err, ty, expr, self.find_expr(span), Some(move_spans));
1517+
if let Some(expr) = self.find_expr(borrow_span) {
1518+
// This is a borrow span, so we want to suggest cloning the referent.
1519+
if let hir::ExprKind::AddrOf(_, _, borrowed_expr) = expr.kind
1520+
&& let Some(ty) = typeck_results.expr_ty_opt(borrowed_expr)
1521+
{
1522+
self.suggest_cloning(&mut err, ty, borrowed_expr, Some(move_spans));
1523+
} else if typeck_results.expr_adjustments(expr).first().is_some_and(|adj| {
1524+
matches!(
1525+
adj.kind,
1526+
ty::adjustment::Adjust::Borrow(ty::adjustment::AutoBorrow::Ref(
1527+
_,
1528+
ty::adjustment::AutoBorrowMutability::Not
1529+
| ty::adjustment::AutoBorrowMutability::Mut {
1530+
allow_two_phase_borrow: ty::adjustment::AllowTwoPhase::No
1531+
}
1532+
))
1533+
)
1534+
}) && let Some(ty) = typeck_results.expr_ty_opt(expr)
1535+
{
1536+
self.suggest_cloning(&mut err, ty, expr, Some(move_spans));
1537+
}
16131538
}
16141539
self.buffer_error(err);
16151540
}

‎compiler/rustc_borrowck/src/diagnostics/move_errors.rs

+4-12
Original file line numberDiff line numberDiff line change
@@ -565,9 +565,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
565565

566566
fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) {
567567
match error {
568-
GroupedMoveError::MovesFromPlace {
569-
mut binds_to, move_from, span: other_span, ..
570-
} => {
568+
GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
571569
self.add_borrow_suggestions(err, span);
572570
if binds_to.is_empty() {
573571
let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
@@ -577,7 +575,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
577575
};
578576

579577
if let Some(expr) = self.find_expr(span) {
580-
self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span), None);
578+
self.suggest_cloning(err, place_ty, expr, None);
581579
}
582580

583581
err.subdiagnostic(crate::session_diagnostics::TypeNoCopy::Label {
@@ -609,13 +607,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
609607
};
610608

611609
if let Some(expr) = self.find_expr(use_span) {
612-
self.suggest_cloning(
613-
err,
614-
place_ty,
615-
expr,
616-
self.find_expr(span),
617-
Some(use_spans),
618-
);
610+
self.suggest_cloning(err, place_ty, expr, Some(use_spans));
619611
}
620612

621613
err.subdiagnostic(crate::session_diagnostics::TypeNoCopy::Label {
@@ -740,7 +732,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
740732
let place_desc = &format!("`{}`", self.local_names[*local].unwrap());
741733

742734
if let Some(expr) = self.find_expr(binding_span) {
743-
self.suggest_cloning(err, bind_to.ty, expr, None, None);
735+
self.suggest_cloning(err, bind_to.ty, expr, None);
744736
}
745737

746738
err.subdiagnostic(crate::session_diagnostics::TypeNoCopy::Label {

‎tests/ui/associated-types/associated-types-outlives.stderr

+7-3
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,14 @@ LL | drop(x);
1111
LL | return f(y);
1212
| - borrow later used here
1313
|
14-
help: consider cloning the value if the performance cost is acceptable
14+
help: if `T` implemented `Clone`, you could clone the value
15+
--> $DIR/associated-types-outlives.rs:17:21
1516
|
16-
LL | 's: loop { y = denormalise(&x).clone(); break }
17-
| ++++++++
17+
LL | pub fn free_and_use<T: for<'a> Foo<'a>,
18+
| ^ consider constraining this type parameter with `Clone`
19+
...
20+
LL | 's: loop { y = denormalise(&x); break }
21+
| - you could clone this value
1822

1923
error: aborting due to 1 previous error
2024

‎tests/ui/augmented-assignments.rs

-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ fn main() {
1717
x;
1818
//~^ ERROR cannot move out of `x` because it is borrowed
1919
//~| move out of `x` occurs here
20-
//~| HELP consider cloning
2120

2221
let y = Int(2);
2322
//~^ HELP consider changing this to be mutable

‎tests/ui/augmented-assignments.stderr

+1-6
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,9 @@ LL | x
88
...
99
LL | x;
1010
| ^ move out of `x` occurs here
11-
|
12-
help: consider cloning the value if the performance cost is acceptable
13-
|
14-
LL | x.clone();
15-
| ++++++++
1611

1712
error[E0596]: cannot borrow `y` as mutable, as it is not declared as mutable
18-
--> $DIR/augmented-assignments.rs:25:5
13+
--> $DIR/augmented-assignments.rs:24:5
1914
|
2015
LL | y
2116
| ^ cannot borrow as mutable

‎tests/ui/binop/binop-move-semantics.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ help: if `T` implemented `Clone`, you could clone the value
6565
LL | fn move_borrowed<T: Add<Output=()>>(x: T, mut y: T) {
6666
| ^ consider constraining this type parameter with `Clone`
6767
LL | let m = &x;
68-
| -- you could clone this value
68+
| - you could clone this value
6969

7070
error[E0505]: cannot move out of `y` because it is borrowed
7171
--> $DIR/binop-move-semantics.rs:23:5
@@ -88,7 +88,7 @@ LL | fn move_borrowed<T: Add<Output=()>>(x: T, mut y: T) {
8888
| ^ consider constraining this type parameter with `Clone`
8989
LL | let m = &x;
9090
LL | let n = &mut y;
91-
| ------ you could clone this value
91+
| - you could clone this value
9292

9393
error[E0507]: cannot move out of `*m` which is behind a mutable reference
9494
--> $DIR/binop-move-semantics.rs:30:5

‎tests/ui/borrowck/borrow-tuple-fields.stderr

+4-6
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,8 @@ LL | r.use_ref();
1313
|
1414
help: consider cloning the value if the performance cost is acceptable
1515
|
16-
LL - let r = &x.0;
17-
LL + let r = x.0.clone();
18-
|
16+
LL | let r = &x.0.clone();
17+
| ++++++++
1918

2019
error[E0502]: cannot borrow `x.0` as mutable because it is also borrowed as immutable
2120
--> $DIR/borrow-tuple-fields.rs:18:13
@@ -51,9 +50,8 @@ LL | r.use_ref();
5150
|
5251
help: consider cloning the value if the performance cost is acceptable
5352
|
54-
LL - let r = &x.0;
55-
LL + let r = x.0.clone();
56-
|
53+
LL | let r = &x.0.clone();
54+
| ++++++++
5755

5856
error[E0502]: cannot borrow `x.0` as mutable because it is also borrowed as immutable
5957
--> $DIR/borrow-tuple-fields.rs:33:13

‎tests/ui/borrowck/borrowck-bad-nested-calls-move.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ LL | a);
1414
help: consider cloning the value if the performance cost is acceptable
1515
|
1616
LL - &*a,
17-
LL + a.clone(),
17+
LL + &a.clone(),
1818
|
1919

2020
error[E0505]: cannot move out of `a` because it is borrowed
@@ -32,7 +32,7 @@ LL | a);
3232
help: consider cloning the value if the performance cost is acceptable
3333
|
3434
LL - &*a,
35-
LL + a.clone(),
35+
LL + &a.clone(),
3636
|
3737

3838
error: aborting due to 2 previous errors

‎tests/ui/borrowck/borrowck-field-sensitivity.stderr

+4-6
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,8 @@ LL | drop(**p);
5252
|
5353
help: consider cloning the value if the performance cost is acceptable
5454
|
55-
LL - let p = &x.b;
56-
LL + let p = x.b.clone();
57-
|
55+
LL | let p = &x.b.clone();
56+
| ++++++++
5857

5958
error[E0505]: cannot move out of `x.b` because it is borrowed
6059
--> $DIR/borrowck-field-sensitivity.rs:41:14
@@ -70,9 +69,8 @@ LL | drop(**p);
7069
|
7170
help: consider cloning the value if the performance cost is acceptable
7271
|
73-
LL - let p = &x.b;
74-
LL + let p = x.b.clone();
75-
|
72+
LL | let p = &x.b.clone();
73+
| ++++++++
7674

7775
error[E0499]: cannot borrow `x.a` as mutable more than once at a time
7876
--> $DIR/borrowck-field-sensitivity.rs:48:13

‎tests/ui/borrowck/borrowck-issue-48962.stderr

-5
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,6 @@ LL | {src};
1717
| --- value moved here
1818
LL | src.0 = 66;
1919
| ^^^^^^^^^^ value used here after move
20-
|
21-
help: consider cloning the value if the performance cost is acceptable
22-
|
23-
LL | {src.clone()};
24-
| ++++++++
2520

2621
error: aborting due to 2 previous errors
2722

‎tests/ui/borrowck/borrowck-loan-blocks-move-cc.stderr

+4-6
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,8 @@ LL | w.use_ref();
1616
|
1717
help: consider cloning the value if the performance cost is acceptable
1818
|
19-
LL - let w = &v;
20-
LL + let w = v.clone();
21-
|
19+
LL | let w = &v.clone();
20+
| ++++++++
2221

2322
error[E0505]: cannot move out of `v` because it is borrowed
2423
--> $DIR/borrowck-loan-blocks-move-cc.rs:24:19
@@ -38,9 +37,8 @@ LL | w.use_ref();
3837
|
3938
help: consider cloning the value if the performance cost is acceptable
4039
|
41-
LL - let w = &v;
42-
LL + let w = v.clone();
43-
|
40+
LL | let w = &v.clone();
41+
| ++++++++
4442

4543
error: aborting due to 2 previous errors
4644

‎tests/ui/borrowck/borrowck-loan-blocks-move.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,8 @@ LL | w.use_ref();
1212
|
1313
help: consider cloning the value if the performance cost is acceptable
1414
|
15-
LL - let w = &v;
16-
LL + let w = v.clone();
17-
|
15+
LL | let w = &v.clone();
16+
| ++++++++
1817

1918
error: aborting due to 1 previous error
2019

‎tests/ui/borrowck/borrowck-move-from-subpath-of-borrowed-path.stderr

+2-3
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,8 @@ LL | b.use_ref();
1313
|
1414
help: consider cloning the value if the performance cost is acceptable
1515
|
16-
LL - let b = &a;
17-
LL + let b = a.clone();
18-
|
16+
LL | let b = &a.clone();
17+
| ++++++++
1918

2019
error: aborting due to 1 previous error
2120

‎tests/ui/borrowck/borrowck-move-mut-base-ptr.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ LL | p.use_ref();
1414
help: consider cloning the value if the performance cost is acceptable
1515
|
1616
LL - let p: &isize = &*t0; // Freezes `*t0`
17-
LL + let p: &isize = t0.clone(); // Freezes `*t0`
17+
LL + let p: &isize = &t0.clone(); // Freezes `*t0`
1818
|
1919

2020
error: aborting due to 1 previous error

0 commit comments

Comments
 (0)