Is this use of unsafe trivially safe?

Question

I'm running into a Rust borrow checker error that I believe is a limitation of the current implementation of non-lexical lifetimes. The code I want to write looks something like this:

struct Thing {
    value: i32
}

impl Thing {
    fn value(&self) -> &i32 {
        &self.value
    }
    fn increment(&mut self) {
        self.value += 1;
    }
}

/// Increments the value of `thing` if it is odd, and returns a reference to the value.
fn increment_if_odd(thing: &mut Thing) -> &i32 {
    let ref_to_value = thing.value();
    if (*ref_to_value % 2) == 0 {
        return ref_to_value;
    }
    thing.increment();  // fails to compile because the immutable borrow `ref_to_value` is still alive
    thing.value()
}

Rust Playground.

First question: am I right in thinking that this code is 100% safe and the borrow checker is being too conservative? The branch that returns ref_to_value doesn't mutate thing so the reference is guaranteed to be valid, and the other branch doesn't use ref_to_value at all. (I understand that if I replace return ref_to_value; with return thing.value(); it will compile, but in my actual code the value method is expensive.)

It seems I can fix this by "laundering" the reference through a pointer:

if (*ref_to_value % 2) == 0 {
    return unsafe {
        &*(ref_to_value as *const i32)
    }
}

Second question: is this trivially safe? I've never used unsafe before so I'm nervous.

I guess a third question: is there a way to rewrite this in safe Rust? The constaint is that value should only be called once on the non-mutating path.

Lactalbumin

You can not do that without the unsafe or without droping the reference first (since you cant hold the non mutable reference while holding the other). Why does the access to thing is expensive if it is a reference to something? – Jetpropelled

I would say the compiler is buggy here. Because by enforcing an extra escope, the reference should be dropped. But it i not. play.rust-lang.org/… – Jetpropelled

even explicit drop doesnt work: play.rust-lang.org/… – Jetpropelled

Take a look at the RFC for non-lexical lifetimes. The third example is similar to yours, and the workaround in the fourth can be adapted. – Peplum

Is this implying that because the return value needs to live as long as thing that therefore so does ref_to_value? That's weird. Even with the extra scope brackets from @Jetpropelled it doesn't work because of such. That's what I'm reading from your link @Peplum – Waldack

This isn't a compiler bug, but rather a known limitation. I know we have several duplicates of this question, but I wasn't able to find one right now. I'll keep digging. – Neace

@Netwave: in my actual code, the method is not a trivial getter. It executes some if branches to determine which reference to return. It appears in an extremely performance-sensitive part of my project where every CPU instruction counts. – Lactalbumin

@JamesFennell maybe you could improve on that, by caching the decision somehow, or making some hashable state? – Jetpropelled

Would something like this work for you? It moves the increment operation onto a newtype for the value, which is obtained by mutably borrowing the Thing. – Samoyedic

Or perhaps this? The value method now returns an accessor for the value rather than a reference to the value itself. – Samoyedic

I'd add that with Polonius the code in question already compiles - play.rust-lang.org/…. – Nebulose

Answer 1

The reason the compiler won't allow your code is because ref_to_value must have a lifetime at least as long as the lifetime of the increment_if_odd in order to be returned.

If you add back in the elided lifetimes, ref_to_value must have lifetime 'a. And it's my understanding that the compiler can't change the lifetime of a reference. One way to write safe rust to get around this is to make ref_to_value mutable, and modify Thing::increment.

What your unsafe code does is allow the compiler to give ref_to_value a shorter lifetime, and the new reference, created by casting the pointer, lifetime 'a. I think your unsafe code is "safe" because none of rust's borrowing rules are broken, and we know that the new reference won't outlive the data.

struct Thing {
    value: i32
}

impl Thing {
    fn value(&self) -> &i32 {
        &self.value
    }
    fn mut_value(&mut self) -> &mut i32{
        &mut self.value
    }
    fn increment(val: &mut i32) {
        *val += 1;
    }
}

/// Increments the value of `thing` if it is odd, and returns a reference to the value.
fn increment_if_odd<'a>(thing: &'a mut Thing) -> &'a i32 {
    
    let ref_to_value : &'a mut i32 = thing.mut_value();
    if (*ref_to_value % 2) != 0 {
        Thing::increment(ref_to_value);
    }
    ref_to_value
}

Isacco

Answer 2

Might be safe, haven't looked at it closely enough. On the other hand you could rewrite the function as:

fn increment_if_odd(thing: &mut Thing) -> &i32 {
    if (*thing.value() % 2) != 0 {
        thing.increment();
    }
    thing.value()
}

Behest

Thanks! This calls value twice though, which I want to avoid. In my code value is expensive. – Lactalbumin

@JamesFennell ah okay. The problem is that a &mut self in increment means to the compiler that any of self's data/memory can change and invalidate the data behind the reference returned by value(), unsafe is fine if you know you won't invalidate it but is strongly discouraged as it makes it easy to break if you change the behaviour in the future. – Behest

If increment mutates different data to what is returned by value() then I would recommend splitting the Thing struct up into two separate structs. Then you can mutate one side without invalidating immutable references from the other. Otherwise maybe create a value_mut() and then have increment take a &mut i32 rather than &mut self...? Might need some more context to give a better suggestion :) – Behest

The key point though is that there are two branches, and mutation cannot occur along the branch that returns the reference, so the code is fine. It's the borrow checker being too conservative. – Lactalbumin

i don't know that lang, but why not x & 1 == 1? – Kathrynkathryne