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.

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

Not sufficiently useful error message when FnMut is needed instead of Fn #31701

Closed
eugene-bulkin opened this issue Feb 16, 2016 · 3 comments · Fixed by #68816
Closed

Not sufficiently useful error message when FnMut is needed instead of Fn #31701

eugene-bulkin opened this issue Feb 16, 2016 · 3 comments · Fixed by #68816
Labels
A-closures Area: closures (`|args| { .. }`) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eugene-bulkin
Copy link

This code:

fn counter() -> Box<Fn() -> usize> {
    let mut x = 0;
    Box::new(move || {
        x += 1;
        x
    })
}

results in the following error messages in stable:

<anon>:4:9: 4:15 error: cannot assign to captured outer variable in an `Fn` closure
<anon>:4         x += 1;
                 ^~~~~~
<anon>:3:14: 6:6 help: consider changing this closure to take self by mutable reference
<anon>:3     Box::new(move || {
<anon>:4         x += 1;
<anon>:5         x
<anon>:6     })

The problem here is that we're using Fn() instead of FnMut(), but the phrase "consider changing this closure to take self by mutable reference" is not particularly useful in telling us that that specifically is the problem. That is, the fact that we cannot assign to the captured outer variable is because we are trying to return Box<Fn() -> usize> instead of Box<FnMut() -> usize>, but that's not immediately clear from the original error message.

I'm not entirely sure how this would be rectified, but at the very least mentioning FnMut somewhere would be useful. @eddyb mentioned that this issue was "because a bound/trait object of Fn() was used but only FnMut() allows mutable borrows", so I think some variation on that would be reasonably sufficient to clarify the error.

@eddyb eddyb added A-diagnostics Area: Messages for errors, warnings, and lints A-closures Area: closures (`|args| { .. }`) labels Feb 16, 2016
@djmcgill
Copy link

I was just bitten by this too - in the end I googled the error message, taking me to this page which solved my...issue.

@steveklabnik steveklabnik removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 24, 2017
@estebank
Copy link
Contributor

Current output doesn't point at the return type, but otherwise says the right thing:

error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
 --> src/main.rs:4:9
  |
4 |         x += 1;
  |         ^^^^^^ cannot assign
  |
help: consider changing this to accept closures that implement `FnMut`
 --> src/main.rs:3:14
  |
3 |       Box::new(move || {
  |  ______________^
4 | |         x += 1;
5 | |         x
6 | |     })
  | |_____^

@estebank estebank added D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 22, 2020
@estebank
Copy link
Contributor

estebank commented Feb 3, 2020

cc #66097

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: closures (`|args| { .. }`) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants