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 once_cell_try #109737

Open
1 of 4 tasks
Tracked by #21
tgross35 opened this issue Mar 29, 2023 · 14 comments
Open
1 of 4 tasks
Tracked by #21

Tracking Issue for once_cell_try #109737

tgross35 opened this issue Mar 29, 2023 · 14 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@tgross35
Copy link
Contributor

tgross35 commented Mar 29, 2023

This supercedes #74465 after a portion of once_cell was stabilized with #105587

Feature gate: #![feature(once_cell_try)]

This is a tracking issue for the *try* methods that go with the once_cell feature. Initially they were part of stabilization under once_cell (#105587) but was removed due to compatibility concerns with try trait v2 (#105587 (comment)).

Public API

impl<T> OnceCell<T> {
    pub fn get_or_try_init<F, E>(&self, f: F) -> Result<&T, E> where F: FnOnce() -> Result<T, E>;
}

impl<T> OnceLock<T> {
    pub fn get_or_try_init<F, E>(&self, f: F) -> Result<&T, E> where F: FnOnce() -> Result<T, E>;
}

Steps / History

Unresolved Questions

  • None yet.

cc @joboet @matklad just for reference

Footnotes

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

@tgross35 tgross35 added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 29, 2023
@BurntSushi
Copy link
Member

I think I'd say that the question of whether to make the fallible routines generic over Try is unresolved at this point. See: #107122 (comment) and #84277 (comment)

@tgross35
Copy link
Contributor Author

I'm kind of curious of how and how often these functions are used in the ecosystem, does anyone know of any examples? They're not used anywhehre in rustc

@BurntSushi
Copy link
Member

I posted a comment on that PR with the results of a quick search that I did: #107122 (comment)

@sutes-work
Copy link

Related to this, I want something like: TryLazyLock. Not sure if this is the right place to comment (sorry if it's not!).

I looked to see how get_or_try_init is implemented, but unfortunately, the poison method on Once isn't exposed publicly. It would be nice of Once had an API that would support this.

@tgross35
Copy link
Contributor Author

@sutes-work can you give an example of what a TryLazyLock API would do differently from get_or_try_init?

@sutes-work
Copy link

sutes-work commented Feb 22, 2024

So I can do this:

struct TryLazyLock<T, F> {
    data: OnceLock<T>,   
    init_fn: UnsafeCell<Option<F>>,
}

and it's not so bad, but it costs me space for the init_fn. I should be able to share that space with T. I should be able to have:

struct TryLazyLock<T, F> {
    once: Once,
    value: UnsafeCell<State<T, F>>,
}

enum State {
   PreInit(F),
   Complete(T)
}

It's a bit rough, but hopefully you get the idea.

@tgross35
Copy link
Contributor Author

tgross35 commented Feb 22, 2024

Is the main goal of the suggestion to save storage space? That is the case already, the actual implementation has

// We use the state of a Once as discriminant value. Upon creation, the state is
// "incomplete" and `f` contains the initialization closure. In the first call to
// `call_once`, `f` is taken and run. If it succeeds, `value` is set and the state
// is changed to "complete". If it panics, the Once is poisoned, so none of the
// two fields is initialized.
union Data<T, F> {
    value: ManuallyDrop<T>,
    f: ManuallyDrop<F>,
}

pub struct LazyLock<T, F = fn() -> T> {
    once: Once,
    data: UnsafeCell<Data<T, F>>,
}

#[unstable(feature = "lazy_cell", issue = "109736")]
pub struct LazyLock<T, F = fn() -> T> {
once: Once,
data: UnsafeCell<Data<T, F>>,
}

See #107329

@sutes-work
Copy link

Right, but F is fn() -> T when I want F to be fn() -> Result<T, E>.

@tgross35
Copy link
Contributor Author

Oh are you just looking for something to make LazyCell/LazyLock with result functions more ergonomic?

This works as-is:

#![feature(lazy_cell)]

use std::sync::LazyLock;

static OK: LazyLock<Result<&'static str, &'static str>> =
    LazyLock::new(|| Ok("success!"));
static ERROR: LazyLock<Result<&'static str, &'static str>> =
    LazyLock::new(|| Err("error!"));


fn main() -> Result<(), String> {
    println!("ok value: {}", (*OK)?);
    println!("error value: {}", (*ERROR)?);
    
    Ok(())
}

That prints:

ok value: success!
Error: "error!"

There is an ergonomic downside that you can't get an owned error. Feel free to propose an API if you have something in mind, or share code snippets that could be improved.

@sutes-work
Copy link

That's not what I want. I want to call the callback multiple times so it keeps trying. I want:

// Returns the value if the evaluation has already succeeded. Otherwise,
// tries to evaluate the lazy value and returns an error if it fails. Multiple
// calls to try_force will result in the initializer being called multiple times
// until it succeeds.
pub fn try_force(this: &LazyLock<T, F>) -> Result<&T, E>;

As you noted, you don't get an owned error, which is problematic for me since the errors I'm dealing with aren't copyable (which I believe is generally the case).

It's possible that such an API isn't generally useful which is why I hinted that exposing an API on Once that allows me to build such a thing myself would be helpful; I think my own TryOnceLock would be relatively simple: it's Once that seems hard. One option is to expose the poison method, but that feels like a hack to me since in this case it's really the same as the Incomplete state (there was no panic).

@tgross35
Copy link
Contributor Author

The usecase in general makes sense, you might be interested to bring it up to the once_cell crate, where a lot of this API was initially developed.

Having a panicky initializer function sounds like a pretty unfortunate interface. If you really can't use a Result API instead, call_once_force might be what you need. Or use a catch_unwind wrapper around your initializer function to convert those poisons to Results.

Feel free to drop by on Zulip to discuss this further if you're trying to refine some ideas for new APIs https://rust-lang.zulipchat.com/

@sutes-work
Copy link

you might be interested to bring it up to the once_cell crate,

I'll have a look into that.

Having a panicky initializer function sounds like a pretty unfortunate interface.

I think there might be a misunderstanding: I'm commenting on the way that get_or_try_init seems to be implemented: https://doc.rust-lang.org/src/std/sync/once_lock.rs.html#385. It calls poison there, but poison isn't public so I can't implement my own TryLazyLock using Once.

For now, I need stable Rust so I've just used the equivalent of:

struct TryLazyLock<T, F> {
    data: OnceCell<T>,   
    init_fn: UnsafeCell<Option<F>>,
}

(You can see it here: https://fxrev.dev/995693.)

@briansmith
Copy link
Contributor

The documentation says:

If the cell was empty and f failed, an error is returned.

But, it is missing the most important part:

- If the cell was empty and f failed, an error is returned.
+ If the cell was empty and f failed, an error is returned, and the cell remains uninitialized.

Importantly, even though the internal Once in OnceLock may be poisoned, there is actually no poisoning in OnceLock, as OnceLock will consider a poisoned internal Once to be uninitialized.

twwn added a commit to twwn/anki that referenced this issue Sep 27, 2024
…s possible

Since 1.80: rust-lang/rust#109736 and rust-lang/rust#98165

Non-Thread-Safe Lazy → std::cell::LazyCell https://doc.rust-lang.org/nightly/std/cell/struct.LazyCell.html

Thread-safe SyncLazy → std::sync::LazyLock https://doc.rust-lang.org/nightly/std/sync/struct.LazyLock.html

The compiler accepted LazyCell only in minilints.

The final use in rslib/src/log.rs couldn't be replaced since get_or_try_init has not yet been standardized: rust-lang/rust#109737
twwn added a commit to twwn/anki that referenced this issue Sep 27, 2024
…s possible

Since 1.80: rust-lang/rust#109736 and rust-lang/rust#98165

Non-Thread-Safe Lazy → std::cell::LazyCell https://doc.rust-lang.org/nightly/std/cell/struct.LazyCell.html

Thread-safe SyncLazy → std::sync::LazyLock https://doc.rust-lang.org/nightly/std/sync/struct.LazyLock.html

The compiler accepted LazyCell only in minilints.

The final use in rslib/src/log.rs couldn't be replaced since get_or_try_init has not yet been standardized: rust-lang/rust#109737
twwn added a commit to twwn/anki that referenced this issue Sep 27, 2024
… possible

Since 1.80: rust-lang/rust#109736 and rust-lang/rust#98165

Non-Thread-Safe Lazy → std::cell::LazyCell https://doc.rust-lang.org/nightly/std/cell/struct.LazyCell.html

Thread-safe SyncLazy → std::sync::LazyLock https://doc.rust-lang.org/nightly/std/sync/struct.LazyLock.html

The compiler accepted LazyCell only in minilints.

The final use in rslib/src/log.rs couldn't be replaced since get_or_try_init has not yet been standardized: rust-lang/rust#109737
twwn added a commit to twwn/anki that referenced this issue Sep 27, 2024
… possible

Since 1.80: rust-lang/rust#109736 and rust-lang/rust#98165

Non-Thread-Safe Lazy → std::cell::LazyCell https://doc.rust-lang.org/nightly/std/cell/struct.LazyCell.html

Thread-safe SyncLazy → std::sync::LazyLock https://doc.rust-lang.org/nightly/std/sync/struct.LazyLock.html

The compiler accepted LazyCell only in minilints.

The final use in rslib/src/log.rs couldn't be replaced since get_or_try_init has not yet been standardized: rust-lang/rust#109737
dae pushed a commit to ankitects/anki that referenced this issue Sep 30, 2024
* Anki: Replace lazy_static with once_cell

Unify to once_cell, lazy_static's replacement. The latter in unmaintained.

* Anki: Replace once_cell with stabilized LazyCell / LazyLock as far as possible

Since 1.80: rust-lang/rust#109736 and rust-lang/rust#98165

Non-Thread-Safe Lazy → std::cell::LazyCell https://doc.rust-lang.org/nightly/std/cell/struct.LazyCell.html

Thread-safe SyncLazy → std::sync::LazyLock https://doc.rust-lang.org/nightly/std/sync/struct.LazyLock.html

The compiler accepted LazyCell only in minilints.

The final use in rslib/src/log.rs couldn't be replaced since get_or_try_init has not yet been standardized: rust-lang/rust#109737

* Declare correct MSRV (dae)

Some of our deps require newer Rust versions, so this was misleading.

Updating the MSRV also allows us to use .inspect() on Option now
novacrazy added a commit to Lantern-chat/server that referenced this issue Oct 5, 2024
@matklad
Copy link
Member

matklad commented Nov 21, 2024

If OnceCell gains get_or_try_init, then likely Option would want get_or_try_insert_with, as it currently has only non-trying version:

https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.get_or_insert_with

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC 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

5 participants