Auto merge of #118879 - Nadrieril:lint-range-gap, r=<try>

bors · bors · commit 8b07d6ff4d5a · 2023-12-12T16:09:23.000Z
WIP: Lint small gaps between ranges In the discussion to stabilize exclusive range patterns (#37854), it has often come up that they're likely to cause off-by-one mistakes. We already have the `overlapping_range_endpoint` lint, so I [proposed](#37854 (comment)) the `small_gap_between_ranges` lint as a proof of concept. Here's the idea (see the test file to get a better idea): ```rust match x { 0..10 => ..., // WARN: this range has a small gap on `10_u8`... 11..20 => ..., // ... with this range. _ => ..., } // note: you likely meant to match `10_u8` too ``` For now the PR is meant as illustration so the discussion can progress. I'll make this a real lint if we end up wanting it (and if we can find a good name). r? ghost
diff --git 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
@@ -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<Self> {
         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<Self> {
         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
@@ -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
@@ -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<GappedRange<'tcx>>,
+}
+
+pub struct GappedRange<'tcx> {
+    pub span: Span,
+    pub range: Pat<'tcx>,
+}
+
+impl<'tcx> AddToDiagnostic for GappedRange<'tcx> {
+    fn add_to_diagnostic_with<F>(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
@@ -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
@@ -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
@@ -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
