In Option::get_or_insert_with(), forget the None instead of dropping it.

[kpreid]

Per #148486 (comment)

In Option::get_or_insert_with(), after replacing the None with Some, forget the None instead of dropping it.

This allows eliminating the T: [const] Destruct bounds, making the functions more flexible in (unstable) const contexts, and avoids generating an implicit drop_in_place::<Option<T>>() that will never do anything (and which might even persist after optimization).

[rustbot]

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[kpreid]

cc @cyb0124 (this change is based on their idea)

[hkBst]

Thanks! Looks good to me.

View changes since this review

[clarfonthey]

Presumably, this should have FIXME(const-hack) at the beginning of the comment since, presumably, const_live_precise_drops should be able to account for this. Or, if it's ultimately unable to be removed, that bit can be removed, but we don't know that for now.

[kpreid]

I get the maintenance principle you're going for, but on further consideration of this code, const_precise_live_drops is actually a red herring and I should not have brought it up. const_precise_live_drops is IIUC about distinguishing places that are possibly dropped from places that are always moved out of (where the compiler currently assumes all places are possibly dropped), but in this case, the compiler would need to look not just at the data flow in the code, but also the values in those places — the fact that self == None at this point. It would have to, stably, follow the logic that:

• The compiler analyzed the body and proved that drop_in_place::<T>() was unreachable,
• therefore it's valid to omit T: [const] Destruct from this function signature.

Isn't that the sort of hard-to-have-guarantees-about static analysis tarpit we don’t want in Rust's type system? Don't we want to stick to types and data-flow and not rely on statically knowing properties of values except for lints?

Accordingly, I have deleted the mention of const_precise_live_drops, and rephrased the comment to express that:

• This implementation allows the signature to be relaxed to T: [const] Destruct, benefiting const callers.
• Separately, this implementation avoids needless drop code, which is a performance benefit outside of const.

[clarfonthey]

That's very fair; I guess my impulse is to still put FIXME(const-hack) so that folks who are trying to close all of the weird gaps in the const logic know that this is partially to remove the [const] Destruct bound, and can see whether it would be worth it to ultimately improve the code so this hack isn't necessary.

And since a lot of the const stuff does revolve around making sure the drop checker, etc. is robust enough to handle these cases, it could be that it ends up removing drop glue for non-const cases too, like you mention how this helps here as well.

[kpreid]

That seems reasonable. I've pushed a new version; included here for convenience:

// This implementation strategy
//
// * avoids needing a `T: [const] Destruct` bound, to the benefit of `const` callers,
// * and avoids possibly compiling needless drop code (as would sometimes happen in the
//   previous implementation), to the benefit of non-`const` callers.
//
// FIXME(const-hack): It would be nice if this weird trick were made obsolete
// (though that is likely to be hard/wontfix).

[clarfonthey]

Looks good to me!