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

Returning a borrowed value extends the borrow to the end of the function #40307

Closed
SimonSapin opened this issue Mar 6, 2017 · 5 comments
Closed

Comments

@SimonSapin
Copy link
Contributor

Consider code reduced to this:

fn a(v: &mut Vec<String>) -> &str {
    match v.first() {
        Some(s) => s,
        None => {
            v.push("".to_owned());
            ""
        }
    }
}

It fails to build in current Rust, as it is the typical case where we want non-lexical borrows. But the usual work-around doesn’t work here

fn a(v: &mut Vec<String>) -> &str {
    match v.first() {
        Some(s) => return s,
        None => {}
    }
    // Move this here to try to work around non-lexical borrow
    v.push("".to_owned());
    ""
}
error[E0502]: cannot borrow `*v` as mutable because it is also borrowed as immutable
  --> a.rs:7:13
   |
4  |     match v.first() {
   |           - immutable borrow occurs here
...
7  |             v.push("".to_owned());
   |             ^ mutable borrow occurs here
...
11 | }
   | - immutable borrow ends here

error: aborting due to previous error

The borrow of v in v.first() ends at the end of the entire function, not merely at the end of the match expression. Returning something other than s compiles, as the borrow shrinks to the match expression.

Is it possible to make return not extend borrows to an entire function without fully implementing non-lexical borrow? Is there another work-around in this case?

@djzin
Copy link
Contributor

djzin commented Mar 6, 2017

v must be borrowed for the lifetime of the return value. Desugared, the function is:

fn a<'a>(v: &'a mut Vec<String>) -> &'a str {
    match v.first::<'a>() {
        Some(s) => return &**s as &'a str,
        None => {}
    }
    // Move this here to try to work around non-lexical borrow
    v.push("".to_owned());
    return "" as &'a str;
}

There is no way for it to be otherwise because the signature of Vec::first is fn first<'a>(&'a self) -> Option<&'a T>. So v must be borrowed for 'a.

I suspect what you are asking for may be unsound.

@djzin
Copy link
Contributor

djzin commented Mar 6, 2017

I did find a workaround, but it is specific to Vec

fn a(v: &mut Vec<String>) -> &str {
    match v.len() {
        0 => {
            v.push("".to_owned());
            ""
        },
        _ => &*v[0]
    }
}

@SimonSapin
Copy link
Contributor Author

Yes, that workaround is based on separately checking which case you’re on, then separately taking the borrow. In my non-reduce code the optional borrow is returned by a ~40 lines method that is not easily split into these two parts, unfortunately.

@SimonSapin
Copy link
Contributor Author

I think I’m now convinced that despite my earlier impression, fixing this is in fact non-lexical borrows. Closing in favor of rust-lang/rfcs#811.

@djzin
Copy link
Contributor

djzin commented Mar 6, 2017

Was playing around with this and came across unsoundness in a particular case that looks very similar; I think we need to be very careful around non-lexical borrows.

// will swap x and y provided the memory addresses are distinct
// in rust they always will be!
fn swap(x: &mut i32, y: &mut i32) {
    assert!(*x != 0 && *y != 0, "invalid inputs");
    *x ^= *y; *y ^= *x; *x ^= *y;
    assert!(*x != 0 && *y != 0, "can't happen");
}

struct Struct<'a> {
    reference: Option<&'a mut i32>,
    data: i32,
}

impl<'a> Struct<'a> {
    fn get(&'a mut self) -> Option<&'a i32> {
        match self.reference {
            Some(ref s) => Some(s),
            None => {
                self.reference = Some(&mut self.data);
                None
            }
        }
    }
    
    fn do_something(&mut self) {
        let x = &mut self.data;
        if let Some(ref mut y) = self.reference {
            swap(x, y);
        }
    }
    
    fn violate(&'a mut self) -> &'a i32 {
        // needed to overcome the borrow checker
        let self_ = unsafe { &mut *(self as *mut Struct<'a>) };
        match self.get() {
            Some(s) => s,
            None => {
                // we assume everything is OK to take another borrow
                // of self here, since None was returned
                self_.do_something();
                &self_.data
            }
        }
    }
}

fn main() {
    let mut s = Struct {
        reference: None,
        data: 42,
    };
    s.violate(); // uh-oh 
}

kyren added a commit to kyren/hashlink that referenced this issue Jul 6, 2019
There is a problem with the entry API which also exists with the `produce` API,
and that is it is difficult to maintain the correct cache size when producing a
new value.

When implemented naively, after the mutable reference to newly produced value is
obtained, the LruCache must be shrunk to fit the space requirements.  However,
this is not safe to do, as the returned reference must not be mutated or moved
while it is alive, so we cannot (in general, safely) shrink the underlying map
after obtaining this reference.

Less naively, you could hash the provided key and use the raw entry API to look
up the key, return an entry wrapping a raw entry if it is found, and otherwise
shrink the map by 1 and use the raw api to look up an entry again (using the
same computed hash).  This way, you would not exceed the cache capacity and hits
would be very fast, but a miss would incur two hash lookups (but not necessarily
two key hashes due to the raw API allowing pre-computed hashes).  The same basic
issue exists if you try to do this with a `produce` style API.  This is also
problematic, because trying to do this safely runs into
rust-lang/rust#40307

The simplest thing to do is simply let the cache grow by 1 too many elements and
document the behavior.
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

2 participants