rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

[RFC] Add `Option::todo` and `Result::todo`

Open zkldi opened this issue 1 year ago • 42 comments

This RFC proposes Result::todo and Option::todo functions which work like .unwrap() but imply that error handling should be implemented later.

As an example:

// unwrap is still used for cases where you *actually* want to panic-on-err
TcpListener::bind(&addr).unwrap();

// we're panicking because error handling is not implemented yet.
// this use case is common in prototype applications.
let int: i32 = input.parse().todo();
let arg2 = std::env::args().nth(2).todo();
let file_content = fs::read("file.txt").todo();

n.b. The initial version of this RFC also proposed .unreachable(). Upon more thought and some feedback I've decided that .unreachable() isn't ideal -- It is easily emulated with .expect("reason for why error cannot happen"). Attaching .unreachable() onto this RFC drags it down quite a bit. I think .todo() is a strong improvement to Rust, but I can't think a strong case for .unreachable().


Rendered

zkldi avatar Oct 03 '24 12:10 zkldi

This has a better chance as an ACP (API change proposal) over at https://github.com/rust-lang/libs-team/issues.

fmease avatar Oct 03 '24 12:10 fmease

As an initial reader, I really like todo.

My concern with unreachable is confusion with https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/std/intrinsics/fn.unreachable.html (std::intrinsincs::unreachable), which is unsafe, and marks that the code as unreachable, and can lead to UB if the code is reached.

ChrisJefferson avatar Oct 03 '24 14:10 ChrisJefferson

Isn't it called unreachable_unchecked? I notice you linked a docs page from 1.25 which is quite a while ago. (EDIT: actually I see you linked the name of the core intrinsic, rather than the actual function in std. I'm pretty sure there is a pretty strong rule that for public API at least, _unchecked is required for functions with a narrow contract like this.)

digama0 avatar Oct 03 '24 14:10 digama0

Sorry, I should have checked the version -- I remembered seeing, and using the unreachable intrinsinc (although it was experimental), and didn't realise it was renamed when stabilized.

ChrisJefferson avatar Oct 03 '24 14:10 ChrisJefferson

The unreachable! macro indicates a line of code that should never be reached by the flow of execution, but Some(4).unreachable() does get reached and, in fact, does some work (unwraps the Option). Mostly I think expect is adequate here, but otherwise, maybe something more like Some(4).assert_some()?

jongiddy avatar Oct 03 '24 15:10 jongiddy

The unreachable! macro indicates a line of code that should never be reached by the flow of execution, but Some(4).unreachable() does get reached and, in fact, does some work (unwraps the Option). Mostly I think expect is adequate here, but otherwise, maybe something more like Some(4).assert_some()?

I agree that .unreachable() is sort of ugly here. I'm not 100% sure about assert_some/assert_ok because they don't 100% convey the intention of "i'm unwrapping BECAUSE this case cannot happen". In my mind, an assertion is something that could happen but implies the program is in a screwed state (which is analogous to panic!()).

That is to say, .assert_some() reads to me exactly the same as .unwrap() does.

We could combine these a bit to get err_unreachable() and none_unreachable(), which are obvious names that convey intent, but there aren't a lot of function names that look like that.

unreachable_err is sadly unusable because unwrap_err means something completely different.

unwrap_unreachable is also an option.


In general, I think .todo() should be an acceptable name, but .unreachable() could do with some bikeshedding. It looks wrong - it looks like that line should not be reached, but what it's actually saying is that the error is unreachable.

If .unreachable() is enough of a point-of-contention, we can drop it from the RFC and proceed with just .todo(). I think that function has immediately visible benefits and the name is obvious/has-prior-art/is not confusing.

zkldi avatar Oct 03 '24 15:10 zkldi

I've updated the RFC to remove Option::unreachable() and Result::unreachable() from consideration.

After reading some comments and thinking over it more, I don't think .unreachable() provides that much benefit to Rust. It can be simulated with .expect("reason for unreachability") quite easily and there are not many benefits for having a dedicated method for it.

There are also many more points of contention in the naming of .unreachable(); it's pretty confusing, not very good, and needlessly distracts from .todo(), which I think is a much stronger improvement.

I'm looking into moving this over into an ACP now. I had no idea that ACPs were a thing; they're not mentioned anywhere in the rfc readme :(

zkldi avatar Oct 03 '24 18:10 zkldi

Some further obvious alternatives you can already use:

  • just use todo!() to handle the None/Err case, e.g.
    • with a unwrap_or_else
    • or with let else
    • or with a match
    • etc…
  • add a TODO comment

some code examples for illustration

let int: i32 = input.parse().unwrap_or_else(|| todo!());
let Some(arg2) = std::env::args().nth(2) else { todo!() };
// TODO: handle error case
let file_content = fs::read("file.txt").unwrap();

steffahn avatar Oct 03 '24 18:10 steffahn

i think you'd also want a todo_err or todo_ok which is the equivalent of unwrap_err

programmerjake avatar Oct 03 '24 20:10 programmerjake

From what I understand, .expect is supposed to imply unreachable. If it is reachable, it should have been handled instead of panicking.

But how is .todo() any better than .expect("TODO")?

SOF3 avatar Oct 04 '24 03:10 SOF3

But how is .todo() any better than .expect("TODO")?

IDE could mark .todo differently from .expect

(similar to the justification that IDE could mark todo! differently from umimplemented! see https://doc.rust-lang.org/stable/std/macro.todo.html)

Also, clippy could warn .todo separately from .expect

lebensterben avatar Oct 04 '24 03:10 lebensterben

Isn't unwrap… the same, as unreachable?

Like, the whole point of calling unwrap is to say "I'm so confident that this Option is Some that I'm not even going to provide an error message in case it isn't" and while it's unfortunate that the output isn't very similar to unreachable!(), that is what the code means.

I kind of like the idea of todo, but as folks have said, expect("TODO") or expect("FIXME") works just as well, and most IDEs will automatically highlight those strings anyway regardless of context. So… 🤷🏻

clarfonthey avatar Oct 04 '24 04:10 clarfonthey

Also, clippy could warn .todo separately from .expect

clippy could easily warn .expect("TODO") differently from other generic .expect(...)

kennytm avatar Oct 04 '24 06:10 kennytm

Isn't unwrap… the same, as unreachable?

But it's not. Both compile to an explicit panic!(), but both have different messages, and signal different intent in code.

.unwrap() could and often does mean you're confident it won't happen, yes, but it could also mean there's no meaningful behavior so the best thing is just to crash. Or it could mean todo!() as above. But more importantly, "not yet implemented" is a very different message from "Option::unwrap() called on None".

Qyriad avatar Oct 04 '24 06:10 Qyriad

Having some functions doing exactly the same thing except for some artificial "semantic differences" seems strange to me, especially when the motivation can already be addressed to some extent by simply comments / expect.

slanterns avatar Oct 04 '24 07:10 slanterns

Having some functions doing exactly the same thing except for some artificial "semantic differences" seems strange to me, especially when the motivation can already be addressed to some extent by simply comments.

But how is .todo() any better than .expect("TODO")?

clippy could easily warn .expect("TODO") differently from other generic .expect(...)

It is a bit strange, but I don't think this is without precedence in Rust. All of these arguments could -- too -- be applied onto the todo!() macro.

One can rewrite all panic!()s with panic!("todo") to get a similar result, but that gives you:

  • a worse error message (worse than "Not yet implemented", cannot be localised either)
  • is more prone to typos (also, Todo? TODO? ToDo? I've seen all of these in comments).
  • Can be inconsistent across languages (.expect("する"). I've seen this before in japanese code, ~~although i might've translated it wrong~~)
  • is likely to be forgotten or not done because it's additional work

And the sort of ugly part here is that if the solution for an analogous-to-todo-unwrap is notably more difficult to write than unwrap, I suspect it will ignored frequently, like the situation we're in today.

At the moment, I rarely see .expect("TODO") in work or at home. I don't write it myself because it's more annoying than writing .unwrap().

Although today you can do all of these things:

// this is 27 characters long; no prototype code will ever realistically go through the hassle of writing this
let int: i32 = input.parse().unwrap_or_else(|| todo!());

// This is 19 additional characters and doesn't chain.
// this is useful in Non-Option/Result cases though, where you want to pattern match a single variant.
let Some(arg2) = std::env::args().nth(2) else { todo!() };
// i.e. is not easily possible
let thing = getstate().todo().thing

// And storing important information in comments just never seems to work in practice,
// it falls out of sync with the code and isn't easy to grep for.
// People forget, people don't do it, and there's no guarantee that comment is in english either.

zkldi avatar Oct 04 '24 09:10 zkldi

I'll throw out that this could easily be a tiny crate using an extension trait. Going into STD is a high bar. If this is so necessary it seems to me that one could make such a crate and prove the popularity first.

That's not a yes or a no, I'm just saying the obvious thing. I'm sort of meh one way or the other. However to me if it can't become a popular crate and it isn't something that the compiler itself has to help with, then it doesn't seem to hit the bar to go into std in my opinion.

ahicks92 avatar Oct 05 '24 08:10 ahicks92

I'll throw out that this could easily be a tiny crate using an extension trait. Going into STD is a high bar. If this is so necessary it seems to me that one could make such a crate and prove the popularity first.

That's not a yes or a no, I'm just saying the obvious thing. I'm sort of meh one way or the other. However to me if it can't become a popular crate and it isn't something that the compiler itself has to help with, then it doesn't seem to hit the bar to go into std in my opinion.

Sure. I have this functionality in a project at the moment; I can shape it up into a crate.

If this crate catches on with this name -- does that block that name from going into STD? I remember hearing some similar issues with popular Itertools constructs effectively blocking the name from going onto Iterator.

zkldi avatar Oct 05 '24 10:10 zkldi

If this crate catches on with this name -- does that block that name from going into STD? I remember hearing some similar issues with popular Itertools constructs effectively blocking the name from going onto Iterator.

The main problem with Itertools is that both are trait methods, and there’s no supertrait shadowing yet, so you’ll get ambiguity errors. An inherent method will take precedent, so unless the extension trait has a different type signature (e.g. like here), it shouldn’t create any issues.

steffahn avatar Oct 05 '24 10:10 steffahn

If this crate catches on with this name -- does that block that name from going into STD? I remember hearing some similar issues with popular Itertools constructs effectively blocking the name from going onto Iterator.

The main problem with Itertools is that both are trait methods, and there’s no supertrait shadowing yet, so you’ll get ambiguity errors. An inherent method will take precedent, so unless the extension trait has a different type signature (e.g. like here), it shouldn’t create any issues.

Awesome. I'll publish my crate for this tonight.

zkldi avatar Oct 05 '24 13:10 zkldi

This is now published as a crate: https://github.com/zkrising/unwrap_todo (cargo add unwrap_todo).

zkldi avatar Oct 05 '24 18:10 zkldi

I think the unreachable variant has merit too. I often find myself writing: .unwrap_or_else(|| unreachable!()). Which is very verbose, and makes the code less readable (especially when the line gets long enough that Clippy formats it in chain form).

Kriskras99 avatar Oct 10 '24 07:10 Kriskras99

Unwrap todo seems great. I remember when I first learned Rust, I only knew panic!, but then I found todo! and unreachable!. They are not life changing, honestly, but I do use them instead of panic! as I want my code to more clearly express my intention and thinking process.

Going into STD is a high bar. If this is so necessary it seems to me that one could make such a crate and prove the popularity first.

That said, would/will I add a crate just for todo! and/or unreachable!? No.....

I've seen a lot of APIs gone into std because they are just convenient to use or express intention. When I saw them stabilized, I first got surprised, just like you could just do X instead, why need this, but then I am really glad that they were added when I use them.

ifsheldon avatar Oct 11 '24 07:10 ifsheldon

I can suggest the rules that I use in my code:

  • Use expect as panic and write a message why this code is actually will not panic
  • Use expect as unreachable and write a message why this code is actually unreachable
  • Use unwrap as todo

In tests and documentation examples the ? should be used.

You can also add this lint to the config:

[lints.clippy]
unwrap-used = "deny"

It would be great to rename unwrap to todo, but unfortunately unwrap is already widely used. However, I think it's worth to reduce usage of unwrap. expect is always more readable and produces clearer error messages. unwrap is ok in rapid prototyping code, this implies its todo semantics in more production code

nanoqsh avatar Oct 11 '24 18:10 nanoqsh

I cannot agree with this suggestion.

First, there could possibly be an arbitrary large set of possible reasons why an author would use .unwrap(), are we going to take care of every single possible reason? todo() and unreachable() are the only two that the author of this RFC can think of currently, if he comes out with more, are we going to add more methods to Result and Option?

Second, todo!(), unreachable!(), coupled with unwrap_or_else() already cover the scenarios the author of the RFC raised, why add methods to Result and Option?

Third, unwrap() has monadic origins, Result and Option are monads. From a type theoretic perspective, there isn't a need to include new methods that behaves the same as unwrap() that serves no more purpose than what unwrap() already did.

jymchng avatar Oct 13 '24 10:10 jymchng

This sounds like the RFC author is desperate to 'contribute' to Rust.

Please don't do this here. This is uncalled for.

BurntSushi avatar Oct 13 '24 11:10 BurntSushi

This sounds like the RFC author is desperate to 'contribute' to Rust.

Please don't do this here. This is uncalled for.

Apologies, let me withdraw it.

jymchng avatar Oct 13 '24 12:10 jymchng

I've been reading all the responses to this and I wanted to point out two things I found sort of interesting:

Temporary panicking in prototypes is common

That is to say, using something for temporary error handling for prototype code isn't a niche thing. A lot of people in those discussions have expressed their usage of something for temporary handling.

A lot of people have chimed in with what they do in their codebases for these semantics. I think that implies heavily to me that there is demand for this semantic, but since the language doesn't provide a standard choice, people assign their own semantics ontop of the existing APIs.

That suggests, to me, that the semantics I'm describing in the OP are at least "real" -- "unwrap todo" is something that people already want to express, and I'm not just suggesting something nobody uses :sweat_smile:.

People have different ways of expressing it

If the above point establishes that "just panic here temporarily" is a real thing people want, then the rest of the discussion goes to show that there's not an agreed way of expressing it.

I've seen, so far:

  • .unwrap() implies temporary handling, use .expect("reason") for anything deliberate.
  • .unwrap() is for deliberate cases that need no further explanation, use .expect("todo") to indicate todo-semantics.
  • .unwrap() implies unreachability, use .expect("todo").
  • Just use comments above unwraps to attach semantics.
  • Use .unwrap_or_else(|| todo!()), or let-else, and some others...

There's nothing necessarily wrong with this, but there's a lot of talking-past-eachother in these threads; it seems to me like there's not a consensus on what .unwrap() is even supposed to mean in code. Some people think it should only be used for todo-semantics, some think it should be used for self-evident .expect().

The standard library docs don't seem to take a stance on this either, which likely contributes to this confusion.

I think the discussions (at least, to me) evince .unwrap() being semantically overloaded? I'm writing a bit too much in this thread now, but I did want to point this out!

zkldi avatar Oct 13 '24 15:10 zkldi

Semantics aren't the same as...I don't have a good word, let's go with conventions. You're suggesting effectively multiple names for the same semantics and that they be so blessed that they belong in stdlib.

I would go so far as to also say that panicking in prototypes isn't great either. You can throw Anyhow or similar at many of these problems and then it's just ? your problems away. There's an implicit assumption that panicking like that needs to be common.

As I already said it's possible to do this as a crate. If such a crate became popular I'd be less negative about this. You're showing that people like to express temporary panicking, but just because people can't agree on .expect("todo") versus whatever else doesn't mean that it's the place of Rust to impose a specific style, nor does it mean that enough people are going to find and adopt it either given that almost a decade of learning material teaches unwrap. You're implicitly arguing:

  • Panicking in prototypes is the best way and should be encouraged.
  • We need a consistent style for it
  • Your favorite style is the right style for it
  • This is important enough that having a bunch of functions that do the same thing with different names, one of the basic "don't do this" coding practices, is worth it.
  • Enough people will find it so that it becomes widely adopted and people won't just keep doing their own thing.

That's a whole lot of points to argue. You can argue the importance of them, but they're all present in the proposal here. There's also an implicit assumption that people who hang out on RFC threads on GitHub are representative as well.

I will personally ignore these if they landed but that's because I just don't unwrap like this. Anyhow and ok_or_else solve the problem quite nicely and don't require a big refactor/cleanup later and the code can move from some form of prototype toy to some form of production by just not panicking way up at the top in main. If it was my job to make this decision I'd honestly start at requiring you to argue me into why we should encourage this kind of code.

ahicks92 avatar Oct 14 '24 00:10 ahicks92

Semantics aren't the same as...I don't have a good word, let's go with conventions. You're suggesting effectively multiple names for the same semantics and that they be so blessed that they belong in stdlib.

I would go so far as to also say that panicking in prototypes isn't great either. You can throw Anyhow or similar at many of these problems and then it's just ? your problems away. There's an implicit assumption that panicking like that needs to be common.

As I already said it's possible to do this as a crate. If such a crate became popular I'd be less negative about this. You're showing that people like to express temporary panicking, but just because people can't agree on .expect("todo") versus whatever else doesn't mean that it's the place of Rust to impose a specific style, nor does it mean that enough people are going to find and adopt it either given that almost a decade of learning material teaches unwrap. You're implicitly arguing:

  • Panicking in prototypes is the best way and should be encouraged.
  • We need a consistent style for it
  • Your favorite style is the right style for it
  • This is important enough that having a bunch of functions that do the same thing with different names, one of the basic "don't do this" coding practices, is worth it.
  • Enough people will find it so that it becomes widely adopted and people won't just keep doing their own thing.

That's a whole lot of points to argue. You can argue the importance of them, but they're all present in the proposal here. There's also an implicit assumption that people who hang out on RFC threads on GitHub are representative as well.

I will personally ignore these if they landed but that's because I just don't unwrap like this. Anyhow and ok_or_else solve the problem quite nicely and don't require a big refactor/cleanup later and the code can move from some form of prototype toy to some form of production by just not panicking way up at the top in main. If it was my job to make this decision I'd honestly start at requiring you to argue me into why we should encourage this kind of code.

Agree with you. Is there a standard protocol to close RFC or a way to deem a particular RFC is not worth pursuing? OP could have made his suggestions into a crate and use the empirical evidences (if any) to back up his claim.

jymchng avatar Oct 14 '24 00:10 jymchng