Incorrect "unreachable pattern" warning.

[steffahn]

Not much to comment about. The supposedly “unreachable” pattern is obviously reached.

fn main() {
    match (3,42) {
        (a,_) | (_,a) if a > 10 => {println!("{}", a)}
        _ => ()
    }
}

(Playground)

Output:

42

Errors:

   Compiling playground v0.0.1 (/playground)
warning: unreachable pattern
 --> src/main.rs:4:17
  |
4 |         (a,_) | (_,a) if a > 10 => {println!("{}", a)}
  |                 ^^^^^
  |
  = note: `#[warn(unreachable_patterns)]` on by default

    Finished release [optimized] target(s) in 0.78s
     Running `target/release/playground`

This issue has been assigned to @AminArria via this comment.

[Centril]

Looks like this was injected in 1.41.0, ostensibly in the exhaustiveness checking refactorings.
To find out, let's cc @rustbot ping cleanup

cc @varkor @Nadrieril

[rustbot]

Hey Cleanup Crew ICE-breakers! This bug has been identified as a good
"Cleanup ICE-breaking candidate". In case it's useful, here are some
instructions for tackling these sorts of bugs. Maybe take a look?
Thanks! <3

cc @AminArria @chrissimpkins @contrun @DutchGhost @elshize @ethanboxx @h-michael @HallerPatrick @hdhoang @hellow554 @imtsuki @jakevossen5 @kanru @KarlK90 @LeSeulArtichaut @MAdrianMattocks @matheus-consoli @mental32 @nmccarty @Noah-Kennedy @pard68 @PeytonT @pierreN @Redblueflame @RobbieClarken @RobertoSnap @robjtede @SarthakSingh31 @senden9 @shekohex @sinato @spastorino @turboladen @woshilapin @yerke

[Centril]

The relevant code is in check_match::check_arms:

fn check_arms<'p, 'tcx>(
    cx: &mut MatchCheckCtxt<'p, 'tcx>,
    arms: &[(&'p super::Pat<'tcx>, HirId, bool)],
) -> Matrix<'p, 'tcx> {
    let mut seen = Matrix::empty();
    let mut catchall = None;
    for (arm_index, (pat, id, has_guard)) in arms.iter().copied().enumerate() {
        let v = PatStack::from_pattern(pat);
        match is_useful(cx, &seen, &v, LeaveOutWitness, id, true) {
            NotUseful => { ... } // Elided, this branch should not be taken in this case.
            Useful(unreachable_subpatterns) => {
                for pat in unreachable_subpatterns {
                    unreachable_pattern(cx.tcx, pat.span, id, None);
                }
            }
            UsefulWithWitness(_) => bug!(),
        }
        if !has_guard {
            seen.push(v);
            if catchall.is_none() && pat_is_catchall(pat) {
                catchall = Some(pat.span);
            }
        }
    }
    seen
}

From the point of exhaustiveness checking this algorithm is correct, but not from a linting POV in the presence of guards. Previously, before the or-patterns reform, we merely interpreted each | as a separate arm, so this was correctly handled from a linting perspective in the NotUseful => { ... } arm. Now however, the linting will happen in the Useful(unreachable_subpatterns) => { ... } arm, as according to is_useful(...), unaware of whether the arm has_guard, the pattern itself has an unreachable alternative.

To fix this, presumably we have to propagate has_guard into is_useful, while being careful not to declare anything exhaustive which isn't for the soundness sensitive bits.

[steffahn]

In case this is still relevant.. I went trying out cargo-bisect-rustc and it gave me:

regressed nightly: nightly-2019-12-04
regressed commit: f577b0e

[Nadrieril]

I'm going to assume we're ok with not detecting

match (3,42) {
    (a, _) | (a, _) if a > 10 => {println!("{}", a)}
    _ => ()
}

as redundant, otherwise we violate one of the assumptions of match checking, namely that variables and wildcards behave identically, and that's a whole other can of worms.

To fix this, presumably we have to propagate has_guard into is_useful, while being careful not to declare anything exhaustive which isn't for the soundness sensitive bits.

Agreed. Specifically, what we want to avoid is adding to the matrix any pattern that is under a match guard. The code in check_match does that correctly, but the or-pattern-handling code in _match doesn't. Once again I can't fix that myself right now, but I suspect it should be enough to disable this line:

rust/src/librustc_mir_build/hair/pattern/_match.rs

Line 1667 in cdb50c6

matrix.push(v);

when we are under a match guard. So passing a is_under_guard bool to is_useful (recursively) should do the trick.
@Centril (or anyone really) would you mind trying that ? 👼

Two more test cases to be sure we're doing it right:

match Some((3,42)) {
    Some((a, _)) | Some((_, a)) if a > 10 => {println!("{}", a)}
    _ => ()
}
match Some((3,42)) {
    Some((a, _) | (_, a)) if a > 10 => {println!("{}", a)}
    _ => ()
}

[varkor]

With such clear instructions, this looks like a good issue for beginners! I'll add the labels.

[AminArria]

Hi, I want to take this

I understand the solution explained by @Nadrieril, but have one question regarding check_not_useful because it also calls is_useful. From what I gather it generates a wildcard pattern and check it isn't useful, so for this task I can treat it as having no guard right?

[spastorino]

@rustbot assign @AminArria

[Nadrieril]

From what I gather it generates a wildcard pattern and check it isn't useful, so for this task I can treat it as having no guard right?

Absolutely !

[Centril]

@AminArria Thanks for tackling this. :)

I think it would also be neat to add some comments on fn is_useful around what it means to pass in "yes it has a guard", specifically that it shouldn't be relied on for soundness (i.e. in check_not_useful). Perhaps also consider an enum HasGuard { Yes, No } or some such for added robustness (consider also turning is_top_level: bool into enum TopLevel { Yes, No } for stronger typing).

Best of luck!

[Centril]

I'm going to assume we're ok with not detecting

match (3,42) {
    (a, _) | (a, _) if a > 10 => {println!("{}", a)}
    _ => ()
}

as redundant, otherwise we violate one of the assumptions of match checking, namely that variables and wildcards behave identically, and that's a whole other can of worms.

👍 -- that seems like quite a reasonable tradeoff for what is a rare occurrence. :)

[varkor]

Perhaps also consider an enum HasGuard { Yes, No } or some such for added robustness (consider also turning is_top_level: bool into enum TopLevel { Yes, No } for stronger typing).

I don't think this is any more readable, or safe, than a bool. The variable name makes it very clear in the method, and we can comment the call site if that seems ambiguous.