Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak borrow error on FnMut when Fn is expected #68816

Merged
merged 2 commits into from
Feb 11, 2020
Merged

Tweak borrow error on FnMut when Fn is expected #68816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
109d5c1
Tweak borrow error on `FnMut` when `Fn` is expected
estebank Feb 4, 2020
d51f2bd
review comments
estebank Feb 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 96 additions & 6 deletions src/librustc_mir/borrow_check/diagnostics/mutability_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use rustc_span::Span;
use crate::borrow_check::diagnostics::BorrowedContentSource;
use crate::borrow_check::MirBorrowckCtxt;
use crate::util::collect_writes::FindAssignments;
use rustc_errors::Applicability;
use rustc_errors::{Applicability, DiagnosticBuilder};

#[derive(Copy, Clone, Debug, Eq, PartialEq)]
pub(crate) enum AccessKind {
Expand Down Expand Up @@ -412,11 +412,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
projection: [ProjectionElem::Deref],
// FIXME document what is this 1 magic number about
} if local == Local::new(1) && !self.upvars.is_empty() => {
err.span_label(span, format!("cannot {ACT}", ACT = act));
err.span_help(
self.body.span,
"consider changing this to accept closures that implement `FnMut`",
);
self.expected_fn_found_fn_mut_call(&mut err, span, act);
}

PlaceRef { local: _, projection: [.., ProjectionElem::Deref] } => {
Expand Down Expand Up @@ -448,6 +444,100 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {

err.buffer(&mut self.errors_buffer);
}

/// Targetted error when encountering an `FnMut` closure where an `Fn` closure was expected.
fn expected_fn_found_fn_mut_call(&self, err: &mut DiagnosticBuilder<'_>, sp: Span, act: &str) {
err.span_label(sp, format!("cannot {ACT}", ACT = act));

let hir = self.infcx.tcx.hir();
let closure_id = hir.as_local_hir_id(self.mir_def_id).unwrap();
let fn_call_id = hir.get_parent_node(closure_id);
let node = hir.get(fn_call_id);
let item_id = hir.get_parent_item(fn_call_id);
let mut look_at_return = true;
// If we can detect the expression to be an `fn` call where the closure was an argument,
// we point at the `fn` definition argument...
match node {
hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Call(func, args), .. }) => {
let arg_pos = args
.iter()
.enumerate()
.filter(|(_, arg)| arg.span == self.body.span)
.map(|(pos, _)| pos)
.next();
let def_id = hir.local_def_id(item_id);
let tables = self.infcx.tcx.typeck_tables_of(def_id);
if let Some(ty::FnDef(def_id, _)) =
tables.node_type_opt(func.hir_id).as_ref().map(|ty| &ty.kind)
{
let arg = match hir.get_if_local(*def_id) {
Some(hir::Node::Item(hir::Item {
ident,
kind: hir::ItemKind::Fn(sig, ..),
..
}))
| Some(hir::Node::TraitItem(hir::TraitItem {
ident,
kind: hir::TraitItemKind::Method(sig, _),
..
}))
| Some(hir::Node::ImplItem(hir::ImplItem {
ident,
kind: hir::ImplItemKind::Method(sig, _),
..
})) => Some(
arg_pos
.and_then(|pos| {
sig.decl.inputs.get(
pos + if sig.decl.implicit_self.has_implicit_self() {
1
} else {
0
},
)
})
.map(|arg| arg.span)
.unwrap_or(ident.span),
),
_ => None,
};
if let Some(span) = arg {
err.span_label(span, "change this to accept `FnMut` instead of `Fn`");
err.span_label(func.span, "expects `Fn` instead of `FnMut`");
if self.infcx.tcx.sess.source_map().is_multiline(self.body.span) {
err.span_label(self.body.span, "in this closure");
}
look_at_return = false;
}
}
}
_ => {}
}
if look_at_return && hir.get_return_block(closure_id).is_some() {
// ...otherwise we are probably in the tail expression of the function, point at the
// return type.
match hir.get(hir.get_parent_item(fn_call_id)) {
hir::Node::Item(hir::Item { ident, kind: hir::ItemKind::Fn(sig, ..), .. })
| hir::Node::TraitItem(hir::TraitItem {
ident,
kind: hir::TraitItemKind::Method(sig, _),
..
})
| hir::Node::ImplItem(hir::ImplItem {
ident,
kind: hir::ImplItemKind::Method(sig, _),
..
}) => {
err.span_label(ident.span, "you might have to change this...");
err.span_label(sig.decl.output.span(), "...to return `FnMut` instead of `Fn`");
err.span_label(self.body.span, "in this closure");
}
parent => {
err.note(&format!("parent {:?}", parent));
}
}
}
}
}

fn suggest_ampmut_self<'tcx>(
Expand Down
21 changes: 20 additions & 1 deletion src/test/ui/borrowck/borrow-immutable-upvar-mutation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,10 @@ fn main() {
let _g = to_fn(|| set(&mut y)); //~ ERROR cannot borrow

let mut z = 0;
let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); }); //~ ERROR cannot assign
let _h = to_fn_mut(|| {
set(&mut z);
to_fn(|| z = 42); //~ ERROR cannot assign
});
}

// By-value captures
Expand All @@ -33,3 +36,19 @@ fn main() {
let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); }); //~ ERROR cannot assign
}
}

fn foo() -> Box<dyn Fn() -> usize> {
let mut x = 0;
Box::new(move || {
x += 1; //~ ERROR cannot assign
x
})
}

fn bar() -> impl Fn() -> usize {
let mut x = 0;
move || {
x += 1; //~ ERROR cannot assign
x
}
}
123 changes: 74 additions & 49 deletions src/test/ui/borrowck/borrow-immutable-upvar-mutation.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,76 +1,101 @@
error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:15:27
|
LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | let _f = to_fn(|| x = 42);
| ^^^^^^ cannot assign
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-immutable-upvar-mutation.rs:15:24
|
LL | let _f = to_fn(|| x = 42);
| ^^^^^^^^^
| ----- ^^^^^^ cannot assign
| |
| expects `Fn` instead of `FnMut`

error[E0596]: cannot borrow `y` as mutable, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:18:31
|
LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | let _g = to_fn(|| set(&mut y));
| ^^^^^^ cannot borrow as mutable
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-immutable-upvar-mutation.rs:18:24
|
LL | let _g = to_fn(|| set(&mut y));
| ^^^^^^^^^^^^^^
| ----- ^^^^^^ cannot borrow as mutable
| |
| expects `Fn` instead of `FnMut`

error[E0594]: cannot assign to `z`, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:21:55
|
LL | let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); });
| ^^^^^^ cannot assign
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-immutable-upvar-mutation.rs:21:52
|
LL | let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); });
| ^^^^^^^^^
--> $DIR/borrow-immutable-upvar-mutation.rs:23:22
|
LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | to_fn(|| z = 42);
| ----- ^^^^^^ cannot assign
| |
| expects `Fn` instead of `FnMut`

error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:27:32
|
LL | let _f = to_fn(move || x = 42);
| ^^^^^^ cannot assign
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-immutable-upvar-mutation.rs:27:24
--> $DIR/borrow-immutable-upvar-mutation.rs:30:32
|
LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | let _f = to_fn(move || x = 42);
| ^^^^^^^^^^^^^^
| ----- ^^^^^^ cannot assign
| |
| expects `Fn` instead of `FnMut`

error[E0596]: cannot borrow `y` as mutable, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:30:36
|
LL | let _g = to_fn(move || set(&mut y));
| ^^^^^^ cannot borrow as mutable
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-immutable-upvar-mutation.rs:30:24
--> $DIR/borrow-immutable-upvar-mutation.rs:33:36
|
LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | let _g = to_fn(move || set(&mut y));
| ^^^^^^^^^^^^^^^^^^^
| ----- ^^^^^^ cannot borrow as mutable
| |
| expects `Fn` instead of `FnMut`

error[E0594]: cannot assign to `z`, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:33:65
--> $DIR/borrow-immutable-upvar-mutation.rs:36:65
|
LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); });
| ^^^^^^ cannot assign
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-immutable-upvar-mutation.rs:33:57
|
LL | let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); });
| ^^^^^^^^^^^^^^
| ----- ^^^^^^ cannot assign
| |
| expects `Fn` instead of `FnMut`

error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:43:9
|
LL | fn foo() -> Box<dyn Fn() -> usize> {
| --- ---------------------- ...to return `FnMut` instead of `Fn`
| |
| you might have to change this...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of this message seems reversed with the message above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about in this function.../...you might have to change this to return FnMut instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual text is fine — I'm just inclined to read the topmost line first:

...to return `FnMut` instead of `Fn`
you might have to change this...

when it's intended to be read in the opposite order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the span for the method ident with no label and shortened the wording to change this to return FnMut instead of Fn, as it seems more consistent with the terseness of the other suggestions we give.

LL | let mut x = 0;
LL | Box::new(move || {
| ______________-
LL | | x += 1;
| | ^^^^^^ cannot assign
LL | | x
LL | | })
| |_____- in this closure

error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-immutable-upvar-mutation.rs:51:9
|
LL | fn bar() -> impl Fn() -> usize {
| --- ------------------ ...to return `FnMut` instead of `Fn`
| |
| you might have to change this...
LL | let mut x = 0;
LL | / move || {
LL | | x += 1;
| | ^^^^^^ cannot assign
LL | | x
LL | | }
| |_____- in this closure

error: aborting due to 6 previous errors
error: aborting due to 8 previous errors

Some errors have detailed explanations: E0594, E0596.
For more information about an error, try `rustc --explain E0594`.
32 changes: 16 additions & 16 deletions src/test/ui/borrowck/borrow-raw-address-of-mutability.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,32 +27,32 @@ LL | f();
error[E0596]: cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-raw-address-of-mutability.rs:29:17
|
LL | let y = &raw mut x;
| ^^^^^^^^^^ cannot borrow as mutable
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-raw-address-of-mutability.rs:28:21
|
LL | fn make_fn<F: Fn()>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | let f = make_fn(|| {
| _____________________^
| _____________-------_-
| | |
| | expects `Fn` instead of `FnMut`
LL | | let y = &raw mut x;
| | ^^^^^^^^^^ cannot borrow as mutable
LL | | });
| |_____^
| |_____- in this closure

error[E0596]: cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure
--> $DIR/borrow-raw-address-of-mutability.rs:37:17
|
LL | let y = &raw mut x;
| ^^^^^^^^^^ cannot borrow as mutable
|
help: consider changing this to accept closures that implement `FnMut`
--> $DIR/borrow-raw-address-of-mutability.rs:36:21
|
LL | fn make_fn<F: Fn()>(f: F) -> F { f }
| - change this to accept `FnMut` instead of `Fn`
...
LL | let f = make_fn(move || {
| _____________________^
| _____________-------_-
| | |
| | expects `Fn` instead of `FnMut`
LL | | let y = &raw mut x;
| | ^^^^^^^^^^ cannot borrow as mutable
LL | | });
| |_____^
| |_____- in this closure

error: aborting due to 5 previous errors

Expand Down
Loading