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 uninhabited error enums in std with never #49039

Closed
wants to merge 1 commit into from

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Mar 15, 2018

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2018
@kennytm kennytm added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 15, 2018
@alexcrichton
Copy link
Member

@bors: try

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

@bors
Copy link
Contributor

bors commented Mar 15, 2018

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

bors added a commit that referenced this pull request Mar 15, 2018
Replace uninhabited error enums in std with never

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
@bors
Copy link
Contributor

bors commented Mar 15, 2018

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

@kennytm kennytm added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 15, 2018
@kennytm
Copy link
Member

kennytm commented Mar 15, 2018

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
Copy link
Member Author

@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
Copy link
Member

aidanhs commented Mar 20, 2018

Crater run started

@scottmcm
Copy link
Member Author

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

@aidanhs
Copy link
Member

aidanhs commented Mar 26, 2018

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
Copy link
Member

kennytm commented Mar 26, 2018

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`

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Mar 26, 2018
@alexcrichton
Copy link
Member

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

Luckily I only found two, and one of them isn't in beta yet, so can disappear completely :)
@scottmcm
Copy link
Member Author

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
Copy link
Member Author

scottmcm commented Mar 27, 2018

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
Copy link
Member

eddyb commented Mar 27, 2018

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

/// [`FromStr`]: ../../std/str/trait.FromStr.html
/// [`String`]: struct.String.html
/// [`from_str`]: ../../std/str/trait.FromStr.html#tymethod.from_str
#[stable(feature = "str_parse_error", since = "1.5.0")]
#[derive(Copy)]
pub enum ParseError {}
pub type ParseError = !;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 added a commit to TeXitoi/structopt that referenced this pull request Mar 27, 2018
@TeXitoi
Copy link
Contributor

TeXitoi commented Mar 27, 2018

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
Copy link
Member Author

scottmcm commented Mar 27, 2018

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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

@SimonSapin
Copy link
Contributor

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
Copy link
Member

Nemo157 commented Mar 30, 2018

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

@SimonSapin
Copy link
Contributor

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
Copy link
Member

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

@kennytm kennytm added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 3, 2018
@kennytm
Copy link
Member

kennytm commented Apr 3, 2018

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.

@kennytm kennytm added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2018
@bors
Copy link
Contributor

bors commented Apr 13, 2018

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

@alexcrichton
Copy link
Member

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

@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.