Skip to content

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

Closed
@estebank

Description

@estebank
Contributor

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.

Activity

added
A-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.
A-diagnosticsArea: Messages for errors, warnings, and lints
T-langRelevant to the language team
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.
A-suggestion-diagnosticsArea: Suggestions generated by the compiler applied by `cargo fix`
on Aug 29, 2019
Centril

Centril commented on Aug 29, 2019

@Centril
Contributor
estebank

estebank commented on Aug 29, 2019

@estebank
ContributorAuthor

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

hellow554 commented on Aug 30, 2019

@hellow554
Contributor

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

estebank

estebank commented on Aug 31, 2019

@estebank
ContributorAuthor

@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

Centril commented on Aug 31, 2019

@Centril
Contributor

@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

hellow554 commented on Aug 31, 2019

@hellow554
Contributor

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

estebank commented on Aug 31, 2019

@estebank
ContributorAuthor

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.

added a commit that references this issue on Oct 19, 2019

Rollup merge of rust-lang#64007 - estebank:overlapping-patterns, r=ma…

53a3bfc
Nadrieril

Nadrieril commented on Dec 19, 2023

@Nadrieril
Member

And now we might also get a lint for the case of a small gap: #118879

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-diagnosticsArea: Messages for errors, warnings, and lintsA-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.C-enhancementCategory: An issue proposing an enhancement or a PR with one.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.T-langRelevant to the language team

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @Centril@hellow554@estebank@jonas-schievink@Nadrieril

      Issue actions

        Lint against overlapping pattern ranges and pattern ranges with small holes caught by `_` · Issue #63987 · rust-lang/rust