rust-lang / rust

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

Tracking Issue for `once_cell_try` #109737

Open tgross35 opened 1 year ago

tgross35 commented 1 year ago

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 (https://github.com/rust-lang/rust/pull/105587#issuecomment-1396609610).

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

cc @joboet @matklad just for reference

BurntSushi commented 1 year ago

I think I'd say that the question of whether to make the fallible routines generic over Try is unresolved at this point. See: https://github.com/rust-lang/rust/pull/107122#issuecomment-1422712471 and https://github.com/rust-lang/rust/issues/84277#issuecomment-1483958706

tgross35 commented 1 year ago

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 commented 1 year ago

I posted a comment on that PR with the results of a quick search that I did: https://github.com/rust-lang/rust/pull/107122#issuecomment-1422995901

sutes-work commented 8 months ago

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 commented 8 months ago

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

sutes-work commented 8 months ago

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 commented 8 months ago

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>>,
}

https://github.com/rust-lang/rust/blob/397937d812852f9bbeb671005cb399dbcb357cde/library/std/src/sync/lazy_lock.rs#L80-L84

See https://github.com/rust-lang/rust/pull/107329

sutes-work commented 8 months ago

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

tgross35 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 5 months ago

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.