Lint small gaps between ranges

Author: Nadrieril

compiler/rustc_lint_defs/src/builtin.rs

@@ -87,6 +87,7 @@ declare_lint_pass! {
         RUST_2021_PRELUDE_COLLISIONS,
         SEMICOLON_IN_EXPRESSIONS_FROM_MACROS,
         SINGLE_USE_LIFETIMES,
+        SMALL_GAPS_BETWEEN_RANGES,
         SOFT_UNSTABLE,
         STABLE_FEATURES,
         SUSPICIOUS_AUTO_TRAIT_IMPLS,
@@ -835,6 +836,36 @@ declare_lint! {
     "detects range patterns with overlapping endpoints"
 }
 
+declare_lint! {
+    /// The `small_gaps_between_ranges` lint detects `match` expressions that use [range patterns]
+    /// that skip over a single number.
+    ///
+    /// [range patterns]: https://doc.rust-lang.org/nightly/reference/patterns.html#range-patterns
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// # #![feature(exclusive_range_pattern)]
+    /// let x = 123u32;
+    /// match x {
+    ///     0..100 => { println!("small"); }
+    ///     101..1000 => { println!("large"); }
+    ///     _ => { println!("larger"); }
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// It is likely a mistake to have range patterns in a match expression that miss out a single
+    /// number. Check that the beginning and end values are what you expect, and keep in mind that
+    /// with `..=` the right bound is inclusive, and with `..` it is exclusive.
+    pub SMALL_GAPS_BETWEEN_RANGES,
+    Warn,
+    "detects range patterns separated by a single number"
+}
+
 declare_lint! {
     /// The `bindings_with_variant_name` lint detects pattern bindings with
     /// the same name as one of the matched variants.

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 = this range doesn't match `{$gap}` because `..` is a non-inclusive range
+    .suggestion = use an inclusive range instead
+
 pattern_analysis_uncovered = {$count ->
         [1] pattern `{$witness_1}`
         [2] patterns `{$witness_1}` and `{$witness_2}`

compiler/rustc_pattern_analysis/src/constructor.rs

@@ -193,7 +193,7 @@ impl fmt::Display for RangeEnd {
 #[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.
@@ -230,25 +230,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 => panic!("Called `MaybeInfiniteInt::minus_one` on 0"),
-            },
-            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 => panic!("Called `MaybeInfiniteInt::plus_one` on u128::MAX+1"),
-            x => x,
+            JustAfterMax => None,
+            x => Some(x),
         }
     }
 }
@@ -269,18 +266,25 @@ impl IntRange {
     pub 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.

compiler/rustc_pattern_analysis/src/errors.rs

@@ -72,6 +72,43 @@ impl<'tcx> AddToDiagnostic for Overlap<'tcx> {
     }
 }
 
+#[derive(LintDiagnostic)]
+#[diag(pattern_analysis_small_gap_between_ranges)]
+pub struct SmallGapBetweenRanges<'tcx> {
+    #[label]
+    #[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")]
+    /// This is an exclusive range that looks like `lo..gap` (i.e. doesn't match `gap`).
+    pub first_range: Span,
+    /// Suggest `lo..=gap` instead.
+    pub suggestion: String,
+    pub gap: Pat<'tcx>,
+    #[subdiagnostic]
+    /// All these ranges skipped over `gap` which we think is probably a mistake.
+    pub gap_with: Vec<GappedRange<'tcx>>,
+}
+
+pub struct GappedRange<'tcx> {
+    pub span: Span,
+    pub gap: Pat<'tcx>,
+    pub first_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, gap, first_range } = self;
+
+        // FIXME(mejrs) unfortunately `#[derive(LintDiagnostic)]`
+        // does not support `#[subdiagnostic(eager)]`...
+        let message = format!(
+            "this seems to continue range `{first_range}`, but `{gap}` isn't matched by either of them"
+        );
+        diag.span_label(span, message);
+    }
+}
+
 #[derive(LintDiagnostic)]
 #[diag(pattern_analysis_non_exhaustive_omitted_pattern)]
 #[help]

compiler/rustc_pattern_analysis/src/lib.rs

@@ -26,15 +26,7 @@ use rustc_index::Idx;
 use rustc_middle::ty::Ty;
 
 use crate::constructor::{Constructor, ConstructorSet};
-#[cfg(feature = "rustc")]
-use crate::lints::{
-    lint_nonexhaustive_missing_variants, lint_overlapping_range_endpoints, PatternColumn,
-};
 use crate::pat::DeconstructedPat;
-#[cfg(feature = "rustc")]
-use crate::rustc::RustcMatchCheckCtxt;
-#[cfg(feature = "rustc")]
-use crate::usefulness::{compute_match_usefulness, ValidityConstraint};
 
 // It's not possible to only enable the `typed_arena` dependency when the `rustc` feature is off, so
 // we use another feature instead. The crate won't compile if one of these isn't enabled.
@@ -107,27 +99,33 @@ pub struct MatchArm<'p, Cx: TypeCx> {
 /// The entrypoint for this crate. Computes whether a match is exhaustive and which of its arms are
 /// useful, and runs some lints.
 #[cfg(feature = "rustc")]
-pub fn analyze_match<'p, 'thir, 'tcx>(
-    tycx: &RustcMatchCheckCtxt<'p, 'thir, 'tcx>,
+pub fn analyze_match<'p, 'thir, 'tcx: 'thir>(
+    tycx: &rustc::RustcMatchCheckCtxt<'p, 'thir, 'tcx>,
     arms: &[rustc::MatchArm<'p, 'thir, 'tcx>],
     scrut_ty: Ty<'tcx>,
 ) -> rustc::UsefulnessReport<'p, 'thir, 'tcx> {
+    use crate::lints::PatternColumn;
+    use crate::usefulness::ValidityConstraint;
+
     // Arena to store the extra wildcards we construct during analysis.
     let wildcard_arena = tycx.pattern_arena;
     let scrut_validity = ValidityConstraint::from_bool(tycx.known_valid_scrutinee);
     let cx = MatchCtxt { tycx, wildcard_arena };
 
-    let report = compute_match_usefulness(cx, arms, scrut_ty, scrut_validity);
+    let report = usefulness::compute_match_usefulness(cx, arms, scrut_ty, scrut_validity);
 
-    let pat_column = PatternColumn::new(arms);
+    // Only run the lints if the match is exhaustive.
+    if report.non_exhaustiveness_witnesses.is_empty() {
+        let pat_column = PatternColumn::new(arms);
 
-    // Lint on ranges that overlap on their endpoints, which is likely a mistake.
-    lint_overlapping_range_endpoints(cx, &pat_column);
+        // 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 tycx.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 tycx.refutable {
+            lints::lint_nonexhaustive_missing_variants(cx, arms, &pat_column, scrut_ty)
+        }
     }
 
     report