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

Add Option::todo and Result::todo #456

Open
zkrising opened this issue Oct 3, 2024 · 4 comments
Open

Add Option::todo and Result::todo #456

zkrising opened this issue Oct 3, 2024 · 4 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@zkrising
Copy link

zkrising commented Oct 3, 2024

Proposal

Problem statement

.unwrap() is semantically overloaded in rust. It finds itself used for two significantly different reasons:

  • If this is None/Err, our program is in a bad state and we want to exit (missing config files, missing resource files, some external invariant not upheld)
// e.g.
// our web server should just die if it can't open a tcp socket
TcpListener::bind(&addr).unwrap();
  • I haven't added None/Err handling here yet (prototype code)
// e.g.
// parse user input as an int,
// can't be bothered handling bad input right now
let int: i32 = input.parse().unwrap();
// my CLI needs a second argument to do anything useful,
// i should handle this properly later
let arg2 = std::env::args().nth(2).unwrap();

Motivating examples or use cases

Users find themselves using .unwrap() for these different reasons, but the semantic reason why unwrapping was done is not stored in the source code.
Some users write comments near unwraps to justify them, but this is easy to forget or fall out of sync with the codebase.

In my experience, the unwrap as todo!() paradigm is much more common in application rust.

This problem becomes more pronounced when one wants to go back over the code and fix all of those "todo unwraps".
It's not easy to figure out whether the .unwrap() has a good reason for being there, or is simply a result of a hastily-written MVP.

While in terms of actual program execution, nothing is different (the program will panic), the source code itself doesn't necessarily track the information why that panic was placed there.

This is also a benefit for documentation/writing code for others. A .todo() clearly indicates that error handling is an exercise for the reader, whereas .unwrap() can be conflated with legitimate error handling/an assertion that this doesn't fail.

Solution sketch

I imagine an implementation would look vaguely like this:

impl Result<T, E> where E: fmt::Debug {
  pub const fn todo(self) -> T {
    match self {
      Ok(t) => t,
      Err(err) => {
        todo!("Error not handled: {err:?}")
      }
    }
  }
}

impl Option<T> {
  pub const fn todo(self) -> T {
    match self {
      Some(t) => t,
      None => {
        todo!("None not handled")
      }
    }
  }
}

Alternatives

This could be done with ResultExt

Then everyone could import a crate that does this for them. However, I think there's value in standardising this, as its an extremely common use case. Plus, if standardised, IDEs and Rust-Analyzer could treat it better.

For example, #[clippy::todo] could highlight .todo()s and so on.

Other Method Names

We could call this method .unwrap_todo() instead, which might make it more obvious that this will panic. However, I'm conscious that these names are rather long, and having to write out .unwrap_todo() in prototype code is unlikely to catch on as a result.

I don't think there's any good reason to choose names other than todo, it already exists prominently in Rust.

What about .expect()?

We do already have {Option, Result}::expect which serves a similar-ish purpose of "unwrap with a reason".

I argue that this doesn't necessarily map as nicely onto the semantics of todo.

While this feature can be emulated with .expect("todo"), this is frustrating to type, easy to typo, harder to grep for and cannot be highlighted nicely by an IDE.

Should this take an argument?

.expect() takes an argument which allows more info to be strapped on about the panic.

I don't think .todo taking an argument would be good as it makes the code harder to write, plus, I don't see what you'd ever write there.

Links and related work

I initially proposed this as an RFC. I was informed that it made more sense to put this here.

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@zkrising zkrising added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Oct 3, 2024
@oskgo
Copy link

oskgo commented Oct 10, 2024

I'd like to highlight a different alternative; using expect or unwrap_or_else with an appropriate panic for code that isn't prototyping.

That allows both short code and easy grepability (unwrap\() for the prototyping case.

@lolbinarycat
Copy link

For what it's worth: people writing a MVP with no regard for error handling are unlikely to go out of their way to add a dependency on an extension crate just to make the MVP a bit easier to maintain in the future, especially since the extension trait will need to be imported in every file.

I'd like to highlight a different alternative; using expect or unwrap_or_else with an appropriate panic for code that isn't prototyping.
That allows both short code and easy grepability (unwrap() for the prototyping case.

what would you recommend for the unreachable case? i use unwrap a lot in applications where i have a lot of "unreachable" cases that exist mainly because i don't want to dip into unsafe just to remove the unwraps from an algorithm.

@oskgo
Copy link

oskgo commented Oct 10, 2024

what would you recommend for the unreachable case?

unwrap_or_else(||unreachable!()) or expect("unreachable"). Those are a bit verbose, but it's not too bad IMO, especially in non-prototyping code.

@ssokolow
Copy link

ssokolow commented Oct 11, 2024

I'd like to highlight a different alternative; using expect or unwrap_or_else with an appropriate panic for code that isn't prototyping.

That allows both short code and easy grepability (unwrap\() for the prototyping case.

Just as a data point, I use .unwrap() for TODOs and .expect("rationale") for "cannot fail" and then turn on Clippy's unwrap_used lint when prototyping is over.

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

4 participants