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

Two MUTABLE borrows to local var, by pushing closures into RefCell'd Vec through a trait. #18783

Closed
cristicbz opened this issue Nov 8, 2014 · 9 comments · Fixed by #19780
Closed
Assignees
Labels
E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Milestone

Comments

@cristicbz
Copy link
Contributor

Looks like a bug in the borrow checker:

use std::cell::RefCell;

fn main() {
    let c = Caller::new();
    let mut y = 1;  // NOT in RefCell.
    c.pusher().push(|| { y = y * 2u; });
    c.pusher().push(|| { y = y * 3u; });  // Second mutable borrow of y?!
    c.call();
    println!("{}", y); // Prints 6.
}

// Collects multiple closures then call all of them.
struct Caller<'c> {
    funs: RefCell<Vec<||:'c -> ()>>,
} impl<'c> Caller<'c> {
    fn new() -> Caller<'c> {
        Caller { funs: RefCell::new(Vec::new()) }
    }

    fn pusher(&'c self) -> Pusher<'c> {
        Pusher { caller: self }
    }

    fn call(&self) {
        let ref mut funs = self.funs.borrow_mut();
        loop {
            match funs.pop() {
                Some(f) => f(),
                _ => break,
            }
        }
    }
}

// Pushes closures into a caller.
trait Push<'c> {
    fn push<'f: 'c>(self, push: ||:'f -> ());
}

struct Pusher<'c> {
    caller: &'c Caller<'c>
}
// /*
impl<'c> Push<'c> for Pusher<'c> {
    fn push<'f: 'c>(self, fun: ||:'f -> ()) {
        self.caller.funs.borrow_mut().push(fun)
    }
}
// Using the following impl (without trait) makes compilation fail correctly: */
/*
impl<'c> Pusher<'c> {
    fn push<'f: 'c>(self, fun: ||:'f -> ()) {
        self.caller.funs.borrow_mut().push(fun)
    }
}
*/

Playpen

This compiles and runs fine. It also compiles when using unboxed closures instead (using type param F: FnOnce<(), ()>+'c and F members). Using the non-trait impl, however, causes compilation to fail correctly:

<anon>:7:21: 7:39 error: cannot borrow `y` as mutable more than once at a time
<anon>:7     c.pusher().push(|| { y = y * 3u; });  // Second mutable borrow of y?!
                             ^~~~~~~~~~~~~~~~~~
<anon>:7:30: 7:31 note: borrow occurs due to use of `y` in closure
<anon>:7     c.pusher().push(|| { y = y * 3u; });  // Second mutable borrow of y?!
                                      ^
<anon>:6:21: 6:39 note: previous borrow of `y` occurs here due to use in closure; the mutable borrow prevents subsequent moves, borrows, or modification of `y` until the borrow ends
<anon>:6     c.pusher().push(|| { y = y * 2u; });
                             ^~~~~~~~~~~~~~~~~~
<anon>:10:2: 10:2 note: previous borrow ends here
<anon>:3 fn main() {
...
<anon>:10 }
          ^
<anon>:9:20: 9:21 error: cannot borrow `y` as immutable because it is also borrowed as mutable
<anon>:9     println!("{}", y); // Prints 6.
                            ^
note: in expansion of format_args!
<std macros>:2:23: 2:77 note: expansion site
<std macros>:1:1: 3:2 note: in expansion of println!
<anon>:9:5: 9:23 note: expansion site
<anon>:6:21: 6:39 note: previous borrow of `y` occurs here due to use in closure; the mutable borrow prevents subsequent moves, borrows, or modification of `y` until the borrow ends
<anon>:6     c.pusher().push(|| { y = y * 2u; });
                             ^~~~~~~~~~~~~~~~~~
<anon>:10:2: 10:2 note: previous borrow ends here
<anon>:3 fn main() {
...
<anon>:10 }
          ^
<anon>:9:20: 9:21 error: cannot borrow `y` as immutable because it is also borrowed as mutable
<anon>:9     println!("{}", y); // Prints 6.
                            ^
note: in expansion of format_args!
<std macros>:2:23: 2:77 note: expansion site
<std macros>:1:1: 3:2 note: in expansion of println!
<anon>:9:5: 9:23 note: expansion site
<anon>:7:21: 7:39 note: previous borrow of `y` occurs here due to use in closure; the mutable borrow prevents subsequent moves, borrows, or modification of `y` until the borrow ends
<anon>:7     c.pusher().push(|| { y = y * 3u; });  // Second mutable borrow of y?!
                             ^~~~~~~~~~~~~~~~~~
<anon>:10:2: 10:2 note: previous borrow ends here
<anon>:3 fn main() {
...
<anon>:10 }
          ^
error: aborting due to 3 previous errors
playpen: application terminated with error code 101
Program ended.
@bkoropoff
Copy link
Contributor

Reduced a bit further:

use std::cell::RefCell;

fn main() {
    let c = RefCell::new(vec![]);
    let mut y = 1u;
    c.push(|| y = 0);
    c.push(|| y = 0);
    // confliciting borrow detected when not going through trait
    // c.borrow_mut().push(|| y = 0);
    // c.borrow_mut().push(|| y = 0);
}

trait Push<'c> {
    fn push<'f: 'c>(&self, push: ||:'f -> ());
}

impl<'c> Push<'c> for RefCell<Vec<||:'c>> {
    fn push<'f: 'c>(&self, fun: ||:'f -> ()) {
        self.borrow_mut().push(fun)
    }
}

This is a regression from 0.12. cc @nikomatsakis

@ghost ghost added the I-nominated label Nov 8, 2014
@bkoropoff
Copy link
Contributor

git bisect shows this came in with #18121

@bkoropoff
Copy link
Contributor

I tried #18694 on a hunch, and it fixes this issue.

@ghost ghost added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Nov 9, 2014
@pnkfelix
Copy link
Member

P-backcompat-lang, 1.0.

@pnkfelix pnkfelix added this to the 1.0 milestone Nov 13, 2014
@nikomatsakis
Copy link
Contributor

This is not entirely fixed by #18694, there is another guise. Here is a revised 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.

use std::cell::RefCell;

fn main() {
    let c = RefCell::new(vec![]);
    let mut y = 1u;

    c.push(|| y = 0);
    c.push(|| y = 0);
    // confliciting borrow detected when not going through trait
    // c.borrow_mut().push(|| y = 0);
    // c.borrow_mut().push(|| y = 0);
}

fn ufcs() {
    let c = RefCell::new(vec![]);
    let mut y = 1u;

    Push::push(&c, || y = 0);
    Push::push(&c, || y = 0);
}

trait Push<'c> {
    fn push<'f: 'c>(&self, push: ||:'f -> ());
}

impl<'c> Push<'c> for RefCell<Vec<||:'c>> {
    fn push<'f: 'c>(&self, fun: ||:'f -> ()) {
        self.borrow_mut().push(fun)
    }
}

the second set of calls using UFCS form passes.

@nikomatsakis
Copy link
Contributor

(I suspect though this second form is a dup of #18899)

@nikomatsakis
Copy link
Contributor

Ah, no, I think what is happening is that the 'c in the trait is actually inferred to be bivariant, since it does not appear in the interface itself. My variance branch would fix it by making bivariance illegal, but I feel like that's papering over the problem -- I want to get a better understanding of what's going wrong here.

@nikomatsakis
Copy link
Contributor

(I presume the problem is that the variance inference must take the 'f:'c relationship into account, which it currently does not)

@nikomatsakis nikomatsakis self-assigned this Nov 19, 2014
@nikomatsakis
Copy link
Contributor

I'll take care of this.

bors added a commit that referenced this issue Dec 18, 2014
Closes #5988.
Closes #10176.
Closes #10456.
Closes #12744.
Closes #13264.
Closes #13324.
Closes #14182.
Closes #15381.
Closes #15444.
Closes #15480.
Closes #15756.
Closes #16822.
Closes #16966.
Closes #17351.
Closes #17503.
Closes #17545.
Closes #17771.
Closes #17816.
Closes #17897.
Closes #17905.
Closes #18188.
Closes #18232.
Closes #18345.
Closes #18389.
Closes #18400.
Closes #18502.
Closes #18611.
Closes #18783.
Closes #19009.
Closes #19081.
Closes #19098.
Closes #19127.
Closes #19135.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants