Rust: Stabilize never_type *again*

[cramertj]

https://github.com/rust-lang/rust/issues/49593 has now been fixed. This was the reason for the previous revert of stabilization (#50121). Previous stabilization report is [here]. Let's try this again!

@rfcbot fcp merge
cc https://github.com/rust-lang/rust/issues/35121

(pnkfelix notes :the previous issue discussing stabilization of ! was #48950; its possible the [here] above was supposed to link to that...)

[cramertj]

@rfcbot fcp merge

c'mon, rfcbot!

[rfcbot]

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

• [x] @Centril
• [x] @Kimundi
• [x] @SimonSapin
• [x] @alexcrichton
• [x] @aturon
• [x] @cramertj
• [x] @dtolnay
• [x] @eddyb
• [x] @joshtriplett
• [x] @nikomatsakis
• [x] @nrc
• [x] @pnkfelix
• [x] @scottmcm
• [x] @sfackler
• [x] @withoutboats

Concerns:

• 49039-needs-decision resolved by https://github.com/rust-lang/rust/issues/57012#issuecomment-450354662
• fallback (https://github.com/rust-lang/rust/issues/57012#issuecomment-449098855)
• from-impl resolved by https://github.com/rust-lang/rust/issues/57012#issuecomment-452183165
• fundamental-and-fn-traits resolved by https://github.com/rust-lang/rust/issues/57012#issuecomment-450026808
• impls resolved by https://github.com/rust-lang/rust/issues/57012#issuecomment-449865955
• indirect-coherence-breakage (https://github.com/rust-lang/rust/issues/57012#issuecomment-460740678)
• more-elaborate-report resolved by https://github.com/rust-lang/rust/issues/57012#issuecomment-452490521

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.

[SimonSapin]

@rfcbot concern impls

The remaining unchecked checkbox in the description of https://github.com/rust-lang/rust/issues/35121 is:

What traits should we implement for !? The initial PR #35162 includes Ord and a few others. This is probably more of a T-libs issue, so I'm adding that tag to the issue.

Stabilizing ! and then (in a) later (release cycle) adding a new impl of existing traits is not a breaking change, is it? If not, this doesn’t need to be a blocker and let’s mark this as resolved immediately.

If it is, let’s discuss it in this issue. Currently in https://doc.rust-lang.org/nightly/std/primitive.never.html we have:

impl Hash for !
impl Copy for !
impl Debug for !
impl Clone for !
impl PartialOrd<!> for !
impl Ord for !
impl Eq for !
impl PartialEq<!> for !
impl Display for !
impl Error for !
impl Termination for !

@rust-lang/libs, anything to add?

[withoutboats]

@SimonSapin it's not a breaking change AFAIK, so we can add them over time.

I'm not sure if we should implement traits for !. Under some coherence related proposals, its possible to write impls guaranteeing that a type will never implement a trait - negative impls, in other words. I don't know the pros and cons of providing positive vs negative impls for ! (which could go either way) if that feature were added to the language.

[SimonSapin]

@rfcbot concern fallback

In addition to reverting the stabilization, https://github.com/rust-lang/rust/pull/50121 also reverted “to the previous fallback semantics (i.e. it is once again dependent on whether the crate has opted into #[feature(never_type)]”.

As far as I understand, this is about type inference of a diverging expression like panic!(…), where there is no constraint from surrounding code.

When it previously landed in Nightly, we found out https://github.com/rust-lang/rust/issues/35121#issuecomment-380790789 that the fallback change could cause some previously-valid code to not compile anymore: https://github.com/rust-lang/rust/issues/48950#issuecomment-380255783, and some (other) previously-valid unsafe code to now have Undefined Behavior https://github.com/rust-lang/rust/issues/48950#issuecomment-380603530 (returning Result<!, _>::Ok in reachable/reached code).

[SimonSapin]

Is this a duplicate of https://github.com/rust-lang/rust/issues/48950?

[cramertj]

@SimonSapin That issue was created during the first round of stabilization to capture the conversation around that issue. This is the new issue for the new round of stabilization.

[Ericson2314]

for<E> E: From<!>

Is nice for error handling, but I recall there are orphan/overlapping issues adding it later. I think this is because that impl is technically an overlapping impl with for<A> A: From<A>, so downstream impls that would overlap with it aren't orpans. But if we somehow hack that into core, then the downstream impls can't exist.

[SimonSapin]

@Ericson2314 I agree that impl<T> From<!> for T would be nice to have. Indeed, if we ever add it, that needs to happen before or in the same release cycle as stabilization of !. And as you said, we can’t "just" add it because it overlaps with impl<T> From<T> for T.

Adding "some hack" to bend the language rules and somehow allow both these impls (picking an arbitrary one to "win" is ok, since they behave the same at the intersection) has been discussed before in the abstract, but there has never been a concrete proposal. Concretely, this would need:

• A plan for how this would work in terms of compiler implementation
• Some buy-in from the language team that bending the language rules in libcore is not a non-starter
• Someone who volunteers to implement it

Without all of these things soon, I suspect that consensus would be to give up on impl<T> From<!> for T and stabilize ! without it.

[Centril]

Noting so I/we remember from this week's T-lang meeting:

@rfcbot concern fundamental-and-fn-traits

We had some discussion around Fn* traits (which are #[fundamental]) and !. (Are there other fundamental traits?) @cramertj and @withoutboats had some ideas that would be nice to hear more in-depth about.

@rfcbot concern more-elaborate-report

It would be nice to have all the behavior changes more clearly specified in one place before we stabilize so it's clear what is being stabilized.

@rfcbot concern from-impl

Noting this as a concern temporarily until conversation has resolved itself.

[SimonSapin]

When it previously landed in Nightly, we found out that the fallback change could […]

I meant to add: in https://github.com/rust-lang/rust/issues/35121#issuecomment-364248648 we discussed only making the fallback change in a new edition. We missed the 2018 train, so this would be for 2021 (or whenever we’ll make the next one).

Also, in the https://github.com/rust-lang/rust/issues/48950#issuecomment-380603530 case, fallback occurred in the expansion of a macro_rules macro. But macros are not edition-hygienic, are they?

[glaebhoerl]

Adding "some hack" to bend the language rules and somehow allow both these impls (picking an arbitrary one to "win" is ok, since they behave the same at the intersection) has been discussed before in the abstract, but there has never been a concrete proposal. Concretely, this would need:

I don't know whether this would actually be easier to implement than the "some hack" itself which would allow the From impls to coexist, but another option which could unblock this question for now, without forever giving up on the right to add the From<!> impl, would be to add a temporary hack which just forbids downstream crates from writing any impls which would conflict with it (as-if the impl did exist), without actually adding the impl itself in core yet.

To try to avoid confusion I'll call the (hypothetical) hack which allows From<!> and From<T> to coexist the "overlap hack", and the hack which merely forbids downstream from conflicting with it the "future-proofing hack".

Even if it's not easier to implement, the future-proofing hack requires fewer commitments -- we don't have to commit to the From<!> impl existing and being stable (IINM, impls are still insta-stable, exacerbating the whole issue), we don't have to commit to the overlap hack existing and continuing to work in the future, we can revert the future-proofing hack at any point if we decide it's not worth it.

[SimonSapin]

But the future-proofing hack blocks a useful use case.

For the same reason that impl<T> From<!> for T is desirable, if it’s not implemented then impl From<!> for Foo might be desirable as the next best thing.

[glaebhoerl]

Yes, which is why it's good that we can choose to revert it at any time.

[cramertj]

An alternative in the future would be to add the impl but ignore coherence completely for any From<!> impl and just allow overlap, since all possible implementations are equivalent (we could potentially do the same for any traits where all functions have uninhabited arguments, and there are no non-function associated items).

[SimonSapin]

I like the idea of tweaking coherence not with a special case for a particular pair of impls but with a rule about allowing overlap when the intersection only has unreachable functions. There is somewhat similar precedent in https://rust-lang.github.io/rfcs/1268-allow-overlapping-impls-on-marker-traits.html, although like in that case we might want to make it opt-in.

[cramertj]

@Centril

concern fundamental-and-fn-traits
We had some discussion around Fn* traits (which are #[fundamental]) and !. (Are there other fundamental traits?) @cramertj and @withoutboats had some ideas that would be nice to hear more in-depth about.

TL;DR: We shouldn't implement the Fn traits for !, because they have an associated type and we'd have to choose what it is. One option would be to choose !, but there's no real logical / fundamental reason for this. Instead, providing this implementation would actually prevent users from implementing custom traits for ! where they also wanted to add a blanket impl over all T: Fn.... This seems like a potentially useful thing to do, whereas using ! as a Fn...<...> -> ! or Fn...<...> -> () seems not particularly useful.

[Ericson2314]

@SimonSapin I really like that idea! We could use the same logic that deduces there is only one possible impl to also allow elliding the trait items:

impl<T> From<!> for T;

[SimonSapin]

https://github.com/rust-lang/rust/issues/57012#issuecomment-449096910

it's not a breaking change AFAIK, so we can add them over time.

@rfcbot resolve impls

---

@cramertj How do you feel about making this issue explicitly not propose any change to type inference fallback from () to !, and leave that change to future discussions? This would resolve my "fallback" concern: https://github.com/rust-lang/rust/issues/57012#issuecomment-449098855

[SimonSapin]

https://github.com/rust-lang/rust/issues/57012#issuecomment-449445621

I like the idea of tweaking coherence not with a special case for a particular pair of impls but with a rule about allowing overlap when the intersection only has unreachable functions.

I believe this would be compatible to add later, after the never type is stabilized. And so this doesn’t need to block this FCP. @Centril, does this resolve your from-impl concern?

[Ericson2314]

@SimonSapin ah so to be clear, if we have Foo: From<!>, and then we add the blanket impl, the Foo impl becomes a harmless overlap (even if it is fully subsumed) by the same analysis.

[SimonSapin]

Exactly, this is why I say this would be compatible to add later.

[Ericson2314]

Good point! Glad to see this hit stable!

[cramertj]

@Centril

concern more-elaborate-report

It would be nice to have all the behavior changes more clearly specified in one place before we stabilize so it's clear what is being stabilized.

That was done here, and is linked above-- sorry if it was hard to find.

[SimonSapin]

This includes “Type inference will now default unconstrained type variables to ! instead of ()”, which as already mentioned in this thread I think we should not do without some mitigation, including at least only changing the default in a future language edition.

[cramertj]

@SimonSapin Yeah, it does include it-- my intention in this issue was to include that. I think that this behavior change is correct and that it doesn't make sense to do as part of an edition change-- having different type fallback behavior across editions seems really complicated and potentially perplexing to end-users (e.g. why does my code silently have different runtime behavior when I change this Span from call_site to def_site). If we're ever going to make this change (and I think we should), then I think we should do it now and for all editions.

[Centril]

TL;DR: We shouldn't implement the Fn traits for !, because they have an associated type and we'd have to choose what it is. One option would be to choose !, but there's no real logical / fundamental reason for this. Instead, providing this implementation would actually prevent users from implementing custom traits for ! where they also wanted to add a blanket impl over all T: Fn.... This seems like a potentially useful thing to do, whereas using ! as a Fn...<...> -> ! or Fn...<...> -> () seems not particularly useful.

@rfcbot resolve fundamental-and-fn-traits

[SimonSapin]

If we're ever going to make this change (and I think we should), then I think we should do it now and for all editions.

Ok, then we are left with my concern from https://github.com/rust-lang/rust/issues/57012#issuecomment-449098855. What do we do about code that used to compile but doesn’t anymore with this change? About unsafe code that used to be sound but has UB with this change?

I spent days in that particular debugging rabbit hole.

[Centril]

Do we want to reopen https://github.com/rust-lang/rust/pull/49039? cc @scottmcm

@rfcbot concern 49039-needs-decision

---

@SimonSapin

I believe this would be compatible to add later, after the never type is stabilized. And so this doesn’t need to block this FCP. @Centril, does this resolve your from-impl concern?

Our rules around coherence are not simple, so extending them needs careful thought. For now, in the absence of stable specialization, I think a better solution would be akin to what @glaebhoerl suggested and which @scottjmaddox has written about. I think this would also be useful on other occasions as well, so we should imo add some #[rustc_reserved] impl<T> From<!> for T {}.

If we're ever going to make this change (and I think we should), then I think we should do it now and for all editions.

I agree with @cramertj; Having tyvars fallback to () does seem like a singularly poor choice as @nikomatsakis put it.

Ok, then we are left with my concern from #57012 (comment).

What do we do about code that used to compile but doesn’t anymore with this change?

It's unfortunate, but I think we should fix what I can only call a bug in the type system. It's also explicitly noted in RFC 1122 that changes to type inference and fallback are considered acceptable _(tho certainly we should be mindful of impact)_. The language team already decided once that it was OK with the change and I see no reason to change our minds.

About unsafe code that used to be sound but has UB with this change?

I spent _days_ in that particular debugging rabbit hole.

For the UB, it seems to me that we could have a correctness lint akin to https://github.com/rust-lang/rust/issues/39216 in the compiler or in clippy to catch these. Maybe we could limit the lint to unsafe { .. } blocks?

---

@cramertj

That was done here, and is linked above-- sorry if it was hard to find.

Thanks for the link; I did read that one but didn't find it satisfactory. Is that summary issue fully up to date? After reading the entirely of the RFC, tracking issue, and summary issue, ideally I'd still like a report in this issue that includes:

• Everything in https://github.com/rust-lang/rust/issues/48950.
• Divergences from the RFC and why.
• Locations of all relevant test files (run-pass, compile-pass, compile-fail, ui...). In particular, test files for fallback seems important.
• A more exactly specified behaviour of what unification variables default to !.
  For example, None doesn't fallback to Option<!>. A rationale for this would be helpful.
  Also, after stabilizing, are there any unification variables falling back to ()?
  cc @nikomatsakis

[SimonSapin]

concern 49039-needs-decision

The decision to do this was already made, only its execution was delayed. We need to make sure to land it in the same release cycle as stabilization of ! (I’m not sure where to track that exactly) but that happens after FCP, so it shouldn’t be a concern that blocks FCP from starting.

Maybe we could limit the lint to unsafe { .. } blocks?

In the https://github.com/rust-lang/rust/issues/48950#issuecomment-380603530 case, the fallback occurred outside of any unsafe block. (The inferred type that changed from () to ! was used as the return type of a generic FFI call: https://docs.rs/objc/0.2.5/objc/macro.msg_send.html. The crate’s documentation suggests using let _: () = msg_send!(…); to force the return type, but many users didn’t.)

[Centril]

The decision to do this was already made, only its execution was delayed. We need to make sure to land it in the same release cycle as stabilization of ! (I’m not sure where to track that exactly) but that happens after FCP, so it shouldn’t be a concern that blocks FCP from starting.

@SimonSapin ah; alright, just wanted to make sure we were aware / it didn't slip. :)
@rfcbot resolve 49039-needs-decision

In the #48950 (comment) case, the fallback occurred outside of any unsafe block. (The inferred type that changed from () to ! was used as the return type of a generic FFI call: https://docs.rs/objc/0.2.5/objc/macro.msg_send.html. The crate’s documentation suggests using let _: () = msg_send!(…); to force the return type, but many users didn’t.)

Alright, seems like we should lint outside of unsafe blocks as well then.

[cramertj]

@Centril

concern from-impl

Context: this is in reference to the fact that we cannot currently add impl<T> From<!> for T { ... } due to overlap with impl<T> From<T> for T { ... }, which will eventually (very likely) be resolved through specialization. Landing now without this would allow users to write impl From<!> for MyType { ... } which would overlap with a future addition of the above blanket impl.

Above we've outlined two broad solutions to this problem:

• Specifically ban all implementations of From<..> for ! with the compiler until we can add the blanket impl. This has the very unfortunate side-effect that users would be unable to implement From<!> for MyType anywhere, which is a very useful impl to have for e.g. error type conversion and the like. This would make ! much less useful than a custom enum Never {} with the appropriate From impls, so I don't think we should do this if we can avoid it.

• Allow a future implementation of impl<T> From<!> for T to not conflict with the existing impls. Since the From trait has only one associated item (a function) and it is un-callable because its argument is uninhabited, all implementations of From<!> are equivalent. A variant of this would be to tweak coherence in this way not just for From<!>, but for all trait impls that only provide unreachable functions (as determined by uninhabited argument types). This would be possible to add later, requires no additional future-proofing, and has additional use-cases beyond those strictly scoped to this feature.

I believe that we should go ahead and stabilize the feature with no additional future-proofing and with the second option as plan-of-record.

[cramertj]

@SimonSapin

concern fallback

As you noted in https://github.com/rust-lang/rust/issues/57012#issuecomment-449098855, this feature tracks not only the stabilization of the ! type, but also the inference change to make diverging expressions such as panic!, return, break, etc. fall back to the ! type.

When this change happened previously, it unfortunately caused some previously-working code to stop compiling, and caused some unsafe code to become undefined behavior. Before I address why I think we should go ahead and do this, there's some justification required as to whether this is allowable under our backwards-compatibility rules or not. I believe that this is covered under the existing rule for "under-specified language semantics" in RFC 1122, specifically the part that says "details of type inference may change"-- it even specifically cites another type of inference fallback (integer fallback) as an area in which we may change the behavior in the future. I wasn't able to find any documentation stating that these type variables should fall back to !, so this seems like the same old allowable but unfortunate inference breakage. Some cases of this issue have been linted against for a while under the resolve_trait_on_defaulted_unit warning (see https://github.com/rust-lang/rust/issues/39216 for more info).

As for why we should make this change, the RFC lists a couple of motivating factors, the most relevant of which is (IMO) better dead-code detection, such as in cases like this:

let t = std::thread::spawn(|| panic!("nope"));
t.join().unwrap();
println!("hello");

Under this RFC: the closure body gets typed ! instead of (), the unwrap() gets typed !, and the println! will raise a dead code warning. There's no way current rust can detect cases like that.

This change makes it possible to catch more bugs via dead-code warnings, expand the reachability analysis of the compiler allowing for smarter optimizations, and helps to provide "more correct" types like Result<(), !> rather than Result<(), ()> in more places (which may turn previously manual user assertions into no-ops). It also helps introduce the user to the concept of ! by giving a "more correct" return type to diverging expressions.

[Centril]

Taking @cramertj's notes in https://github.com/rust-lang/rust/issues/57012#issuecomment-452150775 into account I think I've come to the realization that the future proofing hack doesn't buy much. Either impl<T> From<!> for T will become available to us via specialization and the proofing doesn't affect that, or we'll use the coherence "hack". A third solution doesn't seem likely and as such:

@rfcbot resolve from-impl

Still, having a #[rustc_reserve] mechanism may prove useful in other circumstances but it need not block never_type's stabilization.

[glaebhoerl]

(I agree that since the actual coherence hack turns out to be one which can be done backwards compatibly, there isn't much motivation for future-proofing.)

[SimonSapin]

@cramertj I’m not disputing that the inference fallback change is desirable or that it’s allowed by the our documented stability rules. This concern is about: how do we deal with the real negative impact on the ecosystem? Last time around, we pretty much didn’t. One option that I proposed above is to only make the change effective in a future language edition. This seems to be precisely what editions are for, but maybe we could manage in some other way. (I also proposed separating this discussion from stabilization of the never type which would unblock other features, but there doesn’t seem to be interest in that.)

Are you proposing that we make the change in Rust 1.33 (or some other version) and don’t do anything else to help the ecosystem through the transition? Why is that more desirable than, say, gating on an edition?

[cramertj]

@SimonSapin Ah, thank you for clarifying your question!

Are you proposing that we make the change in Rust 1.33 (or some other version) and don’t do anything else to help the ecosystem through the transition?

We already went to some effort to help the transition go more smoothly by introducing the resolve_trait_on_defaulted_unit warning (see #39216 for more info). I'm not proposing any additional mechanisms to help the ecosystem transition, although perhaps a crater run with that lint set to deny could help? If that's something you're interested in, I'd be happy to put up a PR setting the lint to deny so we can try it out if it hasn't been done already. Still, I'd prefer not to block this issue on the completion of that experiment, since it's just a best-effort attempt to help people correct their code. (Edit: there was already a crater run done when the lint was set to deny, which is already its current default, and has been since July 2017-- see https://github.com/rust-lang/rust/pull/42894).

Why is that more desirable than, say, gating on an edition?

I mentioned this a bit above, but I don't believe that we should use editions to modify inference behavior that is quite this subtle. I'm not even sure how we would do it-- in this world, type-checking would change how it worked depending on the edition of the particular Span it was adjusting. This seems likely to result in subtle and hard-to-debug issues, compared to introducing a consistent behavior everywhere. It's unfortunate that it's inconsistent across time, but I don't think we'd gain much by making this behavior edition-aware, and I think doing so could both make the compiler's logic more complex and frustrate users' ability to reason about the semantic behavior of code with imprecise Spans. There's also the fact that the next edition won't be released for several years, and I'd like to get rustaceans used to the new behavior as soon as possible, which will help introduce them to the ! type and prevent more soon-to-be-broken code from being written.

[cramertj]

@Centril

I'd still like a report in this issue that includes:
Everything in #48950.
Divergences from the RFC and why.
Locations of all relevant test files (run-pass, compile-pass, compile-fail, ui...). In particular, test files for fallback seems important.
A more exactly specified behaviour of what unification variables default to !.
For example, None doesn't fallback to Option. A rationale for this would be helpful.
Also, after stabilizing, are there any unification variables falling back to ()?
cc @nikomatsakis

What is being stabilized

• ! is now a full-fledged type and can now be used in any type position (eg. RFC 1216). The ! type can coerce into any other type, see https://github.com/rust-lang/rust/tree/master/src/test/run-fail/adjust_never.rs for an example. Tests for this behavior can be found in the following locations:

  • ! as an associated type
  • ! to any type coercion
  • ! to any type explicit cast
  • calling a function with a ! argument
  • !-containing enum variants don't take up a descriminant "slot"
  • ! as an argument to a trait impl
  • calling methods on !
  • !-type as a value expression
  • !-type can be matched out of an enum and then coerced appropriately

• Type inference will now default unconstrained type variables to ! instead of (). As discussed above, this is an inference-breaking change that has been warned against for years now (see https://github.com/rust-lang/rust/issues/39216 for more info). The precise behavior of when fallback occurs today isn't clearly specified either in this RFC nor in any previous one I could find, however the general blanket-scenario for when fallback occurs is when an unconstrained type variable is being unified with a diverging expression such as a panic, break, return, continue, etc. The choice of when to apply this fallback is not being affected by this RFC, and fallback is not added to any new positions (such as unconstrained type parameters not tied to a diverging expression). The only change to fallback is to that the default is now ! instead of (). This can be seen in the following test:

  • diverging-fallback-ccontrol-flow.rs (note: @nikomatsakis has a comment in this test saying "These represent current behavior, but are pretty dubious. I would like to revisit these and potentially change them." This is probably worth discussing, but I believe that any changes to when we perform fallback are out-of-scope for the feature we're discussing here, except for insofar as they could affect the same areas of code and might lead to an unconstrained-type-parameter compile-failure rather than e.g. undefined behavior as a result of unsafe code falling back to !.

What is not being stabilized

Exhaustive pattern-matching for uninhabited types. eg.

let x: Result<u32, !> = ...;
let Ok(y) = x;

This code will still complain thatOk(_)is a refutable pattern. This can be fixed by using the exhaustive_patterns feature gate. See RFC 1872 for progress on this issue. See https://github.com/rust-lang/rust/tree/master/src/test/ui/feature-gate-exhaustive-patterns.rs for the testcase which confirms that this behaviour is still gated.

Divergence from the RFC

I reread the RFC and didn't find any significant divergence, although the end of the detailed design section says this:

Add an implementation for ! of any trait that it can trivially implement. Add methods to Result and Result for safely extracting the inner value. Name these methods along the lines of unwrap_nopanic, safe_unwrap or something.

We have added some of these trait impls and methods, but this stabilization doesn't include any such methods, and the set of trait impls provided for ! is relatively small. This set can be expanded in the future backwards-compatibly.

Unresolved Questions

The RFC included one unresolved question as to whether we should automate creation of traits whose only items are non-static methods. This isn't something that we're doing today, but is something I'd personally be interested in seeing in the future, definitely excluding unsafe traits and perhaps "marker" traits. In any case, this isn't currently RFC'd or implemented and isn't being considered in this round of stabilization.

[SimonSapin]

@cramertj Thanks for the explanation, this makes sense.

perhaps a crater run with that lint set to deny could help? If that's something you're interested in, I'd be happy to put up a PR setting the lint to deny so we can try it out if it hasn't been done already.

Yes please. Can we make this lint cause errors in dependencies despite Cargo passing --cap-lints=allow?

Also, because most Rust code in the world is not tested (or even reachable) by Crater, it would be nice to provide instructions for projects to run a similar experiment on their own dependency graph. Ideally with an unmodified Nightly build. Would a RUSTFLAGS="-Dresolve_trait_on_defaulted_unit" environment variable override Cargo’s --cap-lints=allow?

[cramertj]

@SimonSapin https://github.com/rust-lang/rust/pull/42894 already made the lint deny-by-default, but https://github.com/rust-lang/rust/issues/39216 suggests the possible next step of making it a hard error. However, a crater run was already done in https://github.com/rust-lang/rust/pull/42894 and there was only one regression, which was fixed in https://github.com/embali/rjq/pull/1. I doubt that another crater run would turn up anything new since this has been a deny-by-default lint since July of 2017.

[Centril]

@rfcbot resolve more-elaborate-report
@rfcbot reviewed

[nikomatsakis]

Do we know if the lint indeed would capture the cases that @SimonSapin pointed out as having occurred in the past? (have those cases been fixed in the meantime...?)

[cramertj]

@nikomatsakis The issue that broke servo was fixed in https://github.com/tomaka/winit/pull/428. I don't know whether the current lint would have triggered on that code or not (though my impression was that that was the point of the lint?).

[SimonSapin]

The instances that we know of have been fixed. The problem with having this lint being a lint is that Cargo silences it in dependencies. For most warnings like "this API is deprecated, you should use that one instead" this makes sense: downstream users can’t easily fix them, and keeping them unfixed has basically no negative consequences. This is not the same.

When we are (potentially) silently introducing UB in previously-valid programs, mitigations should be in a form that is not disabled in dependencies. Servo currently has 462 [[package]] entries in its Cargo.lock, testing them manually one by one is not feasible.

[cramertj]

@SimonSapin If they're on crates.io, then they've already been tested by the crater run.

[SimonSapin]

That is plainly not true.

Let’s take a recent run for example: https://crater-reports.s3.amazonaws.com/beta-1.32-2/index.html. Out of 50551 crates tested, 6446 (12%) are in "error" state: both "before" and "after" builds failed, and so Crater cannot deduce anything about them.

There could be a number of reasons for this. For example, https://github.com/tomaka/winit/pull/428 that we discussed above was related to macOS-specific usage of the objc crate, but Crater only runs Linux. Some other crates might rely on native dependencies that happen not to be available in Crater’s environment.

Crater is extremely valuable but it can never cover everything, for any definition of "everything". Even if we consider what it does cover a sample, this sampling is not at all random and is full of selection bias. I wish we wouldn’t rely so much on Crater results to make decisions.

[Centril]

@cramertj So for the purposes of making progress and reaching consensus, even if you think a new crater run wouldn't be beneficial, let's please do a new one anyways along the lines @SimonSapin suggested in https://github.com/rust-lang/rust/issues/57012#issuecomment-452441160.

[cramertj]

@SimonSapin The crater run on them happened in https://github.com/rust-lang/rust/pull/42894, when the warning was moved to deny-by-default. This would have tested direct compilation of all crates on crates.io, so cap-lints is irrelevant here.

[SimonSapin]

In the diff for #42894, RESOLVE_TRAIT_ON_DEFAULTED_UNIT looks most relevant. When running RUSTFLAGS="-Dresolve_trait_on_defaulted_unit" cargo build in a project with multiple dependencies, I get some output for each local (path dependency) crate but not crates.io dependencies.

This suggests that --cap-lints=allows as passed by Cargo to crates.io dependencies "wins" over -D passed through RUSTFLAGS. This is unfortunate, for this scenario. Is there some other mechanism for denying a given lint in a whole dependency graph?

However in this particular case the output is:

warning: lint `resolve_trait_on_defaulted_unit` has been removed: `converted into hard error, see https://github.com/rust-lang/rust/issues/48950`
  |
  = note: requested on the command line with `-D resolve_trait_on_defaulted_unit`

@cramertj Does this message mean that any code that would be impacted by the fallback change would cause a hard error (not a lint affected by --cap-lints), and have for several release cycles now? This would completely resolve my concern, if the change (this time around) doesn’t change meaning of previously-accepted programs but only makes more programs accepted by rustc.

[cramertj]

@SimonSapin Yup, looking at it now it looks like the lint was already removed.

[SimonSapin]

The lint was apparently removed but not turned into a hard error as the message in https://github.com/rust-lang/rust/issues/57012#issuecomment-455848963 suggests.

In Nightly 2019-01-21 7164a9f151a56316a382 the code below compiles without a warning or error. So to deal with the inference change, it looks like the best we can advise maintainers is hope they get segfaults rather than CVEs.

#![feature(core_intrinsics)]

fn foo<R>() -> Result<R, String> {
    unsafe {
        println!("{}", std::intrinsics::type_name::<R>());
        Ok(std::mem::uninitialized())
    }
}

fn main() {
    let _ = match foo() {
        Err(e) => panic!("{}", e),
        Ok(x) => x,
    };
}

(Playground)

Output:

()

[SimonSapin]

Given that we do want to make this change and that only applying in a new edition is not practical, I think the next-best thing to help the transition would be to have:

• In some Nightly version (easy to obtain with rustup)
• A -Z flag (can be used in Cargo through a RUSTFLAGS environment variable without being inhibited in dependencies by --cap-lints=allow)
• That causes a warning to be printed for code (like in the previous comment) that would be affected by the change.
• Some publicity in at least the Announcement categories of both users.rust-lang.org and internals.rust-lang.org (and possibly a post on blog.rust-lang.org) with explanations and instructions along the lines of RUSTFLAGS="-Zwarn-on-inference-fallback" rustup run --install nightly-2019-XX-YY cargo check

… before landing the inference fallback change in Nightly.

---

Also, even though the inference fallback change is desirable, as far as I can tell it is not actually required to stabilize the never type. The latter would unblock TryFrom, which would be nice.

[arielb1]

BTW, mem::uninitialized::<!>() causes a panic today, not an unreachable, so these cases would turn to a panic rather than "UB".

[arielb1]

Two things:

1.

According to @SimonSapin, there's a popular-on-mac objc crate that does the equivalent of:

extern "C" {
    fn foo<T>(...) -> T;
}

In that case, calling foo with T = ! is completely "well-defined" to mean foo having a _Noreturn attribute, which is not what we want! We need some way to deal with it (e.g. we already have a "defaulted unit" flag on unit tuples, we can make it a future-compat lint to call an FFI function with a return type that is a defaulted unit/never).

2.

If we want to future-proof adding an "incoherent" impl<T> From<!> for T, we need to prevent indirect coherence abuse, which can't be done in the surface language but could theoretically be done in:

#![feature(never_type)]

struct Foo;
trait Xyz { type T; }
impl Xyz for Foo { type T = (); }
impl<T> Xyz for T where T: From<!> { type T = u32; }

[arielb1]

@rfcbot concern indirect-coherence-breakage

If we stabilize ! without implementing From<!> for T, it will be possible for people to rely on From<!> for MyLocalType it not being implemented:

# #![feature(never_type)]

struct Foo;
trait Xyz { type T; }
impl Xyz for Foo { type T = (); }
impl<T> Xyz for T where T: From<!> { type T = u32; }

Therefore, if we want to keep the ability to implement From<!> for T, we need to prevent people from relying on that sort of "indirect coherence". I see two feasible ways to do that:

1. Make a "compiler magic" impl<T> From<!> for T, using "magic" to bypass the coherence problems. This should not have any more complications than the existing overlapping_marker_trait, but has the annoying problem of being a non-conservative extension to the language.
2. Make a "negative-reasoning only" impl for impl<T> From<!> for T. This also requires some sort of "compiler magic", but fairly easy one. This impl wouldn't be usable "normally" (even through specialization), but it will make the above set of impls cause a coherence error.
   In that case, users will still be able to implement From<!> for MyType for their own types, keeping the usefulness, and we'll eventually be able to add a for<T> From<!> for T impl.

Actually, in this option, the standard way of implementing lattice specialization would have some problems with this sort of user impl:
```Rust
trait Foo {}
impl Foo for ! {}
impl Foo for OtherType {}
impl<T: Foo> From<T> for MyType { /* .. */ }
```
This occurs because we can't have the "lattice" impl be in libcore. So we could prohibit these sorts of impls as well (allowing only `impl From<!> for MyType<...>`), requiring all "conflicting" impls other than the "diagnoal" impl to be specializations of "From<!> for T".

[eddyb]

@rfcbot concern indirect-coherence-breakage

---

Originally raised by @arielb1 in https://github.com/rust-lang/rust/issues/57012#issuecomment-460738555 (i.e. I'm doing manual @rfcbot delegation, let me know if there's a better way to do this)

---

If we stabilize ! without implementing From<!> for T, it will be possible for people to rely on From<!> for MyLocalType it not being implemented:

# #![feature(never_type)]

struct Foo;
trait Xyz { type T; }
impl Xyz for Foo { type T = (); }
impl<T> Xyz for T where T: From<!> { type T = u32; }

Therefore, if we want to keep the ability to implement From<!> for T, we need to prevent people from relying on that sort of "indirect coherence". I see two feasible ways to do that:

1. Make a "compiler magic" impl<T> From<!> for T, using "magic" to bypass the coherence problems. This should not have any more complications than the existing overlapping_marker_trait, but has the annoying problem of being a non-conservative extension to the language.
2. Make a "negative-reasoning only" impl for impl<T> From<!> for T. This also requires some sort of "compiler magic", but fairly easy one. This impl wouldn't be usable "normally" (even through specialization), but it will make the above set of impls cause a coherence error.
   In that case, users will still be able to implement From<!> for MyType for their own types, keeping the usefulness, and we'll eventually be able to add a for<T> From<!> for T impl.

Actually, in this option, the standard way of implementing lattice specialization would have some problems with this sort of user impl:
```Rust
trait Foo {}
impl Foo for ! {}
impl Foo for OtherType {}
impl<T: Foo> From<T> for MyType { /* .. */ }
```
This occurs because we can't have the "lattice" impl be in libcore. So we could prohibit these sorts of impls as well (allowing only `impl From<!> for MyType<...>`), requiring all "conflicting" impls other than the "diagnoal" impl to be specializations of "From<!> for T".

[arielb1]

The situation with the objc crate is worse than we thought. It performs the following thing:

mod message {
pub unsafe fn send_message<T, A, R>(obj: *const T, sel: Sel, args: A)
        -> Result<R, MessageError>
        where T: Message, A: MessageArguments, R: Any {
    send_unverified(obj, sel, args)
}
}
pub use message::send_message as __send_message;
...
    ($obj:expr, $name:ident) => ({
        let sel = sel!($name);
        match $crate::__send_message(&*$obj, sel, ()) {
            Err(s) => panic!("{}", s),
            Ok(r) => r,
        }
    });

This means it's creating the equivalent of a Result<!, MessageError>, which looks 100% innocent - as long as you are not actually returning an Ok! So I can't think of a way to lint against it.

[arielb1]

This is basically isomorphic to try!(Err(MyError)), which is definitely an OK example.

OTOH, I noticed that the unreachable code warning isn't kicking in in this case. Maybe we should fix it.

[glaebhoerl]

Cross-referencing: https://github.com/rust-lang/rust/issues/58184

[SimonSapin]

Apparently there’s bug/hole in having the never type behind a feature gate, and it’s possible to refer to it on Stable: https://github.com/rust-lang/rust/issues/33417#issuecomment-467053097 Oops that was meant for https://github.com/rust-lang/rust/issues/35121#issuecomment-467070801, please respond there.

[SimonSapin]

Since discussion seems to have stalled, I’ll try to summarize the remaining concerns.

• This FCP proposes two things: stabilizing the never type, and changing type inference fallback. In some rare cases, the latter can change the meaning of previously-valid stable unsafe code and make it unsound: https://github.com/rust-lang/rust/issues/57012#issuecomment-449098855. (Disclosure, I filed this one.) Possible resolutions include:

  • Tweak the proposed inference rules so that they don’t change the meaning of existing code in cases where that would be unsound. It’s not clear if this is possible.
  • Design and implement mitigations that will help project maintainers find out if their code or their dependencies’ code is affected. (Crater is not sufficient since it only tests open-source code that it can find, only on Linux, and only with a fixed set of system packages installed, and only runs unit tests at most.) It’s not clear yet what these mitigations should do exactly.
  • Declare this breaking change to be “minor”, hope that it causes more benign crashes than exploitable vulnerabilities, and let project maintainers deal with it however they can. I personally think this is not acceptable.
  • Stabilize the never type separately (https://github.com/rust-lang/rust/issues/58184) from the inference change, since it is not actually affected by this concern, and keep discussing tweaks or mitigations in another thread.

• It would be nice to have impl<T> From<!> for T to reflect at the trait level that a “value” of the never type can be trivially converted to any type (since that code is unreachable). However trait coherence does not let us write that impl since it overlaps with the stable identity conversion impl<T> From<T> for T, for T = !. And adding it after the never type has already been stable would be a breaking change: https://github.com/rust-lang/rust/issues/57012#issuecomment-460740678. Possible resolutions include:

  • Lattice impl specialization. (However even “basic” specialization has big remaining design questions at this point.)
  • Extend RFC 1268 “Allow overlapping implementations for marker traits” to also include traits with only methods that take at least one parameter whose type is uninhabited.
  • Hack in a narrower special case of one of the above: have a permanently-unstable language rule just for this one standard library impl
  • Give up on this impl. It would be nice to have, but is it really worth blocking the never type from stabilization for so long?

[nikomatsakis]

Nominating for discussion in lang-team meeting to try and get "unstuck". We discussed a bit today.

I think we generally agreed that, with respect to the second question at least, an appealing option would be to extend coherence so that it was treated "as if" such an impl existed, without actually having the impl. That would require some careful coding, though, from what I can tell.

It would certainly be easier to just allow this impl to "opt out" from coherence rules -- I wonder what it would take to convince ourselves that specialization will allow the impl. (It seems very likely to me that it would, but I've not given deep thought to it.)

Regarding the fallback, there is definitely a "core disagreement" here between (a) missing an opportunity to have the "right" fallback and (b) affecting existing code. I am curious about the "mitigations" option -- @SimonSapin are you thinking of something like a lint?

[Centril]

We did not have any time to discuss the issue this week on the language team meeting; rescheduling for next week instead.

[Centril]

Postponing until a future meeting.

[dnrusakov]

@Centril Are there any news on never type stabilization? What is the current status?

[Centril]

@dnrusakov Still the same as before; see https://github.com/rust-lang/rust/issues/57012#issuecomment-460740678 and https://github.com/rust-lang/rust/issues/57012#issuecomment-467889706. I think the main blocker now is the future proofing some sort of perma-unstable reservation hack. I'm not familiar with those parts of the compiler to do that, but maybe @eddyb or @arielb1 can.

[agausmann]

How useful / necessary is the trait impl? Conversion from ! to any type T can be trivially done using the empty match statement. I wouldn't mind writing something like result.map_err(|never| match never {})? until this is resolved.

[agausmann]

I re-read everything and I get it now - avoiding problems from impl<!> for MyType. :+1:

[crlf0710]

Now that #62661 is merged, what's next?
@eddyb i believe indirect-coherence-breakage concern can be resolved now?

what can we do on the fallback part?

[Centril]

Canceling FCP here in favor of the one in https://github.com/rust-lang/rust/pull/65355.

@rfcbot cancel

[rfcbot]

@Centril proposal cancelled.