Subtle breaking change with match patterns on uninhabited types

[seanmonstar]

If an enum contained an uninhabited type, it is now valid to ignore matching that variant (and in fact encouraged, since include that pattern or _ now receives a warning).

I'll take a reduced example from hyper: hyper::Error

Currently, hyper::Error includes a __DontMatchMe variant as part of its way of saying that the Error enum could grow more variants. People should instead use a _ pattern. This is the best one can do to imitate std::io::ErrorKind, but without the additional #[unstable] attribute protecting it.

Since the __DontMatchMe variant should never actually be created, I filled it with an uninhabited type, so allow the optimizer to remove arms using it in std::error::Error and other such impls.

With rustc 1.14, this is correct:

enum Void {}
enum Error {
    Io,
    Utf8,
    Parse,
    __DontMatchMe(Void),
}

fn foo(x: Error) {
    match x {
        Error::Io => (),
        Error::Utf8 => (),
        Error::Parse => (),
        _ => (),
    }
}

With nightly after #38069 was landed, the above will warn that the user should remove the _ pattern. If they do so, then the contract that __DontMatchMe was supposed to provide is broken. With the _ removed, it looks like every variant is handled, and so any upgrade in hyper that adds a new variant will break this code. And that code will look innocent, where as before at least someone you need to explitictly do a match e { Error::__DontMatchMe => () } and know that it could break.

I can remove the uninhabited type in hyper, but this behavior change does feel like one of the subtlely breaking kind.

[nikomatsakis]

@seanmonstar I am opening a thread to discuss some of these changes, but I do want to point out that you could probably fix your original pattern by making the uninhabitedness a private detail of the type. In particular, changing to something like this should work:

enum VoidEnum { } // NOT public
pub struct Void(VoidEnum);

If you set it up this way, it is not publicly visible that Void is uninhabited, so it will be considered inhabited by matches outside of the hyper crate. As an added benefit, matches inside hyper should be able to ignore this dummy variant.

I agree this is a regression of some kind, but I think that it's not the usual kind of regression. Whenever code relies on limitations of the existing type-checker to make guarantees, there is the danger that if the type-checker becomes more intelligent, it will endanger that code. I'm not sure the best way to solve that challenge, other than being careful.

(In retrospect, I think it's safe to say that at minimum I think we should have investigated a way to phase in some warnings or something that might have alerted you to this, though I'm not sure just how that would work, or if it would have been possible.)

[nikomatsakis]

See https://internals.rust-lang.org/t/recent-change-to-make-exhaustiveness-and-uninhabited-types-play-nicer-together/4602

[seanmonstar]

@nikomatsakis thanks for starting the discussion, and also for the alternative suggestion. I'll give it a try and see how it works.

I didn't mean to say that this is "a breaking change how dare you" sort of thing, which I think you understood, but instead just wanted to point out (as you summed up) that this is a behavior change, that would cause already existing code to behave differently, and wanted to make sure the repercussions were considered. So thanks!

[nikomatsakis]

In any case, @seanmonstar, we are going to temporarily back out this change while we evaluate our long-term plan. But I would advise you to try the privacy thing anyhow, as that is likely to be the model we take in any case. The uncertainty is really around corner cases in unsafe code, not simple cases like this one.

I will close the issue for now I think. Feel free to re-open.