RFC: Add Option::replace to the core library

[Kerollmops]

Add the method Option::replace to the core library. This method makes possible to easily replace the value of an option to some value (Some(value)).

Rendered

Tracking issue

let mut x = Some(2);
let old = x.replace(5);
assert_eq!(x, Some(5));
assert_eq!(old, Some(2));

let mut x = None;
let old = x.replace(3);
assert_eq!(x, Some(3));
assert_eq!(old, None);

[Kerollmops]

Conversation started on the Extra methods for Option comments.
Related to the Add give method to Option<T> thread to.

[matthieu-m]

I do like the "replace" name more than the "give" one.

[joshtriplett]

There are two potential interpretations of this method, and I think both are useful operations.

One is the one proposed here: given an x, turn the Option into Some(x) and return its previous value (whether None or Some).

The other is this: given an x, look at the Option, if it's Some then replace its value with x, if it's None then leave it None.

[Kerollmops]

So in this case we can change the name of this method by give or make a more obvious documentation, telling that there is no condition to replace the value. In the examples I provide, the first potential interpretation is explicit but I understand you remark.

[Kerollmops]

I understand the second potential interpretation but it can be a strange function signature if we return an Option and the self value is None what do we do about the argument we give ? We drop it ? No ! So we return it and so the optional return is useless, return T directly.

What do you think ?
It’s not the actual discution here but it’s a good thing to report, thank you !

[joshtriplett]

@Kerollmops No, I think Option::replace is exactly the right name for the method proposed here. I only commented because I think the other method ought to exist too, and I wonder what we should call it, for a good contrast with replace.

[synek317]

Why not

pub fn replace<T>(&mut self, value: Into<Option<T>>) -> Option<T> {
    mem::replace(self, value.into())
}

?

Then take could be just self.replace(None),

Btw, I don't find it very useful, but if we want this in stdlib then I think it should be as generic as possible.

[Kerollmops]

If we add your kind of Option::replace, for me too generic, there is no other interest of not using mem::replace directly.

The actual Option::replace method I want to had is just here to symmetry the actual Option::take and provide a less complex way to just replace the actual value by a Some value. The Option::take method already does that but with a None.

[matthieu-m]

@joshtriplett Wouldn't the second kind of replace be a map_in_place?

[hadronized]

That’d be called map_mut.

[hadronized]

How do you replace a value inside an Option, you can use mem::replace but it can be really unconvenient to import the mem module just for that. Why not adding a useful method to do that ? This is the same kind of method than the take one.

There’re a lot of ways to go with that:

let x = Some(3);
let y = x.map(|a| a * 10);

or, if you want mutable:

let mut x = Some(3);
if let Some(ref mut a) = x {
  *a = *a * 10;
}

You can add such as a method, but I’m not sure it’s that useful, as it’d somehow encourage people to have more mut stuff around.

Also, please replace that paragraph about mem::replace. We do not need that, and it’s completly unsafe.

[Kerollmops]

The problem with the first is that you don't change the value in-place, you change that:

let mut x = Some(3);
x = x.map(|a| a * 10);

But in this case you don't retrieve the old value.

The second solution has the same disavantage, you can't retrieve the old value or by changing the code.

And Option::take need a mut too, this Option::replace method has to be a symmetry to it.

You don't think it's more convenient to write this ?

let mut x = Some(3);
let y = x.replace(30);

[hadronized]

Ok I got what you want.

fn replace<A>(a: &mut Option<A>, v: A) -> Option<A> {
  let old = a.take();
  *a = Some(v);
  old
}

[Kerollmops]

In the rendered file I provide a simple implementation like that:

fn replace<T>(&mut self, value: T) -> Option<T> {
    mem::replace(self, Some(value))
}

More easy to understand and mimic in some way the current Option::take implementation.

[hadronized]

Actually, it’d make a good candidate since most containers have a replace function.

[Kerollmops]

Thanks I will add that to the RFC. It's true, many containers have these replace methods.

[Kerollmops]

Ping! What do I need to do to make it be real ?

[Centril]

Triage ping random person in @rust-lang/libs.

[Kerollmops]

I will try to ping @SimonSapin, so 👍

[hadronized]

Please edit your Rendered link at top of this thread to something that automatically updates by itself. Also, I think the RFC should be added the comment from @joshtriplett about the other interpretation you could have of Option::replace.

Given an x, if it’s Some(_), replace the content, otherwise, do nothing. However, can you please type your proposition, @joshtriplett? Because what you described is actually Option::map. I guess you meant:

impl<T> Option<T> {
  fn map_in_place(&mut self, x: T) { // or map_mut, or whatever name suits you
    if let Some(ref mut inner) = *self {
      *inner = x;
    }
  }
}

[Centril]

Please edit your Rendered link at top of this thread to something that automatically updates by itself.

I went ahead and fixed this :)

[hadronized]

Also, something you should address in the RFC drawback / unresolved section: you might introduce a breaking change. Imagine the following code:

trait Replace {
  type Item;
  
  fn replace(&mut self, x: Self::Item) -> Self;
}

impl<T> Replace for Option<T> {
    type Item = T;
    
    fn replace(&mut self, x: Self::Item) -> Self {
        mem::replace(self, Some(x))
    }
}

Introducing your Option::replace would break it.

[rfcbot]

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Kimundi
• @SimonSapin
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Kerollmops]

@Centril doesn't the FCP last 10 days ? 😄

[SimonSapin]

@Kerollmops It does, but only starts after enough team member reviews. Ping @alexcrichton, @sfackler, @withoutboats

[Centril]

@Kerollmops this is only in PFCP atm and requires team members to ✅ their boxes.

[Centril]

@SimonSapin damn you are fast :)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[thomas-huet]

How would

let mut x = Some(2);
let old = x.replace(5);

be better than

let mut x = Some(2);
x = Some(5);

It seems to me that this function is not worth putting in the standard library.

[Kerollmops]

As the RFC explains it, the Option::replace method is here to mimic the Option::take one. Furthermore the replace method returns the old value as you shows it.

[frol]

UPDATE: There is a separate RFC #2490 for this now.

---

I wonder if somebody has already discussed something like

    #[inline]
    fn replace_with<F>(&mut self, f: F) where F: FnOnce(Option<T>) -> Option<T> {
        let mut x = f(self.take());
        mem::swap(self, &mut x);
        debug_assert!(x.is_none());
        mem::forget(x);
    }

The use-case is somewhat similar to Cell::update (#2171 / rust-lang/rust#49727). Here is how I have to write the code at the moment (link):

let mut merged = merge(lower_node.right.take(), Some(greater_node));
mem::swap(&mut lower_node.right, &mut merged);
mem::forget(merged);
Some(lower_node)

NOTE: If I replace the mem::* incantations with a simple assignment (lower_node.right = merged;), the code still works, but the performance gets worse (the benchmark shows x1.5 slowdown of the overall execution time): https://barrielle.cedeela.fr/research_page/dropping-drops.html

Here is what I would love to write instead:

lower_node.right.replace_with(|node| merge(node, Some(greater_node)));
Some(lower_node)

@thomas-huet The two blocks of code you provide are not equivalent. The equivalent of the first block would be:

let mut x = Some(2);
let old = x.take();
x = Some(5);

Well, this is still not a complete equivalent as it would make unnecessary None assignment to the x on the second line.

[Kerollmops]

This is another discussion and it has already been talked in this thread. Why not opening an RFC ?

The real equivalent of the Option::replace function is the implementation in itself:

let old = mem::replace(&mut self, Some(value));

[frol]

This is another discussion and it has already been talked in this thread. Why not opening an RFC ?

I am new to this process and this issue was the closest that I could have found on the tracker, so I decided to ask here first. I will try to prepare a separate RFC.

[thomas-huet]

@frol I understand that the first block creates the variable old and the second one does not. I'm just not sure if that this added variable is useful.

Actually my main problem with this RFC is its motivation: option can be seen as a container (I guess it depends on your definition of container but I would think that a container is not limited to a size of one) and some other containers have a replace function so let's make one for options too. I think the criterion for inclusion in the standard library should be: this is a common pattern that is being reimplemented in many projects (preferably give a list of projects here) and it would be nice to have a default implementation that everyone can use. It may be the case that it's a common pattern but I haven't seen any evidence of that in the RFC.

[raindev]

I second what @thomas-huet said: no strong arguments to include Option::replace into the standard library were presented. Symmetry with Option::take is not important if the API is not commonly useful. And there were no real examples presented where adding the method would improve the code. Personal data point: I don't remember wishing to have such a method working with Option in Rust (or "optional" type in other languages).

[Kerollmops]

I add faced some cases where this method could have been useful. And I think this is a method that must be present to help newcomers use the standard library. It feels to me like a lack to not have this method, because we have a similar one named take that do some sort of the contrary.

struct Kitchen {
    chef: Option<Chef>,
}

impl Kitchen {
    fn remove_the_chef(&mut self) -> Option<Chef> {
        self.chef.take()
    }

    fn replace_the_chef(&mut self, new: Chef) -> Option<Chef> {
        self.chef.replace(new)
    }
}

[frol]

@raindev Well, here you go some examples from Rust compiler internals:

• https://github.com/rust-lang/rust/blob/142c98dd5a9fbd60c13e62a5c1358a40ee622dbb/src/libstd/sync/mpsc/sync.rs#L475-L476

• https://github.com/rust-lang/rust/blob/142c98dd5a9fbd60c13e62a5c1358a40ee622dbb/src/librustc/infer/sub.rs#L53-L56

[raindev]

@Kerollmops, @frol, thanks for the examples.

Naively repping Rust's codebase (which is a massive project) there are 3 places where mem::replace is used with Option and the result is not discarded:

$ grep -RE 'mem::replace\(.*, Some'
src/libstd/thread/local.rs:        mem::replace(&mut *ptr, Some(value));
src/libstd/sync/mpsc/sync.rs:        let prev = mem::replace(&mut self.buf[pos], Some(t));
src/librustc_mir/interpret/eval_context.rs:        mem::replace(&mut self.locals[local], Some(Value::Scalar(Scalar::undef())))
src/librustc_mir/dataflow/move_paths/builder.rs:                mem::replace(&mut move_paths[parent].first_child, Some(move_path));
src/librustc/infer/sub.rs:        let old_cause = mem::replace(&mut self.fields.cause, Some(cause));

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

@SimonSapin there has been some recent discussion on the usefulness of the operation... Is your position unchanged that it should be merged? If so, I'll run the RFC merge procedure.

[frol]

FYI, I have just created RFC #2490 for Option::replace_with.

[Centril]

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#51998