Skip to content

Commit

Permalink
Lint small gaps between ranges
Browse files Browse the repository at this point in the history
  • Loading branch information
Nadrieril committed Dec 20, 2023
1 parent 57ad505 commit 3ef781c
Showing 13 changed files with 399 additions and 82 deletions.
30 changes: 30 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
@@ -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,35 @@ 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
/// 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.
4 changes: 4 additions & 0 deletions compiler/rustc_pattern_analysis/messages.ftl
Original file line number Diff line number Diff line change
@@ -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}`
36 changes: 20 additions & 16 deletions compiler/rustc_pattern_analysis/src/constructor.rs
Original file line number Diff line number Diff line change
@@ -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.
30 changes: 30 additions & 0 deletions compiler/rustc_pattern_analysis/src/errors.rs
Original file line number Diff line number Diff line change
@@ -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]
32 changes: 15 additions & 17 deletions compiler/rustc_pattern_analysis/src/lib.rs
Original file line number Diff line number Diff line change
@@ -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.
@@ -109,26 +101,32 @@ 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>,
) -> rustc::UsefulnessReport<'p, '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
81 changes: 61 additions & 20 deletions compiler/rustc_pattern_analysis/src/lints.rs
Original file line number Diff line number Diff line change
@@ -7,10 +7,7 @@ use rustc_session::lint::builtin::NON_EXHAUSTIVE_OMITTED_PATTERNS;
use rustc_span::Span;

use crate::constructor::{IntRange, MaybeInfiniteInt};
use crate::errors::{
NonExhaustiveOmittedPattern, NonExhaustiveOmittedPatternLintOnArm, Overlap,
OverlappingRangeEndpoints, Uncovered,
};
use crate::errors;
use crate::rustc::{
Constructor, DeconstructedPat, MatchArm, MatchCtxt, PlaceCtxt, RustcMatchCheckCtxt,
SplitConstructorSet, WitnessPat,
@@ -189,9 +186,9 @@ pub(crate) fn lint_nonexhaustive_missing_variants<'a, 'p, 'tcx>(
NON_EXHAUSTIVE_OMITTED_PATTERNS,
rcx.match_lint_level,
rcx.scrut_span,
NonExhaustiveOmittedPattern {
errors::NonExhaustiveOmittedPattern {
scrut_ty,
uncovered: Uncovered::new(rcx.scrut_span, rcx, witnesses),
uncovered: errors::Uncovered::new(rcx.scrut_span, rcx, witnesses),
},
);
}
@@ -203,7 +200,7 @@ pub(crate) fn lint_nonexhaustive_missing_variants<'a, 'p, 'tcx>(
let (lint_level, lint_level_source) =
rcx.tcx.lint_level_at_node(NON_EXHAUSTIVE_OMITTED_PATTERNS, arm.arm_data);
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: rcx.whole_match_span.map(|span| span.shrink_to_lo()),
lint_level: lint_level.as_str(),
@@ -220,9 +217,10 @@ pub(crate) fn lint_nonexhaustive_missing_variants<'a, '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<'a, 'p, 'tcx>(
pub(crate) fn lint_likely_range_mistakes<'a, 'p, 'tcx>(
cx: MatchCtxt<'a, 'p, 'tcx>,
column: &PatternColumn<'a, 'p, 'tcx>,
) {
@@ -235,24 +233,24 @@ pub(crate) fn lint_overlapping_range_endpoints<'a, '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 = rcx.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();
rcx.tcx.emit_spanned_lint(
lint::builtin::OVERLAPPING_RANGE_ENDPOINTS,
rcx.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`.
@@ -267,29 +265,72 @@ pub(crate) fn lint_overlapping_range_endpoints<'a, '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.data();
let Constructor::IntRange(this_range) = pat.ctor() else { continue };

if point == this_range.hi && !this_range.is_singleton() {
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 = rcx.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();
rcx.tcx.emit_spanned_lint(
lint::builtin::SMALL_GAPS_BETWEEN_RANGES,
rcx.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);
}
}
}
6 changes: 3 additions & 3 deletions compiler/rustc_pattern_analysis/src/rustc.rs
Original file line number Diff line number Diff line change
@@ -659,12 +659,12 @@ impl<'p, 'tcx> 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 }))
Loading

0 comments on commit 3ef781c

Please sign in to comment.