rust-lang/rust

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

Opened this issue · 7 comments

daxpedda commented

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

  • None yet.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html ↩

ChayimFriedman2 commented

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

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

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

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

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.

CodesInChaos commented

@Zollerboy1 wrote:

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.

For that use case you can use this pattern:

let mut new = false;
let reference = cell.get_or_init(|| {
    new = true;
    1
});
if new {}

If the initalizer terminates successfully, the newly generated value will always be used to initialize the cell. And since get_or_init propagates panics, an unsuccessful termination will not reach if new.

@daxpedda wrote:

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.

Can you show a motivating example where this API is easiest to use? The example you showed in the ACP would look simpler using the (&T, Option<T>) or (&T, Result<(), T>) variants:

let (value, my_value) = VALUE.try_insert(calculate_value());
println!("report value: {value:?}");
if let Err(my_value) = my_value {
    assert_eq!(value, &my_value, "somehow calculated values differed");
}
daxpedda commented

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.

Can you show a motivating example where this API is easiest to use? The example you showed in the ACP would look simpler using the (&T, Option<T>) or (&T, Result<(), T>) variants:

let (value, my_value) = VALUE.try_insert(calculate_value());
println!("report value: {value:?}");
if let Err(my_value) = my_value {
    assert_eq!(value, &my_value, "somehow calculated values differed");
}

match VALUE.try_insert(calculate_value()) {
    Ok(value) => println!("report value: {value:?}"),
    Err(old_value, new_value) => assert_eq!(old_value, new_value, "somehow calculated values differed"),
}

I still find Result<&T, (&T, T)> simpler, I guess its subjective.
In a way (&T, Option<T>) or (&T, Result<(), T>) would be simpler if you want to ignore the Option/Result, but simpler APIs exist in this case. So it seems to me that returning a Result upfront is the most appropriate return type for this API.

I'm sure more qualified Rustaceans are able to more objectively assess the pros and cons here.