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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zkrising
Copy link

@zkrising zkrising commented Oct 3, 2024

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

@zkrising zkrising changed the title [RFC] Semantic .unwrap() alternatives [RFC] Add Option::{todo, unreachable} and Result::{todo, unreachable} Oct 3, 2024
@fmease fmease added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Oct 3, 2024
@fmease
Copy link
Member

fmease commented Oct 3, 2024

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

@ChrisJefferson
Copy link

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
Copy link
Contributor

digama0 commented Oct 3, 2024

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
Copy link

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
Copy link

jongiddy commented Oct 3, 2024

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
Copy link
Author

zkrising commented Oct 3, 2024

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.

It doesn't provide much benefit, if at all, and it arguably drags this RFC down a lot. I think `.todo()` is a strong candidate, but `.unreachable()` is contentious and can be emulated nicely with `.expect("REASON WHY UNREACHABLE").`
@zkrising zkrising changed the title [RFC] Add Option::{todo, unreachable} and Result::{todo, unreachable} [RFC] Add Option::todo and Result::todo Oct 3, 2024
@zkrising
Copy link
Author

zkrising commented Oct 3, 2024

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
Copy link
Member

steffahn commented Oct 3, 2024

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

@programmerjake
Copy link
Member

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

@SOF3
Copy link

SOF3 commented Oct 4, 2024

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
Copy link

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
Copy link
Contributor

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
Copy link
Member

kennytm commented Oct 4, 2024

Also, clippy could warn .todo separately from .expect

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

@Qyriad
Copy link

Qyriad commented Oct 4, 2024

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
Copy link
Contributor

slanterns commented Oct 4, 2024

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
Copy link
Author

zkrising commented Oct 4, 2024

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.

@ahicks92
Copy link

ahicks92 commented Oct 5, 2024

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
Copy link
Author

zkrising commented Oct 5, 2024

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
Copy link
Member

steffahn commented Oct 5, 2024

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
Copy link
Author

zkrising commented Oct 5, 2024

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
Copy link
Author

zkrising commented Oct 5, 2024

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

@Kriskras99
Copy link

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
Copy link

ifsheldon commented Oct 11, 2024

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.

@zkrising
Copy link
Author

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

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!

@ahicks92
Copy link

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.

@jymchng
Copy link

jymchng commented Oct 14, 2024

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
Copy link

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
Copy link

SOF3 commented Oct 14, 2024

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
Copy link

jymchng commented Oct 14, 2024

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
Copy link

@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
Copy link

jymchng commented Oct 14, 2024

@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
Copy link
Member

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
Copy link

jymchng commented Oct 14, 2024

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
Copy link
Author

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
Copy link

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
Copy link
Contributor

liigo commented Oct 17, 2024

there're two branches in unwrap/expect

  • (a) unwrap: to move something out of container
  • (b) panic: maybe dangerous to do that

a good method name should matches both of them.

  • unwrap isn't a good name, because it matches (a) but not (b).
  • expect isn't a good name, because it matches (b) but not (a).
  • todo is a bad name, because it matches neither (a) nor (b). (to do what? who knows?)
  • unwrap_or_panic is good, because it matches both (a) and (b), but it's too long.
  • defuse is a better name, because it matches both (a) and (b) and it's short.

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
Copy link

@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
Copy link

ronanM commented Oct 21, 2024

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:

  • let cache_len = cache.read().poison().len();

  • let regex = Regex::new("a*bc").re();

@SOF3
Copy link

SOF3 commented Oct 25, 2024

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
Copy link

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
Copy link

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
Copy link
Author

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

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
Copy link

SOF3 commented Oct 25, 2024

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
Copy link

SOF3 commented Oct 25, 2024

What about ok_or_todo and/or unwrap_or_todo?

@zkrising
Copy link
Author

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
Copy link

SOF3 commented Oct 25, 2024

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.

@Srlion
Copy link

Srlion commented Oct 25, 2024

It would be great to have something like this. I often find myself thinking, "Why isn't there a tool, like todo!() for Result and Option, that can remind me to handle it later?" Sure, you can set deny rules, but am I really going to deny unwrap everywhere when I have mutex locks scattered around using unwrap? This feature would be perfect for building quick applications where I can focus on error handling later. It's frustrating to have to worry about it right from the start, especially when I might forget an unwrap I used or end up searching through files for an unwrap I left in while testing.

@leb-kuchen
Copy link

What's wrong with Option::unwrap_or_else? todo! is a macro, so the signature wouldn't even be clear. Should it be &str or format_args!, latter wouldn't even be more ergonomic. There is even a clippy lint for expect(format!), but I guess former can be inefficient, since it should be only used or todos. What about Option::unimplemented, which I guess would have to be efficient, since it would be used in application code?
With todo!, unreachable! and unimplemented! there are already 3 similar ways to panic, so why add more?

@castarco
Copy link

castarco commented Oct 27, 2024

As a not sophisticated Rust user (I follow many of the weird theoretical discussions about the language, but I rarely engage, as my real use cases are pretty simple) I'm sympathetic to the goal of being able to write very clear and concise code (where the intent is always "self-evident", or almost).

I understand that .unwrap() and its siblings might not feel as enough given that many people have slightly different understanding of the intent behind their usage and how and when to use them...

But .todo() seems to me like a strange choice of words to convey the specific intent expressed in this RFC. I see how it relates to the todo! macro, but that's the only reason I can understand what .todo() does without having to go into its documentation (seeing it called as a trait method doesn't feel the same as seeing a standalone "function" call), I think it shouldn't be like that.

In short, I support the idea of continuing the exploration for better names, even if this leads to a small amount of redundancy on the std lib side, but I also think that RFCs like this one (which one should expect to be at least slightly controversial) should have some extra work behind before being pushed:

  • a wider exploration of possible names, and explanations on why ones have been discarded and others chosen
  • as others have pointed out, maybe publish a crate and let it be used for some time to gather objective empirical data that can be used to support the proposal.

@danakj
Copy link

danakj commented Oct 27, 2024

It may be nice if you could shove a bug number or arbitrary string in the arguments.

@SOF3
Copy link

SOF3 commented Oct 28, 2024

Rephrasing @leb-kuchen's comment, .unwrap_or_else(|| todo!()) totally satisfies the "IDE could mark .todo differently from .expect" requirement as @lebensterben mentioned above as well. So .todo() only serves the purpose of conciseness, but if we cannot come up with a good, short, clear name, it would be no better than .unwrap_or_else(|| todo!()).

@Takashiidobe
Copy link

The link in the original post to the crate is incorrect. It should be https://github.com/zkrising/unwrap_todo instead of https://github.com/zkrising/unwrap-todo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.