Skip to content

MIR borrowck: AST error message refers to kind of item that can't be borrowed #46629

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

Closed
arielb1 opened this issue Dec 10, 2017 · 7 comments
Closed
Assignees
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-diagnostics Working towards the "diagnostic parity" goal
Milestone

Comments

@arielb1
Copy link
Contributor

arielb1 commented Dec 10, 2017

e.g. in E0596:

fn main() {
    let x = 1;
    let y = &mut x; //[ast]~ ERROR [E0596]
                    //[mir]~^ ERROR [E0596]
}

AST borrowck knows that x is an "immutable local variable", but MIR semi-inaccurately calls it an immutable item:

error[E0596]: cannot borrow immutable local variable `x` as mutable (Ast)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/E0596.rs:16:18
   |
15 |     let x = 1;
   |         - consider changing this to `mut x`
16 |     let y = &mut x; //[ast]~ ERROR [E0596]
   |                  ^ cannot borrow mutably

error[E0596]: cannot borrow immutable item `x` as mutable (Mir)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/E0596.rs:16:13
   |
16 |     let y = &mut x; //[ast]~ ERROR [E0596]
   |             ^^^^^^ cannot borrow as mutable

error: aborting due to 2 previous errors

The AST code for doing that is actually in mem_categorization, and should be ported to MIR borrowck:

pub fn descriptive_string(&self, tcx: TyCtxt) -> String {

@arielb1 arielb1 added A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints WG-compiler-nll labels Dec 10, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 10, 2017

I think the mc code sometimes generates ugly error messages - it should be called on the lvalue that causes the mutability error, not on the full lvalue - but you can use it as a reference to improve the errors.

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 10, 2017

Note: if you do this correctly, you should also be able to reroute closure errors into things like E0387 - "cannot assign to data in a captured outer variable in an Fn closure" instead of the standard "cannot borrow immutable item as mutable". See e.g. the borrow-immutable-upvar-mutation test:

// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(unboxed_closures)]

// Tests that we can't assign to or mutably borrow upvars from `Fn`
// closures (issue #17780)

fn set(x: &mut usize) { *x = 5; }

fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
fn to_fn_mut<A,F:FnMut<A>>(f: F) -> F { f }

fn main() {
    // By-ref captures
    {
        let mut x = 0;
        let _f = to_fn(|| x = 42); //~ ERROR cannot assign

        let mut y = 0;
        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
    }

    // By-value captures
    {
        let mut x = 0;
        let _f = to_fn(move || x = 42); //~ ERROR cannot assign

        let mut y = 0;
        let _g = to_fn(move || set(&mut y)); //~ ERROR cannot borrow

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

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 10, 2017

In that case, the error message we emit is especially terrible, because it is not obvious that the problem is with the closure:

error[E0594]: cannot assign to immutable item `x` (Mir)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:25:27
   |
25 |         let _f = to_fn(|| x = 42); //~ ERROR cannot assign
   |                           ^^^^^^ cannot mutate

error[E0596]: cannot borrow immutable item `y` as mutable (Mir)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:28:31
   |
28 |         let _g = to_fn(|| set(&mut y)); //~ ERROR cannot borrow
   |                               ^^^^^^ cannot borrow as mutable

error[E0594]: cannot assign to immutable item `x` (Mir)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:37:32
   |
37 |         let _f = to_fn(move || x = 42); //~ ERROR cannot assign
   |                                ^^^^^^ cannot mutate

error[E0596]: cannot borrow immutable item `y` as mutable (Mir)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:40:36
   |
40 |         let _g = to_fn(move || set(&mut y)); //~ ERROR cannot borrow
   |                                    ^^^^^^ cannot borrow as mutable

error[E0387]: cannot assign to data in a captured outer variable in an `Fn` closure (Ast)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:25:27
   |
25 |         let _f = to_fn(|| x = 42); //~ ERROR cannot assign
   |                           ^^^^^^
   |
help: consider changing this closure to take self by mutable reference
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:25:24
   |
25 |         let _f = to_fn(|| x = 42); //~ ERROR cannot assign
   |                        ^^^^^^^^^

error[E0387]: cannot borrow data mutably in a captured outer variable in an `Fn` closure (Ast)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:28:36
   |
28 |         let _g = to_fn(|| set(&mut y)); //~ ERROR cannot borrow
   |                                    ^
   |
help: consider changing this closure to take self by mutable reference
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:28:24
   |
28 |         let _g = to_fn(|| set(&mut y)); //~ ERROR cannot borrow
   |                        ^^^^^^^^^^^^^^

error[E0387]: cannot assign to data in a captured outer variable in an `Fn` closure (Ast)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:31:55
   |
31 |         let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); }); //~ ERROR cannot assign
   |                                                       ^^^^^^
   |
help: consider changing this closure to take self by mutable reference
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:31:52
   |
31 |         let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); }); //~ ERROR cannot assign
   |                                                    ^^^^^^^^^

error[E0594]: cannot assign to captured outer variable in an `Fn` closure (Ast)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:37:32
   |
36 |         let mut x = 0;
   |             ----- help: consider making `mut x` mutable: `mut mut x`
37 |         let _f = to_fn(move || x = 42); //~ ERROR cannot assign
   |                                ^^^^^^
   |
help: consider changing this closure to take self by mutable reference
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:37:24
   |
37 |         let _f = to_fn(move || x = 42); //~ ERROR cannot assign
   |                        ^^^^^^^^^^^^^^

error[E0596]: cannot borrow captured outer variable in an `Fn` closure as mutable (Ast)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:40:41
   |
40 |         let _g = to_fn(move || set(&mut y)); //~ ERROR cannot borrow
   |                                         ^
   |
help: consider changing this closure to take self by mutable reference
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:40:24
   |
40 |         let _g = to_fn(move || set(&mut y)); //~ ERROR cannot borrow
   |                        ^^^^^^^^^^^^^^^^^^^

error[E0594]: cannot assign to captured outer variable in an `Fn` closure (Ast)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:43:65
   |
42 |         let mut z = 0;
   |             ----- help: consider making `mut z` mutable: `mut mut z`
43 |         let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); }); //~ ERROR cannot assign
   |                                                                 ^^^^^^
   |
help: consider changing this closure to take self by mutable reference
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:43:57
   |
43 |         let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); }); //~ ERROR cannot assign
   |                                                         ^^^^^^^^^^^^^^

warning: variable does not need to be mutable
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:36:13
   |
36 |         let mut x = 0;
   |             ---^^
   |             |
   |             help: remove this `mut`
   |
   = note: #[warn(unused_mut)] on by default

warning: variable does not need to be mutable
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:39:13
   |
39 |         let mut y = 0;
   |             ---^^
   |             |
   |             help: remove this `mut`

error[E0594]: cannot assign to immutable item `z` (Mir)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:31:55
   |
31 |         let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); }); //~ ERROR cannot assign
   |                                                       ^^^^^^ cannot mutate

error[E0594]: cannot assign to immutable item `z` (Mir)
  --> /home/ariel/Rust/rust-master/src/test/compile-fail/borrow-immutable-upvar-mutation.rs:43:65
   |
43 |         let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); }); //~ ERROR cannot assign
   |                                                                 ^^^^^^ cannot mutate

error: aborting due to 12 previous errors

@pnkfelix pnkfelix added the A-NLL Area: Non-lexical lifetimes (NLL) label Jan 10, 2018
@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Mar 26, 2018
@nikomatsakis
Copy link
Contributor

Semi-dup of #47388

@nikomatsakis nikomatsakis added the NLL-diagnostics Working towards the "diagnostic parity" goal label Apr 3, 2018
@nikomatsakis
Copy link
Contributor

It seems like we still say things like "immutable item". This doesn't seem great. Tagging for release candidate.

@nikomatsakis
Copy link
Contributor

athena. rustc +nightly E0596.rs -Zborrowck=mir
warning: unused variable: `y`
  --> E0596.rs:16:9
   |
16 |     let y = &mut x; //[ast]~ ERROR [E0596]
   |         ^ help: consider using `_y` instead
   |
   = note: #[warn(unused_variables)] on by default

error[E0596]: cannot borrow immutable item `x` as mutable
  --> E0596.rs:16:13
   |
15 |     let x = 1;
   |         - help: consider changing this to be mutable: `mut x`
16 |     let y = &mut x; //[ast]~ ERROR [E0596]
   |             ^^^^^^ cannot borrow as mutable

error: aborting due to previous error

@nikomatsakis
Copy link
Contributor

Honestly, I think I would prefer to see this phrased rather differently. Something like:

"cannot borrow x mutably, as it is not declared as mutable"

@matthewjasper matthewjasper self-assigned this Jul 9, 2018
bors added a commit that referenced this issue Jul 21, 2018
[NLL] Mutability errors

cc #51028
cc #51170
cc #46559
Closes #46629

* Better explain why the place is immutable ("immutable item" is gone)
* Distinguish &T and *const T
* Use better spans when a mutable borrow is for a closure capture

r? @pnkfelix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. NLL-diagnostics Working towards the "diagnostic parity" goal
Projects
None yet
Development

No branches or pull requests

5 participants