Skip to content

Commit

Permalink
Auto merge of #118879 - Nadrieril:lint-range-gap, r=estebank
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
bors committed Mar 9, 2024
2 parents 4d4bb49 + 8ac9a04 commit 1b427b3
Show file tree
Hide file tree
Showing 21 changed files with 679 additions and 157 deletions.
31 changes: 31 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_pattern_analysis/messages.ftl
Original file line number Diff line number Diff line change
@@ -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
Expand Down
35 changes: 19 additions & 16 deletions compiler/rustc_pattern_analysis/src/constructor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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),
}
}
}
Expand All @@ -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.
Expand Down
51 changes: 51 additions & 0 deletions compiler/rustc_pattern_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
23 changes: 16 additions & 7 deletions compiler/rustc_pattern_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand Down Expand Up @@ -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.
Expand All @@ -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 =
Expand Down
72 changes: 68 additions & 4 deletions compiler/rustc_pattern_analysis/src/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() }))
Expand Down Expand Up @@ -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.
Expand Down
Loading

0 comments on commit 1b427b3

Please sign in to comment.