Skip to content

Commit 4de0d24

Browse files
qaijuangqaijuang
committed
borrowck: avoid misleading return-type note for foreign Fn callees
1 parent 4933094 commit 4de0d24

4 files changed

Lines changed: 212 additions & 42 deletions

File tree

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

Lines changed: 65 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ use either::Either;
44
use hir::{ExprKind, Param};
55
use rustc_abi::FieldIdx;
66
use rustc_errors::{Applicability, Diag};
7+
use rustc_hir::def_id::DefId;
78
use rustc_hir::intravisit::Visitor;
89
use rustc_hir::{self as hir, BindingMode, ByRef, Node};
910
use rustc_middle::bug;
@@ -1092,42 +1093,65 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
10921093
let mut look_at_return = true;
10931094

10941095
err.span_label(closure_span, "in this closure");
1095-
// If the HIR node is a function or method call, get the DefId
1096-
// of the callee function or method, the span, and args of the call expr
1097-
let get_call_details = || {
1098-
let hir::Node::Expr(hir::Expr { hir_id, kind, .. }) = node else {
1099-
return None;
1096+
let closure_arg_has_fn_trait_bound =
1097+
|callee_def_id, input_index, generic_args: ty::GenericArgsRef<'tcx>| {
1098+
let sig = tcx.fn_sig(callee_def_id).instantiate(tcx, generic_args).skip_binder();
1099+
let Some(input_ty): Option<Ty<'tcx>> = sig.inputs().get(input_index).copied()
1100+
else {
1101+
return false;
1102+
};
1103+
1104+
tcx.predicates_of(callee_def_id)
1105+
.instantiate(tcx, generic_args)
1106+
.predicates
1107+
.iter()
1108+
.any(|predicate| {
1109+
predicate.as_trait_clause().is_some_and(|trait_pred| {
1110+
trait_pred.polarity() == ty::PredicatePolarity::Positive
1111+
&& tcx.fn_trait_kind_from_def_id(trait_pred.def_id())
1112+
== Some(ty::ClosureKind::Fn)
1113+
&& trait_pred.self_ty().skip_binder().peel_refs()
1114+
== input_ty.peel_refs()
1115+
})
1116+
})
11001117
};
11011118

1102-
let typeck_results = tcx.typeck(def_id);
1119+
// If the HIR node is a function or method call, get the DefId
1120+
// of the callee function or method, the span, and argument info for the call expr.
1121+
let get_call_details =
1122+
|| -> Option<(DefId, Span, usize, usize, ty::GenericArgsRef<'tcx>)> {
1123+
let hir::Node::Expr(hir::Expr { hir_id, kind, .. }) = node else {
1124+
return None;
1125+
};
11031126

1104-
match kind {
1105-
hir::ExprKind::Call(expr, args) => {
1106-
if let Some(ty::FnDef(def_id, _)) =
1107-
typeck_results.node_type_opt(expr.hir_id).as_ref().map(|ty| ty.kind())
1108-
{
1109-
Some((*def_id, expr.span, *args))
1110-
} else {
1111-
None
1127+
let typeck_results = tcx.typeck(def_id);
1128+
1129+
match kind {
1130+
hir::ExprKind::Call(expr, args) => {
1131+
if let Some(ty::FnDef(def_id, generic_args)) =
1132+
typeck_results.node_type_opt(expr.hir_id).as_ref().map(|ty| ty.kind())
1133+
{
1134+
let arg_pos = args.iter().position(|arg| arg.hir_id == closure_id)?;
1135+
Some((*def_id, expr.span, arg_pos, arg_pos, generic_args))
1136+
} else {
1137+
None
1138+
}
11121139
}
1140+
hir::ExprKind::MethodCall(_, _, args, span) => {
1141+
let arg_pos = args.iter().position(|arg| arg.hir_id == closure_id)?;
1142+
let def_id = typeck_results.type_dependent_def_id(*hir_id)?;
1143+
let generic_args = typeck_results.node_args_opt(*hir_id)?;
1144+
Some((def_id, *span, arg_pos, arg_pos + 1, generic_args))
1145+
}
1146+
_ => None,
11131147
}
1114-
hir::ExprKind::MethodCall(_, _, args, span) => typeck_results
1115-
.type_dependent_def_id(*hir_id)
1116-
.map(|def_id| (def_id, *span, *args)),
1117-
_ => None,
1118-
}
1119-
};
1148+
};
11201149

11211150
// If we can detect the expression to be a function or method call where the closure was
11221151
// an argument, we point at the function or method definition argument...
1123-
if let Some((callee_def_id, call_span, call_args)) = get_call_details() {
1124-
let arg_pos = call_args
1125-
.iter()
1126-
.enumerate()
1127-
.filter(|(_, arg)| arg.hir_id == closure_id)
1128-
.map(|(pos, _)| pos)
1129-
.next();
1130-
1152+
if let Some((callee_def_id, call_span, arg_pos, input_index, generic_args)) =
1153+
get_call_details()
1154+
{
11311155
let arg = match tcx.hir_get_if_local(callee_def_id) {
11321156
Some(
11331157
hir::Node::Item(hir::Item {
@@ -1144,16 +1168,12 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
11441168
..
11451169
}),
11461170
) => Some(
1147-
arg_pos
1148-
.and_then(|pos| {
1149-
sig.decl.inputs.get(
1150-
pos + if sig.decl.implicit_self().has_implicit_self() {
1151-
1
1152-
} else {
1153-
0
1154-
},
1155-
)
1156-
})
1171+
sig.decl
1172+
.inputs
1173+
.get(
1174+
arg_pos
1175+
+ if sig.decl.implicit_self().has_implicit_self() { 1 } else { 0 },
1176+
)
11571177
.map(|arg| arg.span)
11581178
.unwrap_or(ident.span),
11591179
),
@@ -1163,6 +1183,13 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
11631183
err.span_label(span, "change this to accept `FnMut` instead of `Fn`");
11641184
err.span_label(call_span, "expects `Fn` instead of `FnMut`");
11651185
look_at_return = false;
1186+
} else if closure_arg_has_fn_trait_bound(callee_def_id, input_index, generic_args) {
1187+
// The callee is not local, so we cannot point at its argument declaration, but we
1188+
// can still explain that this call site expects an `Fn` closure. Avoid falling
1189+
// through to the enclosing function's return type, which is misleading in cases
1190+
// like `flat_map(|_| external::map(|_| ...))`.
1191+
err.span_label(call_span, "expects `Fn` instead of `FnMut`");
1192+
look_at_return = false;
11661193
}
11671194
}
11681195

‎tests/ui/closures/auxiliary/wrong-closure-arg-suggestion-aux.rs‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
pub struct P;
2+
pub struct Map<F>(pub F);
3+
4+
pub trait PIter: Sized {
5+
fn map<F>(self, f: F) -> Map<F>
6+
where
7+
F: Fn(i32);
8+
9+
fn flat_map<F, U>(self, f: F)
10+
where
11+
F: Fn(i32) -> U;
12+
}
13+
14+
impl PIter for P {
15+
fn map<F>(self, f: F) -> Map<F>
16+
where
17+
F: Fn(i32),
18+
{
19+
Map(f)
20+
}
21+
22+
fn flat_map<F, U>(self, _: F)
23+
where
24+
F: Fn(i32) -> U,
25+
{
26+
}
27+
}
28+
29+
pub fn to_fn<F>(f: F) -> Map<F>
30+
where
31+
F: Fn(i32),
32+
{
33+
Map(f)
34+
}

‎tests/ui/closures/wrong-closure-arg-suggestion-125325.rs‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
//@ aux-build:wrong-closure-arg-suggestion-aux.rs
2+
13
// Regression test for #125325
24

35
// Tests that we suggest changing an `impl Fn` param
@@ -6,6 +8,10 @@
68
// Ensures that it works that way for both
79
// functions and methods
810

11+
extern crate wrong_closure_arg_suggestion_aux as aux;
12+
13+
use aux::{P, PIter, to_fn};
14+
915
struct S;
1016

1117
impl S {
@@ -26,4 +32,37 @@ fn test_func(s: &S) -> usize {
2632
//~^ ERROR cannot assign to `x`, as it is a captured variable in a `Fn` closure
2733
}
2834

35+
// Regression test for <https://github.com/rust-lang/rust/issues/155727>.
36+
//
37+
// When the relevant `Fn` bound comes from a non-local callee, we should still
38+
// explain the call-site expectation instead of falling back to the enclosing
39+
// function's return type.
40+
struct Counter {
41+
counter: i32,
42+
}
43+
44+
impl Counter {
45+
fn external_fn(mut self) -> i32 {
46+
take(|_| to_fn(|_| self.counter += 1));
47+
//~^ ERROR cannot assign to `self.counter`, as `Fn` closures cannot mutate their captured variables [E0594]
48+
//~| ERROR lifetime may not live long enough
49+
//~| ERROR cannot borrow `self` as mutable, as it is a captured variable in a `Fn` closure [E0596]
50+
self.counter
51+
}
52+
53+
fn external_method(mut self) -> i32 {
54+
P.flat_map(|_| P.map(|_| self.counter += 1));
55+
//~^ ERROR cannot assign to `self.counter`, as `Fn` closures cannot mutate their captured variables [E0594]
56+
//~| ERROR lifetime may not live long enough
57+
//~| ERROR cannot borrow `self` as mutable, as it is a captured variable in a `Fn` closure [E0596]
58+
self.counter
59+
}
60+
}
61+
62+
fn take<F, U>(_: F)
63+
where
64+
F: Fn(i32) -> U,
65+
{
66+
}
67+
2968
fn main() {}

‎tests/ui/closures/wrong-closure-arg-suggestion-125325.stderr‎

Lines changed: 74 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
2-
--> $DIR/wrong-closure-arg-suggestion-125325.rs:23:21
2+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:29:21
33
|
44
LL | fn assoc_func(&self, _f: impl Fn()) -> usize {
55
| --------- change this to accept `FnMut` instead of `Fn`
@@ -14,7 +14,7 @@ LL | s.assoc_func(|| x = ());
1414
| expects `Fn` instead of `FnMut`
1515

1616
error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
17-
--> $DIR/wrong-closure-arg-suggestion-125325.rs:25:13
17+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:31:13
1818
|
1919
LL | fn func(_f: impl Fn()) -> usize {
2020
| --------- change this to accept `FnMut` instead of `Fn`
@@ -28,6 +28,76 @@ LL | func(|| x = ())
2828
| | in this closure
2929
| expects `Fn` instead of `FnMut`
3030

31-
error: aborting due to 2 previous errors
31+
error[E0594]: cannot assign to `self.counter`, as `Fn` closures cannot mutate their captured variables
32+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:46:28
33+
|
34+
LL | take(|_| to_fn(|_| self.counter += 1));
35+
| ----- --- ^^^^^^^^^^^^^^^^^ cannot assign
36+
| | |
37+
| | in this closure
38+
| expects `Fn` instead of `FnMut`
39+
40+
error: lifetime may not live long enough
41+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:46:18
42+
|
43+
LL | take(|_| to_fn(|_| self.counter += 1));
44+
| --- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
45+
| | |
46+
| | return type of closure `aux::Map<{closure@$DIR/wrong-closure-arg-suggestion-125325.rs:46:24: 46:27}>` contains a lifetime `'2`
47+
| lifetime `'1` represents this closure's body
48+
|
49+
= note: closure implements `Fn`, so references to captured variables can't escape the closure
50+
51+
error[E0596]: cannot borrow `self` as mutable, as it is a captured variable in a `Fn` closure
52+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:46:24
53+
|
54+
LL | take(|_| to_fn(|_| self.counter += 1));
55+
| ---- --- ^^^ ------------ mutable borrow occurs due to use of `self` in closure
56+
| | | |
57+
| | | cannot borrow as mutable
58+
| | in this closure
59+
| expects `Fn` instead of `FnMut`
60+
...
61+
LL | fn take<F, U>(_: F)
62+
| - change this to accept `FnMut` instead of `Fn`
63+
64+
error[E0594]: cannot assign to `self.counter`, as `Fn` closures cannot mutate their captured variables
65+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:54:34
66+
|
67+
LL | P.flat_map(|_| P.map(|_| self.counter += 1));
68+
| --------^^^^^^^^^^^^^^^^^-
69+
| | | |
70+
| | | cannot assign
71+
| | in this closure
72+
| expects `Fn` instead of `FnMut`
73+
74+
error: lifetime may not live long enough
75+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:54:24
76+
|
77+
LL | P.flat_map(|_| P.map(|_| self.counter += 1));
78+
| --- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
79+
| | |
80+
| | return type of closure `aux::Map<{closure@$DIR/wrong-closure-arg-suggestion-125325.rs:54:30: 54:33}>` contains a lifetime `'2`
81+
| lifetime `'1` represents this closure's body
82+
|
83+
= note: closure implements `Fn`, so references to captured variables can't escape the closure
84+
help: consider adding 'move' keyword before the nested closure
85+
|
86+
LL | P.flat_map(|_| P.map(move |_| self.counter += 1));
87+
| ++++
88+
89+
error[E0596]: cannot borrow `self` as mutable, as it is a captured variable in a `Fn` closure
90+
--> $DIR/wrong-closure-arg-suggestion-125325.rs:54:30
91+
|
92+
LL | P.flat_map(|_| P.map(|_| self.counter += 1));
93+
| -------------------^^^--------------------
94+
| | | | |
95+
| | | | mutable borrow occurs due to use of `self` in closure
96+
| | | cannot borrow as mutable
97+
| | in this closure
98+
| expects `Fn` instead of `FnMut`
99+
100+
error: aborting due to 8 previous errors
32101

33-
For more information about this error, try `rustc --explain E0594`.
102+
Some errors have detailed explanations: E0594, E0596.
103+
For more information about an error, try `rustc --explain E0594`.

0 commit comments

Comments
 (0)