Replace uninhabited error enums in std with never

[scottmcm]

Luckily I only found two, and one of them isn't in beta yet, so can disappear completely 😎

Are there any others I forgot? (There are lots in things like liblibc and libstd/sys, but AFAIK those don't matter, nor do things like btree::node::LeafOrInternal or str::pattern::RejectAndMatch.)

The unstable convert::Infallible is being handled by #49038

⚠️ This change may be a 1.26-or-never one.

cc #48950 (comment)
r? @alexcrichton

[alexcrichton]

@bors: try

@rust-lang/infra, mind doing a crater run on this?

[bors]

⌛ Trying commit 6c7ec67 with merge 26137fe...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[kennytm]

I've inserted it above the two other pending PRs due to the claimed urgency. Still, we'll likely need to wait 9 to 10 days (March 25th) to get a crater result (5 days for the current two to finish, and 5 more days for running this), which is dangerously close to the beta promotion date (March 28th).

If this is really urgent we could postpone #48814 (that started just 4 hours ago) and run this immediately. (cc @aidanhs)

But since you are changing the stable type string::ParseError already, I don't see how this is "⚠️ 1.26-or-never" even if ParsePathError got stabilized first.

[scottmcm]

@kennytm The thing that I think is ⚠️ is that if done in 1.26 this can't cause overlapping impls, but later than that it can, because I expect impl From<!> for MyError and impl From<ParseError> for MyError to both be somewhat common as part of using ?. That can't happen right now, but if this change happens after ! stabilization, that's one of those problems that we don't have a great way to solve if it's used.

I'm fairly confident in this change, so I hope that Mar25 will be fine.

[aidanhs]

Crater run started

[scottmcm]

Any news from crater, @aidanhs? It's now been 6 days.

[aidanhs]

Apologies, I've been pretty busy over the past week. A quick scan of the results shows at least one legit failure (prom-attire)

---

Hi @alexcrichton (crater requester/PR reviewer)! Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49039/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

[kennytm]

Two minor legit regressions involving custom #[derive].

xkpwgen-0.6.1 (unreachable code in `#[derive(StructOpt)]`)

Mar 23 13:28:42.820 INFO kablam! error: unreachable expression
Mar 23 13:28:42.820 INFO kablam!    --> src/main.rs:106:10
Mar 23 13:28:42.820 INFO kablam!     |
Mar 23 13:28:42.820 INFO kablam! 106 | #[derive(StructOpt, Debug)]
Mar 23 13:28:42.820 INFO kablam!     |          ^^^^^^^^^
Mar 23 13:28:42.820 INFO kablam!     |
Mar 23 13:28:42.820 INFO kablam! note: lint level defined here
Mar 23 13:28:42.821 INFO kablam!    --> src/main.rs:20:9
Mar 23 13:28:42.821 INFO kablam!     |
Mar 23 13:28:42.821 INFO kablam! 20  | #![deny(warnings)]
Mar 23 13:28:42.821 INFO kablam!     |         ^^^^^^^^
Mar 23 13:28:42.821 INFO kablam!     = note: #[deny(unreachable_code)] implied by #[deny(warnings)]

prom-attire-0.1.0

Mar 22 09:27:02.668 INFO kablam! error[E0277]: the trait bound `std::error::Error: std::marker::Sized` is not satisfied
Mar 22 09:27:02.669 INFO kablam!   --> tests/default.rs:71:14
Mar 22 09:27:02.669 INFO kablam!    |
Mar 22 09:27:02.669 INFO kablam! 71 |     #[derive(PromAttire)]
Mar 22 09:27:02.669 INFO kablam!    |              ^^^^^^^^^^ `std::error::Error` does not have a constant size known at compile-time
Mar 22 09:27:02.669 INFO kablam!    |
Mar 22 09:27:02.669 INFO kablam!    = help: the trait `std::marker::Sized` is not implemented for `std::error::Error`
Mar 22 09:27:02.669 INFO kablam!    = note: required by `<std::boxed::Box<T>>::new`

[alexcrichton]

@scottmcm mind looking into those errors to see what's going on?

[scottmcm]

The xkpwgen failure looks like it's because structopt (cc @TeXitoi) is used on a struct with a String field, and this PR changes impl FromStr for String to accurately report that it cannot error, which probably is the cause of the unreachable code error, though I didn't trace it fully. (Introducing a warning is allowed even if a crate uses deny(warnings), so I'm not too concerned about this one.)

[scottmcm]

The prom-attire failure is interesting. The proc macro generates this, which reduces to:

pub fn demo(x: !) -> Box<std::error::Error> {
    Box::new(x)
}

error[E0277]: the trait bound `std::error::Error: std::marker::Sized` is not satisfied
 --> src/lib.rs:4:5
  |
4 |     Box::new(x)
  |     ^^^^^^^^ `std::error::Error` does not have a constant size known at compile-time
  |
  = help: the trait `std::marker::Sized` is not implemented for `std::error::Error`
  = note: required by `<std::boxed::Box<T>>::new`

So even though !: Error, maybe it's not getting its Unsize impls? Edit: Nope, it's definitely getting !: Unsize<Error> (and thus Box<!>: CoerceUnsized<Box<Error>>).

Apparently this is just another "coercions and inference hate each other" 😞

Edit 2: FYI @Nemo157: regardless of whether this PR lands, you may still want to update the proc-macro to handle user types with FromStr<Err = !>.

[eddyb]

@scottmcm I think that's just because ! gets automatically coerced and so inference ends up with the Box::new call take x: dyn std::error::Error which isn't a valid call because of the trait object.

[eddyb]

You can't do this because people can impl it. You should (almost) never change stable nominal types.

[scottmcm]

While there's no problem with impl MyTrait for ParseError (you can impl through type aliases and there are no stable impls for !), @eddyb pointed out on IRC that impl MyTrait for fn()->! and impl MyTrait for fn()->ParseError are allowed on stable, so this may not be allowed even though the break found by crater is allowed inference/coercion changes (it's possible to write in a way that works before&after by specifying the type explicitly).

[eddyb]

I was informed on IRC that this could be fine because it's the only such type and you can't impl trait for ! on stable, but that's not enough, e.g.:

trait Trait {}
impl Trait for fn() -> ! {}
impl Trait for fn() -> ParseError {}

[TeXitoi]

Reproduction for the structopt case:

#![deny(warnings)]
fn foo() -> Result<(), !> {
    Ok(())
}
fn main() {
    let bar = foo() .map_err(|e| e.to_string());
    println!("{:?}", bar);
}

error: unreachable expression
 --> src/main.rs:6:33
  |
6 |     let bar = foo().map_err(|e| e.to_string());
  |                                 ^^^^^^^^^^^^^
  |
note: lint level defined here
 --> src/main.rs:1:9
  |
1 | #![deny(warnings)]
  |         ^^^^^^^^
  = note: #[deny(unreachable_code)] implied by #[deny(warnings)]

error: aborting due to previous error

[scottmcm]

Ok, I can see at least four options here:

• Decide that we want ! as the error types enough to just take this
  • The two things found by crater as affected by this PR will want to take updates to support ! error types regardless of whether std has any
• Decide that we'd like ! as the error types, but only going forward, so ParseError would continue to exist but we take Replace new-in-1.26 ParsePathError with ! #49404 to replace ParsePathError with !
• Decide that ! isn't actually a good choice for error types because of its behaviour with Box (Replace uninhabited error enums in std with never #49039 (comment)), so we want std::convert::Infallible back as the canonical (unstable for now) "can't actually error" error type, and a new PR to make ParseError a will-be-deprecated-in-future alias for that, use it in Path: FromStr, etc (I'll sign up to make a PR for that, but right now I need to sleep 😴)
  • Or, easier, just have Path's FromStr use string::ParseError, and introduce Infallible later
• Same as above, but we want distinct error types for each case, so we do nothing.

Thoughts, libs folks?

[SimonSapin]

This compiles:

#[allow(unreachable_code)]
pub fn demo(x: !) -> Box<std::error::Error> {
    Box::new(x) as _
}

So I don’t think that ! as an error type is really problematic.

[SimonSapin]

But then if you actually have that precise signature (rather than something more generic) you can take advantage of the unreachability / universal coercion:

pub fn demo(x: !) -> Box<std::error::Error> {
    x
}

[scottmcm]

That's an elegant fix, @SimonSapin. The code in the case is macro-generated, like match FromStr::from_str(x) { Ok(v) => v, Err(v) => return Err(MyCustomError(Box::new(v))) }, that intends to work whatever the type so long as it's FromStr, so the "If you have ! just return it" doesn't apply.

Any thoughts on whether we should take this, #49404, or something else? I think if we're going to do something, it needs to be soon because of things stabilizing in 1.26...

[SimonSapin]

Oh I didn’t consider the macro-generated case. Does the explicit conversion to DST with as work there?

I’ll bring this up with libs team member today.

[scottmcm]

Any news, @SimonSapin? (Sorry for pinging again, but with where we are in the 1.26 timeline...)

[SimonSapin]

Libs has a dedicated time slot at the Berlin All Hands in ~4 hours. I moved this to near the top of the agenda.

My personal opinion is that:

• We should definitely replace ParsePathError (the conversion to Box<Error> can be fixed with as, as shown above)
• Replacing string::ParseError seems very suspicious since it’s already stable, but maybe we can get away with it because there is only one type in that case, and the ! replacement only became stable in the same cycle, so maybe any code that would be broken (because of previously valid impls now overlapping) couldn’t be written on the stable channel before?

[Nemo157]

I can confirm as _ is enough to fix prom-attire, so I'm all for this change.

[SimonSapin]

The libs team discussed this and while a work-around is possible for this specific prom-attire code, consensus was that the fact that the obvious code doesn’t compile is concerning. Stabilizing the never type but recommending “don’t use it for error types” would be disastrous since errors were one of the big motivations for the never type.

The ideal outcome would be to tweak the language’s coercion rules so that Box::new(x: !): Box<Error> works (creating Box<!> and then unsizing it to Box<Error>, rather than trying to call Box::<Error>::new which can not exist.) We’ll work with the language and compiler teams to try and make this happen and backport it to 1.26 beta before it reaches the Stable channel in ~6 weeks. If we cannot make this happen in time, we’ll consider reverting the stabilization of the ! type (and reverting the addition of impl FromStr for Path) for another release cycle. (Though of course we’d rather find a satisfying solution.)

[alexcrichton]

I've opened a tracking issue for this at #49593

[kennytm]

I've beta-nominated this in case we can fix the coercion rule easily and decided to keep ! and TryFrom stable.

Also marking as blocked by #49593.

[bors]

☔ The latest upstream changes (presumably #49669) made this pull request unmergeable. Please resolve the merge conflicts.

[alexcrichton]

Given that ! stabilization is being reverted, I'm going to close this for now