Remove From<!> for T *reservation* impl

[WaffleLapkin]

View all comments

This PR removes the <T> From<!> for T reservation 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:

impl<T> From<!> for T {
    fn from(never: !) -> T { never }
}

It is meant to prevent users from writing some impls that would overlap if the From<!> for T impl 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 T is not implemented making the following not compile:

struct LocalType;
trait SomeTrait { }
impl<T: From<!>> SomeTrait for T { }
impl SomeTrait for LocalType { }

However, it does not prevent all implementation that would overlap given From<!> for T. Namely, From<!> for T would overlap with the following impls, all of which are currently permitted (and exist):

// T for T identity impl in `core`
impl<T> From<T> for T { ... }

// Various T->wrapper of T impls present in both the standard library,
// and in external crates
impl<T> From<T> for W<T> { ... }

// !->Local is also allowed
impl From<!> for Local {}

Also note that the reservation impl only exists for From<!>, but not for From<Infallible>, so even the impls that the reservation impl is meant to forbid, are currently allowed through Infallible anyway (we are planning to make Infallible a type alias to ! at the same time as stabilizing !).

Motivation for From<!> for T impl

It is surprisingly hard to find the original motivation for From<!> for T impl or the reservation impl, other than "people vaguely think that all types should implement From<!>, 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 ?:

let Ok(owo) = infallible_function();  

Another use-case that I've seen mentioned is "fallible function with a set error, taking an infallible function":

fn try_from<T>(t: T) -> Result<Meow, MyError>
where
    Meow: TryFrom<T>,
    <Meow as TryFrom<T>>::Error: Into<MyError>
{ ... }

With such definition, you can't pass Meow into try_from, because MyError: 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 T impl

Adding From<!> for T seems... 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:

The precise mechanism to permit us to add the From<!> for T impl is not yet clear. The current "plan of record" is to extend the "marker trait mechanism" to accommodate the idea of impls whose entire body consists of unreachable methods and to permit overlap.

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 — only From<!>/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 impl A overlaps with impl B, but impl B doesn't overlap with impl A¹). 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 for From<Infallible> for T, making Infallible an alias for ! makes some code fail to compile. See #155924:

3. standard library contains a reservation impl, which forbids certain From<!> impls. After making Infallible = !, this reservation impl can conflict with existing implementations for Infallible - This breaks 14 crates total (including reverse-dependencies of broken crates)

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 making Infallible = !).

Proposal

After trying to add From<!> for T as 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 making Infallible = !, as described above.

Alternatives

• Add proper impl<T> From<!> for T, accepting the breakage, overlap, and the complexity.
  • I'm not sure how feasible this is, after trying to do this approach, it doesn't feel right
• Keep the reservation impl / add the reservation impl From<Infallible> for T formally accepting the breakage of that, with the hopes that we can still add impl<T> From<!> for T in the future
  • This is the most breaking of the option, as it breaks code that depends on From<Infallible> for T not existing, and expects future breakage when adding impl<T> From<!> for T
  • It is unlikely that adding impl<T> From<!> for T in the future will be much easier than right now

---

Closes #64715
r? types

I'll remove the rustc_reservation_impl attribute in a separate PR (cc #64631).

Footnotes

1. i.e. in the context of impl A a 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 impl B same type might be known to be uninhabited, allowing the overlap. ↩

[lcnr]

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".

rustc_trait_selection::traits::query::type_op::implied_outlives_bounds

It felt fine to me to accept overlap between the blanket From<T> for T impl and the From<!> for T impl, but also having overlap with arbitrary user written impls feels a lot worse. As argued by @WaffleLapkin I don't think there's a sensible language feature which would supersede any hack we do here and that, together with the somewhat minor arguments for adding this impl, leads me to quite strongly prefer removing this reservation impl.

[lcnr]

🤣 guess who just had the wrong thing in their clipboard

@rfcbot fcp merge types

[rust-rfcbot]

@lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

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.

[theemathas]

Nominating for libs-api in case they're aware of any important use cases that require this reservation impl.

[clarfonthey]

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 From<!> for Error and would rather not have to do that, but it's what people are doing for Infallible and if types can't find a way to make that work without breakage, we will deal.

[nikomatsakis]

Let me give some historical context—

The concept of a "reservation impl" was indeed ill-documented and quite frankly confusing. It was never meant to ensure that we could add a From<!> for T impl -- and I would not want that impl to exist.

The purpose of it was different. We were discussing the possibility of stabilizing ! without redefining Infallible = ! and the point was raised that, if we did that, people would like go and add impls like the following

impl From<!> for MyErrorType { }

But those same people already have

impl From<Infallible> for MyErrorType { }

and that would then create overlapping impls.

There was also way that you could depend on this with negative reasoning..? I may have the fine-grained details wrong, but that was the general idea. It was not a general purpose mechanism for "we may add this impl later" but rather something very tailored to ! and Infallible.

So, the idea was to add this rather wacky reservation impl which permitted the stdlib to add From<!> impls and the like but which prevented downstream crates from doing so. That way, when we later redefined Infallible = !, we could remove the reservation impl.

It seems like we are at that point, so I believe removing the reservation impl makes sense -- or at least it does at the point where we redefine Infallible = !.

[nikomatsakis]

(Independently, I think it'd be useful to have a syntax like impl<T> ?SomeTrait for Box<T> which would mean the more straightforward concept of: we are not adding this impl, but we are reserving the right to add it later.)

[jackh726]

On the note: we should not be landing this PR separately. It should be done together with changing Infallible to !.

[traviscross]

(We discussed this in the lang call today for context and to uncover the history and original motivation.)

[clarfonthey]

On the note: we should not be landing this PR separately. It should be done together with changing Infallible to !.

Are we close enough to stabilisation that this isn't going to break anything? Mostly concerned about people being unable to reference Infallible properly if it's just an alias to an unstable type.

[jackh726]

What I mean is: we should just remove the impl in the never type stabilization PR, not that we should changing Infallibe to !` prior to stabilization.

[jackh726]

(I suppose, this actually shouldn't matter to remove ahead of time, since you shouldn't be able to stably write an impl using !; but, I still personally prefer just rolling this into the stabilization.)

[clarfonthey]

Okay, yeah, that makes sense. I have seen a stabilisation PR but wasn't sure if it was past the experimental stage yet.

[WaffleLapkin]

I would prefer to merge this separately, as I don't want to make the stabilization PR any bigger (it's already kinda huge for a stabilization PR!). As you said, there is no danger of people implementing From<!>, since it's unstable right now.

[clarfonthey]

As long as this and the stabilisation PR both make it on the same release cycle, I think that's fair.

[WaffleLapkin]

@clarfonthey I don't see why it matters that it lands in the same release. This only allows writing a From<!> impl, which you can't write on stable anyway (without hacks), so the specific release is irrelevant. The only thing that matters is that we do this before changing infallible.

[clarfonthey]

…right, I'm definitely tired. I guess that if we're close enough to stabilisation that we don't anticipate anyone writing their own From impls that break the Infallible conversion and we need to coordinate fixing before stabilisation, then, it should be fine. The concern was with Infallible becoming unstable to use if done before stabilisation.

[BoxyUwU]

I'm unsure what any of this PR has to do with either types or lang 🤔 I would expect libs should just feel free to remove the reservation impl if they wish 🤔 I feel like im missing something here about what im actually doing by ticking my box 🤷‍♀️

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[clarfonthey]

@BoxyUwU To be clear, libs' decision here is basically that we expect type Infallible = ! in the future and otherwise just see this impl as types trying their best to help ensure that. So, if types thinks that stabilisation is close enough we can just remove this now without making that change a headache in the future, you're free to clean up your code and remove rustc_reservation_impl now.

Like, we're mildly annoyed by the presence of boilerplate From<!> being the standard regardless, but this was never about that, just knowing that From<Infallible> boilerplate exists and there shouldn't be From<!> too that would conflict and prevent making them the same type.

[rustbot]

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.

[JonathanBrouwer]

I verified the implementation matches the FCP

r=me after FCP finishes

I am slightly concerned by the branch name (tho it is very cute :3), I think old bors broke with unicode characters in branch names, I assume this is fixed in new bors? cc @Kobzol

View changes since this review

[Kobzol]

I can't imagine why it would be an issue (so let's hope I won't be surprised).