Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 of 4 tasks
daxpedda opened this issue Oct 13, 2023 · 5 comments
Open
2 of 4 tasks

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

daxpedda opened this issue Oct 13, 2023 · 5 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@daxpedda
Copy link
Contributor

daxpedda commented Oct 13, 2023

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

@daxpedda daxpedda added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 13, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 14, 2023
…, r=Mark-Simulacrum

Implement `OnceCell/Lock::try_insert()`

I took inspiration from [`once_cell`](https://crates.io/crates/once_cell):
- [`once_cell::unsync::OnceCell::try_insert()`](https://github.com/matklad/once_cell/blob/874f9373abd7feaf923a3b3c34bfb3383529c671/src/lib.rs#L551-L563)
- [`once_cell::sync::OnceCell::try_insert()`](https://github.com/matklad/once_cell/blob/874f9373abd7feaf923a3b3c34bfb3383529c671/src/lib.rs#L1080-L1087)

I tried to change as little code as possible in the first commit and applied some obvious optimizations in the second one.

ACP: rust-lang/libs-team#276
Tracking issue: rust-lang#116693
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 15, 2023
Rollup merge of rust-lang#116540 - daxpedda:once-cell-lock-try-insert, r=Mark-Simulacrum

Implement `OnceCell/Lock::try_insert()`

I took inspiration from [`once_cell`](https://crates.io/crates/once_cell):
- [`once_cell::unsync::OnceCell::try_insert()`](https://github.com/matklad/once_cell/blob/874f9373abd7feaf923a3b3c34bfb3383529c671/src/lib.rs#L551-L563)
- [`once_cell::sync::OnceCell::try_insert()`](https://github.com/matklad/once_cell/blob/874f9373abd7feaf923a3b3c34bfb3383529c671/src/lib.rs#L1080-L1087)

I tried to change as little code as possible in the first commit and applied some obvious optimizations in the second one.

ACP: rust-lang/libs-team#276
Tracking issue: rust-lang#116693
@ChayimFriedman2
Copy link
Contributor

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
Copy link
Contributor

tisonkun commented Feb 26, 2024

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants