Skip to content

Commit

Permalink
Only lint ranges that really overlap
Browse files Browse the repository at this point in the history
  • Loading branch information
Nadrieril committed Jan 7, 2024
1 parent 8e104e2 commit 7931ee6
Show file tree
Hide file tree
Showing 6 changed files with 131 additions and 111 deletions.
4 changes: 3 additions & 1 deletion compiler/rustc_pattern_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ pub fn analyze_match<'p, 'tcx>(
let pat_column = PatternColumn::new(arms);

// Lint ranges that overlap on their endpoints, which is likely a mistake.
lint_overlapping_range_endpoints(cx, &pat_column);
if !report.overlapping_range_endpoints.is_empty() {
lint_overlapping_range_endpoints(cx, &report.overlapping_range_endpoints);
}

// 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.
Expand Down
83 changes: 1 addition & 82 deletions compiler/rustc_pattern_analysis/src/lints.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,13 @@
use smallvec::SmallVec;

use rustc_data_structures::captures::Captures;
use rustc_middle::ty;
use rustc_session::lint;
use rustc_session::lint::builtin::NON_EXHAUSTIVE_OMITTED_PATTERNS;

use crate::constructor::MaybeInfiniteInt;
use crate::errors::{
self, NonExhaustiveOmittedPattern, NonExhaustiveOmittedPatternLintOnArm, Uncovered,
};
use crate::rustc::{
self, Constructor, DeconstructedPat, MatchArm, MatchCtxt, PlaceCtxt, RevealedTy,
RustcMatchCheckCtxt, SplitConstructorSet, WitnessPat,
};
use crate::usefulness::OverlappingRanges;

/// A column of patterns in the matrix, where a column is the intuitive notion of "subpatterns that
/// inspect the same subvalue/place".
Expand Down Expand Up @@ -56,10 +50,6 @@ impl<'p, 'tcx> PatternColumn<'p, 'tcx> {
pcx.ctors_for_ty().split(pcx, column_ctors)
}

fn iter(&self) -> impl Iterator<Item = &'p DeconstructedPat<'p, 'tcx>> + Captures<'_> {
self.patterns.iter().copied()
}

/// Does specialization: given a constructor, this takes the patterns from the column that match
/// the constructor, and outputs their fields.
/// This returns one column per field of the constructor. They usually all have the same length
Expand Down Expand Up @@ -205,81 +195,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.
#[instrument(level = "debug", skip(cx))]
pub(crate) fn collect_overlapping_range_endpoints<'a, 'p, 'tcx>(
cx: MatchCtxt<'a, 'p, 'tcx>,
column: &PatternColumn<'p, 'tcx>,
overlapping_range_endpoints: &mut Vec<rustc::OverlappingRanges<'p, 'tcx>>,
) {
let Some(ty) = column.head_ty() else {
return;
};
let pcx = &PlaceCtxt::new_dummy(cx, ty);

let set = column.analyze_ctors(pcx);

if matches!(ty.kind(), ty::Char | ty::Int(_) | ty::Uint(_)) {
// 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() {
if overlap_range.is_singleton() {
let overlap: MaybeInfiniteInt = overlap_range.lo;
// Ranges that look like `lo..=overlap`.
let mut prefixes: SmallVec<[_; 1]> = Default::default();
// Ranges that look like `overlap..=hi`.
let mut suffixes: SmallVec<[_; 1]> = Default::default();
// Iterate on patterns that contained `overlap`.
for pat in column.iter() {
let Constructor::IntRange(this_range) = pat.ctor() else { continue };
if this_range.is_singleton() {
// Don't lint when one of the ranges is a singleton.
continue;
}
if this_range.lo == overlap {
// `this_range` looks like `overlap..=this_range.hi`; it overlaps with any
// ranges that look like `lo..=overlap`.
if !prefixes.is_empty() {
overlapping_range_endpoints.push(OverlappingRanges {
pat,
overlaps_on: *overlap_range,
overlaps_with: prefixes.as_slice().to_vec(),
});
}
suffixes.push(pat)
} else if this_range.hi == overlap.plus_one() {
// `this_range` looks like `this_range.lo..=overlap`; it overlaps with any
// ranges that look like `overlap..=hi`.
if !suffixes.is_empty() {
overlapping_range_endpoints.push(OverlappingRanges {
pat,
overlaps_on: *overlap_range,
overlaps_with: suffixes.as_slice().to_vec(),
});
}
prefixes.push(pat)
}
}
}
}
} else {
// Recurse into the fields.
for ctor in set.present {
for col in column.specialize(pcx, &ctor) {
collect_overlapping_range_endpoints(cx, &col, overlapping_range_endpoints);
}
}
}
}

#[instrument(level = "debug", skip(cx))]
pub(crate) fn lint_overlapping_range_endpoints<'a, 'p, 'tcx>(
cx: MatchCtxt<'a, 'p, 'tcx>,
column: &PatternColumn<'p, 'tcx>,
overlapping_range_endpoints: &[rustc::OverlappingRanges<'p, 'tcx>],
) {
let mut overlapping_range_endpoints = Vec::new();
collect_overlapping_range_endpoints(cx, column, &mut overlapping_range_endpoints);

let rcx = cx.tycx;
for overlap in overlapping_range_endpoints {
let overlap_as_pat = rcx.hoist_pat_range(&overlap.overlaps_on, overlap.pat.ty());
Expand Down
111 changes: 108 additions & 3 deletions compiler/rustc_pattern_analysis/src/usefulness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1333,6 +1333,83 @@ impl<Cx: TypeCx> WitnessMatrix<Cx> {
}
}

/// Collect ranges that overlap like `lo..=overlap`/`overlap..=hi`. Must be called during
/// exhaustiveness checking, if we find a singleton range after constructor splitting. This reuses
/// row intersection information to only detect ranges that truly overlap.
///
/// If two ranges overlapped, the split set will contain their intersection as a singleton.
/// Specialization will then select rows that match the overlap, and exhaustiveness will compute
/// which rows have an intersection that includes the overlap. That gives us all the info we need to
/// compute overlapping ranges without false positives.
///
/// We can however get false negatives because exhaustiveness does not explore all cases. See the
/// section on relevancy at the top of the file.
fn collect_overlapping_range_endpoints<'p, Cx: TypeCx>(
overlap_range: IntRange,
matrix: &Matrix<'p, Cx>,
specialized_matrix: &Matrix<'p, Cx>,
overlapping_range_endpoints: &mut Vec<OverlappingRanges<'p, Cx>>,
) {
let overlap = overlap_range.lo;
// Ranges that look like `lo..=overlap`.
let mut prefixes: SmallVec<[_; 1]> = Default::default();
// Ranges that look like `overlap..=hi`.
let mut suffixes: SmallVec<[_; 1]> = Default::default();
// Iterate on patterns that contained `overlap`. We iterate on `specialized_matrix` which
// contains only rows that matched the current `ctor` as well as accurate intersection
// information. It doesn't contain the column that contains the range; that can be found in
// `matrix`.
for (child_row_id, child_row) in specialized_matrix.rows().enumerate() {
let pat = matrix.rows[child_row.parent_row].head();
let Constructor::IntRange(this_range) = pat.ctor() else { continue };
// Don't lint when one of the ranges is a singleton.
if this_range.is_singleton() {
continue;
}
if this_range.lo == overlap {
// `this_range` looks like `overlap..=this_range.hi`; it overlaps with any
// ranges that look like `lo..=overlap`.
if !prefixes.is_empty() {
let overlaps_with: Vec<_> = prefixes
.iter()
.filter(|&&(other_child_row_id, _)| {
child_row.intersects.contains(other_child_row_id)
})
.map(|&(_, pat)| pat)
.collect();
if !overlaps_with.is_empty() {
overlapping_range_endpoints.push(OverlappingRanges {
pat,
overlaps_on: overlap_range,
overlaps_with,
});
}
}
suffixes.push((child_row_id, pat))
} else if this_range.hi == overlap.plus_one() {
// `this_range` looks like `this_range.lo..=overlap`; it overlaps with any
// ranges that look like `overlap..=hi`.
if !suffixes.is_empty() {
let overlaps_with: Vec<_> = suffixes
.iter()
.filter(|&&(other_child_row_id, _)| {
child_row.intersects.contains(other_child_row_id)
})
.map(|&(_, pat)| pat)
.collect();
if !overlaps_with.is_empty() {
overlapping_range_endpoints.push(OverlappingRanges {
pat,
overlaps_on: overlap_range,
overlaps_with,
});
}
}
prefixes.push((child_row_id, pat))
}
}
}

/// The core of the algorithm.
///
/// This recursively computes witnesses of the non-exhaustiveness of `matrix` (if any). Also tracks
Expand All @@ -1351,6 +1428,7 @@ impl<Cx: TypeCx> WitnessMatrix<Cx> {
fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>(
mcx: MatchCtxt<'a, 'p, Cx>,
matrix: &mut Matrix<'p, Cx>,
overlapping_range_endpoints: &mut Vec<OverlappingRanges<'p, Cx>>,
is_top_level: bool,
) -> WitnessMatrix<Cx> {
debug_assert!(matrix.rows().all(|r| r.len() == matrix.column_count()));
Expand Down Expand Up @@ -1430,7 +1508,12 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>(
let ctor_is_relevant = matches!(ctor, Constructor::Missing) || missing_ctors.is_empty();
let mut spec_matrix = matrix.specialize_constructor(pcx, &ctor, ctor_is_relevant);
let mut witnesses = ensure_sufficient_stack(|| {
compute_exhaustiveness_and_usefulness(mcx, &mut spec_matrix, false)
compute_exhaustiveness_and_usefulness(
mcx,
&mut spec_matrix,
overlapping_range_endpoints,
false,
)
});

// Transform witnesses for `spec_matrix` into witnesses for `matrix`.
Expand All @@ -1452,6 +1535,21 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>(
}
}
}

// Detect ranges that overlap on their endpoints.
if let Constructor::IntRange(overlap_range) = ctor {
if overlap_range.is_singleton()
&& spec_matrix.rows.len() >= 2
&& spec_matrix.rows.iter().any(|row| !row.intersects.is_empty())
{
collect_overlapping_range_endpoints(
overlap_range,
matrix,
&spec_matrix,
overlapping_range_endpoints,
);
}
}
}

// Record usefulness in the patterns.
Expand Down Expand Up @@ -1492,6 +1590,7 @@ pub struct UsefulnessReport<'p, Cx: TypeCx> {
/// If the match is exhaustive, this is empty. If not, this contains witnesses for the lack of
/// exhaustiveness.
pub non_exhaustiveness_witnesses: Vec<WitnessPat<Cx>>,
pub overlapping_range_endpoints: Vec<OverlappingRanges<'p, Cx>>,
}

/// Computes whether a match is exhaustive and which of its arms are useful.
Expand All @@ -1502,8 +1601,14 @@ pub fn compute_match_usefulness<'p, Cx: TypeCx>(
scrut_ty: Cx::Ty,
scrut_validity: ValidityConstraint,
) -> UsefulnessReport<'p, Cx> {
let mut overlapping_range_endpoints = Vec::new();
let mut matrix = Matrix::new(arms, scrut_ty, scrut_validity);
let non_exhaustiveness_witnesses = compute_exhaustiveness_and_usefulness(cx, &mut matrix, true);
let non_exhaustiveness_witnesses = compute_exhaustiveness_and_usefulness(
cx,
&mut matrix,
&mut overlapping_range_endpoints,
true,
);

let non_exhaustiveness_witnesses: Vec<_> = non_exhaustiveness_witnesses.single_column();
let arm_usefulness: Vec<_> = arms
Expand All @@ -1520,5 +1625,5 @@ pub fn compute_match_usefulness<'p, Cx: TypeCx>(
(arm, usefulness)
})
.collect();
UsefulnessReport { arm_usefulness, non_exhaustiveness_witnesses }
UsefulnessReport { arm_usefulness, non_exhaustiveness_witnesses, overlapping_range_endpoints }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// check-pass
fn main() {
match (0i8, 0i8) {
(0, _) => {}
(..=-1, ..=0) => {}
(1.., 0..) => {}
(1.., ..=-1) | (..=-1, 1..) => {}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,18 +44,25 @@ fn main() {
match (0u8, true) {
(0..=10, true) => {}
(10..20, true) => {} //~ ERROR multiple patterns overlap on their endpoints
(10..20, false) => {} //~ ERROR multiple patterns overlap on their endpoints
(10..20, false) => {}
_ => {}
}
match (true, 0u8) {
(true, 0..=10) => {}
(true, 10..20) => {} //~ ERROR multiple patterns overlap on their endpoints
(false, 10..20) => {} //~ ERROR multiple patterns overlap on their endpoints
(false, 10..20) => {}
_ => {}
}
match Some(0u8) {
Some(0..=10) => {}
Some(10..20) => {} //~ ERROR multiple patterns overlap on their endpoints
_ => {}
}

// The lint has false negatives when we skip some cases because of relevancy.
match (true, true, 0u8) {
(true, _, 0..=10) => {}
(_, true, 10..20) => {}
_ => {}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,17 +84,6 @@ LL | (10..20, true) => {}
|
= note: you likely meant to write mutually exclusive ranges

error: multiple patterns overlap on their endpoints
--> $DIR/overlapping_range_endpoints.rs:47:10
|
LL | (0..=10, true) => {}
| ------ this range overlaps on `10_u8`...
LL | (10..20, true) => {}
LL | (10..20, false) => {}
| ^^^^^^ ... with this range
|
= note: you likely meant to write mutually exclusive ranges

error: multiple patterns overlap on their endpoints
--> $DIR/overlapping_range_endpoints.rs:52:16
|
Expand All @@ -105,17 +94,6 @@ LL | (true, 10..20) => {}
|
= note: you likely meant to write mutually exclusive ranges

error: multiple patterns overlap on their endpoints
--> $DIR/overlapping_range_endpoints.rs:53:17
|
LL | (true, 0..=10) => {}
| ------ this range overlaps on `10_u8`...
LL | (true, 10..20) => {}
LL | (false, 10..20) => {}
| ^^^^^^ ... with this range
|
= note: you likely meant to write mutually exclusive ranges

error: multiple patterns overlap on their endpoints
--> $DIR/overlapping_range_endpoints.rs:58:14
|
Expand All @@ -126,5 +104,5 @@ LL | Some(10..20) => {}
|
= note: you likely meant to write mutually exclusive ranges

error: aborting due to 12 previous errors
error: aborting due to 10 previous errors

0 comments on commit 7931ee6

Please sign in to comment.