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

MIR-borrowck: add "consider changing this to mut x" note, as in AST borrowck #46020

Closed
arielb1 opened this issue Nov 15, 2017 · 9 comments
Closed
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@arielb1
Copy link
Contributor

arielb1 commented Nov 15, 2017

Currently, when you try to access an immutable local, AST borrowck suggests that you should change it to mut x, for example:

fn main() {
    let x = 0;
    x = 1;
    std::mem::replace(&mut x, 2);
}
warning: value assigned to `x` is never read
 --> src/main.rs:2:9
  |
2 |     let x = 0;
  |         ^
  |
  = note: #[warn(unused_assignments)] on by default

error[E0596]: cannot borrow immutable local variable `x` as mutable
 --> src/main.rs:4:28
  |
2 |     let x = 0;
  |         - consider changing this to `mut x` <---- THIS NOTE
3 |     x = 1;
4 |     std::mem::replace(&mut x, 2);
  |                            ^ cannot borrow mutably

error[E0384]: cannot assign twice to immutable variable `x`
 --> src/main.rs:3:5
  |
2 |     let x = 0;
  |         - first assignment to `x`
3 |     x = 1;
  |     ^^^^^ cannot assign twice to immutable variable

error: aborting due to 2 previous errors

MIR borrowck should be doing this too.

FIXME: I should write mentor notes

@arielb1 arielb1 added A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints E-needs-mentor labels Nov 15, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Nov 21, 2017

Note: this is somewhat lower priority than the rest of the NLL work.

How to implement this

The AST borrow-checker has mutability blaming code in the following place:

fn note_immutability_blame(&self,

That code needs to be ported to MIR borrowck. The code uses the immutability_blame function on mem_categorization, which needs to be ported to work on MIR lvalues instead of cmt, which I don't think should be a problem.

The AST borrowck looks up data on user local variables using the local's NodeId in order to make this error report. Therefore, it might probably a good idea to change the is_user_variable field of mir::LocalDecl to an Option<ast::NodeId> and store the user local's NodeId in order to look up that information (or there might be a different way of doing this) - https://github.com/rust-lang/rust/blob/f28df200260c89b2a0bdf942510e0f888c29a70d/src/librustc/mir/mod.rs#L440-485

@arielb1 arielb1 added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-needs-mentor labels Nov 21, 2017
@raventid
Copy link
Contributor

I'd like to take this one. I already took #46186(at least I wrote i'd like to :) ), they touch different parts of diagnostic system(as far as I can see now), so I'd like to solve both to get a taste of what's going on from different angles.

@arielb1
Copy link
Contributor Author

arielb1 commented Nov 25, 2017

@raventid

Cool! If you have any questions, feel free to ping me on WG-compiler-nll or WG-compiler-errors on gitter.

@nikomatsakis
Copy link
Contributor

@raventid I'm guessing you never found time to do this, right? (No worries, just checking in.)

@raventid
Copy link
Contributor

@nikomatsakis, yep. To be frank - I even forgot I started this. I actually had some time to tweak this a bit, but it happend to be a bit harder then I expected, I didn't manage to find a way to easily port immutability_blame to MIR vals. I'm still interested in taking this :) I can make some progress in next couple of weeks + some time to accept PR + fix review notes = I can make it real in 3 weeks. If it sounds like an acceptable deadline - you can count on me, if not, well, I'll try something else later.

@nikomatsakis nikomatsakis added this to the NLL: diagnostic parity milestone Feb 2, 2018
@spastorino spastorino self-assigned this Feb 2, 2018
@nikomatsakis
Copy link
Contributor

@spastorino is gonna take a crack at this I think

@jkordish jkordish added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 5, 2018
@nikomatsakis nikomatsakis added the NLL-diagnostics Working towards the "diagnostic parity" goal label Mar 14, 2018
@spastorino spastorino removed their assignment Jun 2, 2018
@nikomatsakis
Copy link
Contributor

This test:

#![feature(nll)]
#![allow(warnings)]

fn main() {
    let x = 0;
    x = 1;
    std::mem::replace(&mut x, 2);
}

currently gives:

error[E0384]: cannot assign twice to immutable variable `x`
 --> src/main.rs:6:5
  |
5 |     let x = 0;
  |         -   - first assignment to `x`
  |         |
  |         consider changing this to `mut x`
6 |     x = 1;
  |     ^^^^^ cannot assign twice to immutable variable

error[E0596]: cannot borrow immutable item `x` as mutable
 --> src/main.rs:7:23
  |
5 |     let x = 0;
  |         - help: consider changing this to be mutable: `mut x`
6 |     x = 1;
7 |     std::mem::replace(&mut x, 2);
  |                       ^^^^^^ cannot borrow as mutable

Note that we do provide the hints, but we also report duplicate errors.

@nikomatsakis
Copy link
Contributor

Well, the AST version does too:

error[E0596]: cannot borrow immutable local variable `x` as mutable
 --> src/main.rs:6:28
  |
4 |     let x = 0;
  |         - consider changing this to `mut x`
5 |     x = 1;
6 |     std::mem::replace(&mut x, 2);
  |                            ^ cannot borrow mutably

error[E0384]: cannot assign twice to immutable variable `x`
 --> src/main.rs:5:5
  |
4 |     let x = 0;
  |         - first assignment to `x`
5 |     x = 1;
  |     ^^^^^ cannot assign twice to immutable variable

@nikomatsakis
Copy link
Contributor

I am going to close this as fixed.

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 C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal 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

5 participants