From 89053d12ca28bca3552c0837f5dba846856a195f Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 12 Dec 2023 16:58:31 +0100 Subject: [PATCH] Lint small gaps between ranges --- compiler/rustc_pattern_analysis/messages.ftl | 4 + .../rustc_pattern_analysis/src/constructor.rs | 38 +++-- compiler/rustc_pattern_analysis/src/cx.rs | 6 +- compiler/rustc_pattern_analysis/src/errors.rs | 30 ++++ compiler/rustc_pattern_analysis/src/lib.rs | 20 +-- compiler/rustc_pattern_analysis/src/lints.rs | 81 +++++++--- .../integer-ranges/gap_between_ranges.rs | 57 +++++++ .../integer-ranges/gap_between_ranges.stderr | 150 ++++++++++++++++++ 8 files changed, 338 insertions(+), 48 deletions(-) create mode 100644 tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.rs create mode 100644 tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.stderr diff --git a/compiler/rustc_pattern_analysis/messages.ftl b/compiler/rustc_pattern_analysis/messages.ftl index 827928f97d7cb..8fd185b1d30e0 100644 --- a/compiler/rustc_pattern_analysis/messages.ftl +++ b/compiler/rustc_pattern_analysis/messages.ftl @@ -11,6 +11,10 @@ pattern_analysis_overlapping_range_endpoints = multiple patterns overlap on thei .label = ... with this range .note = you likely meant to write mutually exclusive ranges +pattern_analysis_small_gap_between_ranges = multiple ranges are one apart + .label = ... with this range + .note = you likely meant to match `{$range}` too + pattern_analysis_uncovered = {$count -> [1] pattern `{$witness_1}` [2] patterns `{$witness_1}` and `{$witness_2}` diff --git a/compiler/rustc_pattern_analysis/src/constructor.rs b/compiler/rustc_pattern_analysis/src/constructor.rs index 716ccdd4dcd00..67f1c2259b3c0 100644 --- a/compiler/rustc_pattern_analysis/src/constructor.rs +++ b/compiler/rustc_pattern_analysis/src/constructor.rs @@ -180,10 +180,12 @@ enum Presence { #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] pub enum MaybeInfiniteInt { NegInfinity, - /// Encoded value. DO NOT CONSTRUCT BY HAND; use `new_finite`. + /// Encoded value. DO NOT CONSTRUCT BY HAND; use `new_finite_{int,uint}`. #[non_exhaustive] Finite(u128), /// The integer after `u128::MAX`. We need it to represent `x..=u128::MAX` as an exclusive range. + // Users likely shouldn't be constructing it directly. + #[non_exhaustive] JustAfterMax, PosInfinity, } @@ -217,25 +219,22 @@ impl MaybeInfiniteInt { } /// Note: this will not turn a finite value into an infinite one or vice-versa. - pub fn minus_one(self) -> Self { + pub fn minus_one(self) -> Option { match self { - Finite(n) => match n.checked_sub(1) { - Some(m) => Finite(m), - None => bug!(), - }, - JustAfterMax => Finite(u128::MAX), - x => x, + Finite(n) => n.checked_sub(1).map(Finite), + JustAfterMax => Some(Finite(u128::MAX)), + x => Some(x), } } /// Note: this will not turn a finite value into an infinite one or vice-versa. - pub fn plus_one(self) -> Self { + pub fn plus_one(self) -> Option { match self { Finite(n) => match n.checked_add(1) { - Some(m) => Finite(m), - None => JustAfterMax, + Some(m) => Some(Finite(m)), + None => Some(JustAfterMax), }, - JustAfterMax => bug!(), - x => x, + JustAfterMax => None, + x => Some(x), } } } @@ -256,18 +255,25 @@ impl IntRange { pub(crate) fn is_singleton(&self) -> bool { // Since `lo` and `hi` can't be the same `Infinity` and `plus_one` never changes from finite // to infinite, this correctly only detects ranges that contain exacly one `Finite(x)`. - self.lo.plus_one() == self.hi + // `unwrap()` is ok since `self.lo` will never be `JustAfterMax`. + self.lo.plus_one().unwrap() == self.hi } + /// Construct a singleton range. + /// `x` be a `Finite(_)` value. #[inline] pub fn from_singleton(x: MaybeInfiniteInt) -> IntRange { - IntRange { lo: x, hi: x.plus_one() } + // `unwrap()` is ok on a finite value + IntRange { lo: x, hi: x.plus_one().unwrap() } } + /// Construct a range with these boundaries. + /// `lo` must not be `PosInfinity` or `JustAfterMax`. `hi` must not be `NegInfinity`. + /// If `end` is `Included`, `hi` must also not be `JustAfterMax`. #[inline] pub fn from_range(lo: MaybeInfiniteInt, mut hi: MaybeInfiniteInt, end: RangeEnd) -> IntRange { if end == RangeEnd::Included { - hi = hi.plus_one(); + hi = hi.plus_one().unwrap(); } if lo >= hi { // This should have been caught earlier by E0030. diff --git a/compiler/rustc_pattern_analysis/src/cx.rs b/compiler/rustc_pattern_analysis/src/cx.rs index 8a4f39a1f4abb..c81ba35bc99c4 100644 --- a/compiler/rustc_pattern_analysis/src/cx.rs +++ b/compiler/rustc_pattern_analysis/src/cx.rs @@ -635,12 +635,12 @@ impl<'p, 'tcx> MatchCheckCtxt<'p, 'tcx> { let value = mir::Const::from_ty_const(c, cx.tcx); lo = PatRangeBoundary::Finite(value); } - let hi = if matches!(range.hi, Finite(0)) { + let hi = if let Some(hi) = range.hi.minus_one() { + hi + } else { // The range encodes `..ty::MIN`, so we can't convert it to an inclusive range. end = RangeEnd::Excluded; range.hi - } else { - range.hi.minus_one() }; let hi = cx.hoist_pat_range_bdy(hi, ty); PatKind::Range(Box::new(PatRange { lo, hi, end, ty })) diff --git a/compiler/rustc_pattern_analysis/src/errors.rs b/compiler/rustc_pattern_analysis/src/errors.rs index 0efa8a0ec0845..07dd70fc25bdf 100644 --- a/compiler/rustc_pattern_analysis/src/errors.rs +++ b/compiler/rustc_pattern_analysis/src/errors.rs @@ -72,6 +72,36 @@ impl<'tcx> AddToDiagnostic for Overlap<'tcx> { } } +#[derive(LintDiagnostic)] +#[diag(pattern_analysis_small_gap_between_ranges)] +#[note] +pub struct SmallGapBetweenRanges<'tcx> { + #[label] + pub first_range: Span, + pub range: Pat<'tcx>, + #[subdiagnostic] + pub gap_with: Vec>, +} + +pub struct GappedRange<'tcx> { + pub span: Span, + pub range: Pat<'tcx>, +} + +impl<'tcx> AddToDiagnostic for GappedRange<'tcx> { + fn add_to_diagnostic_with(self, diag: &mut Diagnostic, _: F) + where + F: Fn(&mut Diagnostic, SubdiagnosticMessage) -> SubdiagnosticMessage, + { + let GappedRange { span, range } = self; + + // FIXME(mejrs) unfortunately `#[derive(LintDiagnostic)]` + // does not support `#[subdiagnostic(eager)]`... + let message = format!("this range has a small gap on `{range}`..."); + diag.span_label(span, message); + } +} + #[derive(LintDiagnostic)] #[diag(pattern_analysis_non_exhaustive_omitted_pattern)] #[help] diff --git a/compiler/rustc_pattern_analysis/src/lib.rs b/compiler/rustc_pattern_analysis/src/lib.rs index 07730aa49d3f7..60cd15accbd8f 100644 --- a/compiler/rustc_pattern_analysis/src/lib.rs +++ b/compiler/rustc_pattern_analysis/src/lib.rs @@ -17,10 +17,9 @@ rustc_fluent_macro::fluent_messages! { "../messages.ftl" } use lints::PatternColumn; use rustc_hir::HirId; use rustc_middle::ty::Ty; -use usefulness::{compute_match_usefulness, UsefulnessReport}; +use usefulness::UsefulnessReport; use crate::cx::MatchCheckCtxt; -use crate::lints::{lint_nonexhaustive_missing_variants, lint_overlapping_range_endpoints}; use crate::pat::DeconstructedPat; /// The arm of a match expression. @@ -41,15 +40,18 @@ pub fn analyze_match<'p, 'tcx>( ) -> UsefulnessReport<'p, 'tcx> { let pat_column = PatternColumn::new(arms); - let report = compute_match_usefulness(cx, arms, scrut_ty); + let report = usefulness::compute_match_usefulness(cx, arms, scrut_ty); - // Lint on ranges that overlap on their endpoints, which is likely a mistake. - lint_overlapping_range_endpoints(cx, &pat_column); + // Only run the lints if the match is exhaustive. + if report.non_exhaustiveness_witnesses.is_empty() { + // Detect possible range-related mistakes. + lints::lint_likely_range_mistakes(cx, &pat_column); - // Run the non_exhaustive_omitted_patterns lint. Only run on refutable patterns to avoid hitting - // `if let`s. Only run if the match is exhaustive otherwise the error is redundant. - if cx.refutable && report.non_exhaustiveness_witnesses.is_empty() { - lint_nonexhaustive_missing_variants(cx, arms, &pat_column, scrut_ty) + // Run the non_exhaustive_omitted_patterns lint. Only run on refutable patterns to avoid + // hitting `if let`s. + if cx.refutable { + lints::lint_nonexhaustive_missing_variants(cx, arms, &pat_column, scrut_ty) + } } report diff --git a/compiler/rustc_pattern_analysis/src/lints.rs b/compiler/rustc_pattern_analysis/src/lints.rs index 8ab559c9e7a41..44f65e64ef625 100644 --- a/compiler/rustc_pattern_analysis/src/lints.rs +++ b/compiler/rustc_pattern_analysis/src/lints.rs @@ -8,10 +8,7 @@ use rustc_span::Span; use crate::constructor::{Constructor, IntRange, MaybeInfiniteInt, SplitConstructorSet}; use crate::cx::MatchCheckCtxt; -use crate::errors::{ - NonExhaustiveOmittedPattern, NonExhaustiveOmittedPatternLintOnArm, Overlap, - OverlappingRangeEndpoints, Uncovered, -}; +use crate::errors; use crate::pat::{DeconstructedPat, WitnessPat}; use crate::usefulness::PatCtxt; use crate::MatchArm; @@ -184,9 +181,9 @@ pub(crate) fn lint_nonexhaustive_missing_variants<'p, 'tcx>( NON_EXHAUSTIVE_OMITTED_PATTERNS, cx.match_lint_level, cx.scrut_span, - NonExhaustiveOmittedPattern { + errors::NonExhaustiveOmittedPattern { scrut_ty, - uncovered: Uncovered::new(cx.scrut_span, cx, witnesses), + uncovered: errors::Uncovered::new(cx.scrut_span, cx, witnesses), }, ); } @@ -198,7 +195,7 @@ pub(crate) fn lint_nonexhaustive_missing_variants<'p, 'tcx>( let (lint_level, lint_level_source) = cx.tcx.lint_level_at_node(NON_EXHAUSTIVE_OMITTED_PATTERNS, arm.hir_id); if !matches!(lint_level, rustc_session::lint::Level::Allow) { - let decorator = NonExhaustiveOmittedPatternLintOnArm { + let decorator = errors::NonExhaustiveOmittedPatternLintOnArm { lint_span: lint_level_source.span(), suggest_lint_on_match: cx.whole_match_span.map(|span| span.shrink_to_lo()), lint_level: lint_level.as_str(), @@ -215,9 +212,10 @@ pub(crate) fn lint_nonexhaustive_missing_variants<'p, 'tcx>( } } -/// Traverse the patterns to warn the user about ranges that overlap on their endpoints. +/// Traverse the patterns to warn the user about ranges that overlap on their endpoints or are +/// distant by one. #[instrument(level = "debug", skip(cx))] -pub(crate) fn lint_overlapping_range_endpoints<'p, 'tcx>( +pub(crate) fn lint_likely_range_mistakes<'p, 'tcx>( cx: &MatchCheckCtxt<'p, 'tcx>, column: &PatternColumn<'p, 'tcx>, ) { @@ -229,24 +227,24 @@ pub(crate) fn lint_overlapping_range_endpoints<'p, 'tcx>( let set = column.analyze_ctors(pcx); if matches!(ty.kind(), ty::Char | ty::Int(_) | ty::Uint(_)) { - let emit_lint = |overlap: &IntRange, this_span: Span, overlapped_spans: &[Span]| { + let emit_overlap_lint = |overlap: &IntRange, this_span: Span, overlapped_spans: &[Span]| { let overlap_as_pat = cx.hoist_pat_range(overlap, ty); let overlaps: Vec<_> = overlapped_spans .iter() .copied() - .map(|span| Overlap { range: overlap_as_pat.clone(), span }) + .map(|span| errors::Overlap { range: overlap_as_pat.clone(), span }) .collect(); cx.tcx.emit_spanned_lint( lint::builtin::OVERLAPPING_RANGE_ENDPOINTS, cx.match_lint_level, this_span, - OverlappingRangeEndpoints { overlap: overlaps, range: this_span }, + errors::OverlappingRangeEndpoints { overlap: overlaps, range: this_span }, ); }; - // If two ranges overlapped, the split set will contain their intersection as a singleton. - let split_int_ranges = set.present.iter().filter_map(|c| c.as_int_range()); - for overlap_range in split_int_ranges.clone() { + // The two cases we are interested in will show up as a singleton after range splitting. + let present_int_ranges = set.present.iter().filter_map(|c| c.as_int_range()); + for overlap_range in present_int_ranges { if overlap_range.is_singleton() { let overlap: MaybeInfiniteInt = overlap_range.lo; // Ranges that look like `lo..=overlap`. @@ -261,29 +259,72 @@ pub(crate) fn lint_overlapping_range_endpoints<'p, 'tcx>( // Don't lint when one of the ranges is a singleton. continue; } - if this_range.lo == overlap { + if overlap == this_range.lo { // `this_range` looks like `overlap..=this_range.hi`; it overlaps with any // ranges that look like `lo..=overlap`. if !prefixes.is_empty() { - emit_lint(overlap_range, this_span, &prefixes); + emit_overlap_lint(overlap_range, this_span, &prefixes); } suffixes.push(this_span) - } else if this_range.hi == overlap.plus_one() { + } else if overlap.plus_one() == Some(this_range.hi) { // `this_range` looks like `this_range.lo..=overlap`; it overlaps with any // ranges that look like `overlap..=hi`. if !suffixes.is_empty() { - emit_lint(overlap_range, this_span, &suffixes); + emit_overlap_lint(overlap_range, this_span, &suffixes); } prefixes.push(this_span) } } } } + + let missing_int_ranges = set.missing.iter().filter_map(|c| c.as_int_range()); + for point_range in missing_int_ranges { + if point_range.is_singleton() { + let point: MaybeInfiniteInt = point_range.lo; + // Ranges that look like `lo..point`. + let mut onebefore: SmallVec<[_; 1]> = Default::default(); + // Ranges that look like `point+1..=hi`. + let mut oneafter: SmallVec<[_; 1]> = Default::default(); + for pat in column.iter() { + let this_span = pat.span(); + let Constructor::IntRange(this_range) = pat.ctor() else { continue }; + + if point == this_range.hi { + onebefore.push(this_span) + } else if point.plus_one() == Some(this_range.lo) { + oneafter.push(this_span) + } + } + + if !onebefore.is_empty() && !oneafter.is_empty() { + // We have some `lo..point` and some `point+1..hi` but no `point`. + let point_as_pat = cx.hoist_pat_range(point_range, ty); + for span_after in oneafter { + let spans_before: Vec<_> = onebefore + .iter() + .copied() + .map(|span| errors::GappedRange { range: point_as_pat.clone(), span }) + .collect(); + cx.tcx.emit_spanned_lint( + lint::builtin::OVERLAPPING_RANGE_ENDPOINTS, + cx.match_lint_level, + span_after, + errors::SmallGapBetweenRanges { + range: point_as_pat.clone(), + first_range: span_after, + gap_with: spans_before, + }, + ); + } + } + } + } } else { // Recurse into the fields. for ctor in set.present { for col in column.specialize(pcx, &ctor) { - lint_overlapping_range_endpoints(cx, &col); + lint_likely_range_mistakes(cx, &col); } } } diff --git a/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.rs b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.rs new file mode 100644 index 0000000000000..09538ace00568 --- /dev/null +++ b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.rs @@ -0,0 +1,57 @@ +#![feature(exclusive_range_pattern)] +#![deny(overlapping_range_endpoints)] + +macro_rules! m { + ($s:expr, $t1:pat, $t2:pat) => { + match $s { + $t1 => {} + $t2 => {} + _ => {} + } + }; +} + +fn main() { + m!(0u8, 20..30, 31..=40); //~ ERROR multiple ranges are one apart + m!(0u8, 31..=40, 20..30); //~ ERROR multiple ranges are one apart + m!(0u8, 20..30, 29..=40); //~ ERROR multiple patterns overlap on their endpoints + m!(0u8, 20..30, 30..=40); + m!(0u8, 20..30, 31..=40); //~ ERROR multiple ranges are one apart + m!(0u8, 20..30, 32..=40); + m!(0u8, 20..30, 31); //~ ERROR multiple ranges are one apart + m!(0u8, 20..30, 31..=32); //~ ERROR multiple ranges are one apart + m!(0u8, 30, 30..=40); + m!(0u8, 30, 31..=40); + m!(0u8, 30, 32..=40); //~ ERROR multiple ranges are one apart + + match 0u8 { + 0..10 => {} + 10 => {} + 11..20 => {} + _ => {} + } + + match 0u8 { + 0..10 => {} + 21..30 => {} //~ ERROR multiple ranges are one apart + 11..20 => {} //~ ERROR multiple ranges are one apart + _ => {} + } + match (0u8, true) { + (0..10, true) => {} + (11..20, true) => {} //~ ERROR multiple ranges are one apart + (11..20, false) => {} //~ ERROR multiple ranges are one apart + _ => {} + } + match (true, 0u8) { + (true, 0..10) => {} + (true, 11..20) => {} //~ ERROR multiple ranges are one apart + (false, 11..20) => {} //~ ERROR multiple ranges are one apart + _ => {} + } + match Some(0u8) { + Some(0..10) => {} + Some(11..20) => {} //~ ERROR multiple ranges are one apart + _ => {} + } +} diff --git a/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.stderr b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.stderr new file mode 100644 index 0000000000000..13fc4a5ba5a0c --- /dev/null +++ b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.stderr @@ -0,0 +1,150 @@ +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:15:21 + | +LL | m!(0u8, 20..30, 31..=40); + | ------ ^^^^^^^ ... with this range + | | + | this range has a small gap on `30_u8`... + | + = note: you likely meant to match `30_u8` too +note: the lint level is defined here + --> $DIR/gap_between_ranges.rs:2:9 + | +LL | #![deny(overlapping_range_endpoints)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:16:13 + | +LL | m!(0u8, 31..=40, 20..30); + | ^^^^^^^ ------ this range has a small gap on `30_u8`... + | | + | ... with this range + | + = note: you likely meant to match `30_u8` too + +error: multiple patterns overlap on their endpoints + --> $DIR/gap_between_ranges.rs:17:21 + | +LL | m!(0u8, 20..30, 29..=40); + | ------ ^^^^^^^ ... with this range + | | + | this range overlaps on `29_u8`... + | + = note: you likely meant to write mutually exclusive ranges + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:19:21 + | +LL | m!(0u8, 20..30, 31..=40); + | ------ ^^^^^^^ ... with this range + | | + | this range has a small gap on `30_u8`... + | + = note: you likely meant to match `30_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:21:21 + | +LL | m!(0u8, 20..30, 31); + | ------ ^^ ... with this range + | | + | this range has a small gap on `30_u8`... + | + = note: you likely meant to match `30_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:22:21 + | +LL | m!(0u8, 20..30, 31..=32); + | ------ ^^^^^^^ ... with this range + | | + | this range has a small gap on `30_u8`... + | + = note: you likely meant to match `30_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:25:17 + | +LL | m!(0u8, 30, 32..=40); + | -- ^^^^^^^ ... with this range + | | + | this range has a small gap on `31_u8`... + | + = note: you likely meant to match `31_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:37:9 + | +LL | 0..10 => {} + | ----- this range has a small gap on `10_u8`... +LL | 21..30 => {} +LL | 11..20 => {} + | ^^^^^^ ... with this range + | + = note: you likely meant to match `10_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:36:9 + | +LL | 21..30 => {} + | ^^^^^^ ... with this range +LL | 11..20 => {} + | ------ this range has a small gap on `20_u8`... + | + = note: you likely meant to match `20_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:42:10 + | +LL | (0..10, true) => {} + | ----- this range has a small gap on `10_u8`... +LL | (11..20, true) => {} + | ^^^^^^ ... with this range + | + = note: you likely meant to match `10_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:43:10 + | +LL | (0..10, true) => {} + | ----- this range has a small gap on `10_u8`... +LL | (11..20, true) => {} +LL | (11..20, false) => {} + | ^^^^^^ ... with this range + | + = note: you likely meant to match `10_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:48:16 + | +LL | (true, 0..10) => {} + | ----- this range has a small gap on `10_u8`... +LL | (true, 11..20) => {} + | ^^^^^^ ... with this range + | + = note: you likely meant to match `10_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:49:17 + | +LL | (true, 0..10) => {} + | ----- this range has a small gap on `10_u8`... +LL | (true, 11..20) => {} +LL | (false, 11..20) => {} + | ^^^^^^ ... with this range + | + = note: you likely meant to match `10_u8` too + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:54:14 + | +LL | Some(0..10) => {} + | ----- this range has a small gap on `10_u8`... +LL | Some(11..20) => {} + | ^^^^^^ ... with this range + | + = note: you likely meant to match `10_u8` too + +error: aborting due to 14 previous errors +