Skip to content
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

Exhaustiveness: track overlapping ranges precisely #119396

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
2 changes: 1 addition & 1 deletion compiler/rustc_pattern_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ pub fn analyze_match<'p, 'tcx>(

let pat_column = PatternColumn::new(arms);

// Lint on ranges that overlap on their endpoints, which is likely a mistake.
// Lint ranges that overlap on their endpoints, which is likely a mistake.
lint_overlapping_range_endpoints(cx, &pat_column)?;

// Run the non_exhaustive_omitted_patterns lint. Only run on refutable patterns to avoid hitting
Expand Down
78 changes: 49 additions & 29 deletions compiler/rustc_pattern_analysis/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,18 @@ 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, Span};
use rustc_span::ErrorGuaranteed;

use crate::constructor::{IntRange, MaybeInfiniteInt};
use crate::constructor::MaybeInfiniteInt;
use crate::errors::{
NonExhaustiveOmittedPattern, NonExhaustiveOmittedPatternLintOnArm, Overlap,
OverlappingRangeEndpoints, Uncovered,
self, NonExhaustiveOmittedPattern, NonExhaustiveOmittedPatternLintOnArm, Uncovered,
};
use crate::pat::PatOrWild;
use crate::rustc::{
Constructor, DeconstructedPat, MatchArm, MatchCtxt, PlaceCtxt, RevealedTy, RustcMatchCheckCtxt,
SplitConstructorSet, WitnessPat,
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 @@ -209,34 +209,19 @@ 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 lint_overlapping_range_endpoints<'a, 'p, 'tcx>(
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>>,
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would be a good idea to use a impl FnMut or &mut dyn FnMut here? I don't think lint_overlapping_range_endpoints needs the whole collection allocated...

Copy link
Member Author

Choose a reason for hiding this comment

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

Performance is irrelevant for this so it's a stylistic choice. I personally prefer to be computing data than calling callbacks in terms of code legibility. Otherwise I'd have made that a method on TypeCx I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it'll depend a lot of what's possible on the rust-analyzer side as well. If it fits in TypeCx on their side too I might change to that

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what I was thinking, you're quite right actually

) -> Result<(), ErrorGuaranteed> {
let Some(ty) = column.head_ty() else {
return Ok(());
};
let pcx = &PlaceCtxt::new_dummy(cx, ty);
let rcx: &RustcMatchCheckCtxt<'_, '_> = cx.tycx;

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 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 })
.collect();
rcx.tcx.emit_spanned_lint(
lint::builtin::OVERLAPPING_RANGE_ENDPOINTS,
rcx.match_lint_level,
this_span,
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() {
Expand All @@ -249,7 +234,6 @@ pub(crate) fn lint_overlapping_range_endpoints<'a, 'p, 'tcx>(
// Iterate on patterns that contained `overlap`.
for pat in column.iter() {
let Constructor::IntRange(this_range) = pat.ctor() else { continue };
let this_span = pat.data().unwrap().span;
if this_range.is_singleton() {
// Don't lint when one of the ranges is a singleton.
continue;
Expand All @@ -258,16 +242,24 @@ pub(crate) fn lint_overlapping_range_endpoints<'a, 'p, 'tcx>(
// `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);
overlapping_range_endpoints.push(OverlappingRanges {
pat,
overlaps_on: *overlap_range,
overlaps_with: prefixes.as_slice().to_vec(),
});
}
suffixes.push(this_span)
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() {
emit_lint(overlap_range, this_span, &suffixes);
overlapping_range_endpoints.push(OverlappingRanges {
pat,
overlaps_on: *overlap_range,
overlaps_with: suffixes.as_slice().to_vec(),
});
}
prefixes.push(this_span)
prefixes.push(pat)
}
}
}
Expand All @@ -276,9 +268,37 @@ pub(crate) fn lint_overlapping_range_endpoints<'a, 'p, 'tcx>(
// Recurse into the fields.
for ctor in set.present {
for col in column.specialize(pcx, &ctor) {
lint_overlapping_range_endpoints(cx, &col)?;
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)?;

let rcx = cx.tycx;
for overlap in overlapping_range_endpoints {
let overlap_as_pat = rcx.hoist_pat_range(&overlap.overlaps_on, overlap.pat.ty());
let overlaps: Vec<_> = overlap
.overlaps_with
.iter()
.map(|pat| pat.data().unwrap().span)
.map(|span| errors::Overlap { range: overlap_as_pat.clone(), span })
.collect();
let pat_span = overlap.pat.data().unwrap().span;
rcx.tcx.emit_spanned_lint(
lint::builtin::OVERLAPPING_RANGE_ENDPOINTS,
rcx.match_lint_level,
pat_span,
errors::OverlappingRangeEndpoints { overlap: overlaps, range: pat_span },
);
}
Ok(())
}
2 changes: 2 additions & 0 deletions compiler/rustc_pattern_analysis/src/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ pub type DeconstructedPat<'p, 'tcx> =
crate::pat::DeconstructedPat<'p, RustcMatchCheckCtxt<'p, 'tcx>>;
pub type MatchArm<'p, 'tcx> = crate::MatchArm<'p, RustcMatchCheckCtxt<'p, 'tcx>>;
pub type MatchCtxt<'a, 'p, 'tcx> = crate::MatchCtxt<'a, 'p, RustcMatchCheckCtxt<'p, 'tcx>>;
pub type OverlappingRanges<'p, 'tcx> =
crate::usefulness::OverlappingRanges<'p, RustcMatchCheckCtxt<'p, 'tcx>>;
pub(crate) type PlaceCtxt<'a, 'p, 'tcx> =
crate::usefulness::PlaceCtxt<'a, 'p, RustcMatchCheckCtxt<'p, 'tcx>>;
pub(crate) type SplitConstructorSet<'p, 'tcx> =
Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_pattern_analysis/src/usefulness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -716,7 +716,7 @@ use rustc_index::bit_set::BitSet;
use smallvec::{smallvec, SmallVec};
use std::fmt;

use crate::constructor::{Constructor, ConstructorSet};
use crate::constructor::{Constructor, ConstructorSet, IntRange};
use crate::pat::{DeconstructedPat, PatOrWild, WitnessPat};
use crate::{Captures, MatchArm, MatchCtxt, TypeCx};

Expand Down Expand Up @@ -1471,6 +1471,15 @@ pub enum Usefulness<'p, Cx: TypeCx> {
Redundant,
}

/// Indicates that the range `pat` overlapped with all the ranges in `overlaps_with`, where the
/// range they overlapped over is `overlaps_on`. We only detect singleton overlaps.
#[derive(Clone, Debug)]
pub struct OverlappingRanges<'p, Cx: TypeCx> {
pub pat: &'p DeconstructedPat<'p, Cx>,
pub overlaps_on: IntRange,
pub overlaps_with: Vec<&'p DeconstructedPat<'p, Cx>>,
}

/// The output of checking a match for exhaustiveness and arm usefulness.
pub struct UsefulnessReport<'p, Cx: TypeCx> {
/// For each arm of the input, whether that arm is useful after the arms above it.
Expand Down