rust-lang / rfcs

RFCs for changes to Rust
https://rust-lang.github.io/rfcs/
Apache License 2.0
5.97k stars 1.57k forks source link

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

Open zkrising opened 1 month ago

zkrising commented 1 month ago

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();

This is available as a crate, in case you want to use it right now. I've been using this for quite a while now.


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

fmease commented 1 month ago

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

ChrisJefferson commented 1 month ago

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.

digama0 commented 1 month ago

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.)

ChrisJefferson commented 1 month ago

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.

jongiddy commented 1 month ago

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()?

zkrising commented 1 month ago

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.

zkrising commented 1 month ago

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 :(

steffahn commented 1 month ago

Some further obvious alternatives you can already use:

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();
programmerjake commented 1 month ago

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

SOF3 commented 1 month ago

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")?

lebensterben commented 1 month ago

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

clarfonthey commented 1 month ago

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… 🤷🏻

kennytm commented 1 month ago

Also, clippy could warn .todo separately from .expect

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

Qyriad commented 1 month ago

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".

slanterns commented 1 month ago

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.

zkrising commented 1 month ago

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:

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.
ahicks92 commented 1 month ago

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.

zkrising commented 1 month ago

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.

steffahn commented 1 month ago

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.

zkrising commented 1 month ago

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.

zkrising commented 1 month ago

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

Kriskras99 commented 1 month ago

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).

ifsheldon commented 1 month ago

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.

nanoqsh commented 1 month ago

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

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

jymchng commented 1 month ago

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.

BurntSushi commented 1 month ago

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

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

jymchng commented 1 month ago

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.

zkrising commented 1 month ago

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:

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!

ahicks92 commented 1 month ago

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:

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.

jymchng commented 1 month ago

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.

ChrisJefferson commented 1 month ago

In my experience of writing academic scientific software, particularly during early development, panicing by unwrap()/expect() in situations where we don't really know how to recover, leads to better quality code than using ?.

If a program panics before finishing, users assume it "did not work", they ignore any partially produced outputs, and they investigate why the panic occurred. This is good, because it means we aren't trusting the possibly incorrect output.

While it's as easy to write '?' and pass the problem up, it's hard to test and get it right. I've seen programs which, (for example), when a hard drive gets full output only partial output but appear to finish successfuly. Or, programs which try, and fail, to recover correctly from unusual error situations, like poisoned mutexes.

If I'm not carefully testing a particular error state, I find it better to 'unwrap' and explode, than believe I'm handling it with ?, but not really bother testing I'm handling it correctly (and it's hard to test things like poisoned mutexes).

Of course, when you are working on a battle-hardened, polished, library intended for use by many people, you add enough tests you can be sure it never panics, and also behaves correctly, but that's a huge amount of work!

SOF3 commented 1 month ago

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?

Not every single reason, just to align it with the std macros. We have unreachable, unimplemented and todo, and it makes sense to add these three to Option/Result as well, just as well as expect/unwrap indicates panic. Of course, now that leads to whether these functions should take a message instead.

jymchng commented 1 month ago

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?

Not every single reason, just to align it with the std macros. We have unreachable, unimplemented and todo, and it makes sense to add these three to Option/Result as well, just as well as expect/unwrap indicates panic. Of course, now that leads to whether these functions should take a message instead.

Yes, then how do we decide which are the methods that we ought to add and which we should not add?

And what about maintainability? Let's not add too much burden onto the Rust team on maintaining these APIs of such common types, the standard library should be lean and absolutely concise. .unwrap() is there for type-theoretic reason - namely, Result (and Option, but for brevity, I will only refer to Result from now on) is a monad, hence for 'unchecked' unwrapping, you need a method that panics if the inner value is not what you expect. .expect() lets you add a message to specify what is the reason for the mismatch in expectation. The proposed .todo() is only either .unwrap_or_else(|| todo!()); or .expect("todo");. I don't understand why we need to burden the Rust team to maintain a new API just so that we have 'a different name' from .unwrap().

You have not convinced me that because we have these macros, hence, we should have those corresponding methods in Result. By extension, should we also have an attribute #[todo] macro that annotates an entire code block or module to specify that this is something not done? Where do we draw the line on what's the Rust's team responsibility on providing concise APIs for common types and what's ought to be left to the community's creativity in terms of crates?

crescentrose commented 1 month ago

@jymchng .expect() exists, when you could just as easily write .unwrap_or_else(|| panic!("")). And, as you have pointed out yourself, todo! is already a macro even though one could just as easily write panic!("todo"). So prior art to extending type-theoretical methods and adding language features for convenience and brevity is there.

Regarding developmental and maintenance burdens, we are discussing whether adding todo() would result in additional maintenance burden to the Rust core team. Looking at OP’s implementation, this is a one-line method calling the panic! macro. This seems quite stable and low-maintenance to me. I put it to you that if the behaviour of the panic! macro changes to such a degree that it breaks this implementation, the Rust community has much larger issues than what is essentially an alias on two types.

jymchng commented 1 month ago

@jymchng .expect() exists, when you could just as easily write .unwrap_or_else(|| panic!("")). And, as you have pointed out yourself, todo! is already a macro even though one could just as easily write panic!("todo"). So prior art to extending type-theoretical methods and adding language features for convenience and brevity is there.

Regarding developmental and maintenance burdens, we are discussing whether adding todo() would result in additional maintenance burden to the Rust core team. Looking at OP’s implementation, this is a one-line method calling the panic! macro. This seems quite stable and low-maintenance to me. I put it to you that if the behaviour of the panic! macro changes to such a degree that it breaks this implementation, the Rust community has much larger issues than what is essentially an alias on two types.

Rust is a language of longevity. It is precisely this mentality that got C++ to become this bloated with almost a 100 keywords and thousands of public APIs to maintain. Today, you are thinking that it is only an additional of two one-liner methods - others would think the same, and in no time, the standard library would be bloated with hundreds of similar APIs. panic!() macro is a mechanism to call the compiler-implemented panic workflow, its existence is justified. If we don't have todo!(), I would find this RFC more acceptable. But since we already have this, I still find the reasons to add two new APIs less convincing.

BurntSushi commented 1 month ago

As a libs-api member, I'm not worried about the maintenance burden here. It's definitively a non-issue. Type theoretic concerns are also not going to be factor. (And Result isn't a monad.)

jymchng commented 1 month ago

As a libs-api member, I'm not worried about the maintenance burden here. It's definitively a non-issue. Type theoretic concerns are also not going to be factor. (And Result isn't a monad.)

Thirty years ago a C++ maintainer probably thought the same as you did now and it is those who are maintaining it now suffering. The standard library should be very concise and small and the maintainers, in my humble opinion, should be hyper selective and most importantly, prudent, on what gets into the standard library.

zkrising commented 1 month ago

Yes, then how do we decide which are the methods that we ought to add and which we should not add?

That is the point of an RFC!

Nobody is proposing a #[todo] attribute or anything like that. The RFC covers exactly what I am proposing and that is .todo().

.unimplemented() and .unreachable() are mentioned, as they draw from the same thing (the existing panic macros). Nothing else is mentioned. Emphatically, there is nothing in this RFC about a #[todo] attribute.

if he comes out with more, are we going to add more methods to Result and Option?

I am not the arbiter on what methods should be added to the language. If I were to propose more things, they would go through the same process.

I don't think it's fair to criticise this RFC based on things it isn't even proposing!

ahicks92 commented 1 month ago

If you use anyhow (for example) and panic in main instead of panicking in the rest of your code, you still get the user-drawing panic and you still get the backtrace if you enable that feature (maybe it's defaulted now that stdlib has backtrace support). In Rust you either ignore compiler warnings, explicitly write an ignore, panic at the location, or pass it to the caller. We can ignore the first 2 in the sense that anyone doing them is outside either approach; since errors can't be swallowed away, someone always makes the panic/not-panic decision and you don't get things like truncated output that claim success unless there is code explicitly ignoring the failure.

I'm not here to argue whether or not panic versus not-panic in prototypes is better if only because I can play devil's advocate and find enough reasons to not take that stance. I don't like those reasons, but that's not the same as them being bad. But since "just don't panic" is a reasonable stance, that's a subset of users who won't be using this. I am only pointing out that it's one of the implicit arguments being made.

I'm not thinking about maintainability because indeed that's a nonissue. I am thinking about whether or not this brings the ecosystem together. That's what I don't see in it.

liigo commented 1 month ago

there're two branches in unwrap/expect

a good method name should matches both of them.

So I'd like to propose adding new Result::defuse() Option::defuse().

Edit: other options, dissect, enucleate, flay, spay, ... and make it more clear that will be new methods instead of rename existing unwrap/expect.

jongiddy commented 1 month ago

@liigo However poorly chosen an existing name is, it is very unlikely that a simple renaming of an existing method will be accepted for the standard library.

This RFC is about adding a new method, albeit it being equivalent to an existing method.

ronanM commented 1 month ago

In my code, I've added some "extension functions" like this:

use regex::Regex;
use std::sync::LockResult;

#[easy_ext::ext(LockResultExt)]
pub impl<T> LockResult<T> {
  fn poison(self) -> T {
    self.expect("LockResult is poisoned")
  }
}

#[easy_ext::ext(RegexExt)]
pub impl Result<Regex, regex::Error> {
  fn re(self) -> Regex {
    self.expect("Bad RegEx")
  }
}

Usage:

SOF3 commented 1 month ago

I just looked at this again, and I realized string.parse().todo() is actually very ambiguous. It is unclear whether todo refers to handling the Ok or Err variant of the result. Would todo_err and todo_none be better names to show that we are actually treating the negative branch as a todo?

ifsheldon commented 1 month ago

I just came across the blog post Rust’s design goals should be about code. I think however it's actually implemented ("what name"), this RFC fits into the goal of "Clarity of purpose", i.e., making clear that this Option or Result is for now out of concern but needs revision later.

jongiddy commented 1 month ago

I just looked at this again, and I realized string.parse().todo() is actually very ambiguous. It is unclear whether todo refers to handling the Ok or Err variant of the result. Would todo_err and todo_none be better names to show that we are actually treating the negative branch as a todo?

This a very good point, similar to my comment about the proposed unreachable() method. In terms of clarity it is not clear that todo() handles the Err/None variant. It could suggest that the processing of the Ok/Some variant is still to be done. (Result::unwrap has the same problem but that ship has sailed.)

Expanding the method name to add clarity, such as using todo_err(), chips away at the advantage that todo() is about as easy as unwrap() to type.

I've felt ambivalent about todo() but this makes me lean towards not proceeding. I think it would be better to keep unwrap() to indicate that code is rough and not production-ready (the use case for todo), and work towards better methods for production code that currently uses unwrap. Something like the unwrap-infallible crate or an assert_some() method.

zkrising commented 1 month ago

I just looked at this again, and I realized string.parse().todo() is actually very ambiguous. It is unclear whether todo refers to handling the Ok or Err variant of the result. Would todo_err and todo_none be better names to show that we are actually treating the negative branch as a todo?

I'm not sure if .todo on its own is ambiguous; I think .todo() maps clearly onto .unwrap()/.expect(), which themselves don't specify they're for the Positive path.

A really big problem with .todo_err/.todo_none as names is that they are ambiguous depending on how you read them.

If you mentally see .todo as a synonym for unwrap, then .todo_err looks like .unwrap_err, which means "panic on Ok".

Whereas if you read .todo_err as "todo, handle err", then it means "panic on Err".

This alone I think will be the source of a lot of confusion if they were added like this, .todo_err either directly conflicts with .unwrap_err as a name, or directly conflicts with how it reads.

It actually took me a minute to realise what you were proposing was using .todo_err to mean "todo, handle err" rather than akin to .unwrap_err :sweat_smile:.

As for handling the negative paths, I struggle to think of a case where someone would want to write todo for a None path or Err path. We might want to look into them for completeness' sake, but there's some fairly decent naming concerns here.

SOF3 commented 1 month ago

I'm not sure if .todo on its own is ambiguous; I think .todo() maps clearly onto .unwrap()/.expect(), which themselves don't specify they're for the Positive path.

For Option, unwrap/expect is unambiguously the Some case since there is nothing to unwrap/expect for the None case (ok, you can still argue that we want to expect_none...)

For Result, writing get_data().unwrap() is probably clear that we want to unwrap the data and binary_search().expect("element must exist") actually expects the condition inside the string. But I agree these are ambiguous.

But read_file().todo() is magnitudes more confusing. This sounds like we are trying to implement some logic on the read contents instead of the error. Option/Result generally represents the positive type (especially considering the many type aliases for Result, e.g. io::Result<String> clearly represents "a result of string"), so verbs used on options/results naturally refers to the positive type. Meanwhile saying that "we have a result of bytes, and this result of bytes is TODO" makes no sense.

SOF3 commented 1 month ago

What about ok_or_todo and/or unwrap_or_todo?

zkrising commented 1 month ago

What about ok_or_todo and/or unwrap_or_todo?

Between those two, .unwrap_or_todo() is the sensible choice. It has precedent in .unwrap_or_default.

The main problem is the length of it. If this feature is added but it's notably harder to write than just .unwrap(), then it won't catch on and will just end up a vestigial rarely-used alternative to .unwrap().

But read_file().todo() is magnitudes more confusing. This sounds like we are trying to implement some logic on the read contents instead of the error.

I'm not sure I agree, but I have used this function in my own code for quite some time now, so I'm definitely used to it. I read .todo() as "ah i'll handle the negative case later", same as one would read .unwrap().

I don't think .todo() is confusing enough to warrant a longer name. We still have todo!() as precedent for this name, and I don't think people are confused by todo!() not being called panic_todo!().

SOF3 commented 1 month ago

todo!() doesn't exactly mean "panic todo" to me though. For me at least, it means "todo, just let this code compile and do whatever not insane in place of it" (in contrast to a comment which would cause compile error if a value is required). result.todo() can't convey this because you actually care about what happens when the branch is positive.