Skip to content

Commit 52f7d33

Browse files
matthiaskrgrmatthiaskrgr
authored
Rollup merge of #128244 - compiler-errors:move-clone-sugg, r=estebank
Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck, remove some hacks Also remove a heck of a lot of weird hacks in `suggest_cloning` that I don't think we should have around. I know this regresses tests, but I don't believe most of these suggestions were accurate, b/c: 1. They either produced type errors (e.g. turning `&x` into `x.clone()`) 2. They don't fix the issue 3. They fix the issue ostensibly, but introduce logic errors (e.g. cloning a `&mut Option<T>` to then `Option::take` out...) Most of the suggestions are still wrong, but they're not particularly *less* wrong IMO. Stacked on top of #128241, which is an "obviously worth landing" subset of this PR. r? estebank
2 parents 19f6ff0 + 91acacf commit 52f7d33

File tree

45 files changed

+111
-351
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

+111
-351
lines changed

Diff for: ‎compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

+24-68
Original file line numberDiff line numberDiff line change
@@ -563,11 +563,11 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
563563
} = move_spans
564564
&& can_suggest_clone
565565
{
566-
self.suggest_cloning(err, ty, expr, None, Some(move_spans));
566+
self.suggest_cloning(err, ty, expr, Some(move_spans));
567567
} else if self.suggest_hoisting_call_outside_loop(err, expr) && can_suggest_clone {
568568
// The place where the type moves would be misleading to suggest clone.
569569
// #121466
570-
self.suggest_cloning(err, ty, expr, None, Some(move_spans));
570+
self.suggest_cloning(err, ty, expr, Some(move_spans));
571571
}
572572
}
573573

@@ -1229,8 +1229,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
12291229
&self,
12301230
err: &mut Diag<'_>,
12311231
ty: Ty<'tcx>,
1232-
mut expr: &'tcx hir::Expr<'tcx>,
1233-
mut other_expr: Option<&'tcx hir::Expr<'tcx>>,
1232+
expr: &'tcx hir::Expr<'tcx>,
12341233
use_spans: Option<UseSpans<'tcx>>,
12351234
) {
12361235
if let hir::ExprKind::Struct(_, _, Some(_)) = expr.kind {
@@ -1242,66 +1241,6 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
12421241
return;
12431242
}
12441243

1245-
if let Some(some_other_expr) = other_expr
1246-
&& let Some(parent_binop) =
1247-
self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
1248-
if let (hir_id, hir::Node::Expr(e)) = n
1249-
&& let hir::ExprKind::AssignOp(_binop, target, _arg) = e.kind
1250-
&& target.hir_id == expr.hir_id
1251-
{
1252-
Some(hir_id)
1253-
} else {
1254-
None
1255-
}
1256-
})
1257-
&& let Some(other_parent_binop) =
1258-
self.infcx.tcx.hir().parent_iter(some_other_expr.hir_id).find_map(|n| {
1259-
if let (hir_id, hir::Node::Expr(expr)) = n
1260-
&& let hir::ExprKind::AssignOp(..) = expr.kind
1261-
{
1262-
Some(hir_id)
1263-
} else {
1264-
None
1265-
}
1266-
})
1267-
&& parent_binop == other_parent_binop
1268-
{
1269-
// Explicitly look for `expr += other_expr;` and avoid suggesting
1270-
// `expr.clone() += other_expr;`, instead suggesting `expr += other_expr.clone();`.
1271-
other_expr = Some(expr);
1272-
expr = some_other_expr;
1273-
}
1274-
'outer: {
1275-
if let ty::Ref(..) = ty.kind() {
1276-
// We check for either `let binding = foo(expr, other_expr);` or
1277-
// `foo(expr, other_expr);` and if so we don't suggest an incorrect
1278-
// `foo(expr, other_expr).clone()`
1279-
if let Some(other_expr) = other_expr
1280-
&& let Some(parent_let) =
1281-
self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
1282-
if let (hir_id, hir::Node::LetStmt(_) | hir::Node::Stmt(_)) = n {
1283-
Some(hir_id)
1284-
} else {
1285-
None
1286-
}
1287-
})
1288-
&& let Some(other_parent_let) =
1289-
self.infcx.tcx.hir().parent_iter(other_expr.hir_id).find_map(|n| {
1290-
if let (hir_id, hir::Node::LetStmt(_) | hir::Node::Stmt(_)) = n {
1291-
Some(hir_id)
1292-
} else {
1293-
None
1294-
}
1295-
})
1296-
&& parent_let == other_parent_let
1297-
{
1298-
// Explicitly check that we don't have `foo(&*expr, other_expr)`, as cloning the
1299-
// result of `foo(...)` won't help.
1300-
break 'outer;
1301-
}
1302-
}
1303-
}
1304-
let ty = ty.peel_refs();
13051244
if self.implements_clone(ty) {
13061245
self.suggest_cloning_inner(err, ty, expr);
13071246
} else if let ty::Adt(def, args) = ty.kind()
@@ -1573,10 +1512,27 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
15731512
);
15741513
self.suggest_copy_for_type_in_cloned_ref(&mut err, place);
15751514
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
1576-
if let Some(expr) = self.find_expr(borrow_span)
1577-
&& let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
1578-
{
1579-
self.suggest_cloning(&mut err, ty, expr, self.find_expr(span), Some(move_spans));
1515+
if let Some(expr) = self.find_expr(borrow_span) {
1516+
// This is a borrow span, so we want to suggest cloning the referent.
1517+
if let hir::ExprKind::AddrOf(_, _, borrowed_expr) = expr.kind
1518+
&& let Some(ty) = typeck_results.expr_ty_opt(borrowed_expr)
1519+
{
1520+
self.suggest_cloning(&mut err, ty, borrowed_expr, Some(move_spans));
1521+
} else if typeck_results.expr_adjustments(expr).first().is_some_and(|adj| {
1522+
matches!(
1523+
adj.kind,
1524+
ty::adjustment::Adjust::Borrow(ty::adjustment::AutoBorrow::Ref(
1525+
_,
1526+
ty::adjustment::AutoBorrowMutability::Not
1527+
| ty::adjustment::AutoBorrowMutability::Mut {
1528+
allow_two_phase_borrow: ty::adjustment::AllowTwoPhase::No
1529+
}
1530+
))
1531+
)
1532+
}) && let Some(ty) = typeck_results.expr_ty_opt(expr)
1533+
{
1534+
self.suggest_cloning(&mut err, ty, expr, Some(move_spans));
1535+
}
15801536
}
15811537
self.buffer_error(err);
15821538
}

Diff for: ‎compiler/rustc_borrowck/src/diagnostics/move_errors.rs

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

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

578576
if let Some(expr) = self.find_expr(span) {
579-
self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span), None);
577+
self.suggest_cloning(err, place_ty, expr, None);
580578
}
581579

582580
err.subdiagnostic(crate::session_diagnostics::TypeNoCopy::Label {
@@ -608,13 +606,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
608606
};
609607

610608
if let Some(expr) = self.find_expr(use_span) {
611-
self.suggest_cloning(
612-
err,
613-
place_ty,
614-
expr,
615-
self.find_expr(span),
616-
Some(use_spans),
617-
);
609+
self.suggest_cloning(err, place_ty, expr, Some(use_spans));
618610
}
619611

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

741733
if let Some(expr) = self.find_expr(binding_span) {
742-
self.suggest_cloning(err, bind_to.ty, expr, None, None);
734+
self.suggest_cloning(err, bind_to.ty, expr, None);
743735
}
744736

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

Diff for: ‎tests/ui/associated-types/associated-types-outlives.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ LL | pub fn free_and_use<T: for<'a> Foo<'a>,
1818
| ^ consider constraining this type parameter with `Clone`
1919
...
2020
LL | 's: loop { y = denormalise(&x); break }
21-
| -- you could clone this value
21+
| - you could clone this value
2222

2323
error: aborting due to 1 previous error
2424

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎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

Diff for: ‎tests/ui/borrowck/borrowck-move-subcomponent.stderr

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ LL | struct S {
1717
| ^^^^^^^^ consider implementing `Clone` for this type
1818
...
1919
LL | let pb = &a;
20-
| -- you could clone this value
20+
| - you could clone this value
2121

2222
error: aborting due to 1 previous error
2323

Diff for: ‎tests/ui/borrowck/borrowck-multiple-captures.stderr

+6-9
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,8 @@ LL | borrow(&*p1);
1717
|
1818
help: consider cloning the value if the performance cost is acceptable
1919
|
20-
LL - let p1 = &x1;
21-
LL + let p1 = x1.clone();
22-
|
20+
LL | let p1 = &x1.clone();
21+
| ++++++++
2322

2423
error[E0505]: cannot move out of `x2` because it is borrowed
2524
--> $DIR/borrowck-multiple-captures.rs:12:19
@@ -39,9 +38,8 @@ LL | borrow(&*p2);
3938
|
4039
help: consider cloning the value if the performance cost is acceptable
4140
|
42-
LL - let p2 = &x2;
43-
LL + let p2 = x2.clone();
44-
|
41+
LL | let p2 = &x2.clone();
42+
| ++++++++
4543

4644
error[E0382]: use of moved value: `x1`
4745
--> $DIR/borrowck-multiple-captures.rs:27:19
@@ -108,9 +106,8 @@ LL | borrow(&*p);
108106
|
109107
help: consider cloning the value if the performance cost is acceptable
110108
|
111-
LL - let p = &x;
112-
LL + let p = x.clone();
113-
|
109+
LL | let p = &x.clone();
110+
| ++++++++
114111

115112
error[E0382]: use of moved value: `x`
116113
--> $DIR/borrowck-multiple-captures.rs:52:14

0 commit comments

Comments
 (0)