Remove From<!> for T *reservation* impl - #160705
Hidden character warning
Conversation
This comment has been minimized.
This comment has been minimized.
b367736 to
7c94b9a
Compare
|
cc @rust-lang/lang @rust-lang/libs-api Given that this is largely a "the type system complexity of adding this impl significantly outweights its potential benefits".
It felt fine to me to accept overlap between the blanket |
|
🤣 guess who just had the wrong thing in their clipboard @rfcbot fcp merge types |
|
@lcnr has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
Nominating for libs-api in case they're aware of any important use cases that require this reservation impl. |
7c94b9a to
27b7dcc
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
In our libs meeting we agree that it's fine to remove. We definitely lament the fact that error types have to essentially add their own boilerplate |
This PR removes the
<T> From<!> for Treservation implementation added in #62661 and tracked in #64715 and #64631.The reservation impl in question was added in order to reserve some space for adding the following impl:
It is meant to prevent users from writing some impls that would overlap if the
From<!> for Timpl is to be added.This requires T-types FCP. Below is my proposal and necessary context:
The reservation impl is not sufficient
The reservation impl prevents one from assuming that
From<!> for Tis not implemented making the following not compile:However, it does not prevent all implementation that would overlap given
From<!> for T. Namely,From<!> for Twould overlap with the following impls, all of which are currently permitted (and exist):Also note that the reservation impl only exists for
From<!>, but not forFrom<Infallible>, so even the impls that the reservation impl is meant to forbid, are currently allowed throughInfallibleanyway (we are planning to makeInfalliblea type alias to!at the same time as stabilizing!).Motivation for
From<!> for TimplIt is surprisingly hard to find the original motivation for
From<!> for Timpl or the reservation impl, other than "people vaguely think that all types should implementFrom<!>, since there is never-to-any coercion".One use-case seems to be "calling infallible function in a fallible one, and unwrapping
Result<_, !>with?". However, nowdays it is trivial to unwrap the result safely without?:Another use-case that I've seen mentioned is "fallible function with a set error, taking an infallible function":
With such definition, you can't pass
Meowintotry_from, becauseMyError: From<Infallible>doesn't hold.This is more unfortunate, but it's not clear how widespread this problem is and how bad the workarounds would be. If a function expects
impl FnOnce(...) -> Result<...>, it should be trivial to coerce the!error to an appropriate type. With other trait bounds (like in the example above) it could be solved by adding a custom impl for your specific error type (annoying, but workable). Certaintly this doesn't feel like a big roadblock to me.(let me know if you know more prior art on this)
There is no clear path for adding
From<!> for TimplAdding
From<!> for Tseems... hard... and hard to argue for.It would require ignoring overlap with a bunch of impls (as described above) and would also require low priority impls (to avoid inference failures in cases where previously the only applicable impl was the identity one, so adding
From<!>makes "one impl rule" not apply).The tracking issue says:
Considering "traits with all methods having arguments of uninhabited types" as marker traits is technically possible (I think?), but feels like a bit of a stretch. Making overlap check consider if all trait functions take arguments which are uninhabited (known to be uninhabited in the current context) seems like a big complication, especially considering how
From<T>would not be a marker trait in the general case — onlyFrom<!>/From<OtherUninhabitedTypes>would be (also that requires attaching the overlap check to some context from which we can check if a type is publically uninhabited, which can also lead to situations where implAoverlaps with implB, but implBdoesn't overlap with implA1). Allowing overlap with arbitrary user impls also is likely to cause unforcene issues in my opinion.The reservation impl causes problems for the never type stabilization
Because the reservation impl reserves space for
From<!> for T, but not forFrom<Infallible> for T, makingInfalliblean alias for!makes some code fail to compile. See #155924:Given that both keeping the reservation impl (while making
Infallible = !) and making the reservation a proper impl break code, we should decide which path we want to pursue before stabilizing the never type (and makingInfallible = !).Proposal
After trying to add
From<!> for Tas a proper impl, I'm not convinced that it's worth the complexity and messiness of allowing such widespread overlap (with user defined impls too!). As such, I propose to remove the reservation impl, to prevent unnecessary breakage from its combination with makingInfallible = !, as described above.Alternatives
impl<T> From<!> for T, accepting the breakage, overlap, and the complexity.From<Infallible> for Tformally accepting the breakage of that, with the hopes that we can still addimpl<T> From<!> for Tin the futureFrom<Infallible> for Tnot existing, and expects future breakage when addingimpl<T> From<!> for Timpl<T> From<!> for Tin the future will be much easier than right nowCloses #64715
r? types
I'll remove the
rustc_reservation_implattribute in a separate PR (cc #64631).Footnotes
i.e. in the context of impl
Aa certain type is not known to be uninhabited, and thus the overlap between impls should not be allowed. at the same time in the context of implBsame type might be known to be uninhabited, allowing the overlap. ↩