From 98416d7f6ca4cc803d423e0897ccdf1deb08fe31 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 9 Nov 2021 05:44:02 +0200 Subject: [PATCH 1/6] Remove `unimplemented!()` case in matches code This unbounded case never actually happens because `all_ranges(..)` uses the scrutinee type bounds for open ranges. Switch to our own `Bound` enum so that we don't have this case. --- clippy_lints/src/matches.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 88b1685f5..6973a3743 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -33,7 +33,6 @@ use rustc_span::source_map::{Span, Spanned}; use rustc_span::sym; use std::cmp::Ordering; use std::collections::hash_map::Entry; -use std::ops::Bound; declare_clippy_lint! { /// ### What it does @@ -1596,7 +1595,7 @@ fn opt_parent_let<'a>(cx: &LateContext<'a>, ex: &Expr<'a>) -> Option<&'a Local<' None } -/// Gets all arms that are unbounded `PatRange`s. +/// Gets the ranges for each range pattern arm. Applies `ty` bounds for open ranges. fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) -> Vec> { arms.iter() .filter_map(|arm| { @@ -1637,6 +1636,12 @@ fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) .collect() } +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum Bound { + Included(T), + Excluded(T), +} + #[derive(Debug, Eq, PartialEq)] pub struct SpannedRange { pub span: Span, @@ -1730,8 +1735,6 @@ where value_cmp } }, - // Range patterns cannot be unbounded (yet) - (Bound::Unbounded, _) | (_, Bound::Unbounded) => unimplemented!(), (Bound::Included(a), Bound::Excluded(b)) => match a.cmp(&b) { Ordering::Equal => Ordering::Greater, other => other, From 949b25981cdaf2c7708ee257651f1cb9dc951332 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 9 Nov 2021 05:44:02 +0200 Subject: [PATCH 2/6] Change `Bound` to `EndBound` Only the end bounds of ranges can actually be included or excluded. This commit changes the SpannedRange type to reflect that. Update `Kind::value` to and `Kind::cmp` for this change. `Kind::cmp` gets flipped to check value first and then the bound details and is much shorter. --- clippy_lints/src/matches.rs | 78 +++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 42 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 6973a3743..7a528cb0b 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1614,8 +1614,8 @@ fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) let rhs_val = rhs.int_value(cx, ty)?; let rhs_bound = match range_end { - RangeEnd::Included => Bound::Included(rhs_val), - RangeEnd::Excluded => Bound::Excluded(rhs_val), + RangeEnd::Included => EndBound::Included(rhs_val), + RangeEnd::Excluded => EndBound::Excluded(rhs_val), }; return Some(SpannedRange { span: pat.span, @@ -1627,7 +1627,7 @@ fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) let value = constant_full_int(cx, cx.typeck_results(), value)?; return Some(SpannedRange { span: pat.span, - node: (value, Bound::Included(value)), + node: (value, EndBound::Included(value)), }); } } @@ -1636,8 +1636,8 @@ fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) .collect() } -#[derive(Copy, Clone, Debug, Eq, PartialEq)] -pub enum Bound { +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum EndBound { Included(T), Excluded(T), } @@ -1645,7 +1645,7 @@ pub enum Bound { #[derive(Debug, Eq, PartialEq)] pub struct SpannedRange { pub span: Span, - pub node: (T, Bound), + pub node: (T, EndBound), } // Checks if arm has the form `None => None` @@ -1701,14 +1701,13 @@ where #[derive(Copy, Clone, Debug, Eq, PartialEq)] enum Kind<'a, T> { Start(T, &'a SpannedRange), - End(Bound, &'a SpannedRange), + End(EndBound, &'a SpannedRange), } impl<'a, T: Copy> Kind<'a, T> { - fn value(self) -> Bound { + fn value(self) -> T { match self { - Kind::Start(t, _) => Bound::Included(t), - Kind::End(t, _) => t, + Kind::Start(t, _) | Kind::End(EndBound::Included(t) | EndBound::Excluded(t), _) => t, } } } @@ -1721,28 +1720,23 @@ 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)) => { - 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 - } - }, - (Bound::Included(a), Bound::Excluded(b)) => match a.cmp(&b) { - Ordering::Equal => Ordering::Greater, - other => other, - }, - (Bound::Excluded(a), Bound::Included(b)) => match a.cmp(&b) { - Ordering::Equal => Ordering::Less, - other => other, + match self.value().cmp(&other.value()) { + Ordering::Equal => match (self, other) { + // End excluded before start and end included + (Kind::End(EndBound::Excluded(_), _), Kind::Start(..) | Kind::End(EndBound::Included(_), _)) => { + Ordering::Less + }, + (Kind::Start(..) | Kind::End(EndBound::Included(_), _), Kind::End(EndBound::Excluded(_), _)) => { + Ordering::Greater + }, + + // Start before end included + (Kind::Start(..), Kind::End(EndBound::Included(_), _)) => Ordering::Less, + (Kind::End(EndBound::Included(_), _), Kind::Start(..)) => Ordering::Greater, + + _ => Ordering::Equal, }, + other => other, } } } @@ -2224,29 +2218,29 @@ fn test_overlapping() { }; assert_eq!(None, overlapping::(&[])); - assert_eq!(None, overlapping(&[sp(1, Bound::Included(4))])); + assert_eq!(None, overlapping(&[sp(1, EndBound::Included(4))])); assert_eq!( None, - overlapping(&[sp(1, Bound::Included(4)), sp(5, Bound::Included(6))]) + overlapping(&[sp(1, EndBound::Included(4)), sp(5, EndBound::Included(6))]) ); assert_eq!( None, overlapping(&[ - sp(1, Bound::Included(4)), - sp(5, Bound::Included(6)), - sp(10, Bound::Included(11)) + sp(1, EndBound::Included(4)), + sp(5, EndBound::Included(6)), + sp(10, EndBound::Included(11)) ],) ); assert_eq!( - Some((&sp(1, Bound::Included(4)), &sp(3, Bound::Included(6)))), - overlapping(&[sp(1, Bound::Included(4)), sp(3, Bound::Included(6))]) + Some((&sp(1, EndBound::Included(4)), &sp(3, EndBound::Included(6)))), + overlapping(&[sp(1, EndBound::Included(4)), sp(3, EndBound::Included(6))]) ); assert_eq!( - Some((&sp(5, Bound::Included(6)), &sp(6, Bound::Included(11)))), + Some((&sp(5, EndBound::Included(6)), &sp(6, EndBound::Included(11)))), overlapping(&[ - sp(1, Bound::Included(4)), - sp(5, Bound::Included(6)), - sp(6, Bound::Included(11)) + sp(1, EndBound::Included(4)), + sp(5, EndBound::Included(6)), + sp(6, EndBound::Included(11)) ],) ); } From f829523340a84624b07a929b5092a5b996f090a6 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 9 Nov 2021 05:44:02 +0200 Subject: [PATCH 3/6] Simplify range comparison code Reword the `Kind` type so that the `cmp` function is simpler. --- clippy_lints/src/matches.rs | 67 ++++++++++++++----------------------- 1 file changed, 25 insertions(+), 42 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 7a528cb0b..cedf0777a 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1698,75 +1698,58 @@ pub fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, & where T: Copy + Ord, { + #[derive(Copy, Clone, Debug, Eq, Ord, PartialEq, PartialOrd)] + enum BoundKind { + EndExcluded, + Start, + EndIncluded, + } + #[derive(Copy, Clone, Debug, Eq, PartialEq)] - enum Kind<'a, T> { - Start(T, &'a SpannedRange), - End(EndBound, &'a SpannedRange), - } + struct RangeBound<'a, T>(T, BoundKind, &'a SpannedRange); - impl<'a, T: Copy> Kind<'a, T> { - fn value(self) -> T { - match self { - Kind::Start(t, _) | Kind::End(EndBound::Included(t) | EndBound::Excluded(t), _) => t, - } - } - } - - impl<'a, T: Copy + Ord> PartialOrd for Kind<'a, T> { + impl<'a, T: Copy + Ord> PartialOrd for RangeBound<'a, T> { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) } } - impl<'a, T: Copy + Ord> Ord for Kind<'a, T> { - fn cmp(&self, other: &Self) -> Ordering { - match self.value().cmp(&other.value()) { - Ordering::Equal => match (self, other) { - // End excluded before start and end included - (Kind::End(EndBound::Excluded(_), _), Kind::Start(..) | Kind::End(EndBound::Included(_), _)) => { - Ordering::Less - }, - (Kind::Start(..) | Kind::End(EndBound::Included(_), _), Kind::End(EndBound::Excluded(_), _)) => { - Ordering::Greater - }, - - // Start before end included - (Kind::Start(..), Kind::End(EndBound::Included(_), _)) => Ordering::Less, - (Kind::End(EndBound::Included(_), _), Kind::Start(..)) => Ordering::Greater, - - _ => Ordering::Equal, - }, - other => other, - } + impl<'a, T: Copy + Ord> Ord for RangeBound<'a, T> { + fn cmp(&self, RangeBound(other_value, other_kind, _): &Self) -> Ordering { + let RangeBound(self_value, self_kind, _) = *self; + (self_value, self_kind).cmp(&(*other_value, *other_kind)) } } let mut values = Vec::with_capacity(2 * ranges.len()); - for r in ranges { - values.push(Kind::Start(r.node.0, r)); - values.push(Kind::End(r.node.1, r)); + for r @ SpannedRange { node: (start, end), .. } in ranges { + values.push(RangeBound(*start, BoundKind::Start, r)); + values.push(match end { + EndBound::Excluded(val) => RangeBound(*val, BoundKind::EndExcluded, r), + EndBound::Included(val) => RangeBound(*val, BoundKind::EndIncluded, r), + }); } values.sort(); let mut started = vec![]; - for value in values { - match value { - Kind::Start(_, r) => started.push(r), - Kind::End(_, er) => { + for RangeBound(_, kind, r) in values { + match kind { + BoundKind::Start => started.push(r), + BoundKind::EndExcluded | BoundKind::EndIncluded => { let mut overlap = None; while let Some(sr) = started.pop() { - if sr == er { + if sr == r { break; } overlap = Some(sr); } if let Some(sr) = overlap { - return Some((er, sr)); + return Some((r, sr)); } }, } From c8f909ee54f74b5fefa0d16afe92188368c9ccac Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 9 Nov 2021 05:44:02 +0200 Subject: [PATCH 4/6] Improve variable naming --- clippy_lints/src/matches.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index cedf0777a..206cf8928 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1735,21 +1735,21 @@ where let mut started = vec![]; - for RangeBound(_, kind, r) in values { + for RangeBound(_, kind, range) in values { match kind { - BoundKind::Start => started.push(r), + BoundKind::Start => started.push(range), BoundKind::EndExcluded | BoundKind::EndIncluded => { let mut overlap = None; - while let Some(sr) = started.pop() { - if sr == r { + while let Some(last_started) = started.pop() { + if last_started == range { break; } - overlap = Some(sr); + overlap = Some(last_started); } - if let Some(sr) = overlap { - return Some((r, sr)); + if let Some(first_overlapping) = overlap { + return Some((range, first_overlapping)); } }, } From 81fa75835652040f2773c44b34830a1107c07550 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 9 Nov 2021 05:44:02 +0200 Subject: [PATCH 5/6] Improve variable naming 2 --- clippy_lints/src/matches.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 206cf8928..97a19d55c 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1601,17 +1601,17 @@ fn all_ranges<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'_>], ty: Ty<'tcx>) .filter_map(|arm| { if let Arm { pat, guard: None, .. } = *arm { if let PatKind::Range(ref lhs, ref rhs, range_end) = pat.kind { - let lhs = match lhs { + let lhs_const = match lhs { Some(lhs) => constant(cx, cx.typeck_results(), lhs)?.0, None => miri_to_const(ty.numeric_min_val(cx.tcx)?)?, }; - let rhs = match rhs { + let rhs_const = match rhs { Some(rhs) => constant(cx, cx.typeck_results(), rhs)?.0, None => miri_to_const(ty.numeric_max_val(cx.tcx)?)?, }; - let lhs_val = lhs.int_value(cx, ty)?; - let rhs_val = rhs.int_value(cx, ty)?; + let lhs_val = lhs_const.int_value(cx, ty)?; + let rhs_val = rhs_const.int_value(cx, ty)?; let rhs_bound = match range_end { RangeEnd::Included => EndBound::Included(rhs_val), From 8b7691551acb5f27fe54a6b609f12c88eba78631 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Tue, 9 Nov 2021 05:44:02 +0200 Subject: [PATCH 6/6] matches: remove `pub` from some items There is no reason for these to be `pub`. They aren't used anywhere else. --- clippy_lints/src/matches.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 97a19d55c..8e170f86c 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1643,7 +1643,7 @@ pub enum EndBound { } #[derive(Debug, Eq, PartialEq)] -pub struct SpannedRange { +struct SpannedRange { pub span: Span, pub node: (T, EndBound), } @@ -1694,7 +1694,7 @@ where ref_count > 1 } -pub fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &SpannedRange)> +fn overlapping(ranges: &[SpannedRange]) -> Option<(&SpannedRange, &SpannedRange)> where T: Copy + Ord, {