Skip to content

Solution for asserting infallibility of a Result<T, Infallible> #329

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

Open
Rahix opened this issue Nov 22, 2021 · 1 comment
Open

Solution for asserting infallibility of a Result<T, Infallible> #329

Rahix opened this issue Nov 22, 2021 · 1 comment

Comments

@Rahix
Copy link

Rahix commented Nov 22, 2021

(From #328)

Back in the days, before core::convert::Infallible was added, we had void::Void to mark Results which cannot ever produce the error variant. With it came the void::ResultExt extension trait that adds a .void_unwrap() method to Result<T, Void>, to unwrap such an infallible result. There were nice advantages to that over just using normal unwrap() on such results:

  • For one, a reader could immediately understand that there is no potential panic location here, even though a Result is getting unwrapped.
  • Secondly, if the called function which returns said result ever changes to a different, concrete error type, any downstream code using void_unwrap() would immediately stop to compile, forcing one to revisit the code. When just using unwrap(), this would silently become a panic site on dependency upgrade.

Now, the void crate is largely unmaintained and as core::convert::Infallible is becoming the core-language equivalent, I'd like to see a similar feature available. There is an unstable Result<T, !>::into_ok() method but that doesn't help much for stable users. To note, it is trivial to reimplement but I feel like this needs to be done somewhere "official" and embraced throughout the ecosystem.

What I care about most here is how we can teach proper coding habits: By experience, people copy from crate and documentation examples so if these use unwrap(), downstream code will as well. In turn, this will make it hard for upstream crates to eventually introduce concrete error types because of the risk of silently breaking downstream code. If there is a commonly accepted version of "infallible_unwrap()" (please bikeshed a better name) which is used throughout examples, downstream users will naturally move to using it as well.

I see a few possible solutions to here:

  • Create a very small crate with such an implementation and adopt it throughout the ecosystem to encourage its use.
  • Actually add such an extension trait to embedded-hal. While not technically a fitting place, embedded-hal (and its prelude!) is pulled in by lots of projects already, so having it available there would reach the most people.
  • Push for stabilization of the standard library into_ok() method such that we can just promote use of that.
@Beanow
Copy link

Beanow commented Oct 19, 2023

Breaking the silence on this topic after a couple years, I'll add some takes.

Poking around a little bit, if you set any opt-level at all, any of these alternatives will just compile away for Infallible / ! errors. So that's at least one aspect we don't need to care about. https://rust.godbolt.org/z/vor63P3j4

But I found the into_ok docs made a better argument than readability alone:

Unlike unwrap, this method is known to never panic on the result types it is implemented for. Therefore, it can be used instead of unwrap as a maintainability safeguard that will fail to compile if the error type of the Result is later changed to an error that can actually occur.

And with that said! I stumbled upon unwrap-infallible 🎉.
With nearly identical docs and arguments about maintainability.

TL;DR I get to the following table:

Method Familiar terms Method readability Maintenance safeguard Method in core Error in core
.into_ok() Yes After adoption Yes Nightly Nightly
.unwrap() Yes No1 No 1.34.0 Yes
.unwrap_unchecked() Yes No1 No 1.58.0 Yes
.void_unwrap() No2 With IDE Yes No No
.unwrap_infallible() Yes Yes Yes No Yes

No1 = It's not obvious from .unwrap() and friends that this is infallible.
No2 = Personal opinion. I think void is an overloaded term and doesn't make it obviously infallible to me.

So my opinion is that:

  1. Result<_, Infallible> is better than Result<_, Void> and confirmed in v1: Fix error types #150.
  2. The maintainability safeguard is a great argument, especially because embedded-hal is primarily for other maintainers. And it may be a motivation to use a "stopgap" crate before .into_ok() lands in stable.
  3. unwrap_infallible and is_ok are better method names than void_unwrap.

However I cannot propose what the tradeoff should be here. If we're going to push hard on .into_ok() adoption, this will impact the MSRV at that time. unwrap-infallible seems well positioned to add deprecation warnings when .into_ok() is available.

Possibly even with feature flags and adding UnwrapInfallible::is_ok as a PR there, it could be a drop-in replacement simply by making the use unwrap_infallible::UnwrapInfallible; and crate dependency conditional.

Currently I don't see any v1 code that actually unwraps an infallible result though! If that remains the case, it might actually be up to other maintainers whether they want to use unwrap-infallible or not?

Rahix added a commit to Rahix/avr-hal that referenced this issue Jan 13, 2024
After removing `void`, we're still missing a way to explicitly unwrap
infallible results such that it is clear that no panic can occur in this
location.  Because the standard library does not yet provide a solution
to this, let's use the `unwrap-infallible` crate [1] in the meantime.

This topic was also discussed in more detail in the embedded-hal project [2].

[1]: https://crates.io/crates/unwrap-infallible
[2]: rust-embedded/embedded-hal#329
Rahix added a commit to Rahix/avr-hal that referenced this issue Jan 13, 2024
After removing `void`, we're still missing a way to explicitly unwrap
infallible results such that it is clear that no panic can occur in this
location.  Because the standard library does not yet provide a solution
to this, let's use the `unwrap-infallible` crate [1] in the meantime.

This topic was also discussed in more detail in the embedded-hal project [2].

[1]: https://crates.io/crates/unwrap-infallible
[2]: rust-embedded/embedded-hal#329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants