rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.23k stars 12.7k forks source link

Tracking Issue for `OnceCell/Lock::try_insert()` #116693

Open daxpedda opened 1 year ago

daxpedda commented 1 year ago

Feature gate: #![feature(once_cell_try_insert)]

This is a tracking issue for OnceCell::try_insert() and OnceLock::try_insert().

This adds a method similarly to OnceCell/Lock::set() but returns a reference at the same time. This is also similar to OnceCell/Lock::get_or_init() but the return value also tells you if the value was actually inserted or if the OnceCell/Lock was already occupied.

Public API

impl<T> OnceCell<T> {
    pub fn try_insert(&self, value: T) -> Result<&T, (&T, T)>;
}

Steps / History

Unresolved Questions

ChayimFriedman2 commented 10 months ago

Should the return type be Result<&T, (&T, T)> or (&T, Option<T>) or (&T, Result<T, ()>)? It is easier to use the (&T, Option<T>) version but it does not encode the fact that there was a failure to insert ((&T, Result<T, ()>) encodes this better, but still not as good as Result<&T, (&T, T)>). On the other hand, the differences are not that significant since one is likely to match or the result of try_insert() anyway, since for try_insert().unwrap() we have the better set().unwrap().

tisonkun commented 8 months ago

I'm working on #121641 and wonder if we should add &mut both in the receiver and the result. Generally insert is a mutation and we can get a mut ref for the result if succeed.

Zollerboy1 commented 4 months ago

Is there a reason, why this takes a value directly instead of an FnOnce like get_or_init and friends do?

I would like to use this in a case where I have to know after a get_or_init if the call inserted a value or not, but with the current design it seems that I have to check with get first to avoid creating a value that's not actually needed.

daxpedda commented 4 months ago

I'm unsure how I missed these comments but here goes:

Should the return type be Result<&T, (&T, T)> or (&T, Option<T>) or (&T, Result<T, ()>)? It is easier to use the (&T, Option<T>) version but it does not encode the fact that there was a failure to insert ((&T, Result<T, ()>) encodes this better, but still not as good as Result<&T, (&T, T)>). On the other hand, the differences are not that significant since one is likely to match or the result of try_insert() anyway, since for try_insert().unwrap() we have the better set().unwrap().

Given that all these options fulfill the requirements, personally I'm still in favor of Result<&T, (&T, T)> because it seems to be the easiest to use.

I'm working on #121641 and wonder if we should add &mut both in the receiver and the result. Generally insert is a mutation and we can get a mut ref for the result if succeed.

I'm assuming you mean adding new &mut APIs, because altering the ones proposed here would defeat its purpose.

The only use case I know of is only valid if the value is shared, otherwise get_or_try_init() would cover it. So I'm unsure if a &mut API addition would bring any value to the table in this case, seeing that get_mut_or_try_init() exists.

Is there a reason, why this takes a value directly instead of an FnOnce like get_or_init and friends do?

I would like to use this in a case where I have to know after a get_or_init if the call inserted a value or not, but with the current design it seems that I have to check with get first to avoid creating a value that's not actually needed.

That sounds quite reasonable to me, I will make a PR proposing this change when I get to it.

daxpedda commented 4 months ago

Is there a reason, why this takes a value directly instead of an FnOnce like get_or_init and friends do? I would like to use this in a case where I have to know after a get_or_init if the call inserted a value or not, but with the current design it seems that I have to check with get first to avoid creating a value that's not actually needed.

That sounds quite reasonable to me, I will make a PR proposing this change when I get to it.

While implementing it I realized that this is a bit different.

This API is more similar to set(), which takes a value as well. Taking a function would have different tradeoffs, e.g. any values owned by this function would be lost, users would have to use workarounds like this:

let mut value = Some(value);

match self.try_insert(|| value.take().unwrap()) {
    Ok(value) => do_your_thing(),
    Err(old_value) => {
        let new_value = value.unwrap();
        do_your_other_thing();
    }
}

So I think this would need another API addition.