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

Borrow checker gets confused by a conditionally returned borrows #12147

Closed
huonw opened this issue Feb 10, 2014 · 6 comments
Closed

Borrow checker gets confused by a conditionally returned borrows #12147

huonw opened this issue Feb 10, 2014 · 6 comments

Comments

@huonw
Copy link
Member

huonw commented Feb 10, 2014

fn foo<'a>(x: &'a mut int) -> &'a mut int {
    {
        let y = &mut *x;
        if *y == 1 {
            return y;
        }
    }
    {
        let y = &mut *x;
        if *y == 2 {
            return y;
        }
    }
    fail!()
}

fn main() {}
conditional-return.rs:11:21: 11:28 error: cannot borrow `*x` because it is already borrowed as mutable
conditional-return.rs:11         let mut y = &mut *x;
                                             ^~~~~~~
conditional-return.rs:5:21: 5:28 note: previous borrow of `*x` as mutable occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `*x` until the borrow ends
conditional-return.rs:5         let mut y = &mut *x;
                                            ^~~~~~~
conditional-return.rs:17:2: 17:2 note: previous borrow ends here
conditional-return.rs:3 fn foo<'a>(x: &'a mut int) -> &'a mut int {
...
conditional-return.rs:17 }
                         ^

Can be worked around by going via * with some unsafe:

fn foo<'a>(x: &'a mut int) -> &'a mut int {
    {
        let y = &mut *x;
        let yy: *mut int = y;
        unsafe {
            if *yy == 1 {
                return &mut *yy;
            }
        }
    }
    {
        let y = &mut *x;
        let yy: *mut int = y;
        unsafe {
            if *yy == 2 {
                return &mut *yy;
            }
        }
    }
    fail!()
}

fn main() {}
@huonw
Copy link
Member Author

huonw commented Feb 10, 2014

cc @nikomatsakis

@flaper87
Copy link
Contributor

cc @flaper87

@nikomatsakis
Copy link
Contributor

This is not a bug, actually. Or, put another way, it's a dup of #6393.

@nikomatsakis
Copy link
Contributor

The problem is that the borrow let y = &mut *x must borrow x for the entire lifetime 'a, not just for the block, because y is returned (that is, y escapes the block).

@TyOverby
Copy link
Contributor

Here is another case of this bug in action:

/// Try to fetch a resource.  If the resource has not been loaded yet, block
/// until it is loaded.                                                     
fn fetch_block<'a>(&'a mut self, path: &str) -> Result<&'a Vec<u8>, E> {    
    loop {                                                                  
        match self.fetch(path) {                                            
            Ok(Some(x)) => { return Ok(x); },                               
            Ok(None) => { continue; },                                      
            Err(e) => { return Err(e); }                                    
        }                                                                   
    }                                                                       
}                          
/Users/tyoverby/workspace/rust/asset_store/src/lib.rs:62:19: 62:23 error: cannot borrow `*self` as mutable more than once at a time
/Users/tyoverby/workspace/rust/asset_store/src/lib.rs:62             match self.fetch(path) {
                                                                           ^~~~
/Users/tyoverby/workspace/rust/asset_store/src/lib.rs:62:19: 62:23 note: previous borrow of `*self` occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `*self` until the borrow ends
/Users/tyoverby/workspace/rust/asset_store/src/lib.rs:62             match self.fetch(path) {
                                                                           ^~~~
/Users/tyoverby/workspace/rust/asset_store/src/lib.rs:68:6: 68:6 note: previous borrow ends here
/Users/tyoverby/workspace/rust/asset_store/src/lib.rs:60     fn fetch_block<'a>(&'a mut self, path: &str) -> Result<&'a Vec<u8>, E> {
...
/Users/tyoverby/workspace/rust/asset_store/src/lib.rs:68     }
                                                             ^
error: aborting due to previous error

@nikomatsakis
Copy link
Contributor

This is a dup of #6393. What is happening is that the borrow extends to the end of the scope -- which is outside of the fn as a whole. Since the returns in all these cases are conditional, the borrow is considered to remain in force for the remainder of the fn.

flip1995 pushed a commit to flip1995/rust that referenced this issue Jan 25, 2024
…or, r=llogiq

Find function path references early in the same lint pass

This removes a visitor that existed to collect paths to functions in a context where the exact signature is required in order to cancel the lint.
E.g. when there's a `let _: fn(&mut i32) = path_to_fn_ref_mut_i32;` statement somewhere in the crate, we shouldn't suggest removing the mutable reference in the function signature.

It was doing a whole pass through the crate at the end, which seems unnecessary.
It seems like we should be able to add entries to the map in the same lint pass.
The map is untouched all the way until `check_crate_post` (at which point it will be populated by the visitor and finally checked), so it doesn't seem like this changes behavior: it will only be fully populated by the time we reach `check_crate_post` no matter what.

I don't think this will have a significant perf impact but it did show up in a profile with 0.5% for a crate I was looking into and looked like a low hanging fruit.

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants