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 12, 2023
1 parent 21cce21 commit 66e644c
Show file tree
Hide file tree
Showing 9 changed files with 337 additions and 48 deletions.
4 changes: 4 additions & 0 deletions compiler/rustc_pattern_analysis/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -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}`
Expand Down
38 changes: 22 additions & 16 deletions compiler/rustc_pattern_analysis/src/constructor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
Expand Down Expand Up @@ -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),
}
}
}
Expand All @@ -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.
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_pattern_analysis/src/cx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 }))
Expand Down
30 changes: 30 additions & 0 deletions compiler/rustc_pattern_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
20 changes: 11 additions & 9 deletions compiler/rustc_pattern_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down
81 changes: 61 additions & 20 deletions compiler/rustc_pattern_analysis/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
},
);
}
Expand All @@ -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(),
Expand All @@ -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>,
) {
Expand All @@ -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`.
Expand All @@ -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 && !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 = 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);
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/ui/match/issue-18060.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// run-pass
// Regression test for #18060: match arms were matching in the wrong order.

#[allow(overlapping_range_endpoints)]
fn main() {
assert_eq!(2, match (1, 3) { (0, 2..=5) => 1, (1, 3) => 2, (_, 2..=5) => 3, (_, _) => 4 });
assert_eq!(2, match (1, 3) { (1, 3) => 2, (_, 2..=5) => 3, (_, _) => 4 });
Expand Down
Loading

0 comments on commit 66e644c

Please sign in to comment.