Use Try trait to make Once[Cell | Lock]::get_or_try_init generic over return type

[joboet]

This came up in the stabilization PR: #105587 (comment)

Using Try allows these methods to accept types like Option in cases where there is no applicable error type.

@rustbot label +T-libs-api +T-libs
r? @scottmcm

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[c410-f3r]

With this approach, get_or_try_init won't be stabilized before the stabilization of try_trait_v2.

IFAICT and judging from the development around try_trait_v2, such thing will probably take time (years?).

[matklad]

Might make sense to check for other uses of res in the relevant files, as that’s the name I use a lot.

[joboet]

There is one other case in Clone, but I think it's fine as is. I just don't want to use res here since it could cause confusion as to whether it means "result" or "residual".

[joboet]

@scottmcm I'm assuming this looks good to you? Assigning it to someone from libs-api for their approval...

r? libs-api

[BurntSushi]

I'm not sure how I feel about this. The type signature here is really unwieldy. But I suppose that will be a problem for any API that is generic over Try. But do we really need to make this API generic? It's not obvious to me that we do. If Result is the 99% case, then I'm not sure making the API much more difficult to understand is worth doing. And it also means that the API for returning Result is probably not going to get stabilized soon, which is also unfortunate.

[joboet]

I'm not sure how I feel about this. The type signature here is really unwieldy. But I suppose that will be a problem for any API that is generic over Try. But do we really need to make this API generic? It's not obvious to me that we do. If Result is the 99% case, then I'm not sure making the API much more difficult to understand is worth doing. And it also means that the API for returning Result is probably not going to get stabilized soon, which is also unfortunate.

I'd say Option is actually more useful here, since it can be used to simply abort the initialization without "giving reasons". Most truly fallible operations (like filesystem accesses) cache their errors and so should use get_or_init. Making the function generic avoids restricting either use.

From a syntax point of view, having to specify the result type explicitly is very unlikely as just using Some(val) or Err(error) already indicates to the compiler that Option/Result is meant. I do agree that the signature looks unwieldy, however.

[BurntSushi]

I'd say Option is actually more useful here

Is it? I just did a search and I can't find any use cases using Option. Many are using Result with errors though. This is for existing uses of OnceCell (whether from tokio or once_cell):

• https://github.com/quickwit-oss/quickwit/blob/01a6775ffac2360881c03f59ba7e32bfebd0df53/quickwit/quickwit-metastore/src/metastore/file_backed_metastore/lazy_file_backed_index.rs#L73
• https://github.com/charles-r-earp/autograph/blob/59fb4a026e999d929c0b82184639d7be3cf78444/examples/hello-compute/src/main.rs#L15
• https://github.com/openssh-rust/openssh/blob/99b11c2870c39cf76ccf62e6c610b1430c62b128/src/native_mux_impl/stdio.rs#L20
• https://github.com/subconsciousnetwork/noosphere/blob/96a3752c1f70e8d088293c98d2638b67f1a733f5/rust/noosphere-core/src/view/versioned_map.rs#L51
• https://github.com/ankitects/anki/blob/f616bea580f6174c6b95f00e9374d48ce5ab520b/rslib/src/log.rs#L24
• https://github.com/MystenLabs/sui/blob/617c9b66f154125ac719403a0ffb59e8ea269ed4/crates/sui-core/src/authority/authority_store.rs#L331
• https://github.com/openebs/mayastor/blob/76cb58ace4d3edbdd4850ca9af5f4a4c45c10561/io-engine/src/core/env.rs#L665
• https://github.com/facebook/sapling/blob/8b54b0ff9c9428698f90e10113cfe5d81c3ebbd9/eden/mononoke/megarepo_api/src/lib.rs#L109
• https://github.com/holochain/holochain/blob/f67996858335405fe83c631c81ba4417beaf7369/crates/holochain_types/src/db_cache.rs#L84
• https://github.com/trailofbits/dylint/blob/e9d387d52b873d738e090b998aa7fc41b5e49626/dylint/src/name_toolchain_map.rs#L40

[BurntSushi]

Making the function generic avoids restricting either use.

My mind's eye sees it as less "restrictive" and more "less convenient." If you really do want to return an Option<T> but the only API available to use is Result, then you can return a Result<T, ()> and then get an option by calling Result::ok.

[bors]

☔ The latest upstream changes (presumably #105587) made this pull request unmergeable. Please resolve the merge conflicts.

[workingjubilee]

In any case, this seems to need a rebase.
@rustbot author

[joboet]

@rustbot ready
Rebased.

[bors]

☔ The latest upstream changes (presumably #114272) made this pull request unmergeable. Please resolve the merge conflicts.

[jieyouxu]

cc @joboet from triage: marking this PR as waiting-for-author due to merge conflicts.
@rustbot author

[joboet]

I think this is blocked on a team decision on the proper design of the Try trait. There is an increasing number of functions like this in the library API, and this should IMHO be decided for all of them.

@rustbot label +S-blocked -S-waiting-on-author

[Amanieu]

We discussed this in the libs-api meeting today. Any APIs that could make use of Try should do so, even if that keeps them unstable for now. We've also nominated the Try trait for discussion next week to look into moving it forward.

[joboet]

Sorry, made an error while rebasing.
@rustbot ready

[BurntSushi]

Any APIs that could make use of Try should do so, even if that keeps them unstable for now.

I don't think I'm fully on board with this personally. Or at the very least, I'm pretty unhappy with where we've ended up. The difference in type signature complexity is pretty wild, and it isn't totally clear to me that the complexity is buying us much. Compare the status quo:

pub fn get_or_try_init<F, E>(&self, f: F) -> Result<&T, E>
where
        F: FnOnce() -> Result<T, E>,

With Try trait support:

pub fn get_or_try_init<'a, F, R>(&'a self, f: F) -> <R::Residual as Residual<&'a T>>::TryType
    where
        F: FnOnce() -> R,
        R: Try<Output = T, Residual: Residual<&'a T>>,

I find this signature very difficult to read personally. And using this for the basic case of returning a Result<&T, E> seems like an overpowered hammer. The motivation for making this generic was Option, but AFAIK, one can just as easily use Result<&T, ()> and call Result::ok to get an Option without much fuss. I understand that's kind of a hack on the Result API, but I think I'd rather have that than a type signature like the above.

I think we are in a tough spot. I don't really know what to do here:

• Just keeping the status quo is not ideal because then there isn't a method that supports the full generality of Try for the niche cases where that's required. (Although other than Option, none have been brought up.)
• Adding two methods (one for Result specifically and one that is generic) also feels bad and also sets an annoying precedent. Not to mention trying to come up with names to distinguish them.
• Why have the Try trait and then not use it? That also seems bad.

I am very conflicted on what to do here. But I am very unhappy with type signatures that look like this when the 99% use case would be serviced by something more concrete.

[jplatte]

Why have the Try trait and then not use it? That also seems bad.

To me, the one important purpose of Try is ? overloading. There are other traits for operator overloading that are virtually never used as generic bounds. Personally I'd be quite okay with this being the case for Try too.