add insert to Option

[Canop]

This removes a cause of unwrap and code complexity.

This allows replacing

option_value = Some(build());
option_value.as_mut().unwrap()

with

option_value.insert(build())

It's also useful in contexts not requiring the mutability of the reference.

Here's a typical cache example:

let checked_cache = cache.as_ref().filter(|e| e.is_valid());
let content = match checked_cache {
	Some(e) => &e.content,
	None => {
	    cache = Some(compute_cache_entry());
	    // unwrap is OK because we just filled the option
	    &cache.as_ref().unwrap().content
	}
};

It can be changed into

let checked_cache = cache.as_ref().filter(|e| e.is_valid());
let content = match checked_cache {
	Some(e) => &e.content,
	None => &cache.insert(compute_cache_entry()).content,
};

(edited: I removed insert_with)

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LukasKalbertodt (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[timvermeulen]

I've wanted this for a while. 👍 You could consider also changing Option::get_or_insert_with to call insert, getting rid of its unsafe block.

[Stargateur]

I will better use replace() naming and so naming it replace_and_get, also this add the old value as a return:

fn replace_and_get(&mut self) -> (&mut T, Option<T>)

This allow a more generic use of this feature

Current alternative to unwrap:

let checked_cache = cache.as_ref().filter(|e| e.is_valid());
let content = match checked_cache {
	Some(e) => &e.content,
	None => {
	    cache = None;
	    &checked_cache.get_or_insert_with(build).content
	}
};

[petervaro]

Thumbs up for replace_and_get or insert_and_get. The main problem though with these names is the addition of the _with part: it will look rather clunky: replace_with_and_get or insert_with_and_get which is breaking the current convention where _with is a suffix.

[Stargateur]

@petervaro I see no reason to have with variant, cause the function will always be call.

[petervaro]

@Stargateur that's fair!

[Canop]

The name insert is consistent with the intent, with other collections and with get_or_insert.

The goal isn't to get the old value, using a tupple would add verbosity while there's already take when we need this value.

[Canop]

I agree insert_with doesn't bring a lot of value. I added it for consistency with previous functions but it could be removed.

[timvermeulen]

I also think insert_with is probably best left out, existing methods such as Option::replace and Option::xor don't have a _with variant either because the argument is always evaluated.

[timvermeulen]

If we're changing this method anyway, might as well call insert:

match self {
    Some(v) => v,
    None => self.insert(f()),
}

(GitHub didn't let me add this as a change suggestion)

[pickfire]

Don't we already have Option::get_or_insert? Why do we need this? https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.get_or_insert have the exact same signature

pub fn get_or_insert(&mut self, v: T) -> &mut T

@jyn514 IMHO v is short and clear, please don't change it to val.

It even have similar code, I don't see why is this necessary. https://doc.rust-lang.org/stable/src/core/option.rs.html#852-861

@Canop maybe get_or_insert is what you are looking for? Or is there any other reason to create a similar function?

[Stargateur]

@pickfire No there are not equivalent, if option is Some the feature proposed here discard previous value, get_or_insert() doesn't discard previous value but on the contrary discard the value given in parameter.

[Canop]

Question is: Should I do something, remove or add something, in order for the PR to be merged ?

[jyn514]

@Canop this is waiting on review from @LukasKalbertodt , no one who's reviewed so far is on T-libs.

[m-ou-se]

Nice! (I was just looking for this method a few days ago.)

It might be good to consider some other possible names. insert matches nicely with get_or_insert, but maybe we can come up with something that makes it clearer any existing value is overwritten/dropped. But I think it's fine to unstably add it now as insert and leave the final naming decision until the stabilization PR. By then we'll have some experience using this, which might give some more ideas for a name, or confirm that the current name is good.

Can you open a tracking issue, add its number in the issue = ".." attribute, and address these two comments? 1 2

Thanks!

[m-ou-se]

Thanks for your contribution!

@bors r+ rollup

This PR is now approved and waiting in the queue to be tested more thoroughly before it gets merged. The queue is a bit long at the moment, so it might take a while before this PR is merged. After it's merged, your new function should be available (unstably, feature gated) in the next nightly Rust. Once it's been in a few releases and all questions on the tracking issue are resolved, stabilization can be proposed, which will then require approval from the majority of the libs team.

[bors]

📌 Commit 216d0fe has been approved by m-ou-se

[m-ou-se]

This is now available in nightly Rust: https://doc.rust-lang.org/nightly/std/option/enum.Option.html#method.insert