RFC: Add a replace_with method to Option by frol · Pull Request #2490 · rust-lang/rfcs (original) (raw)

UPDATE: There was a relevant RFC about the ideas I describe below, so feel free to ignore my message.

I have just had a conversation where this replace_with idea had bubbled up in yet another context, that is how to "toggle" enum variants when there is a non-Copy value inside:

#[derive(Debug)] struct Bar;

#[derive(Debug)] enum Foo { A(Bar), B(Bar), }

#[derive(Debug)] struct Baz { foo: Foo, }

impl Baz { fn switch_variant_unsafe(&mut self) { let mut foo_temp: Foo = unsafe { ::std::mem::uninitialized() }; ::std::mem::swap(&mut self.foo, &mut foo_temp); self.foo = match foo_temp { Foo::A(bar) => Foo::B(bar), Foo::B(bar) => Foo::A(bar), } }

fn switch_variant_safe(&mut self) {
    self.foo = match self.foo {
        Foo::A(bar) => Foo::B(bar),
        Foo::B(bar) => Foo::A(bar),
    })
}

}

fn main() { let mut baz = Baz { foo: Foo::A(Bar) }; baz.foo = match baz.foo { Foo::A(bar) => Foo::B(bar), Foo::B(bar) => Foo::A(bar), }; dbg!(&baz); baz.switch_variant_unsafe(); dbg!(&baz); baz.switch_variant_safe(); dbg!(&baz); }

Playground

As is, you get a compilation error:

error[E0507]: cannot move out of borrowed content
  --> src/main.rs:26:26
   |
26 |         self.foo = match self.foo {
   |                          ^^^^^^^^
   |                          |
   |                          cannot move out of borrowed content
   |                          help: consider borrowing here: `&self.foo`
27 |             Foo::A(bar) => Foo::B(bar),
   |                    --- data moved here
28 |             Foo::B(bar) => Foo::A(bar),
   |                    --- ...and here
   |
note: move occurs because these variables have types that don't implement the `Copy` trait
  --> src/main.rs:27:20
   |
27 |             Foo::A(bar) => Foo::B(bar),
   |                    ^^^
28 |             Foo::B(bar) => Foo::A(bar),
   |                    ^^^

There is also a similar question on SO.

Here is the helper that I came up with (based on this RFC):

fn replace_with<T, F>(dest: &mut T, mut f: F) where F: FnMut(T) -> T, { let mut old_value = unsafe { std::mem::uninitialized() }; std::mem::swap(dest, &mut old_value); // dest is "uninitialized" (in fact, it is not touched in release mode) let mut new_value = f(old_value); std::mem::swap(dest, &mut new_value); // dest holds new_value, and new_value is "uninitialized" std::mem::forget(new_value); // since it is "uninitialized", we forget about it }

, and then we can implement switch_variant_safe as:

fn switch_variant_safe(&mut self) {
    replace_with(&mut self.foo, |foo| match foo {
        Foo::A(bar) => Foo::B(bar),
        Foo::B(bar) => Foo::A(bar),
    })
}

The generated assembly for switch_variant_unsafe and switch_variant_safe (with replace_with helper) is identical in release mode.

As to the unsoundness concerns raised in #2490 (comment), in release mode, replace_with gets compiled to:

example::replace_with: mov al, byte ptr [rdi] not al and al, 1 mov byte ptr [rdi], al ret

in fact, it gets automatically inlined unless I put #inline(never):

    mov     al, byte ptr [rsp + 7]
    not     al
    and     al, 1

Thus, there is no unsoundness in the release mode. Yet, in debug mode, there is indeed an explicit uninitialized value gets assigned to the self.foo. I have tried to implement replace_with using raw pointers, but failed at the point that f needs to take ownership ("cannot move out of dereference of raw pointer"). I have also been pointed to std::panic::UnwindSafe, yet I haven't figured out what is the proper use of it.

Another way to implement enum variant "toggle" is to pass ownership to self and return Self:

fn switch_variant_owned(mut self) -> Self {
    self.foo = match self.foo {
        Foo::A(bar) => Foo::B(bar),
        Foo::B(bar) => Foo::A(bar),
    };
    self
}

, but it requires the API changes all the way down to the method (i.e. you have to use this API style all the way through your codebase if it is a low-level method).

Sidenote, while the generated assembly is mostly the same (there are some variables rearrangements), there is one interesting optimization gets applied when switch_variant_owned is implemented, that is xor al, 1 instead of not al; and al, 1.

I believe, there is a need for safe and sound std::mem::replace_with. What do you think?

P.S. This more generic std::mem::replace_with can easily resolve the need for Option::replace_with covered in this RFC. Instead of the proposed:

let mut some_option: Option = Some(123);

some_option.replace_with(|old_value| consume_option_i32_and_produce_option_i32(old_value));

I would write

let mut some_option: Option = Some(123);

std::mem::replace(&mut some_option, |old_value| consume_option_i32_and_produce_option_i32(old_value));