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

Replace all uses of the never type (!) with the stable Infallible #966

Closed
wants to merge 5 commits into from
Closed

Replace all uses of the never type (!) with the stable Infallible #966

wants to merge 5 commits into from

Conversation

jhpratt
Copy link
Contributor

@jhpratt jhpratt commented Apr 9, 2019

Infallible will be stabilized on Thursday.

Most of the diff is simple substitution of types (along with the relevant use). There is one situation that required a compiler hint regarding the function not actually returning; Infallible isn't a special case in the compiler, so it regards it the same as returning any other enum. I worked around this by creating a trivial macro (for readability) that adds unsafe { std::hint::unreachable_unchecked() } after a call; this will allow the compiler to avoid related errors and perform the relevant optimizations.

Related: Compile on stable Rust (#19).

Pinging @jebrosen for review.

@jebrosen jebrosen self-requested a review April 9, 2019 05:37
@jhpratt
Copy link
Contributor Author

jhpratt commented Apr 9, 2019

Looking at the final diff, it looks like VS Code auto-formatted when saving. I can manually undo this if wanted.

Copy link
Collaborator

@jebrosen jebrosen left a comment

Choose a reason for hiding this comment

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

Given the history of the never type, I think it's reasonable to switch to Infallible, which will later become an alias for !. Aside from this bail!() thing, This should be fine to include in a major release.

This is a breaking API change, albeit unlikely to break much if any code in the wild.

core/lib/src/config/mod.rs Outdated Show resolved Hide resolved
core/http/src/cookies.rs Outdated Show resolved Hide resolved
contrib/lib/src/lib.rs Show resolved Hide resolved
@jebrosen jebrosen self-requested a review April 9, 2019 23:09
@jebrosen jebrosen modified the milestone: 0.5.0 Apr 10, 2019
@jebrosen
Copy link
Collaborator

NB: this needs a minimum rustc version bump, probably the same version as #967.

@jhpratt
Copy link
Contributor Author

jhpratt commented Apr 20, 2019

@jebrosen This could technically be done without the version bump if necessary, as Infallible is nothing more than an enum with zero variants.

It would actually be pretty simple to switch over, since we'd just need to add enum Infallible {} in the root, and use it where necessary. The only difference there would be use crate::Infallible rather than use std::convert::Infallible.

@jebrosen
Copy link
Collaborator

The version bump should be covered by #967 (merged).

I personally think a custom empty enum is the worst option - not in std and a breaking change to go to or from -- I'd rather use Infallible (which will later be an alias for !) or wait until ! is stable.

@SergioBenitez
Copy link
Member

How is this looking? Can we merge after a rebase (@jhpratt) @jebrosen?

@jhpratt
Copy link
Contributor Author

jhpratt commented Jul 9, 2019

@SergioBenitez I can perform a rebase if a merge is desired; it shouldn't be too difficult.

@jebrosen
Copy link
Collaborator

jebrosen commented Jul 9, 2019

LGTM pending rebase.

For the most part, this is simple substitution and nothing else. In one
specific location (core/lib/src/config/mod.rs), the compiler threw an
error, as the `Infallible` type is not a special case in the compiler.
As such, we have to explicitly tell it that code after that point cannot
be reached using `unreachable_unchecked()`.
@jhpratt
Copy link
Contributor Author

jhpratt commented Jul 9, 2019

@jebrosen Rebase is complete — should be able to merge after CI is complete.

Copy link
Collaborator

@jebrosen jebrosen left a comment

Choose a reason for hiding this comment

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

LGTM. I had one point about consistency - some places write out type Error = std::convert::Infallible and some use std::convert::Infallible, even if there's only one use. Similarly, some places use Result<T, Infallible> and some use Result<Self, Self::Error>. That should be easy to do during merge, though.

core/http/src/cookies.rs Outdated Show resolved Hide resolved
@jhpratt
Copy link
Contributor Author

jhpratt commented Jul 9, 2019

wrt consistency, I actually intended on doing another PR to switch to Self/Self::Error wherever possible — it makes future diffs (like this pull) cleaner. I thought I caught all places that used the type directly, I'll look at that quickly.

Edit: A quick rg Infallible on my copy of the repo shows no instances of using std::convert::Infallible directly.

@SergioBenitez
Copy link
Member

Wonderful! This was merged in 34a741a.

@SergioBenitez SergioBenitez added the pr: merged This pull request was merged manually. label Jul 9, 2019
@SergioBenitez
Copy link
Member

I didn't see your early comment; I switched to using std::convert::Infallible everywhere to make switching back to ! a simple replacement.

@jhpratt jhpratt deleted the infallible branch July 10, 2019 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: merged This pull request was merged manually.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants