rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
2.03k stars 205 forks source link

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

Open Rahix opened 3 years ago

Rahix commented 3 years ago

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

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:

Beanow commented 1 year ago

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 :tada:. 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: https://github.com/rust-embedded/embedded-hal/pull/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?