From bbf67c3424cb76ce7ae92e012f1bf23c1161eea7 Mon Sep 17 00:00:00 2001 From: Alex Macleod Date: Mon, 4 Sep 2023 14:08:47 +0000 Subject: [PATCH] Preserve literals and range kinds in `manual_range_patterns` --- clippy_lints/src/manual_range_patterns.rs | 129 ++++++++++++++-------- tests/ui/manual_range_patterns.fixed | 17 ++- tests/ui/manual_range_patterns.rs | 11 ++ tests/ui/manual_range_patterns.stderr | 66 ++++++++++- 4 files changed, 171 insertions(+), 52 deletions(-) diff --git a/clippy_lints/src/manual_range_patterns.rs b/clippy_lints/src/manual_range_patterns.rs index 39d8b20d38d..90557b55560 100644 --- a/clippy_lints/src/manual_range_patterns.rs +++ b/clippy_lints/src/manual_range_patterns.rs @@ -1,4 +1,5 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet_opt; use rustc_ast::LitKind; use rustc_data_structures::fx::FxHashSet; use rustc_errors::Applicability; @@ -6,6 +7,7 @@ use rustc_hir::{Expr, ExprKind, PatKind, RangeEnd, UnOp}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{Span, DUMMY_SP}; declare_clippy_lint! { /// ### What it does @@ -49,6 +51,29 @@ fn expr_as_i128(expr: &Expr<'_>) -> Option { } } +#[derive(Copy, Clone)] +struct Num { + val: i128, + span: Span, +} + +impl Num { + fn new(expr: &Expr<'_>) -> Option { + Some(Self { + val: expr_as_i128(expr)?, + span: expr.span, + }) + } + + fn dummy(val: i128) -> Self { + Self { val, span: DUMMY_SP } + } + + fn min(self, other: Self) -> Self { + if self.val < other.val { self } else { other } + } +} + impl LateLintPass<'_> for ManualRangePatterns { fn check_pat(&mut self, cx: &LateContext<'_>, pat: &'_ rustc_hir::Pat<'_>) { if in_external_macro(cx.sess(), pat.span) { @@ -56,71 +81,83 @@ impl LateLintPass<'_> for ManualRangePatterns { } // a pattern like 1 | 2 seems fine, lint if there are at least 3 alternatives + // or at least one range if let PatKind::Or(pats) = pat.kind - && pats.len() >= 3 + && (pats.len() >= 3 || pats.iter().any(|p| matches!(p.kind, PatKind::Range(..)))) { - let mut min = i128::MAX; - let mut max = i128::MIN; + let mut min = Num::dummy(i128::MAX); + let mut max = Num::dummy(i128::MIN); + let mut range_kind = RangeEnd::Included; let mut numbers_found = FxHashSet::default(); let mut ranges_found = Vec::new(); for pat in pats { if let PatKind::Lit(lit) = pat.kind - && let Some(num) = expr_as_i128(lit) + && let Some(num) = Num::new(lit) { - numbers_found.insert(num); + numbers_found.insert(num.val); min = min.min(num); - max = max.max(num); + if num.val >= max.val { + max = num; + range_kind = RangeEnd::Included; + } } else if let PatKind::Range(Some(left), Some(right), end) = pat.kind - && let Some(left) = expr_as_i128(left) - && let Some(right) = expr_as_i128(right) - && right >= left + && let Some(left) = Num::new(left) + && let Some(mut right) = Num::new(right) { + if let RangeEnd::Excluded = end { + right.val -= 1; + } + min = min.min(left); - max = max.max(right); - ranges_found.push(left..=match end { - RangeEnd::Included => right, - RangeEnd::Excluded => right - 1, - }); + if right.val > max.val { + max = right; + range_kind = end; + } + ranges_found.push(left.val..=right.val); } else { return; } } - let contains_whole_range = 'contains: { - let mut num = min; - while num <= max { - if numbers_found.contains(&num) { - num += 1; - } - // Given a list of (potentially overlapping) ranges like: - // 1..=5, 3..=7, 6..=10 - // We want to find the range with the highest end that still contains the current number - else if let Some(range) = ranges_found - .iter() - .filter(|range| range.contains(&num)) - .max_by_key(|range| range.end()) - { - num = range.end() + 1; - } else { - break 'contains false; - } + let mut num = min.val; + while num <= max.val { + if numbers_found.contains(&num) { + num += 1; + } + // Given a list of (potentially overlapping) ranges like: + // 1..=5, 3..=7, 6..=10 + // We want to find the range with the highest end that still contains the current number + else if let Some(range) = ranges_found + .iter() + .filter(|range| range.contains(&num)) + .max_by_key(|range| range.end()) + { + num = range.end() + 1; + } else { + return; } - break 'contains true; - }; - - if contains_whole_range { - span_lint_and_sugg( - cx, - MANUAL_RANGE_PATTERNS, - pat.span, - "this OR pattern can be rewritten using a range", - "try", - format!("{min}..={max}"), - Applicability::MachineApplicable, - ); } + + span_lint_and_then( + cx, + MANUAL_RANGE_PATTERNS, + pat.span, + "this OR pattern can be rewritten using a range", + |diag| { + if let Some(min) = snippet_opt(cx, min.span) + && let Some(max) = snippet_opt(cx, max.span) + { + diag.span_suggestion( + pat.span, + "try", + format!("{min}{range_kind}{max}"), + Applicability::MachineApplicable, + ); + } + }, + ); } } } diff --git a/tests/ui/manual_range_patterns.fixed b/tests/ui/manual_range_patterns.fixed index ea06e27d153..b348d7071f6 100644 --- a/tests/ui/manual_range_patterns.fixed +++ b/tests/ui/manual_range_patterns.fixed @@ -13,8 +13,8 @@ fn main() { let _ = matches!(f, 1 | 2147483647); let _ = matches!(f, 0 | 2147483647); let _ = matches!(f, -2147483647 | 2147483647); - let _ = matches!(f, 1 | (2..=4)); - let _ = matches!(f, 1 | (2..4)); + let _ = matches!(f, 1..=4); + let _ = matches!(f, 1..4); let _ = matches!(f, 1..=48324729); let _ = matches!(f, 0..=48324730); let _ = matches!(f, 0..=3); @@ -25,9 +25,20 @@ fn main() { }; let _ = matches!(f, -5..=3); let _ = matches!(f, -1 | -5 | 3 | -2 | -4 | -3 | 0 | 1); // 2 is missing - let _ = matches!(f, -1000001..=1000001); + let _ = matches!(f, -1_000_001..=1_000_001); let _ = matches!(f, -1_000_000..=1_000_000 | -1_000_001 | 1_000_002); + matches!(f, 0x00..=0x03); + matches!(f, 0x00..=0x07); + matches!(f, -0x09..=0x00); + + matches!(f, 0..=5); + matches!(f, 0..5); + + matches!(f, 0..10); + matches!(f, 0..=10); + matches!(f, 0..=10); + macro_rules! mac { ($e:expr) => { matches!($e, 1..=10) diff --git a/tests/ui/manual_range_patterns.rs b/tests/ui/manual_range_patterns.rs index eb29987b89f..a0750f54b73 100644 --- a/tests/ui/manual_range_patterns.rs +++ b/tests/ui/manual_range_patterns.rs @@ -28,6 +28,17 @@ fn main() { let _ = matches!(f, -1_000_000..=1_000_000 | -1_000_001 | 1_000_001); let _ = matches!(f, -1_000_000..=1_000_000 | -1_000_001 | 1_000_002); + matches!(f, 0x00 | 0x01 | 0x02 | 0x03); + matches!(f, 0x00..=0x05 | 0x06 | 0x07); + matches!(f, -0x09 | -0x08 | -0x07..=0x00); + + matches!(f, 0..5 | 5); + matches!(f, 0 | 1..5); + + matches!(f, 0..=5 | 6..10); + matches!(f, 0..5 | 5..=10); + matches!(f, 5..=10 | 0..5); + macro_rules! mac { ($e:expr) => { matches!($e, 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10) diff --git a/tests/ui/manual_range_patterns.stderr b/tests/ui/manual_range_patterns.stderr index 1cf58d7df6a..60ee75d1b8e 100644 --- a/tests/ui/manual_range_patterns.stderr +++ b/tests/ui/manual_range_patterns.stderr @@ -12,6 +12,18 @@ error: this OR pattern can be rewritten using a range LL | let _ = matches!(f, 4 | 2 | 3 | 1 | 5 | 6 | 9 | 7 | 8 | 10); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `1..=10` +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:16:25 + | +LL | let _ = matches!(f, 1 | (2..=4)); + | ^^^^^^^^^^^ help: try: `1..=4` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:17:25 + | +LL | let _ = matches!(f, 1 | (2..4)); + | ^^^^^^^^^^ help: try: `1..4` + error: this OR pattern can be rewritten using a range --> $DIR/manual_range_patterns.rs:18:25 | @@ -46,10 +58,58 @@ error: this OR pattern can be rewritten using a range --> $DIR/manual_range_patterns.rs:28:25 | LL | let _ = matches!(f, -1_000_000..=1_000_000 | -1_000_001 | 1_000_001); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `-1000001..=1000001` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `-1_000_001..=1_000_001` error: this OR pattern can be rewritten using a range - --> $DIR/manual_range_patterns.rs:33:26 + --> $DIR/manual_range_patterns.rs:31:17 + | +LL | matches!(f, 0x00 | 0x01 | 0x02 | 0x03); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `0x00..=0x03` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:32:17 + | +LL | matches!(f, 0x00..=0x05 | 0x06 | 0x07); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `0x00..=0x07` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:33:17 + | +LL | matches!(f, -0x09 | -0x08 | -0x07..=0x00); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `-0x09..=0x00` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:35:17 + | +LL | matches!(f, 0..5 | 5); + | ^^^^^^^^ help: try: `0..=5` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:36:17 + | +LL | matches!(f, 0 | 1..5); + | ^^^^^^^^ help: try: `0..5` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:38:17 + | +LL | matches!(f, 0..=5 | 6..10); + | ^^^^^^^^^^^^^ help: try: `0..10` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:39:17 + | +LL | matches!(f, 0..5 | 5..=10); + | ^^^^^^^^^^^^^ help: try: `0..=10` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:40:17 + | +LL | matches!(f, 5..=10 | 0..5); + | ^^^^^^^^^^^^^ help: try: `0..=10` + +error: this OR pattern can be rewritten using a range + --> $DIR/manual_range_patterns.rs:44:26 | LL | matches!($e, 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `1..=10` @@ -59,5 +119,5 @@ LL | mac!(f); | = note: this error originates in the macro `mac` (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 9 previous errors +error: aborting due to 19 previous errors