From 375b4b1583c9c3ebbe9b6bc7716fbc5b5809616a Mon Sep 17 00:00:00 2001 From: zkldi <20380519+zkldi@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:00:15 +0100 Subject: [PATCH 1/3] Semantic .unwrap() alternatives --- text/0000-semantic-unwraps.md | 309 ++++++++++++++++++++++++++++++++++ 1 file changed, 309 insertions(+) create mode 100644 text/0000-semantic-unwraps.md diff --git a/text/0000-semantic-unwraps.md b/text/0000-semantic-unwraps.md new file mode 100644 index 00000000000..c9c0fc5dd3b --- /dev/null +++ b/text/0000-semantic-unwraps.md @@ -0,0 +1,309 @@ +- Feature Name: `semantic-unwraps` +- Start Date: 2024-10-03 +- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) +- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) + +# Summary +[summary]: #summary + +This RFC proposes `Result::{todo, unreachable}` and `Option::{todo, unreachable}` functions which work like `.unwrap()` but imply different semantic reasons for the unwrapping. + +`.todo()` implies that error handling still needs to be done and that unwrapping here is a temporary thing that should be fixed in the future. This is analogous to `todo!()`. + +`.unreachable()` implies that we are unwrapping because we believe the `Err` or `None` case *cannot* occur. This is analogous to `unreachable!()`. + +In short, this allows users to preserve semantic information about why they are unwrapping. This proves extremely useful in prototype code, and in distinguishing "unreachable unwraps" and "todo unwraps" from "panic unwraps". + +As an example: + +```rs +// unwrap is still used for the analogous-to-panic!() case +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(); + +// these error states are unreachable. +// this use case is common in static declarations. +NonZeroU32::new(10).unreachable(); +Regex::new("^[a-f]{5}$").unreachable(); +``` + +# Motivation +[motivation]: #motivation + +`.unwrap()` is semantically overloaded in rust. It finds itself used for three significantly different reasons: + +- If this is `None/Err`, our program is in a bad state and we want to exit (missing config files, missing resource files, some external invariant not upheld) + +```rs +// e.g. +// our web server should just die if it can't open a tcp socket +TcpListener::bind(&addr).unwrap(); +``` + +- I haven't added `None/Err` handling here yet (prototype code) + +```rs +// e.g. +// parse user input as an int, +// can't be bothered handling bad input right now +let int: i32 = input.parse().unwrap(); +// my CLI needs a second argument to do anything useful, +// i should handle this properly later +let arg2 = std::env::args().nth(2).unwrap(); + +``` + +- I completely assert that the `None/Err` case here cannot happen + +```rs +// e.g. +// this cannot fail +NonZeroU32::new(10).unwrap(); +Regex::new("^[a-f]{5}$").unwrap(); +``` + +### What's wrong with this? + +Users find themselves using `.unwrap()` for these different reasons, but the semantic reason *why* unwrapping was done is not stored in the source code. +Some users write comments near unwraps to justify them, but this is easy to forget or fall out of sync with the codebase. + +In my experience, the `unwrap as todo!()` paradigm is much more common in application rust. + +This problem becomes more pronounced when one wants to go back over the code and fix all of those "todo unwraps". +It's not easy to figure out whether the `.unwrap()` has a good reason for being there, or is simply a result of a hastily-written MVP. + +While in terms of actual program execution, nothing is different (the program will panic), the source code itself doesn't necessarily track the information *why* that panic was placed there. + +### Prior art + +We already have prior art for "different kinds of panics" in the form of `todo!()` and `unreachable!()`. These macros are used frequently in rust and I'm not aware of anyone considering them a bad API. + +This gives the method names `.todo()` and `.unreachable()` a good justification, and they already map to a commonly-used feature. + +### What do we get then? + +```rs +// unwrap is still used for the analogous-to-panic!() case +TcpListener::bind(&addr).unwrap(); + +// we're panicking because error handling is not implemented yet. +let int: i32 = input.parse().todo(); +let arg2 = std::env::args().nth(2).todo(); + +// these error states are unreachable. +NonZeroU32::new(10).unreachable(); +Regex::new("^[a-f]{5}$").unreachable(); +``` + +And now the semantic reason for "why" we're panicking is preserved! + +# Guide-level explanation +[guide-level-explanation]: #guide-level-explanation + +## `Result::todo()` + +`Result::todo()` returns the value in `Ok`, consuming the result. + +This function will panic if the value is an `Err`, **with a panic message indicating that error handling is not implemented yet here.** This is analogous to `todo!()`. + +This function may be preferred to `Result::unwrap()` for prototype code where error handling should be implemented later. + +### Example + +```rust +// Error handling is not implemented here yet. This is quick and dirty prototype code. +let my_file: Vec = std::fs::read("file.txt").todo(); +``` + +## `Result::unreachable()` + +`Result::unreachable()` Returns the value in `Ok`, consuming the result. + +This function will panic if the value is an `Err`, **with a panic message indicating that the error case should not be reachable.** This is analogous to `unreachable!()`. + +This function may be preferred to `Result::unwrap()` for cases where the error case cannot happen. `Result::unreachable()` makes it clearer that this case is not expected to happen. + +### Example + +```rust +// The error state here cannot be reached. +let my_address: std::net::IpAddr = "127.0.0.1".parse().unreachable(); +``` + +## `Option::todo()` + +`Option::todo()` returns the value in `Some`, consuming the option. + +This function will panic if the value is `None`, **with a panic message indicating that None handling is not implemented yet here.** This is analogous to `todo!()`. + +This function may be preferred to `Option::unwrap()` for prototype code where handling should be implemented later. + +### Example + +```rust +// None handling is not implemented here yet. This is quick and dirty prototype code. +let arg2 = std::env::args().nth(2).todo(); +``` + +## `Option::unreachable()` + +`Option::unreachable()` Returns the value in `Some`, consuming the option. + +This function will panic if the value is `None`, **with a panic message indicating that the None case should not be reachable.** This is analogous to `unreachable!()`. + +This function may be preferred to `Option::unwrap()` for cases where None cannot happen. `Option::unreachable()` makes it clearer that this case is not expected to happen. + +### Example + +```rust +// The error state here cannot be reached. +let amount_of_crabs = NonZeroU32(12).unreachable(); +``` + +# Reference-level explanation +[reference-level-explanation]: #reference-level-explanation + +Since this is just a regular function composed of very regular parts of rust, I don't think there are many technical questions to be answered. + +This would be implemented by adding two features on `Result` and `Option`. + +I imagine an implementation would look vaguely like this: + +```rs +impl Result where E: fmt::Debug { + pub const fn todo(self) -> T { + match self { + Ok(t) => t, + Err(err) => { + todo!("Error handling not implemented: {err:?}") + } + } + } + + pub const fn unreachable(self) -> T { + match self { + Ok(t) => t, + Err(err) => { + unreachable!("Error case should not be reachable: {err:?}") + } + } + } +} + +impl Option { + pub const fn todo(self) -> T { + match self { + Some(t) => t, + None => { + todo!("None handling not implemented") + } + } + } + + pub const fn unreachable(self) -> T { + match self { + Some(t) => t, + None => { + unreachable!("None case should not be reachable") + } + } + } +} +``` + +# Drawbacks +[drawbacks]: #drawbacks + +### Obvious ones + +It's more functionality in the standard library which isn't ideal. + +It might cause some code churn/documentation churn if merged, as documentation now has other ways to indicate "exercise-for-the-reader" error handling. + +### Does `.todo()` encourage bad habits? + +I think a decent argument against this RFC is that `.todo()` encourages lazy error handling in rust. + +I'd argue that we already have this in rust with `.unwrap()`. And in fact, the current state of things is *more* dangerous. +In my experience, people will always elide error handling in prototype code, and the current state of affairs conflates that temporary panicking with permanent/intentional panicking. + +With a theoretical `.todo()`, you can defer error handling later in your software lifecycle, but it's always possible to see *where* your lazy error handling is. + +What I'm saying is -- this doesn't encourage lazy error handling, it just makes it less easy to forget. + +# Rationale and alternatives +[rationale-and-alternatives]: #rationale-and-alternatives + +### This could be done with ResultExt + +Then everyone could import a crate that does this for them. However, I think there's value +in standardising this in rust, as its an extremely common use case. Plus, if standardised, IDEs and Rust-Analyzer could treat it better. + +For example, `#[clippy::todo]` could highlight `.todo()`s and so on. + +### Other Method Names + +We could call these methods `.unwrap_todo()`, `.unwrap_unreachable()` instead, which might make it more obvious that these things panic. However, I'm conscious that these names are rather long, and having to write out `.unwrap_todo()` in prototype code is unlikely to catch on as a result. + +I don't think there's any good reason to choose names other than `todo` and `unreachable`, as they already exist prominently in rust. + +### What about `.expect()`? + +We do already have `{Option, Result}::expect` which serves a similar-ish purpose of "unwrap with a reason". + +I argue that this doesn't necessarily map onto the semantics of `todo` or `unreachable`, Especially `todo`. + +While this feature can be emulated with `.expect("todo")` and `.expect("unreachable")`, this is frustrating to type, easy to typo, harder to grep for and cannot be highlighted nicely by an IDE. + +### Should this take an argument? + +`.expect()` takes an argument which allows more info to be strapped on about the panic. + +I don't think `.todo` taking an argument would be good as it makes the code harder to write, plus, I don't see what you'd ever write there. + +It would maybe be useful to strap a reason on to `.unreachable` (why do you think its unreachable?), but this seems infrequently useful and makes the normal case ("this is self-evidently unreachable") more annoying to work with. + +# Prior art +[prior-art]: #prior-art + +The names `todo` and `unreachable` have prior art in the `todo!()` and `unreachable!()` macros, respectively. + +The concept of "semantic panics" has been in rust since 1.0.0, and this feature is widely used across the ecosystem. + +# Unresolved questions +[unresolved-questions]: #unresolved-questions + +In general, this is a relatively small addition in terms of "new" things, as it's comprised entirely of existing concepts in rust. + +### Error Message Names + +At the moment I've just ripped the names from `todo!()` and `unreachable!()`. + +I'm a little skeptical of the use of the term `None Handling` in `None Handling not implemented` in `Option::todo()`. I don't think that term is used anywhere else in rust and it doesn't look too great. + +There's probably a better phrasing for that. + +### Constness + +It would be ideal if these functions are `const`. However, I understand that `const-unwrap` is not stable yet. + +# Future possibilities +[future-possibilities]: #future-possibilities + +## `::unimplemented()` + +People might want `.unimplemented`. I think introducing this might be confusing and unecessary. + +I don't see `unimplemented!()` used anywhere near as much as `todo!()` or `unreachable!()`, and I don't see `unimplemented!()` ("i am deliberately not handling this") mapping over to `Option` or `Result`. + +It would make sense to add this to complete a mapping between those macros and Result/Option, but it doesn't seem like a very important addition. + +## `::unreachable_unchecked()` + +This has a macro counterpart in `unreachable_unchecked!()`, but I don't really see the point of adding it here. This is already achieved understandably with `unwrap_unchecked()`. + +There's no harm in adding it, but it doesn't seem as important. From 6a8abe8e9a6850bf5ca2124fe097817f25ad1ec0 Mon Sep 17 00:00:00 2001 From: zkldi <20380519+zkldi@users.noreply.github.com> Date: Thu, 3 Oct 2024 13:04:51 +0100 Subject: [PATCH 2/3] feat: fill out 3706 as our number --- text/{0000-semantic-unwraps.md => 3706-semantic-unwraps.md} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename text/{0000-semantic-unwraps.md => 3706-semantic-unwraps.md} (98%) diff --git a/text/0000-semantic-unwraps.md b/text/3706-semantic-unwraps.md similarity index 98% rename from text/0000-semantic-unwraps.md rename to text/3706-semantic-unwraps.md index c9c0fc5dd3b..738b1518448 100644 --- a/text/0000-semantic-unwraps.md +++ b/text/3706-semantic-unwraps.md @@ -1,7 +1,7 @@ - Feature Name: `semantic-unwraps` - Start Date: 2024-10-03 -- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000) -- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000) +- RFC PR: [rust-lang/rfcs#3706](https://github.com/rust-lang/rfcs/pull/3706) +- Rust Issue: [rust-lang/rust#3706](https://github.com/rust-lang/rust/issues/0000) # Summary [summary]: #summary From 9730345792ccd1cdc2f308c5f6561a818034f44c Mon Sep 17 00:00:00 2001 From: zkldi <20380519+zkldi@users.noreply.github.com> Date: Thu, 3 Oct 2024 19:14:45 +0100 Subject: [PATCH 3/3] update: remove ".unreachable()" 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").` --- ...-unwraps.md => 3706-option-result-todo.md} | 120 ++++-------------- 1 file changed, 22 insertions(+), 98 deletions(-) rename text/{3706-semantic-unwraps.md => 3706-option-result-todo.md} (58%) diff --git a/text/3706-semantic-unwraps.md b/text/3706-option-result-todo.md similarity index 58% rename from text/3706-semantic-unwraps.md rename to text/3706-option-result-todo.md index 738b1518448..0aa46b761d2 100644 --- a/text/3706-semantic-unwraps.md +++ b/text/3706-option-result-todo.md @@ -1,4 +1,4 @@ -- Feature Name: `semantic-unwraps` +- Feature Name: `option-result-todo` - Start Date: 2024-10-03 - RFC PR: [rust-lang/rfcs#3706](https://github.com/rust-lang/rfcs/pull/3706) - Rust Issue: [rust-lang/rust#3706](https://github.com/rust-lang/rust/issues/0000) @@ -6,13 +6,11 @@ # Summary [summary]: #summary -This RFC proposes `Result::{todo, unreachable}` and `Option::{todo, unreachable}` functions which work like `.unwrap()` but imply different semantic reasons for the unwrapping. +This RFC proposes `Result::todo` and `Option::todo` functions which work like `.unwrap()` but imply different semantic reasons for the unwrapping. `.todo()` implies that error handling still needs to be done and that unwrapping here is a temporary thing that should be fixed in the future. This is analogous to `todo!()`. -`.unreachable()` implies that we are unwrapping because we believe the `Err` or `None` case *cannot* occur. This is analogous to `unreachable!()`. - -In short, this allows users to preserve semantic information about why they are unwrapping. This proves extremely useful in prototype code, and in distinguishing "unreachable unwraps" and "todo unwraps" from "panic unwraps". +In short, this allows users to preserve semantic information about why they are unwrapping. This proves extremely useful in prototype code, and in distinguishing "todo, add error handling here" from "no, i actually want to panic in this case.". As an example: @@ -24,17 +22,12 @@ TcpListener::bind(&addr).unwrap(); // this use case is common in prototype applications. let int: i32 = input.parse().todo(); let arg2 = std::env::args().nth(2).todo(); - -// these error states are unreachable. -// this use case is common in static declarations. -NonZeroU32::new(10).unreachable(); -Regex::new("^[a-f]{5}$").unreachable(); ``` # Motivation [motivation]: #motivation -`.unwrap()` is semantically overloaded in rust. It finds itself used for three significantly different reasons: +`.unwrap()` is semantically overloaded in rust. It finds itself used for two significantly different reasons: - If this is `None/Err`, our program is in a bad state and we want to exit (missing config files, missing resource files, some external invariant not upheld) @@ -57,15 +50,6 @@ let arg2 = std::env::args().nth(2).unwrap(); ``` -- I completely assert that the `None/Err` case here cannot happen - -```rs -// e.g. -// this cannot fail -NonZeroU32::new(10).unwrap(); -Regex::new("^[a-f]{5}$").unwrap(); -``` - ### What's wrong with this? Users find themselves using `.unwrap()` for these different reasons, but the semantic reason *why* unwrapping was done is not stored in the source code. @@ -80,9 +64,9 @@ While in terms of actual program execution, nothing is different (the program wi ### Prior art -We already have prior art for "different kinds of panics" in the form of `todo!()` and `unreachable!()`. These macros are used frequently in rust and I'm not aware of anyone considering them a bad API. +We already have prior art for "different kinds of panics" in the form of `todo!()`. This macro is used frequently in Rust and I'm not aware of anyone considering them a bad API. -This gives the method names `.todo()` and `.unreachable()` a good justification, and they already map to a commonly-used feature. +This gives the method name `.todo()` a good justification, as it already maps to a commonly-used feature. ### What do we get then? @@ -93,10 +77,6 @@ TcpListener::bind(&addr).unwrap(); // we're panicking because error handling is not implemented yet. let int: i32 = input.parse().todo(); let arg2 = std::env::args().nth(2).todo(); - -// these error states are unreachable. -NonZeroU32::new(10).unreachable(); -Regex::new("^[a-f]{5}$").unreachable(); ``` And now the semantic reason for "why" we're panicking is preserved! @@ -119,21 +99,6 @@ This function may be preferred to `Result::unwrap()` for prototype code where er let my_file: Vec = std::fs::read("file.txt").todo(); ``` -## `Result::unreachable()` - -`Result::unreachable()` Returns the value in `Ok`, consuming the result. - -This function will panic if the value is an `Err`, **with a panic message indicating that the error case should not be reachable.** This is analogous to `unreachable!()`. - -This function may be preferred to `Result::unwrap()` for cases where the error case cannot happen. `Result::unreachable()` makes it clearer that this case is not expected to happen. - -### Example - -```rust -// The error state here cannot be reached. -let my_address: std::net::IpAddr = "127.0.0.1".parse().unreachable(); -``` - ## `Option::todo()` `Option::todo()` returns the value in `Some`, consuming the option. @@ -149,21 +114,6 @@ This function may be preferred to `Option::unwrap()` for prototype code where ha let arg2 = std::env::args().nth(2).todo(); ``` -## `Option::unreachable()` - -`Option::unreachable()` Returns the value in `Some`, consuming the option. - -This function will panic if the value is `None`, **with a panic message indicating that the None case should not be reachable.** This is analogous to `unreachable!()`. - -This function may be preferred to `Option::unwrap()` for cases where None cannot happen. `Option::unreachable()` makes it clearer that this case is not expected to happen. - -### Example - -```rust -// The error state here cannot be reached. -let amount_of_crabs = NonZeroU32(12).unreachable(); -``` - # Reference-level explanation [reference-level-explanation]: #reference-level-explanation @@ -179,16 +129,7 @@ impl Result where E: fmt::Debug { match self { Ok(t) => t, Err(err) => { - todo!("Error handling not implemented: {err:?}") - } - } - } - - pub const fn unreachable(self) -> T { - match self { - Ok(t) => t, - Err(err) => { - unreachable!("Error case should not be reachable: {err:?}") + todo!("Error not handled: {err:?}") } } } @@ -199,16 +140,7 @@ impl Option { match self { Some(t) => t, None => { - todo!("None handling not implemented") - } - } - } - - pub const fn unreachable(self) -> T { - match self { - Some(t) => t, - None => { - unreachable!("None case should not be reachable") + todo!("None not handled") } } } @@ -247,17 +179,17 @@ For example, `#[clippy::todo]` could highlight `.todo()`s and so on. ### Other Method Names -We could call these methods `.unwrap_todo()`, `.unwrap_unreachable()` instead, which might make it more obvious that these things panic. However, I'm conscious that these names are rather long, and having to write out `.unwrap_todo()` in prototype code is unlikely to catch on as a result. +We could call this method `.unwrap_todo()` instead, which might make it more obvious that this will panic. However, I'm conscious that these names are rather long, and having to write out `.unwrap_todo()` in prototype code is unlikely to catch on as a result. -I don't think there's any good reason to choose names other than `todo` and `unreachable`, as they already exist prominently in rust. +I don't think there's any good reason to choose names other than `todo`. They already exist prominently in Rust. ### What about `.expect()`? We do already have `{Option, Result}::expect` which serves a similar-ish purpose of "unwrap with a reason". -I argue that this doesn't necessarily map onto the semantics of `todo` or `unreachable`, Especially `todo`. +I argue that this doesn't necessarily map as nicely onto the semantics of `todo`. -While this feature can be emulated with `.expect("todo")` and `.expect("unreachable")`, this is frustrating to type, easy to typo, harder to grep for and cannot be highlighted nicely by an IDE. +While this feature can be emulated with `.expect("todo")`, this is frustrating to type, easy to typo, harder to grep for and cannot be highlighted nicely by an IDE. ### Should this take an argument? @@ -265,27 +197,23 @@ While this feature can be emulated with `.expect("todo")` and `.expect("unreacha I don't think `.todo` taking an argument would be good as it makes the code harder to write, plus, I don't see what you'd ever write there. -It would maybe be useful to strap a reason on to `.unreachable` (why do you think its unreachable?), but this seems infrequently useful and makes the normal case ("this is self-evidently unreachable") more annoying to work with. - # Prior art [prior-art]: #prior-art -The names `todo` and `unreachable` have prior art in the `todo!()` and `unreachable!()` macros, respectively. +The name `todo` has prior art in the `todo!()` macro, in which it means the exact same thing. -The concept of "semantic panics" has been in rust since 1.0.0, and this feature is widely used across the ecosystem. +The concept of "semantic panic" has been in rust since 1.0.0, and this feature is widely used across the ecosystem. # Unresolved questions [unresolved-questions]: #unresolved-questions In general, this is a relatively small addition in terms of "new" things, as it's comprised entirely of existing concepts in rust. -### Error Message Names - -At the moment I've just ripped the names from `todo!()` and `unreachable!()`. +### Panic Message Names -I'm a little skeptical of the use of the term `None Handling` in `None Handling not implemented` in `Option::todo()`. I don't think that term is used anywhere else in rust and it doesn't look too great. +At the moment I've just ripped the panic messages from `todo!()`. -There's probably a better phrasing for that. +There may be better phrasing. ### Constness @@ -294,16 +222,12 @@ It would be ideal if these functions are `const`. However, I understand that `co # Future possibilities [future-possibilities]: #future-possibilities -## `::unimplemented()` - -People might want `.unimplemented`. I think introducing this might be confusing and unecessary. - -I don't see `unimplemented!()` used anywhere near as much as `todo!()` or `unreachable!()`, and I don't see `unimplemented!()` ("i am deliberately not handling this") mapping over to `Option` or `Result`. +## `::unreachable()` -It would make sense to add this to complete a mapping between those macros and Result/Option, but it doesn't seem like a very important addition. +The initial RFC contained `.unreachable()` alongside `.todo()`. However, I think `.unreachable()` is not of much value, and is *significantly more contentious* than `.todo()`. -## `::unreachable_unchecked()` +`.unreachable()` can be done with `.expect("REASON WHY THIS CANNOT HAPPEN")` with little downside. In fact, I struggle to come up with a convincing argument why adding a method for this would help things. -This has a macro counterpart in `unreachable_unchecked!()`, but I don't really see the point of adding it here. This is already achieved understandably with `unwrap_unchecked()`. +`.expect()` is a lot nicer for these things; you can provide a reason why you believe the error not to occur. Plus, `.unreachable()` is a confusing name. -There's no harm in adding it, but it doesn't seem as important. +The same is true for `unimplemented` and `unreachable_unchecked` equivalents. Both already map decently onto `expect` and `unwrap_unchecked`, respectively.