Proposal To Add ".ok()" function to Option?

Suppose we have this code that takes a github repo url and parse the repo name from it...

use std::error::Error;

pub fn get_repo_name(url: &str) -> Result<String, Box<dyn Error>> {
    Ok(url
        .split("/")
        .collect::<Vec<&str>>()
        .pop()
        .ok_or(format!("couln't parse repo name from url: {url}"))?
        .to_string())
}

#[test]
fn gets_repo_name() -> Result<(), Box<dyn Error>> {
    use crate::name_builder::get_repo_name;

    let example_url = "https://github.com/vivainio/rraf";
    let expected = "rraf";

    assert_eq!(expected, &get_repo_name(example_url)?);

    Ok(())
}

The ok_or is alright, but must we really force the coders to invent some new error string all the time?

I would like to propose that Option has a function simply called "ok" which takes no arguments.

The purpose of this function is to take the Some(x) and convert it to a Result's Ok(x).

If the thing has the None variant then it is converted to a std::error::Error with some intuitive message like, "Option had the None variant" along with the stack trace. This makes things easy to debug if the None variant ever actually occurs while allowing the code to be simple, more succinct, and best of all I don't have to add in my own bad error messages! :sweat_smile:

So what do you all think? Is this a cool idea? Does this already exist, just a differently named function?

How would I even go about proposing a change to such a core thing in the Rust language?

The final code could look something like this:

pub fn get_repo_name(url: &str) -> Result<String, Box<dyn Error>> {
    Ok(url
        .split("/")
        .collect::<Vec<&str>>()
        .pop()
        .ok()?
        .to_string())
}

There used to be a NoneError, which you could produce just by executing None? without even any conversion function. It was never stabilized and removed from nightly usability here. I believe the reason for that was that it was considered undesirable for ? to implicitly convert from Option to Result, so it's plausible NoneError could be resurrected as an explicit thing. That said, I think people will be reluctant to have such an error value because it is unhelpful — the most information it can provide is “panic-like” information that you mention (stack trace), but Error types are supposed to be intentionally designed to convey information meaningful to the caller of the function that returned the error, and “returned None” can't be that.

7 Likes

I agree that being able to just throw a question mark on an option not good- that's why I want an explicit ".ok" function.

I would also argue that "Option had the None variant" along with the stack trace is helpful.

You go to the code, see that pop is returning none in this situation, and think about why is could be returning none...

I really don't see anything I could add in my custom error message that actually makes the error any more helpful.

In this particular example, if you don't have a useful error message to return you could change the return type to an Option. If Result isn't adding value, don't use it.

pub fn get_repo_name(url: &str) -> Option<String> {
    Some(url.split("/").collect::<Vec<&str>>().pop()?.to_string())
}

Or even simpler:

pub fn get_repo_name(url: &str) -> Option<&str> {
    url.split("/").last()
}
15 Likes

If it’s an error that you need to, or are prepared to, look up the problem in the source code, just panic. Result is for returning semantically meaningful errors that could be handled by the program itself or at least reported to an end user who does not have access to the source code or inclination to delve into it.

3 Likes

It's only "inventing some new error string" in a context where a string can be automatically converted to your error type. A lot of contexts, libraries in particular, use strongly-typed errors, provide things like backtraces, etc. Not everyone uses Box<dyn Error>.

The exposure of NoneError, or interconversion as this RFC puts it, was considered an accidental implementation detail leak by the Rust teams as far as I understand it (hence its removal). I doubt it's coming back. ok_or and ok_or_else are absolutely the intended mechanism.

Making good errors more than an afterthought is part of the point. The unhappy path is often important and frequently neglected.


Side note, use ok_or_else to avoid unconditionally allocating the error string.

.ok_or_else(|| format!("couln't parse repo name from url: {url}"))?
8 Likes

Surprisingly, there's no .ok_or_default(), already. I could see that being spelled .ok()? That seems pretty close to the intent here, and remains flexible.

1 Like

I don't really see the purpose, what would be different from opt.unwrap_or_ default()? If it would return a Result<T, T> this would be a new feature, but I highly doubt there is a valid usecase for it.

2 Likes

There is no such thing as "just" an Ok in Rust. Every Ok is an instance of a Result that still has a specific type for its error, even if the error value is absent. Ok with one error type will be incompatible with Ok having another error type.

So an .ok() would still have to pick a concrete error type. It could make Ok::<_, &str> or Ok::<_, ()> or Ok::<_, NoneError>, etc. I don't think any of these types are clearly better or obvious.

Even if it supported generic types, like Ok::<_, T> where T: Default, that won't properly infer from the function's return type unfortunately. This is because ? is generic too, so the ok()'s error type would remain ambiguous.

4 Likes

How would returning an Option be good thing to do here?

The rest of my program is replying on having a repo name so just return None would surely result in bad things...

unwrap_or_default? So now we need to convert the Option to a panic and then convert the panic to a Result? That definitely doesn't seem like the cleanest way to do it...

Actually, no, unwrap_or_default does a completely different thing. It doesn't even compile in this situation.

I don't see why having the ok()'s error be generic like ? is a problem...

Thanks for trying, but "meh, just let it panic" is actually very bad advice... -> error handling - How to panic! in production - Stack Overflow

The problem is when there are two places where the generic must be inferred. Essentially, option.ok_or_default()? would have the same problem as Default::default().into() - it's unknown what is the "type implementing Default", even if it's known what it is converted into.

4 Likes

You have to shift your mental model and understand that your proposal is not a good idea, and people here are trying to tell you why, in multiple ways.

Is panic a good strategy for error handling? Most of the time, it isn't, but in this case it would be preferred to what you are suggesting. That should really tell you something.

You misunderstand me. The alternatives are either "damn, the program has a bug, better panic and dump core print a stack trace so the programmer can investigate the problem" or "this is not a bug but a problem in user input or the environment, better return an Err with a semantically meaningful payload (NOT necessarily just a string but a proper error object!) that tells the user what is wrong with the input/env. There are libraries that help adding context to errors, which is sort of like a stack trace but meaningful to ordinary users.

A Result::Err with an "Option was a None" message and a stack trace is basically only meaningful if the user is also the programmer, and if that is the case, that’s fine, but it’s also not the use case for the majority of programs out there.

4 Likes

Okay, but if you code everything for the end user with a simple nice message how does the programmer ever even see the stack traces? Unless there is a special way to run the binary that does this?

If you have some things panic and some things return an Err isn’t the panicking a bad ux for the non programmers?

Tangent: There are situations where even if bugs are present, panicking is to be avoided. For example, consider a program which (a) is a document editor, long-running game, or other application where the user is putting some effort in which should not be lost by discarding state if at all possible, and (b) is running in a web browser via the wasm32-unknown-unknown target which does not support panic unwinding (and thus catch_unwind() cannot be used). In that case, in a situation where there is a known point of possible failure, it is desirable to return an Err so that recovery can happen.

Some would say “but what if the state you propose to keep is corrupted?” Indeed it might be, but users would prefer to have the option of saving a possibly-corrupted file to the certainty of losing everything up to the previous save. Additionally, it might be the case that the operation which panicked is known not to be able to cause such corruption, e.g. because it does not mutate the state.

(Plausibly all of these situations should be solved via frequent auto-saves and an external supervisor (JS in the Wasm case, or a watchdog timer in embedded systems) that can restart the Rust process, though.)

3 Likes

I think the most common way to handle that is to write it into some log file. This isn't Rust specific, most user-facing applications prefer not to show stack traces to users, but with having a log file you can just tell users to attach it to a bug report or what have you. If they needed to rerun the program with some specific setting turned on and reproduce the situation in which the error was encountered to produce a stack trace, most users will not bother, even if they knew how to do it.

1 Like

This sounds like a good idea to me, but I'm curious how it should be implemented.