Extend overlapping ranges lint to cover cases with more than a single element overlapping

[estebank]

#64007 introduces an overlapping ranges lint that triggers only if the beginning or the end overlap with another arms' pattern, like in 0..10/9..20. There should be a conversation on whether it should also trigger when the overlap is beyond that, like in 0..10/5..15.

[nikomatsakis]

We discussed this in today's @rust-lang/lang meeting. I wouldn't say that there was much of a strong opinion. On the one hand, you can imagine cases where you would intentionally have overlap, but that's always true, and the scenario didn't seem super compelling (example: pre-existing constants for various ranges, and you want to give one range precedence). On the other hand, this doesn't feel like something that arises super often.

We wanted to propose a few options:

• do a crater run to get an idea of how often this comes up in practice and how many of those cases feel like bugs -- this might help to decide for a warn-by-default lint
• create an allow-by-default lint for this scenario (and perhaps others, like full subsumption)
• move the lint to clippy

The last option might be impractical, because the analysis is fairly involved in the compiler. Hence the middle option.

[calimeroteknik]

If I may chime in, as a new user it was utterly confusing when I found out that #![deny(overlapping_patterns)] did not always deny overlapping patterns. This inspired me a quite a bit of distrust in the rust compiler.

Unless the ranges 0..10 and 5..15 can be defined as "not overlapping" (!?), but that seems blatantly false and unarguable to me.

I would conclude that either :

• overlapping_patterns should become what it sounds like, meaning #![deny(overlapping_patterns)] should deny all overlapping patterns, not just some of them;
• and/or, the current behaviour should be renamed to something else, like pattern_boundary_clash.

This is of course a breaking change, so the choice is whether to keep the confusing lint name for backwards compatibility, or to bite the bullet and change it to something that is actually what it looks like.

Edit: I should mention that clippy does apparently catch all overlapping range patterns, from what I tried. toy example

[Nadrieril]

Note: clippy only detects the simple case where there is only a bare range pattern. A more complex pattern doesn't get detected. This is also what the compiler does for this lint. To go beyond that simple case would indeed require the full power of the compiler's exhaustiveness checker.

match x { // single range pattern
    0 ..= 125 => {}
    125 ..= 255 => {} // overlap detected
}
match x { // anything else
    (0 ..= 125, true) => {}
    (125 ..= 255, true) => {} // overlap not detected
}

[Nadrieril]

So, I've been trying to extend the existing lint beyond that simple case. Turns out this does not fit naturally with what the exhaustiveness algorithm currently does. I'm also not sure exactly what we would want to lint:

// Presumably we want to lint about overlap between the third and first
// branches, but not the second. This would require keeping track of which
// branches made a pattern redundant.
match x {
    (0 ..= 125, true, true) => {}
    (0 ..= 125, true, false) => {}
    (125 ..= 255, true, true) => {}
}
// If we also want a similar lint for this one, then this requires computing
// pattern intersections, which is something that the algorithm does not know
// how to do at all.
match x {
    (0 ..= 125, true, true) => {}
    (0 ..= 125, false, false) => {}
    (125 ..= 255, true, _) => {}
}

It is doable, but looks like a massive undertaking. I'd be happy to implement that if I felt this was worth it, but it looks too small for that kind of complexity penalty.
The alternative is living with a half-baked lint that only applies to the simplest patterns.

[calimeroteknik]

By the way, it is obvious that when a default case is present (and isn't the only case present), overlap is by design: the default case overlaps all other patterns.

In that case, I wouldn't expect the compiler to even try to detect overlap between patterns.

Idea: in the cases where the compiler cannot check the absence of overlap between patterns (because they're too complex), it could warn that it was unable to do so. Also, in the presence of a default case, such a warning could be silenced by default.

[varkor]

I think we should simply rename the existing lint (there's precedent for doing this, and we can warn users a lint name has changed) to make it less confusing.

[Nadrieril]

I think we should simply rename the existing lint (there's precedent for doing this, and we can warn users a lint name has changed) to make it less confusing.

Agreed; I'd go with suspiciously_overlapping_ranges.

[varkor]

overlapping_endpoints is descriptive and hopefully unambiguous, or overlapping_range_endpoints.

[Nadrieril]

I can make a PR to change the lint name. I also believe I only need to add a line here to get a rename warning:

rust/compiler/rustc_lint/src/lib.rs

Lines 318 to 319 in 1d27267

	// Register renamed and removed lints.
	store.register_renamed("single_use_lifetime", "single_use_lifetimes");

Is that the right place?

@rustbot claim
@rustbot modify labels: +A-exhaustiveness-checking

[varkor]

Yes, that looks right.

[Nadrieril]

Coming back to this, I think this could be a more general clippy lint, something like "this pattern could be more specific". It would say "use 11..20 instead of 10..20 here", or "use Some(_) instead of _ here". This feels doable once we've librarified exhaustiveness checking (which I'm slowly working towards).