Skip to content

Implement Error traits for Infallible #328

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

Merged
merged 2 commits into from
Nov 20, 2021

Conversation

jannic
Copy link
Member

@jannic jannic commented Nov 19, 2021

No description provided.

@jannic jannic requested a review from a team as a code owner November 19, 2021 22:12
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

Please see the contribution instructions for more information.

@Rahix
Copy link

Rahix commented Nov 20, 2021

While at it, maybe we can also implement them for void::Void, gated by a feature flag?

@jannic
Copy link
Member Author

jannic commented Nov 20, 2021

Is void::Void providing any advantage in comparison to core::convert::Infallible?

@jannic
Copy link
Member Author

jannic commented Nov 20, 2021

At last, an implementation for void::Void would be quite easy: jannic-dev-forks@586c1dc
So if it's wanted, I can add it to this PR. However, I'm not yet convinced that this is really useful. Void seems to be replaced by Infallible, and the Void crate didn't get any updates for years, even though there are some PRs pending.

@burrbull
Copy link
Member

At last, an implementation for void::Void would be quite easy: jannic@586c1dc So if it's wanted, I can add it to this PR. However, I'm not yet convinced that this is really useful. Void seems to be replaced by Infallible, and the Void crate didn't get any updates for years, even though there are some PRs pending.

No, Void is hack when Infallible was absent. It should gone.

Copy link
Contributor

@ryankurte ryankurte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seconding that we don't need void::Void here as it's been functionally replaced by Infallible.

lgtm, thanks!
bors r+

@bors bors bot merged commit a0bd3dc into rust-embedded:master Nov 20, 2021
@Rahix
Copy link

Rahix commented Nov 21, 2021

The void crate provides one nice thing which Infallible currently (AFAIK) does not: The void_unwrap() extension to Result<T, Void>. This makes it immediately obvious that an unwrapping is actually infallible. With Result<T, Infallible> you'll have to use a normal unwrap() which, while compiling down ot the exact same code, is not obviously infallible for a reader of the code.

Also, when the signature of a called function changes from Infallible to an actual error type, the unwrap() will silently compile to a potential panic location while void_unwrap() would immediatly stop working, requiring you to revisit this call site and properly deal with the "upsteam change".

@jannic
Copy link
Member Author

jannic commented Nov 21, 2021

@Rahix, the equivalent to void_unwrap() for Infallible would be https://doc.rust-lang.org/std/result/enum.Result.html#method.into_ok. Unfortunately, it's still unstable.
But adding your own equivalent to void::ResultVoidExt for Infallible is easy and only takes a few lines of code. Just copy https://docs.rs/void/1.0.2/src/void/lib.rs.html#79-101 and replace Void by Infallible.

@ryankurte
Copy link
Contributor

you run into some weird issues when crates use both Void and Infallible too, i did suggest a feature to collapse these some time ago which i think would let you achieve the same (?) but, didn't garner a response.

@Rahix
Copy link

Rahix commented Nov 21, 2021

Result::into_ok(). Unfortunately, it's still unstable.

Neat, didn't know of this yet. Yeah, very unfortunate.

But adding your own equivalent to void::ResultVoidExt for Infallible is easy

Was a bit surprised, but yeah, this actually compiles as well:

fn unreachable(t: core::convert::Infallible) -> ! {
    match t { }
}

The thing I am worried about is not finding a solution for myself, but how do we teach this to others. If there isn't an easily usable method, everyone will resort to just using unwrap() anyway which really is not ideal. With void, I could always use void_unwrap() in the crate and docs examples and thus nudge people the right way. With Infallible, I don't see such an easy method short of adding a HAL-specific extension trait. But that is also very much less than ideal...

I'm all for switching to Infallible where possible, but I'd really like to find a solution here to keep the unwrap()ing in check. Maybe we can establish an alternative crate across the ecosystem which just contains such an extension trait for Infallible? Or maybe add it to embedded-hal's prelude directly (not sure about that...)?

@jannic
Copy link
Member Author

jannic commented Nov 21, 2021

I agree that there is room for improvement. However I don't think this PR is the right place to discuss it.

@Rahix
Copy link

Rahix commented Nov 22, 2021

Totally agree. I opened #329 for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants