Skip to content

Exhaustiveness: track overlapping ranges precisely #119396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -125,7 +125,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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this if important? lint_overlapping_range_endpoints is just a for loop, so for empty overlapping_range_endpoints it will do nothing anyway...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True but I have a general policy of helping both the reader and LLVM figure out that they can skip this most of the time

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it's logically redundant

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
87 changes: 2 additions & 85 deletions compiler/rustc_pattern_analysis/src/lints.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
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 rustc_span::ErrorGuaranteed;

use crate::constructor::MaybeInfiniteInt;
use crate::errors::{
self, NonExhaustiveOmittedPattern, NonExhaustiveOmittedPatternLintOnArm, Uncovered,
};
Expand All @@ -15,7 +10,6 @@ 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 @@ -68,10 +62,6 @@ impl<'p, 'tcx> PatternColumn<'p, 'tcx> {
Ok(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 @@ -207,82 +197,10 @@ pub(crate) fn lint_nonexhaustive_missing_variants<'a, 'p, 'tcx>(
Ok(())
}

/// 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>>,
) -> Result<(), ErrorGuaranteed> {
let Some(ty) = column.head_ty() else {
return Ok(());
};
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)?;
}
}
}
Ok(())
}

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

overlapping_range_endpoints: &[rustc::OverlappingRanges<'p, 'tcx>],
) {
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 All @@ -300,5 +218,4 @@ pub(crate) fn lint_overlapping_range_endpoints<'a, 'p, 'tcx>(
errors::OverlappingRangeEndpoints { overlap: overlaps, range: pat_span },
);
}
Ok(())
}
117 changes: 113 additions & 4 deletions compiler/rustc_pattern_analysis/src/usefulness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1328,6 +1328,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 PatOrWild::Pat(pat) = matrix.rows[child_row.parent_row].head() else { continue };
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 @@ -1346,6 +1423,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,
) -> Result<WitnessMatrix<Cx>, Cx::Error> {
debug_assert!(matrix.rows().all(|r| r.len() == matrix.column_count()));
Expand Down Expand Up @@ -1425,7 +1503,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 @@ -1447,6 +1530,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 @@ -1487,6 +1585,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 @@ -1497,9 +1596,14 @@ pub fn compute_match_usefulness<'p, Cx: TypeCx>(
scrut_ty: Cx::Ty,
scrut_validity: ValidityConstraint,
) -> Result<UsefulnessReport<'p, Cx>, Cx::Error> {
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 @@ -1516,5 +1620,10 @@ pub fn compute_match_usefulness<'p, Cx: TypeCx>(
(arm, usefulness)
})
.collect();
Ok(UsefulnessReport { arm_usefulness, non_exhaustiveness_witnesses })

Ok(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