Better way to replace a a field

There's a type Foo that takes a self parameter to do something, like this:

struct Foo {}
impl Drop for Foo {
    fn drop(&mut self) {
        todo!()
    }
}
impl Foo {
    fn consume_and_regenerate(self) -> Foo {
        Foo {} // it is complicated in the real case
    }
}
}

And a Bar type designed by me that hold a Foo:

struct Bar {
    foo: Foo,
}

impl Bar {
    fn update_foo(&mut self) {
       self.foo = self.foo.consume_and_regenerate();
    }
}

The code will not compile, because I can own the foo from a &mut foo, and here's my solution:

fn update_foo(&mut self) {
        let foo = replace(&mut self.foo, unsafe { MaybeUninit::uninit().assume_init() });
        let foo = foo.consume_and_regenerate();
        let foo = replace(&mut self.foo, foo);
        std::mem::forget(foo);
    }

Is there a better (safe) way to do this?

P.S. I don't use an foo: Option<Foo> because there's no case that the option can be none, using an Option<Foo> makes other codes bloated。

You can use replace_with to temporarily take out the value.

Note that doing this has caveats (read the documentation). If you can't stand them, then you must take the Option<Foo> approach.

3 Likes

Your solution may runs Foo's destructor with dangling reference when consume_and_regenerate panic and unwind. take_mut crate provides safe workaround by immediately kill the process on such unwinding.

1 Like

May off-topic here, do you choose "panic=abort" or not when you're writing rust programs?

Regarding choosing panic=abort:

panic=abort can be useful to reduce code size when that is a critical factor (because then the compiler doesn't need to generate code to clean up during unwinding), and there are some platforms (embedded and Wasm) where panic=unwind is not supported.

Regarding supporting panic=abort:

If you are writing a library for others to use, then you should, if at all possible, make your code panic=abort compatible by:

  • Not panicking (use Result etc. instead) if there is any chance that the error condition is something the caller wants to recover from and it isn't evidence of a bug in your library's code.
  • Not using catch_unwind or relying on thread JoinHandles being able to catch panics.
5 Likes

I think replace_with has ub.

An important note

On panic (or to be more precise, unwinding) of the closure f, default will be called to provide a > replacement value. default should not panic – doing so will constitute a double panic and will most likely abort the process.

Afaik. rust doesn't guarantee that panicing while unwinding aborts, so the following example has double free.

let mut string = String::new();

replace_with(
    &mut string, 
    || panic!(),  // if this panic doesn't cause abort `string` will be freed twice
    |s| { 
        drop(s); // `s´ dropped here
        
        panic!()
    }
);

If so, then perhaps you could file an issue.

I thought it did, since that's certainly what it's always done.

That seems to be what Don't allow unwinding from Drop impls by Amanieu · Pull Request #3288 · rust-lang/rfcs · GitHub says it does today too, though of course a proposed RFC isn't at all normative.

(I tried to look for it in the reference, but couldn't find any text about it.)