Fix handling of empty types in patterns.

[canndrew]

Fix for #12609.

[rust-highfive]

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

[ranma42]

Style: usually else is on the same line as the closing bracket.

[oli-obk]

Why is it necessary to change this example?

[oli-obk]

ah nevermind, because this triggers for E0001 and not for unreachable_patterns

[bluss]

cc @brson because this would be a new (insta-stable?) feature

[nikomatsakis]

Why do we keep this as an error even if there is an _ pattern? Is there a strong reason?

[canndrew]

Not sure, it was @eddyb's suggestion. If that becomes a lint then we don't get to use E0001 at all though.

[arielb1]

Don't use E0001 at all.

[canndrew]

@eddyb Opinion?

[eddyb]

The annoying thing is that a lint loses the help message, but if everyone else is fine with it, then sure, you can remove this special case.

[nikomatsakis]

Seems like we need some more test -- for example, I don't see any tests covering the private field cases, nor the "don't need to match variants" code, right? (is that because this is a WIP?)

[canndrew]

Seems like we need some more test -- for example, I don't see any tests covering the private field cases, nor the "don't need to match variants" code, right? (is that because this is a WIP?)

Yep, it should all be there now though.

[canndrew]

It might be worth @arielb1 having a look at this too, seeing as they wrote the match checking code that I've fiddled with.

[canndrew]

So a quick summary of these changes:

(Most) unreachable patterns now generate warnings instead of errors. I've added a new unreachable_patterns lint rather than reuse unreachable_code just so people can be slightly more specific about {en,dis}abling errors.

Unreachable patterns that used to sneak past the pattern-match checks no longer produce an unreachable_code warning later on.

I've fixed is_uninhabited for enums. Before it used to think that the fields of enum variants were private and so, for example, it would treat Result<!, !> as being inhabited on the assumption that the user can't see the !s inside. I did this by making FieldDefData::is_uninhabited_recurse take an is_enum: bool flag which overrides the field's visibility flag. Is this the best way to do this? Maybe the visibility of enum variant fields should be correctly set in the first place.

is_useful no longer exits early with Useful if it gets a matrix with zero rows. This is incorrect behaviour when empty types are involved. It would be possible to add this check back and short-circuit the rest of the algorithm so long as we also check for empty types and return NotUseful if we find one. But for now I've kept the algorithm simple.

all_constructors(Ty) only returns constructors that are actually possible. eg. for Result<u32, !> it will only return Ok, for &[!] it will only return &[], for empty types it will not return anything etc.

Removed the special-casing that made match uninhabited_expr {} compile before. This should be handled naturally by the pattern-matching algs now.

Correct type information is carried on dummy wildcard patterns. This was necessary to make the above change but I suspect it's also a more general bugfix. It used to be possible for the pattern-match algs to be handling a dummy TyError when they should really know the actual type, even for non-empty matrices.

is_useful produces fewer, more general witnesses using wildcards when it can. eg. instead of returning (Ok(_), Some(_)) and (Err(_), Some(_)) it will just return (_, Some(_)).

PatternKind::Variant now carries Substs info.

rustc_mir::build::Builder::simplify_match_pair can now handle enums that have multiple variants but only one statically-possible variant. This allows code like let x: Result<u32, !> = ...; let Ok(y) = x; to compile by downcasting x to its inner u32.

[bors]

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

[canndrew]

@nikomatsakis this is ready now btw.

@brson any chance I could get a crater run on this?

[nikomatsakis]

@canndrew sorry for delay. Mozilla all hands didn't leave much time for reviewing. I'm going to be on vacation next week but will try to find time, else perhaps someone else can/should review.

[canndrew]

@nikomatsakis That's alright. I was just a bit angsty that this would get ignored for a month or so and eventually bit-rot like the last PR for this did. I understand you guys have a very full plate though.

Probably @arielb1 would be the best person to review this?

[arielb1]

This should be #[deny(unreachable_patterns)]

[arielb1]

I'm worried that the recursion here might cause things to be slow. Could you try adding a cache somewhere?

[canndrew]

I've refactored is_uninhabited_recurse to use a cache. It's now called uninhabited_from and it calculates the full forest of nodes that a type is visibly uninhabited from and caches that. Eg. if we have this:

mod mod_a {
    pub struct SecretlyUninhabited {
        _priv: !,
    }
}
mod mod_b {
    pub mod mod_c {
        pub struct SecretlyUninhabited {
            _priv: !,
        }
        mod mod_d {
            ...
        }
    }
}
struct MyType {
    foo: mod_a::SecretlyUninhabited,
    bar: mod_b::SecretlyUninhabited,
}

Then uninhabited_from(MyType) will give the set containing mod_a, mod_c and all their descendants (represented as [mod_a, mod_c]). We can then check whether that set is non-empty to see whether a type is uninhabited or we can check whether the set contains a given node to see whether the type is visibly uninhabited from that node.

[canndrew]

Anyone know what happened to travis there?

[eddyb]

@canndrew I think Travis has been busted for almost a week now.

[canndrew]

Is there anything more I can do to help get this reviewed?

[nikomatsakis]

@canndrew back now, sorry about that.

[nikomatsakis]

Will review.

[nikomatsakis]

Sorry for delay. I am doing a bit of reading up on the general algorithm since I'm feeling ill-equipped to judge this patch. =)

[canndrew]

That's cool.

is_useful no longer exits early with Useful if it gets a matrix with zero rows. This is incorrect behaviour when empty types are involved. It would be possible to add this check back and short-circuit the rest of the algorithm so long as we also check for empty types and return NotUseful if we find one. But for now I've kept the algorithm simple.

all_constructors(Ty) only returns constructors that are actually possible. eg. for Result<u32, !> it will only return Ok, for &[!] it will only return &[], for empty types it will not return anything etc.

Algorithm-wise these should be the only relevant changes and should be enough to make it correctly handle empty types.

[bors]

📌 Commit 291c84a has been approved by nikomatsakis

[bors]

⌛️ Testing commit 291c84a with merge 85c76a0...

[eddyb]

@bors r=nikomatsakis force

[eddyb]

@bors retry

[eddyb]

@bors r=nikomatsakis

[bors]

📌 Commit 275c19d has been approved by nikomatsakis

[bors]

⌛️ Testing commit 275c19d with merge 6f1ae66...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6f1ae66 to master...

[tomaka]

I suggest a relnotes tag for this PR.

[GuillaumeGomez]

Done.