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

Incorrect borrow checking #39963

Closed
strega-nil opened this issue Feb 19, 2017 · 10 comments · Fixed by #51358
Closed

Incorrect borrow checking #39963

strega-nil opened this issue Feb 19, 2017 · 10 comments · Fixed by #51358
Labels
A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: Writing correctness tests. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@strega-nil
Copy link
Contributor

strega-nil commented Feb 19, 2017

UPDATE: This now works in NLL land. We just need someone to add a test.


https://is.gd/O6RlWi

left.1.0 and left.0.0 should be separate borrows into the same structure.

Note that changing **left to *&mut**left makes it borrow check, and changing Box<Foo> to &'static mut Foo in the Foo struct also makes it borrow check. This makes me think it's something to do with the special casing of Box.

@arielb1
Copy link
Contributor

arielb1 commented Feb 20, 2017

This is intentional. rust-lang/rfcs#130.

@arielb1 arielb1 closed this as completed Feb 20, 2017
@strega-nil
Copy link
Contributor Author

strega-nil commented Feb 24, 2017

@arielb1 no... it isn't. This is incorrect behavior. It should treat left.1.0 and left.0.0 as separate borrows.

(as seen by the fact that it works in the outer match statement)

@eddyb
Copy link
Member

eddyb commented Feb 24, 2017

@ubsan I'm afraid @arielb1 is right, we really dumbed down Box.
Proof: box ref mut makes the error go away while matching on **(f: &mut Box<Foo>) reproduces it.

@strega-nil
Copy link
Contributor Author

strega-nil commented Feb 24, 2017

@eddyb then it's a bug with the specification (and the issue should be left open).

@strega-nil
Copy link
Contributor Author

strega-nil commented Feb 24, 2017

given that this works: https://is.gd/clzryh, rust should be able to figure out the difference between a moving deref and a non-moving; it can already figure out mutable vs. non-mutable.

@arielb1
Copy link
Contributor

arielb1 commented Feb 24, 2017

Sure, the equivalent of your code with a DerefMut type instead of a Box works:

#[derive(Clone)]
struct BoxFoo;
impl std::ops::Deref for BoxFoo {
    type Target = Foo;
    fn deref(&self) -> &Foo { panic!() }
}

impl std::ops::DerefMut for BoxFoo {
    fn deref_mut(&mut self) -> &mut Foo { panic!() }
}


#[derive(Clone)]
struct Foo(Option<BoxFoo>, Option<BoxFoo>);

fn test(f: &mut Foo) {
  match *f {
    Foo(Some(ref mut left), Some(ref mut right)) => match **left {
      Foo(Some(ref mut left), Some(ref mut right)) => panic!(),
      _ => panic!(),
    },
    _ => panic!(),
  }
}

fn main() {
}

The current borrowck is sort-of in maintenance mode, so this is unlikely to be fixed before MIR borrowck.

@arielb1 arielb1 reopened this Feb 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the A-borrow-checker Area: The borrow checker label May 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 27, 2017
@pnkfelix
Copy link
Member

Nominating. I suggest we retag this as C-enhancement rather than C-bug. I separately suggest that we prioritize it as P-medium. (It does not need to land as part of the initial MIR-borrowck / NLL work.)

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Dec 20, 2017
@pnkfelix
Copy link
Member

(looking at #23610, I am wondering: what's the difference between C-feature-request and C-enhancement? Should we collapse those two labels into one...?)

@nikomatsakis
Copy link
Contributor

In the NLL / MIR borrow checker, we decided to just be smarter around Box. Therefore, the code now type-checks.

@nikomatsakis nikomatsakis added E-needs-test Call for participation: Writing correctness tests. and removed I-nominated labels Jan 18, 2018
@nikomatsakis
Copy link
Contributor

I think I would be happy to just add a test for this (with #![feature(nll)]) and close this issue.

@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Jan 18, 2018
barzamin added a commit to barzamin/rust that referenced this issue Jun 4, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jun 5, 2018
bors added a commit that referenced this issue Jun 5, 2018
Rollup of 7 pull requests

Successful merges:

 - #50852 (Add doc comment to hiding portions of code example)
 - #51183 (Update rustdoc book to suggest using Termination trait instead of hidden ‘foo’ function)
 - #51255 (Fix confusing error message for sub_instant)
 - #51256 (Fix crate-name option in rustdoc)
 - #51308 (Check array indices in constant propagation)
 - #51343 (test: Ignore some problematic tests on sparc and sparc64)
 - #51358 (Tests that #39963 is fixed on MIR borrowck)

Failed merges:
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. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. E-needs-test Call for participation: Writing correctness tests. 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