Repository navigation
Extend overlapping ranges lint to cover cases with more than a single element overlapping #65477
Description
Activity
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.
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_patternsshould 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
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 }
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.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.
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.
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.Reacted by varkoroverlapping_endpointsis descriptive and hopefully unambiguous, oroverlapping_range_endpoints.Reacted by calimeroteknik and NadrierilI 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
Is that the right place?@rustbot claim
@rustbot modify labels: +A-exhaustiveness-checkingYes, that looks right.
- added 3 commits that reference this issue
on Dec 21, 2020 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..20instead of10..20here", or "useSome(_)instead of_here". This feels doable once we've librarified exhaustiveness checking (which I'm slowly working towards).Reacted by Esteban Kuber and Scott SteeleExhaustiveness checking has now been librarified and is used in rust-analyzer. I someone wants to make this into a clippy lint, I can assist with how to use
rustc_pattern_analysis.
#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 in0..10/5..15.