Skip to content

Add Option::try_or_else #59

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

Closed
cgwalters opened this issue Jun 28, 2022 · 22 comments
Closed

Add Option::try_or_else #59

cgwalters opened this issue Jun 28, 2022 · 22 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@cgwalters
Copy link

cgwalters commented Jun 28, 2022

Proposal

Add Option::try_or_else.

(note: This was formerly proposed as Option::result_or_else that only handled Result)

Problem statement

This generalizes Option::or_else to also handle Result, which is very useful in cases where we want to convert an Option<T> to a T, but use a fallible (Result<T>) fallback path in a combinator chain.

Motivation, use-cases

    // Our program needs a string for something (e.g. filename, database table name, etc).
    // To start, we can get this as an optional string - here a CLI argument, but it could be anything; e.g.
    // a value parsed from JSON, etc.  The `.nth(1)` call here gives us an `Option<String>`.
    // We want to compute a fallback value, but doing so can fail (e.g. return `Result<String>`).
    // Here's the trick - we use `.map(Ok)` to convert our `Option<String>` to `Option<Result<String>>`,
    // which means the type signature works for `.unwrap_or_else()`, giving us in the end a Result<String>,
    // on which we can use the regular `?` operator.
    let v: String = std::env::args()
        .nth(1)
        .map(Ok)
        .unwrap_or_else(|| std::fs::read_to_string("/etc/someconfig.conf"))?
;

This could instead be:

    let v: String = std::env::args()
        .nth(1)
        .try_or_else(|| std::fs::read_to_string("/etc/someconfig.conf"))?;

This convenience method is particularly useful when chaining further combinators, e.g.:

    let v: String = std::env::args()
        .nth(1)
        .try_or_else(|| std::fs::read_to_string("/etc/someconfig.conf"))
        .map(|v| v.trim().to_string())?;

Solution sketches

index 28ea45ed235..05b48728ae3 100644
--- a/library/core/src/option.rs
+++ b/library/core/src/option.rs
@@ -1346,7 +1346,8 @@ pub const fn or(self, optb: Option<T>) -> Option<T>
     }
 
     /// Returns the option if it contains a value, otherwise calls `f` and
-    /// returns the result.
+    /// returns the result.   See also [`try_or_else`] which can also handle
+    /// [`Result`].
     ///
     /// # Examples
     ///
@@ -1372,6 +1373,35 @@ pub const fn or_else<F>(self, f: F) -> Option<T>
         }
     }
 
+    /// Returns the option if it contains a value, otherwise calls `f` and
+    /// returns the result.  This is a more general version of [`or_else`]
+    /// that can also handle [`Result`].
+    ///
+    /// # Examples
+    ///
+    /// ```
+    /// fn nobody() -> Option<&'static str> { None }
+    /// fn vikings() -> Option<&'static str> { Some("vikings") }
+    /// fn success() -> Result<&'static str> { Ok("vikings") }
+    /// fn barbarians() -> Result<&'static str> {  Err(io::Error::new(io::ErrorKind::Other, "oh no!")) }
+    ///
+    /// assert_eq!(Some("barbarians").try_or_else(vikings), Some("barbarians"));
+    /// assert_eq!(None.try_or_else(vikings), Some("vikings"));
+    /// assert_eq!(None.try_or_else(nobody), None);
+    /// assert_eq!(None.try_or_else(success).unwrap(), "vikings");
+    /// assert_eq!(None.try_or_else(barbarians).is_err());
+    /// ```
+    #[inline]
+    #[unstable(feature = "try_or_else", issue = "1")]
+    #[rustc_const_unstable(feature = "try_or_else", issue = "1")]
+    pub const fn try_or_else<R: ops::Try<Output = T>, F: FnOnce() -> R>(self, f: F) -> R {
+        if let Some(v) = self {
+            ops::Try::from_output(v)
+        } else {
+            f()
+        }
+    }
+
     /// Returns [`Some`] if exactly one of `self`, `optb` is [`Some`], otherwise returns [`None`].
     ///
     /// # Examples

Links and related work

I discovered/made up this idiom of using map(Ok).unwrap_or_else() while working on a project, and it made some code much more elegant versus what I was doing before with multiple-line match statements.

I then did some looking around via code search engines:

And there's quite a lot of usage of this. Enough, I think that there's an argument for promoting this pattern to std.

I was chatting with @cuviper about this and he mentioned:

"another way to write this is option.ok_or(()).or_else(|()| new_result)?"

I did some further searching, and there's also some code using this variant, though less:

Notably the first link shows a hit in cargo.

Later, several people pointed out in discussion that by using the Try trait, we can effectively generalize the current Option::or_else to handle both Option and Result for the callback.

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@cgwalters cgwalters added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Jun 28, 2022
@nickkuk
Copy link

nickkuk commented Jun 29, 2022

Can you also describe the difference with ok_or_else?

@thomcc
Copy link
Member

thomcc commented Jun 29, 2022

Can you also describe the difference with ok_or_else?

ok_or_else takes FnOnce() -> E, this takes FnOnce() -> Result<T, E>.


Anyway, I've wanted this many times, and usually it takes staring at the list of methods on option for a while first to make sure that what I want isn't there (it's not).

I don't really like the name result_or_else, but don't have another suggestion either.

@programmerjake
Copy link
Member

programmerjake commented Jun 29, 2022

I don't really like the name result_or_else, but don't have another suggestion either.

how about try_or_else? it would be equivalent to:

impl<T> Option<T> {
    pub fn try_or_else<F: FnOnce() -> R, R>(self, f: F) -> R
    where
        R: Try<Output = T>,
    {
        try {
            if let Some(v) = self {
                v
            } else {
                f()?
            }
        }
    }
}

@cgwalters
Copy link
Author

cgwalters commented Jun 29, 2022

I don't really like the name result_or_else, but don't have another suggestion either.

I'm not personally attached to it. Another option I see is to break the or_else symmetry and have it be ok_or_result (closer to ok_or_else, but giving a Result instead of an error).

how about try_or_else? it would be equivalent to:

I am not too familiar with the try { stuff; the implementation looks clearer for sure. I'm less sure about having the name include try because ideally this could stabilize independently of try and it'd be a bit problematic if there was a decision to rename try later. Is there any other precedent for already stabilized API in std using the try terminology?

@cuviper
Copy link
Member

cuviper commented Jun 29, 2022

There's Iterator::try_fold etc.

That's an interesting idea if we made Option::try_or_else generic from Option<T> to any Try<Output = T> -- which would be a superset of the current Option::or_else.

@cuviper
Copy link
Member

cuviper commented Jun 29, 2022

It doesn't really need a try block either, just:

if let Some(v) = self {
    Try::from_output(v)
} else {
    f()
}

@cgwalters
Copy link
Author

@cuviper Awesome suggestion, it seems to work really well:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=cf620ea31702de394ffa7c0de5229363

If no one has any objections or other opinions I'll edit the original proposal to do this instead.

(I wonder...since this is obsoleting or_else I wonder if we could compatibly just replace it?)

@cuviper
Copy link
Member

cuviper commented Jun 29, 2022

(I wonder...since this is obsoleting or_else I wonder if we could compatibly just replace it?)

The new R would make that a Minor change: introducing a new type parameter.

Cargo docs now call that Possibly-breaking: introducing a new function type parameter.

But Option::or_else also has unstable const-ness, with the funky ~const trait bounds, so I suppose we would have to propagate that to a const Try for Option at least. (edit: that already exists!)

@cuviper
Copy link
Member

cuviper commented Jun 29, 2022

Here's a break if we try to change or_else, due to the ! type-inference fallback to ():

error[E0277]: the trait bound `(): std::ops::Try` is not satisfied
    --> compiler/rustc_traits/src/chalk/lowering.rs:873:14
     |
873  |             .or_else(|| bug!("Skipped bound var index: parameters={:?}", parameters));
     |              ^^^^^^^ the trait `std::ops::Try` is not implemented for `()`
     |
note: required by a bound in `Option::<T>::or_else`
    --> /home/jistone/rust/rust/library/core/src/option.rs:1369:12
     |
1369 |         R: ~const Try<Output = T>,
     |            ^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `Option::<T>::or_else`

That could have used unwrap_or_else, but still, this is real code that breaks.

@cuviper
Copy link
Member

cuviper commented Jun 29, 2022

A possibly-harebrained idea is to put this on Try itself:

    fn try_or_else<F, R>(self, f: F) -> R
    where
        F: FnOnce(Self::Residual) -> R,
        R: Try<Output = Self::Output>,
    {
        match self.branch() {
            ControlFlow::Continue(c) => R::from_output(c),
            ControlFlow::Break(b) => f(b),
        }
    }

But you would have to use core::ops::Try, and the trait's journey to stabilization is still unclear...

@cgwalters cgwalters changed the title Add Option::result_or_else Add Option::try_or_else Jun 29, 2022
@cgwalters
Copy link
Author

🆕 OK I've updated this proposal to be Option::try_or_else - thanks for all the feedback!

The new R would make that a Minor change: introducing a new type parameter.

(Somewhat tangential to this, but out of curiosity is making that change to drop Option::or_else and replace it with what is now Option::try_or_else something we could do on an edition boundary?)

@cuviper
Copy link
Member

cuviper commented Jul 1, 2022

(Somewhat tangential to this, but out of curiosity is making that change to drop Option::or_else and replace it with what is now Option::try_or_else something we could do on an edition boundary?)

Maybe for high-profile stuff, but for the most part the standard library is edition-neutral. There's not an easy mechanism for that yet, so we end up tinkering in the compiler, which is not great. The primary example is IntoIterator for arrays, but panic macro consistency sort of counts too as an API change via compiler built-in.

@cuviper
Copy link
Member

cuviper commented Jul 1, 2022

A possibly-harebrained idea is to put this on Try itself:

That idea would also be fine on Result alone, just to mirror this Option method.

impl<T, E> Result<T, E> {
    pub fn try_or_else<F, R>(self, f: F) -> R
    where
        F: FnOnce(E) -> R,
        R: ops::Try<Output = T>,
    {
        match self {
            Ok(t) => R::from_output(t),
            Err(e) => f(e),
        }
    }
}

@cgwalters
Copy link
Author

I see the value in the symmetry of also replacing/generalizing Result::or_else, although I can't think of a situation I've run into where I would want to use an Option fallback from a Result.

@jdahlstrom
Copy link

The name try_or_else makes me ask "try what or else?" and get confused because the method only takes one non-self argument rather than two, the "try" part and the "or else" part. Ie. the name makes me think this is some combination of map(T -> impl Try) and unwrap_or_else.

@cgwalters
Copy link
Author

The name try_or_else makes me ask "try what or else?

Do you have any alternative naming suggestions?

I suspect everyone here would probably agree that it would all be nicer if this had just been what or_else currently is from the start.

@Amanieu
Copy link
Member

Amanieu commented May 17, 2023

This can already be done using map_or_else:

    let v: String = std::env::args()
        .nth(1)
        .map_or_else(|| std::fs::read_to_string("/etc/someconfig.conf"), Ok)?

At this point we have a pretty high bar for adding new methods to Option and Result. While this may be useful, the libs team thinks that effort should instead be focused on language feature that make working with enums in general more ergonomic.

@Amanieu Amanieu closed this as completed May 17, 2023
@cgwalters
Copy link
Author

cgwalters commented May 17, 2023

OK, fair. It's not the most intuitive construct; the dangling Ok looks strange. But perhaps it's just an idiom that will become more familiar over time.

How about this patch to the std docs which highlights this case more?

diff --git a/library/core/src/option.rs b/library/core/src/option.rs
index 28ea45ed235..1a0e9273033 100644
--- a/library/core/src/option.rs
+++ b/library/core/src/option.rs
@@ -997,7 +997,7 @@ pub const fn map_or<U, F>(self, default: U, f: F) -> U
     /// Computes a default function result (if none), or
     /// applies a different function to the contained value (if any).
     ///
-    /// # Examples
+    /// # Basic examples
     ///
     /// ```
     /// let k = 21;
@@ -1008,6 +1008,21 @@ pub const fn map_or<U, F>(self, default: U, f: F) -> U
     /// let x: Option<&str> = None;
     /// assert_eq!(x.map_or_else(|| 2 * k, |v| v.len()), 42);
     /// ```
+    ///
+    /// # Handling a Result-based fallback
+    ///
+    /// A somewhat common occurrence when dealing with optional values
+    /// in combination with [`Result<T, E>`] is the case where one wants to invoke
+    /// a fallible fallback if the option is not present.  This example
+    /// parses either an command line argument or the contents of a file to
+    /// an integer.
+    ///
+    /// ```
+    /// let v: u64 = std::env::args()
+    ///    .nth(1)
+    ///    .map_or_else(|| std::fs::read_to_string("/etc/someconfig.conf"), Ok)?
+    ///    .parse()?;
+    /// ```
     #[inline]
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_unstable(feature = "const_option_ext", issue = "91930")]

@programmerjake
Copy link
Member

note std::env::args() is command line arguments, not environment variables.

@cgwalters
Copy link
Author

note std::env::args() is command line arguments, not environment variables.

Thanks, fixed!

@Amanieu
Copy link
Member

Amanieu commented May 17, 2023

The doc change looks fine to me.

@cgwalters
Copy link
Author

rust-lang/rust#111702

cgwalters added a commit to cgwalters/rust that referenced this issue May 18, 2023
Moving this from rust-lang/libs-team#59
where an API addition was rejected.  But I think it's valuable
to add this example to the documentation at least.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 3, 2023
…result, r=Mark-Simulacrum

Option::map_or_else: Show an example of integrating with Result

Moving this from rust-lang/libs-team#59 where an API addition was rejected.  But I think it's valuable to add this example to the documentation at least.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 3, 2023
…result, r=Mark-Simulacrum

Option::map_or_else: Show an example of integrating with Result

Moving this from rust-lang/libs-team#59 where an API addition was rejected.  But I think it's valuable to add this example to the documentation at least.
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Aug 24, 2023
Moving this from rust-lang/libs-team#59
where an API addition was rejected.  But I think it's valuable
to add this example to the documentation at least.
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Aug 24, 2023
…=Mark-Simulacrum

Option::map_or_else: Show an example of integrating with Result

Moving this from rust-lang/libs-team#59 where an API addition was rejected.  But I think it's valuable to add this example to the documentation at least.
@dtolnay dtolnay closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

8 participants