Auto merge of #118879 - Nadrieril:lint-range-gap, r=estebank

Lint singleton gaps after exclusive 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_endpoints` lint, so I [proposed](#37854 (comment)) a lint to catch the complementary mistake.

This PR adds a new `non_contiguous_range_endpoints` lint that catches likely off-by-one errors with exclusive range patterns. Here's the idea (see the test file for more examples):
```rust
match x {
    0..10 => ..., // WARN: this range doesn't match `10_u8` because `..` is an exclusive range
    11..20 => ..., // this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them
    _ => ...,
}
// help: use an inclusive range instead: `0_u8..=10_u8`
```

More precisely: for any exclusive range `lo..hi`, if `hi+1` is matched by another range but `hi` isn't, we suggest writing an inclusive range `lo..=hi` instead. We also catch `lo..T::MAX`.

Author: bors

compiler/rustc_lint_defs/src/builtin.rs

@@ -67,6 +67,7 @@ declare_lint_pass! {
         MISSING_FRAGMENT_SPECIFIER,
         MUST_NOT_SUSPEND,
         NAMED_ARGUMENTS_USED_POSITIONALLY,
+        NON_CONTIGUOUS_RANGE_ENDPOINTS,
         NON_EXHAUSTIVE_OMITTED_PATTERNS,
         ORDER_DEPENDENT_TRAIT_OBJECTS,
         OVERLAPPING_RANGE_ENDPOINTS,
@@ -812,6 +813,36 @@ declare_lint! {
     "detects range patterns with overlapping endpoints"
 }
 
+declare_lint! {
+    /// The `non_contiguous_range_endpoints` lint detects likely off-by-one errors when using
+    /// exclusive [range patterns].
+    ///
+    /// [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 NON_CONTIGUOUS_RANGE_ENDPOINTS,
+    Warn,
+    "detects off-by-one errors with exclusive range patterns"
+}
+
 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

@@ -1,3 +1,11 @@
+pattern_analysis_excluside_range_missing_gap = multiple ranges are one apart
+    .label = this range doesn't match `{$gap}` because `..` is an exclusive range
+    .suggestion = use an inclusive range instead
+
+pattern_analysis_excluside_range_missing_max = exclusive range missing `{$max}`
+    .label = this range doesn't match `{$max}` because `..` is an exclusive range
+    .suggestion = use an inclusive range instead
+
 pattern_analysis_non_exhaustive_omitted_pattern = some variants are not matched explicitly
     .help = ensure that all variants are matched explicitly by adding the suggested match arms
     .note = the matched value is of type `{$scrut_ty}` and the `non_exhaustive_omitted_patterns` attribute was found

compiler/rustc_pattern_analysis/src/constructor.rs

@@ -192,7 +192,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.
@@ -229,25 +229,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),
         }
     }
 }
@@ -268,18 +265,24 @@ 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
+        self.lo.plus_one() == Some(self.hi)
     }
 
+    /// Construct a singleton range.
+    /// `x` must 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

@@ -76,6 +76,57 @@ impl<'tcx> AddToDiagnostic for Overlap<'tcx> {
     }
 }
 
+#[derive(LintDiagnostic)]
+#[diag(pattern_analysis_excluside_range_missing_max)]
+pub struct ExclusiveRangeMissingMax<'tcx> {
+    #[label]
+    #[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")]
+    /// This is an exclusive range that looks like `lo..max` (i.e. doesn't match `max`).
+    pub first_range: Span,
+    /// Suggest `lo..=max` instead.
+    pub suggestion: String,
+    pub max: Pat<'tcx>,
+}
+
+#[derive(LintDiagnostic)]
+#[diag(pattern_analysis_excluside_range_missing_gap)]
+pub struct ExclusiveRangeMissingGap<'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,
+    pub gap: Pat<'tcx>,
+    /// Suggest `lo..=gap` instead.
+    pub suggestion: String,
+    #[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<G: EmissionGuarantee, F: SubdiagMessageOp<G>>(
+        self,
+        diag: &mut Diag<'_, G>,
+        _: F,
+    ) {
+        let GappedRange { span, gap, first_range } = self;
+
+        // FIXME(mejrs) unfortunately `#[derive(LintDiagnostic)]`
+        // does not support `#[subdiagnostic(eager)]`...
+        let message = format!(
+            "this could appear 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

@@ -70,14 +70,8 @@ use rustc_middle::ty::Ty;
 use rustc_span::ErrorGuaranteed;
 
 use crate::constructor::{Constructor, ConstructorSet, IntRange};
-#[cfg(feature = "rustc")]
-use crate::lints::lint_nonexhaustive_missing_variants;
 use crate::pat::DeconstructedPat;
 use crate::pat_column::PatternColumn;
-#[cfg(feature = "rustc")]
-use crate::rustc::RustcMatchCheckCtxt;
-#[cfg(feature = "rustc")]
-use crate::usefulness::{compute_match_usefulness, ValidityConstraint};
 
 pub trait Captures<'a> {}
 impl<'a, T: ?Sized> Captures<'a> for T {}
@@ -145,6 +139,18 @@ pub trait TypeCx: Sized + fmt::Debug {
 
     /// The maximum pattern complexity limit was reached.
     fn complexity_exceeded(&self) -> Result<(), Self::Error>;
+
+    /// Lint that there is a gap `gap` between `pat` and all of `gapped_with` such that the gap is
+    /// not matched by another range. If `gapped_with` is empty, then `gap` is `T::MAX`. We only
+    /// detect singleton gaps.
+    /// The default implementation does nothing.
+    fn lint_non_contiguous_range_endpoints(
+        &self,
+        _pat: &DeconstructedPat<Self>,
+        _gap: IntRange,
+        _gapped_with: &[&DeconstructedPat<Self>],
+    ) {
+    }
 }
 
 /// The arm of a match expression.
@@ -167,11 +173,14 @@ impl<'p, Cx: TypeCx> Copy for MatchArm<'p, Cx> {}
 /// useful, and runs some lints.
 #[cfg(feature = "rustc")]
 pub fn analyze_match<'p, 'tcx>(
-    tycx: &RustcMatchCheckCtxt<'p, 'tcx>,
+    tycx: &rustc::RustcMatchCheckCtxt<'p, 'tcx>,
     arms: &[rustc::MatchArm<'p, 'tcx>],
     scrut_ty: Ty<'tcx>,
     pattern_complexity_limit: Option<usize>,
 ) -> Result<rustc::UsefulnessReport<'p, 'tcx>, ErrorGuaranteed> {
+    use lints::lint_nonexhaustive_missing_variants;
+    use usefulness::{compute_match_usefulness, ValidityConstraint};
+
     let scrut_ty = tycx.reveal_opaque_ty(scrut_ty);
     let scrut_validity = ValidityConstraint::from_bool(tycx.known_valid_scrutinee);
     let report =

compiler/rustc_pattern_analysis/src/rustc.rs

@@ -8,7 +8,7 @@ use rustc_index::{Idx, IndexVec};
 use rustc_middle::middle::stability::EvalResult;
 use rustc_middle::mir::interpret::Scalar;
 use rustc_middle::mir::{self, Const};
-use rustc_middle::thir::{FieldPat, Pat, PatKind, PatRange, PatRangeBoundary};
+use rustc_middle::thir::{self, FieldPat, Pat, PatKind, PatRange, PatRangeBoundary};
 use rustc_middle::ty::layout::IntegerExt;
 use rustc_middle::ty::{self, FieldDef, OpaqueTypeKey, Ty, TyCtxt, TypeVisitableExt, VariantDef};
 use rustc_session::lint;
@@ -718,12 +718,12 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'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 = rustc_hir::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: ty.inner() }))
@@ -900,6 +900,70 @@ impl<'p, 'tcx: 'p> TypeCx for RustcMatchCheckCtxt<'p, 'tcx> {
         let span = self.whole_match_span.unwrap_or(self.scrut_span);
         Err(self.tcx.dcx().span_err(span, "reached pattern complexity limit"))
     }
+
+    fn lint_non_contiguous_range_endpoints(
+        &self,
+        pat: &crate::pat::DeconstructedPat<Self>,
+        gap: IntRange,
+        gapped_with: &[&crate::pat::DeconstructedPat<Self>],
+    ) {
+        let Some(&thir_pat) = pat.data() else { return };
+        let thir::PatKind::Range(range) = &thir_pat.kind else { return };
+        // Only lint when the left range is an exclusive range.
+        if range.end != rustc_hir::RangeEnd::Excluded {
+            return;
+        }
+        // `pat` is an exclusive range like `lo..gap`. `gapped_with` contains ranges that start with
+        // `gap+1`.
+        let suggested_range: thir::Pat<'_> = {
+            // Suggest `lo..=gap` instead.
+            let mut suggested_range = thir_pat.clone();
+            let thir::PatKind::Range(range) = &mut suggested_range.kind else { unreachable!() };
+            range.end = rustc_hir::RangeEnd::Included;
+            suggested_range
+        };
+        let gap_as_pat = self.hoist_pat_range(&gap, *pat.ty());
+        if gapped_with.is_empty() {
+            // If `gapped_with` is empty, `gap == T::MAX`.
+            self.tcx.emit_node_span_lint(
+                lint::builtin::NON_CONTIGUOUS_RANGE_ENDPOINTS,
+                self.match_lint_level,
+                thir_pat.span,
+                errors::ExclusiveRangeMissingMax {
+                    // Point at this range.
+                    first_range: thir_pat.span,
+                    // That's the gap that isn't covered.
+                    max: gap_as_pat.clone(),
+                    // Suggest `lo..=max` instead.
+                    suggestion: suggested_range.to_string(),
+                },
+            );
+        } else {
+            self.tcx.emit_node_span_lint(
+                lint::builtin::NON_CONTIGUOUS_RANGE_ENDPOINTS,
+                self.match_lint_level,
+                thir_pat.span,
+                errors::ExclusiveRangeMissingGap {
+                    // Point at this range.
+                    first_range: thir_pat.span,
+                    // That's the gap that isn't covered.
+                    gap: gap_as_pat.clone(),
+                    // Suggest `lo..=gap` instead.
+                    suggestion: suggested_range.to_string(),
+                    // All these ranges skipped over `gap` which we think is probably a
+                    // mistake.
+                    gap_with: gapped_with
+                        .iter()
+                        .map(|pat| errors::GappedRange {
+                            span: pat.data().unwrap().span,
+                            gap: gap_as_pat.clone(),
+                            first_range: thir_pat.clone(),
+                        })
+                        .collect(),
+                },
+            );
+        }
+    }
 }
 
 /// Recursively expand this pattern into its subpatterns. Only useful for or-patterns.