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 permisson checks to fn access_lvalue #44837

Closed
pnkfelix opened this issue Sep 25, 2017 · 4 comments
Closed

MIR-borrowck: add permisson checks to fn access_lvalue #44837

pnkfelix opened this issue Sep 25, 2017 · 4 comments
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

As noted in a comment by arielb1, the fn access_lvalue needs to check somewhere that the accessor actually has sufficient permissions for the operation being requested, aka no mutable borrows of immutable references or moves through a non-Box reference.

@TimNN TimNN added A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. labels Sep 27, 2017
@pnkfelix pnkfelix added WG-compiler-nll T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2017
@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 9, 2017

To elaborate, here are some examples of the kinds of scenarios we are worried about:

&mut of imm ref

For mutable borrows of immutable references, here is a case that mir-borrowck today does not properly catch (and, I think, none of the PR's in flight will fix):

fn main() {
    let mut x = &3;
    let rw = &mut *x;
    *rw = 4;
    println!("x: {:?}", x);
}

Compiling this with -Z borrowck-mir on today's master yields this single error from the AST-borrowck alone:

error[E0596]: cannot borrow immutable borrowed content `*x` as mutable
 --> check-access-lvalue.rs:3:19
  |
3 |     let rw = &mut *x;
  |                   ^^ cannot borrow as mutable

error: aborting due to previous error

Move through non-Box ref

... hmm... I am having trouble constructing an example of this where AST-borrowck and MIR-borrowck differ in behavior. (Which is strange because I thought there was significant portions of box-support that remained to be implemented. I'll have to look more deeply into this.)

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 9, 2017

@zilbuz also notes this example (though I think it might a bit different thing to check, since this AFAICT is about checking that the local _a has a let mut declaration...)

fn main() {
    let a = 1;
    let _a = &a;
    let __a = &mut _a;
}

@nikomatsakis
Copy link
Contributor

Here is some pseudocode for the various permissions I think we might want to check:

https://gist.github.com/nikomatsakis/90102114e8ba5792d4e28a6960942ae6

In each case, there is a function that, given an lvalue, tells you whether it can be mutated or whether it can be moved from. The function returns Err(lv) if the action is illegal, in which case lv is the lvalue we can "blame" for that.

bors added a commit that referenced this issue Nov 13, 2017
MIR-borrowck: add permisson checks to `fn access_lvalue`

WIP : Some FIXME left and some broken tests.

Fix #44837
@arielb1
Copy link
Contributor

arielb1 commented Nov 15, 2017

The permission checks still let you assign/mutate through an immutable reference:

fn main() {
    let a = &5;
    *a = 6; // no MIR error !!!
}

e.g. the test compile-fail/borrowck/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs

@arielb1 arielb1 reopened this Nov 15, 2017
bors added a commit that referenced this issue Nov 30, 2017
MIR borrowck: finalize `check_access_permissions()`

Fix #44837 (hopefully for good)

r? @arielb1
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 C-bug Category: This is a bug. 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