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

Rust should explain why a closure call requires a mutable borrow #80313

Closed
Aaron1011 opened this issue Dec 22, 2020 · 11 comments
Closed

Rust should explain why a closure call requires a mutable borrow #80313

Aaron1011 opened this issue Dec 22, 2020 · 11 comments
Assignees
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Dec 22, 2020

The following code:

fn main() {
    let mut my_var = false;
    let callback = || {
        my_var = true;
    };
    callback();
}

produces the following error:

error[E0596]: cannot borrow `callback` as mutable, as it is not declared as mutable
 --> src/main.rs:6:5
  |
3 |     let callback = || {
  |         -------- help: consider changing this to be mutable: `mut callback`
...
6 |     callback();
  |     ^^^^^^^^ cannot borrow as mutable

error: aborting due to previous error

The expression callback() tries to mutably borrow callback due to the mutation of my_var. However, the error message doesn't explain this.

We should add a note to the error message pointing to the span of the upvar that caused us to need the mutable borrow.

@Aaron1011 Aaron1011 added C-bug Category: This is a bug. A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. and removed C-bug Category: This is a bug. labels Dec 22, 2020
@camelid camelid self-assigned this Dec 23, 2020
@Aaron1011 Aaron1011 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Dec 23, 2020
@camelid
Copy link
Member

camelid commented Dec 23, 2020

So it looks like this is the function that needs to be modified:

pub(crate) fn report_mutability_error(
&mut self,
access_place: Place<'tcx>,
span: Span,
the_place_err: PlaceRef<'tcx>,
error_access: AccessKind,
location: Location,
) {

It seems I might be able to get the information I need from self.used_mut_upvars?

@camelid
Copy link
Member

camelid commented Dec 23, 2020

Actually, self.used_mut_upvars won't work here because it's only used when we're borrow-checking the closure, not uses of it (self.used_mut_upvars is empty in this context).

@JohnTitor JohnTitor added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 30, 2020
@sledgehammervampire
Copy link
Contributor

@rustbot claim

@sledgehammervampire
Copy link
Contributor

Is anyone going to work on this issue? If not, I want to work on this to help me learn how Rust works.

@Aaron1011
Copy link
Member Author

@1000teslas Go ahead! If you need any help, feel free to reach out to me on Zulip or Discord

@camelid
Copy link
Member

camelid commented Jan 11, 2021

I was working on this, but I guess I should have pinged Aaron: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/finding.20mutated.20upvars.20in.20borrowck. Oh well :)

@1000teslas you can work on this, I have other things that I've been meaning to get to anyway.

@sledgehammervampire
Copy link
Contributor

@Aaron1011 What is the intended error? Is it meant to point to my_var, similarly to room_ref in #78938?

@Aaron1011
Copy link
Member Author

Yes - I was thinking of displaying a message like "a mutable borrow is required because the closure modifies my_var", located at the usage of my_var. Additionally, we could add another note pointing to the definition of my_var.

@sledgehammervampire
Copy link
Contributor

I'm not sure how given a Location or Place of a call, we can obtain the Local pointing to the definition for self.body.local_decls.

@Aaron1011
Copy link
Member Author

@1000teslas I think the information you need already exists in closure_kind_origins().

You can see an example of retrieving this here:

let tables = self.infcx.tcx.typeck(id.expect_local());
let hir_id = self.infcx.tcx.hir().local_def_id_to_hir_id(id.expect_local());
tables.closure_kind_origins().get(hir_id).is_none()

and of using it:

match (found_kind, typeck_results.closure_kind_origins().get(hir_id)) {
(ty::ClosureKind::FnOnce, Some((span, place))) => {
err.span_label(
*span,
format!(
"closure is `FnOnce` because it moves the \
variable `{}` out of its environment",
ty::place_to_string_for_capture(tcx, place)
),
);
}
(ty::ClosureKind::FnMut, Some((span, place))) => {
err.span_label(
*span,
format!(
"closure is `FnMut` because it mutates the \
variable `{}` here",
ty::place_to_string_for_capture(tcx, place)
),
);
}
_ => {}

I don't think you'll need to access the MIR Local

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 27, 2021
…1011

Point to span of upvar making closure FnMut

For rust-lang#80313.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 27, 2021
…1011

Point to span of upvar making closure FnMut

For rust-lang#80313.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 28, 2021
…1011

Point to span of upvar making closure FnMut

For rust-lang#80313.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 29, 2021
…1011

Point to span of upvar making closure FnMut

For rust-lang#80313.
@JohnTitor
Copy link
Member

Fixed by #81158

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants