Skip to content

Commit 7dfc3e9

Browse files
estebankestebank
committed
Rework borrowing suggestions to use Expr instead of just Span
In the suggestion machinery for borrowing expressions and types, always use the available obligation `Span` to find the appropriate `Expr` to perform appropriateness checks no the `ExprKind` instead of on the textual snippet corresponding to the `Span`. Unify the logic for the case where `&` *and* `&mut` are appropriate with the logic for only one of those cases. Handle the case when `S::foo()` should have been `<&S>::foo()` (instead of suggesting the prior `&S::foo()`.
1 parent 78a6e13 commit 7dfc3e9

16 files changed

+217
-213
lines changed

‎compiler/rustc_trait_selection/src/error_reporting/traits/suggestions.rs

Lines changed: 125 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1321,7 +1321,7 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
13211321
let imm_ref_self_ty_satisfies_pred = mk_result(trait_pred_and_imm_ref);
13221322
let mut_ref_self_ty_satisfies_pred = mk_result(trait_pred_and_mut_ref);
13231323

1324-
let (ref_inner_ty_satisfies_pred, ref_inner_ty_mut) =
1324+
let (ref_inner_ty_satisfies_pred, ref_inner_ty_is_mut) =
13251325
if let ObligationCauseCode::WhereClauseInExpr(..) = obligation.cause.code()
13261326
&& let ty::Ref(_, ty, mutability) = old_pred.self_ty().skip_binder().kind()
13271327
{
@@ -1333,117 +1333,139 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
13331333
(false, false)
13341334
};
13351335

1336-
if imm_ref_self_ty_satisfies_pred
1337-
|| mut_ref_self_ty_satisfies_pred
1338-
|| ref_inner_ty_satisfies_pred
1339-
{
1340-
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) {
1341-
// We don't want a borrowing suggestion on the fields in structs,
1342-
// ```
1343-
// struct Foo {
1344-
// the_foos: Vec<Foo>
1345-
// }
1346-
// ```
1347-
if !matches!(
1348-
span.ctxt().outer_expn_data().kind,
1349-
ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop)
1350-
) {
1351-
return false;
1352-
}
1353-
if snippet.starts_with('&') {
1354-
// This is already a literal borrow and the obligation is failing
1355-
// somewhere else in the obligation chain. Do not suggest non-sense.
1356-
return false;
1357-
}
1358-
// We have a very specific type of error, where just borrowing this argument
1359-
// might solve the problem. In cases like this, the important part is the
1360-
// original type obligation, not the last one that failed, which is arbitrary.
1361-
// Because of this, we modify the error to refer to the original obligation and
1362-
// return early in the caller.
1363-
1364-
let msg = format!(
1365-
"the trait bound `{}` is not satisfied",
1366-
self.tcx.short_string(old_pred, err.long_ty_path()),
1367-
);
1368-
let self_ty_str =
1369-
self.tcx.short_string(old_pred.self_ty().skip_binder(), err.long_ty_path());
1370-
if has_custom_message {
1371-
err.note(msg);
1372-
} else {
1373-
err.messages = vec![(rustc_errors::DiagMessage::from(msg), Style::NoStyle)];
1374-
}
1375-
err.span_label(
1376-
span,
1377-
format!(
1378-
"the trait `{}` is not implemented for `{self_ty_str}`",
1379-
old_pred.print_modifiers_and_trait_path()
1380-
),
1381-
);
1336+
let is_immut = imm_ref_self_ty_satisfies_pred
1337+
|| (ref_inner_ty_satisfies_pred && !ref_inner_ty_is_mut);
1338+
let is_mut = mut_ref_self_ty_satisfies_pred || ref_inner_ty_is_mut;
1339+
if !is_immut && !is_mut {
1340+
return false;
1341+
}
1342+
let Ok(_snippet) = self.tcx.sess.source_map().span_to_snippet(span) else {
1343+
return false;
1344+
};
1345+
// We don't want a borrowing suggestion on the fields in structs
1346+
// ```
1347+
// #[derive(Clone)]
1348+
// struct Foo {
1349+
// the_foos: Vec<Foo>
1350+
// }
1351+
// ```
1352+
if !matches!(
1353+
span.ctxt().outer_expn_data().kind,
1354+
ExpnKind::Root | ExpnKind::Desugaring(DesugaringKind::ForLoop)
1355+
) {
1356+
return false;
1357+
}
1358+
// We have a very specific type of error, where just borrowing this argument
1359+
// might solve the problem. In cases like this, the important part is the
1360+
// original type obligation, not the last one that failed, which is arbitrary.
1361+
// Because of this, we modify the error to refer to the original obligation and
1362+
// return early in the caller.
13821363

1383-
if imm_ref_self_ty_satisfies_pred && mut_ref_self_ty_satisfies_pred {
1384-
err.span_suggestions(
1385-
span.shrink_to_lo(),
1386-
"consider borrowing here",
1387-
["&".to_string(), "&mut ".to_string()],
1388-
Applicability::MaybeIncorrect,
1389-
);
1390-
} else {
1391-
let is_mut = mut_ref_self_ty_satisfies_pred || ref_inner_ty_mut;
1392-
let sugg_prefix = format!("&{}", if is_mut { "mut " } else { "" });
1393-
let sugg_msg = format!(
1394-
"consider{} borrowing here",
1395-
if is_mut { " mutably" } else { "" }
1396-
);
1364+
let mut label = || {
1365+
let msg = format!(
1366+
"the trait bound `{}` is not satisfied",
1367+
self.tcx.short_string(old_pred, err.long_ty_path()),
1368+
);
1369+
let self_ty_str =
1370+
self.tcx.short_string(old_pred.self_ty().skip_binder(), err.long_ty_path());
1371+
if has_custom_message {
1372+
err.note(msg);
1373+
} else {
1374+
err.messages = vec![(rustc_errors::DiagMessage::from(msg), Style::NoStyle)];
1375+
}
1376+
err.span_label(
1377+
span,
1378+
format!(
1379+
"the trait `{}` is not implemented for `{self_ty_str}`",
1380+
old_pred.print_modifiers_and_trait_path()
1381+
),
1382+
);
1383+
};
13971384

1398-
// Issue #109436, we need to add parentheses properly for method calls
1399-
// for example, `foo.into()` should be `(&foo).into()`
1400-
if let Some(_) =
1401-
self.tcx.sess.source_map().span_look_ahead(span, ".", Some(50))
1402-
{
1403-
err.multipart_suggestion_verbose(
1404-
sugg_msg,
1405-
vec![
1406-
(span.shrink_to_lo(), format!("({sugg_prefix}")),
1407-
(span.shrink_to_hi(), ")".to_string()),
1408-
],
1409-
Applicability::MaybeIncorrect,
1410-
);
1411-
return true;
1412-
}
1385+
let mut sugg_prefixes = vec![];
1386+
if is_immut {
1387+
sugg_prefixes.push("&");
1388+
}
1389+
if is_mut {
1390+
sugg_prefixes.push("&mut ");
1391+
}
1392+
let sugg_msg = format!(
1393+
"consider{} borrowing here",
1394+
if is_mut && !is_immut { " mutably" } else { "" },
1395+
);
14131396

1414-
// Issue #104961, we need to add parentheses properly for compound expressions
1415-
// for example, `x.starts_with("hi".to_string() + "you")`
1416-
// should be `x.starts_with(&("hi".to_string() + "you"))`
1417-
let Some(body) = self.tcx.hir_maybe_body_owned_by(obligation.cause.body_id)
1418-
else {
1419-
return false;
1420-
};
1421-
let mut expr_finder = FindExprBySpan::new(span, self.tcx);
1422-
expr_finder.visit_expr(body.value);
1423-
let Some(expr) = expr_finder.result else {
1424-
return false;
1425-
};
1426-
let needs_parens = expr_needs_parens(expr);
1397+
// Issue #104961, we need to add parentheses properly for compound expressions
1398+
// for example, `x.starts_with("hi".to_string() + "you")`
1399+
// should be `x.starts_with(&("hi".to_string() + "you"))`
1400+
let Some(body) = self.tcx.hir_maybe_body_owned_by(obligation.cause.body_id) else {
1401+
return false;
1402+
};
1403+
let mut expr_finder = FindExprBySpan::new(span, self.tcx);
1404+
expr_finder.visit_expr(body.value);
14271405

1428-
let span = if needs_parens { span } else { span.shrink_to_lo() };
1429-
let suggestions = if !needs_parens {
1430-
vec![(span.shrink_to_lo(), sugg_prefix)]
1431-
} else {
1406+
if let Some(ty) = expr_finder.ty_result {
1407+
if let hir::Node::Expr(expr) = self.tcx.parent_hir_node(ty.hir_id)
1408+
&& let hir::ExprKind::Path(hir::QPath::TypeRelative(_, _)) = expr.kind
1409+
&& ty.span == span
1410+
{
1411+
// We've encountered something like `str::from("")`, where the intended code
1412+
// was likely `<&str>::from("")`. #143393.
1413+
label();
1414+
err.multipart_suggestions(
1415+
sugg_msg,
1416+
sugg_prefixes.into_iter().map(|sugg_prefix| {
14321417
vec![
1433-
(span.shrink_to_lo(), format!("{sugg_prefix}(")),
1434-
(span.shrink_to_hi(), ")".to_string()),
1418+
(span.shrink_to_lo(), format!("<{sugg_prefix}")),
1419+
(span.shrink_to_hi(), ">".to_string()),
14351420
]
1436-
};
1437-
err.multipart_suggestion_verbose(
1438-
sugg_msg,
1439-
suggestions,
1440-
Applicability::MaybeIncorrect,
1441-
);
1442-
}
1421+
}),
1422+
Applicability::MaybeIncorrect,
1423+
);
14431424
return true;
14441425
}
1426+
return false;
14451427
}
1446-
return false;
1428+
let Some(expr) = expr_finder.result else {
1429+
return false;
1430+
};
1431+
if let hir::ExprKind::AddrOf(_, _, _) = expr.kind {
1432+
return false;
1433+
}
1434+
let needs_parens_post = expr_needs_parens(expr);
1435+
let needs_parens_pre = match self.tcx.parent_hir_node(expr.hir_id) {
1436+
Node::Expr(e)
1437+
if let hir::ExprKind::MethodCall(_, base, _, _) = e.kind
1438+
&& base.hir_id == expr.hir_id =>
1439+
{
1440+
true
1441+
}
1442+
_ => false,
1443+
};
1444+
1445+
label();
1446+
let suggestions = sugg_prefixes.into_iter().map(|sugg_prefix| {
1447+
match (needs_parens_pre, needs_parens_post) {
1448+
(false, false) => vec![(span.shrink_to_lo(), sugg_prefix.to_string())],
1449+
// We have something like `foo.bar()`, where we want to bororw foo, so we need
1450+
// to suggest `(&mut foo).bar()`.
1451+
(false, true) => vec![
1452+
(span.shrink_to_lo(), format!("{sugg_prefix}(")),
1453+
(span.shrink_to_hi(), ")".to_string()),
1454+
],
1455+
// Issue #109436, we need to add parentheses properly for method calls
1456+
// for example, `foo.into()` should be `(&foo).into()`
1457+
(true, false) => vec![
1458+
(span.shrink_to_lo(), format!("({sugg_prefix}")),
1459+
(span.shrink_to_hi(), ")".to_string()),
1460+
],
1461+
(true, true) => vec![
1462+
(span.shrink_to_lo(), format!("({sugg_prefix}(")),
1463+
(span.shrink_to_hi(), "))".to_string()),
1464+
],
1465+
}
1466+
});
1467+
err.multipart_suggestions(sugg_msg, suggestions, Applicability::MaybeIncorrect);
1468+
return true;
14471469
};
14481470

14491471
if let ObligationCauseCode::ImplDerived(cause) = &*code {

‎tests/ui/consts/const-size_of_val-align_of_val-extern-type.stderr

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,25 +2,37 @@ error[E0277]: the size for values of type `Opaque` cannot be known
22
--> $DIR/const-size_of_val-align_of_val-extern-type.rs:10:43
33
|
44
LL | const _SIZE: usize = unsafe { size_of_val(&4 as *const i32 as *const Opaque) };
5-
| ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a known size
5+
| ----------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `MetaSized` is not implemented for `Opaque`
66
| |
77
| required by a bound introduced by this call
88
|
9-
= help: the trait `MetaSized` is not implemented for `Opaque`
9+
= note: the trait bound `Opaque: MetaSized` is not satisfied
1010
note: required by a bound in `std::intrinsics::size_of_val`
1111
--> $SRC_DIR/core/src/intrinsics/mod.rs:LL:COL
12+
help: consider borrowing here
13+
|
14+
LL | const _SIZE: usize = unsafe { size_of_val(&(&4 as *const i32 as *const Opaque)) };
15+
| ++ +
16+
LL | const _SIZE: usize = unsafe { size_of_val(&mut (&4 as *const i32 as *const Opaque)) };
17+
| ++++++ +
1218

1319
error[E0277]: the size for values of type `Opaque` cannot be known
1420
--> $DIR/const-size_of_val-align_of_val-extern-type.rs:12:45
1521
|
1622
LL | const _ALIGN: usize = unsafe { align_of_val(&4 as *const i32 as *const Opaque) };
17-
| ------------ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a known size
23+
| ------------ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `MetaSized` is not implemented for `Opaque`
1824
| |
1925
| required by a bound introduced by this call
2026
|
21-
= help: the trait `MetaSized` is not implemented for `Opaque`
27+
= note: the trait bound `Opaque: MetaSized` is not satisfied
2228
note: required by a bound in `std::intrinsics::align_of_val`
2329
--> $SRC_DIR/core/src/intrinsics/mod.rs:LL:COL
30+
help: consider borrowing here
31+
|
32+
LL | const _ALIGN: usize = unsafe { align_of_val(&(&4 as *const i32 as *const Opaque)) };
33+
| ++ +
34+
LL | const _ALIGN: usize = unsafe { align_of_val(&mut (&4 as *const i32 as *const Opaque)) };
35+
| ++++++ +
2436

2537
error: aborting due to 2 previous errors
2638

‎tests/ui/derives/deriving-copyclone.stderr

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@ error[E0277]: the trait bound `B<C>: Copy` is not satisfied
22
--> $DIR/deriving-copyclone.rs:31:26
33
|
44
LL | is_copy(B { a: 1, b: C });
5-
| ------- ^ the trait `Copy` is not implemented for `B<C>`
6-
| |
5+
| ------- ^
6+
| | |
7+
| | the trait `Copy` is not implemented for `B<C>`
8+
| | help: consider borrowing here: `&`
79
| required by a bound introduced by this call
810
|
911
note: required for `B<C>` to implement `Copy`
@@ -16,17 +18,15 @@ note: required by a bound in `is_copy`
1618
|
1719
LL | fn is_copy<T: Copy>(_: T) {}
1820
| ^^^^ required by this bound in `is_copy`
19-
help: consider borrowing here
20-
|
21-
LL | is_copy(B { a: 1, b: &C });
22-
| +
2321

2422
error[E0277]: the trait bound `B<C>: Clone` is not satisfied
2523
--> $DIR/deriving-copyclone.rs:32:27
2624
|
2725
LL | is_clone(B { a: 1, b: C });
28-
| -------- ^ the trait `Clone` is not implemented for `B<C>`
29-
| |
26+
| -------- ^
27+
| | |
28+
| | the trait `Clone` is not implemented for `B<C>`
29+
| | help: consider borrowing here: `&`
3030
| required by a bound introduced by this call
3131
|
3232
note: required for `B<C>` to implement `Clone`
@@ -39,17 +39,15 @@ note: required by a bound in `is_clone`
3939
|
4040
LL | fn is_clone<T: Clone>(_: T) {}
4141
| ^^^^^ required by this bound in `is_clone`
42-
help: consider borrowing here
43-
|
44-
LL | is_clone(B { a: 1, b: &C });
45-
| +
4642

4743
error[E0277]: the trait bound `B<D>: Copy` is not satisfied
4844
--> $DIR/deriving-copyclone.rs:35:26
4945
|
5046
LL | is_copy(B { a: 1, b: D });
51-
| ------- ^ the trait `Copy` is not implemented for `B<D>`
52-
| |
47+
| ------- ^
48+
| | |
49+
| | the trait `Copy` is not implemented for `B<D>`
50+
| | help: consider borrowing here: `&`
5351
| required by a bound introduced by this call
5452
|
5553
note: required for `B<D>` to implement `Copy`
@@ -62,10 +60,6 @@ note: required by a bound in `is_copy`
6260
|
6361
LL | fn is_copy<T: Copy>(_: T) {}
6462
| ^^^^ required by this bound in `is_copy`
65-
help: consider borrowing here
66-
|
67-
LL | is_copy(B { a: 1, b: &D });
68-
| +
6963

7064
error: aborting due to 3 previous errors
7165

‎tests/ui/extern/unsized-extern-derefmove.stderr

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,10 @@ note: required by a bound in `Box::<T>::from_raw`
2121
--> $SRC_DIR/alloc/src/boxed.rs:LL:COL
2222
help: consider borrowing here
2323
|
24-
LL | Box::from_raw(&0 as *mut _)
25-
| +
26-
LL | Box::from_raw(&mut 0 as *mut _)
27-
| ++++
24+
LL | Box::from_raw(&(0 as *mut _))
25+
| ++ +
26+
LL | Box::from_raw(&mut (0 as *mut _))
27+
| ++++++ +
2828

2929
error[E0277]: the size for values of type `Device` cannot be known
3030
--> $DIR/unsized-extern-derefmove.rs:11:5

‎tests/ui/for/issue-20605.current.stderr

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,13 @@ error[E0277]: `dyn Iterator<Item = &'a mut u8>` is not an iterator
22
--> $DIR/issue-20605.rs:6:17
33
|
44
LL | for item in *things { *item = 0 }
5-
| ^^^^^^^ the trait `IntoIterator` is not implemented for `dyn Iterator<Item = &'a mut u8>`
5+
| -^^^^^^
6+
| |
7+
| the trait `IntoIterator` is not implemented for `dyn Iterator<Item = &'a mut u8>`
8+
| help: consider mutably borrowing here: `&mut`
69
|
710
= note: the trait bound `dyn Iterator<Item = &'a mut u8>: IntoIterator` is not satisfied
811
= note: required for `dyn Iterator<Item = &'a mut u8>` to implement `IntoIterator`
9-
help: consider mutably borrowing here
10-
|
11-
LL | for item in &mut *things { *item = 0 }
12-
| ++++
1312

1413
error: aborting due to 1 previous error
1514

0 commit comments

Comments
 (0)