From c3d45775c438c57450317ccc859d3af10ce08972 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Mon, 1 Nov 2021 06:12:43 +0200 Subject: [PATCH 1/2] Fix `match_overlapping_arm` false negative Fixes #7816 --- clippy_lints/src/matches.rs | 46 +++++++++++++-------------- tests/ui/match_overlapping_arm.rs | 9 ++++++ tests/ui/match_overlapping_arm.stderr | 14 +++++++- 3 files changed, 44 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index f1289a36e777..3511defc59c6 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -34,7 +34,6 @@ use rustc_span::source_map::{Span, Spanned}; use rustc_span::sym; use std::cmp::Ordering; use std::collections::hash_map::Entry; -use std::iter; use std::ops::Bound; declare_clippy_lint! { @@ -1703,12 +1702,6 @@ where } impl<'a, T: Copy> Kind<'a, T> { - fn range(&self) -> &'a SpannedRange { - match *self { - Kind::Start(_, r) | Kind::End(_, r) => r, - } - } - fn value(self) -> Bound { match self { Kind::Start(t, _) => Bound::Included(t), @@ -1726,7 +1719,19 @@ where impl<'a, T: Copy + Ord> Ord for Kind<'a, T> { fn cmp(&self, other: &Self) -> Ordering { match (self.value(), other.value()) { - (Bound::Included(a), Bound::Included(b)) | (Bound::Excluded(a), Bound::Excluded(b)) => a.cmp(&b), + (Bound::Included(a), Bound::Included(b)) | (Bound::Excluded(a), Bound::Excluded(b)) => { + let value_cmp = a.cmp(&b); + // In the case of ties, starts come before ends + if value_cmp == Ordering::Equal { + match (self, other) { + (Kind::Start(..), Kind::End(..)) => Ordering::Less, + (Kind::End(..), Kind::Start(..)) => Ordering::Greater, + _ => Ordering::Equal, + } + } else { + value_cmp + } + }, // Range patterns cannot be unbounded (yet) (Bound::Unbounded, _) | (_, Bound::Unbounded) => unimplemented!(), (Bound::Included(a), Bound::Excluded(b)) => match a.cmp(&b) { @@ -1750,24 +1755,17 @@ where values.sort(); - for (a, b) in iter::zip(&values, values.iter().skip(1)) { - match (a, b) { - (&Kind::Start(_, ra), &Kind::End(_, rb)) => { - if ra.node != rb.node { - return Some((ra, rb)); - } - }, - (&Kind::End(a, _), &Kind::Start(b, _)) if a != Bound::Included(b) => (), - _ => { - // skip if the range `a` is completely included into the range `b` - if let Ordering::Equal | Ordering::Less = a.cmp(b) { - let kind_a = Kind::End(a.range().node.1, a.range()); - let kind_b = Kind::End(b.range().node.1, b.range()); - if let Ordering::Equal | Ordering::Greater = kind_a.cmp(&kind_b) { - return None; + let mut started = vec![]; + + for value in values { + match value { + Kind::Start(_, r) => started.push(r), + Kind::End(_, er) => { + if let Some(sr) = started.pop() { + if sr != er { + return Some((er, sr)); } } - return Some((a.range(), b.range())); }, } } diff --git a/tests/ui/match_overlapping_arm.rs b/tests/ui/match_overlapping_arm.rs index 845986a4eada..c25f59c62a5c 100644 --- a/tests/ui/match_overlapping_arm.rs +++ b/tests/ui/match_overlapping_arm.rs @@ -100,6 +100,15 @@ fn overlapping() { _ => (), } + // Issue #7816 - overlap after included range + match 42 { + 5..=10 => (), + 0..=20 => (), + 21..=30 => (), + 21..=40 => (), + _ => (), + } + // Issue #7829 match 0 { -1..=1 => (), diff --git a/tests/ui/match_overlapping_arm.stderr b/tests/ui/match_overlapping_arm.stderr index c2b3f173c2b8..69f794edb6a3 100644 --- a/tests/ui/match_overlapping_arm.stderr +++ b/tests/ui/match_overlapping_arm.stderr @@ -71,5 +71,17 @@ note: overlaps with this LL | ..26 => println!("..26"), | ^^^^ -error: aborting due to 6 previous errors +error: some ranges overlap + --> $DIR/match_overlapping_arm.rs:107:9 + | +LL | 21..=30 => (), + | ^^^^^^^ + | +note: overlaps with this + --> $DIR/match_overlapping_arm.rs:108:9 + | +LL | 21..=40 => (), + | ^^^^^^^ + +error: aborting due to 7 previous errors From 693df63c7d51aca2bb405a0574717a5d02fd8158 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Mon, 1 Nov 2021 06:12:43 +0200 Subject: [PATCH 2/2] Ensure `match_overlapping_arms` warns on first --- clippy_lints/src/matches.rs | 13 ++++++++++--- tests/ui/match_overlapping_arm.rs | 9 +++++++++ tests/ui/match_overlapping_arm.stderr | 14 +++++++++++++- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 3511defc59c6..b552e3fc7afe 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1761,10 +1761,17 @@ where match value { Kind::Start(_, r) => started.push(r), Kind::End(_, er) => { - if let Some(sr) = started.pop() { - if sr != er { - return Some((er, sr)); + let mut overlap = None; + + while let Some(sr) = started.pop() { + if sr == er { + break; } + overlap = Some(sr); + } + + if let Some(sr) = overlap { + return Some((er, sr)); } }, } diff --git a/tests/ui/match_overlapping_arm.rs b/tests/ui/match_overlapping_arm.rs index c25f59c62a5c..2f85e6357135 100644 --- a/tests/ui/match_overlapping_arm.rs +++ b/tests/ui/match_overlapping_arm.rs @@ -116,6 +116,15 @@ fn overlapping() { _ => (), } + // Only warn about the first if there are multiple overlaps + match 42u128 { + 0..=0x0000_0000_0000_00ff => (), + 0..=0x0000_0000_0000_ffff => (), + 0..=0x0000_0000_ffff_ffff => (), + 0..=0xffff_ffff_ffff_ffff => (), + _ => (), + } + if let None = Some(42) { // nothing } else if let None = Some(42) { diff --git a/tests/ui/match_overlapping_arm.stderr b/tests/ui/match_overlapping_arm.stderr index 69f794edb6a3..b81bb1ecfae0 100644 --- a/tests/ui/match_overlapping_arm.stderr +++ b/tests/ui/match_overlapping_arm.stderr @@ -83,5 +83,17 @@ note: overlaps with this LL | 21..=40 => (), | ^^^^^^^ -error: aborting due to 7 previous errors +error: some ranges overlap + --> $DIR/match_overlapping_arm.rs:121:9 + | +LL | 0..=0x0000_0000_0000_00ff => (), + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: overlaps with this + --> $DIR/match_overlapping_arm.rs:122:9 + | +LL | 0..=0x0000_0000_0000_ffff => (), + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 8 previous errors