Lint against overlapping pattern ranges and pattern ranges with small holes caught by `_`

[estebank]

For both exclusive(nightly) and inclusive range patterns, we don't have any lint about overlapping ranges. We probably should complain about 100..=500 being partially covered already. This would minimize the likelihood of the following off-by-one error going unnoticed:

fn main() {
    let x: u32 = 301;
    match x {
        0..=200 => println!("a"),
        200..=300 => println!("b"),
        300..=400 => println!("c"),
        100..=500 => println!("x"),
        _ => println!("d"),
    }
}

It would also be interesting to have a 1 or 2 value wide holes detection in the presence of a _ pattern to detect confusion between exclusive and inclusive ranges:

#![feature(exclusive_range_pattern)]

fn main() {
    let x: u32 = 301;
    match x {
        0..200 => println!("a"),
        201..300 => println!("b"),
        301..400 => println!("c"),
        _ => println!("d"),
    }
}

We already have range coverage checks when not using _:

error[E0004]: non-exhaustive patterns: `200u32`, `300u32` and `400u32..=std::u32::MAX` not covered
 --> src/main.rs:5:11
  |
5 |     match x {
  |           ^ patterns `200u32`, `300u32` and `400u32..=std::u32::MAX` not covered
  |
  = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

We might want to just lint against _ and instead ask to be explicit to handle the "small holes" case.

CC #37854 for the exclusive_range_pattern case.

[Centril]

cc @varkor

[estebank]

I poked at the code for a bit and there's already code in hair::pattern::_match::split_grouped_constructors() that checks for this. It's just a matter of actually warning when splitting using Borders instead of only remaking the ranges.

[hellow554]

But do you really want a lint for something that is actually ok-ish to do? I'm not sure about that :/

[estebank]

@hellow554 but is it ok? We currently don't lint the following:

    match 0usize {
        1 ..= 8 => {}
        5 ..= 20 => {} // This should probably have been `8 ..= 20`
        20 ..= 100 => {} // This should probably have been `21 ..= 100`
        _ => {}
    }

It seems to me like a reasonable warn by default lint. Silencing them in the scope close to it is reasonable. For the common case I would prefer to spell out the (one or two) holes in the pattern. In the std there were only two cases this lint triggered and both were checking for unicode ranges ignoring \, ' and ".

[Centril]

@hellow554 but is it ok? We currently don't lint the following:

I think the better question is: does the lint prevent something harmful?

I could see this lint being problematic when coupled with pattern aliases if we added those because there might be partial overlap when you use an or-pattern in combination with two pattern aliases.

[hellow554]

We currently don't lint the following:

Yes, indeed. Rust doesn't emit a warning, but clippy does:

warning: some ranges overlap
 --> src/main.rs:3:9
  |
3 |         1 ..= 8 => {}
  |         ^^^^^^^
  |
  = note: `#[warn(clippy::match_overlapping_arm)]` on by default
note: overlaps with this
 --> src/main.rs:4:9
  |
4 |         5 ..= 20 => {} // This should probably have been `8 ..= 20`
  |         ^^^^^^^^
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_overlapping_arm

There are many lints that rustc doesn't cover, but clippy will do.

[estebank]

The existence of the clippy lint is encouraging, although I'd make the argument that this lint deserves to be in rustc as much as the unreachable patterns one.

I will say, though, that the output of the clippy lint could be improved slightly by using labels instead of notes and being more explicit on how the ranges overlap:

warning: multiple patterns covering the same range
  --> $DIR/issue-43253.rs:42:9
   |
LL |         5..7 => {},
   |         ---- this range overlaps on `5i32..=6i32`
LL |         6 => {},
   |         - this range overlaps on `6i32`
LL |         1..10 => {},
   |         ^^^^^ overlapping patterns

I think the better question is: does the lint prevent something harmful?

I think the clear cut case is off-by-one errors when writing 0..=10/10..=20 where the 10 in the second pattern will never be hit. It seems to me like it would protect against logic errors, and opting into silencing the lint is a better default.

That being said, I'd leave it to the lang team to decide whether this lint should be in rustc or remain in clippy.