diff --git a/compiler/rustc_pattern_analysis/messages.ftl b/compiler/rustc_pattern_analysis/messages.ftl index 827928f97d7..41a1d958f10 100644 --- a/compiler/rustc_pattern_analysis/messages.ftl +++ b/compiler/rustc_pattern_analysis/messages.ftl @@ -1,3 +1,11 @@ +pattern_analysis_excluside_range_missing_gap = multiple ranges are one apart + .label = this range doesn't match `{$gap}` because `..` is an exclusive range + .suggestion = use an inclusive range instead + +pattern_analysis_excluside_range_missing_max = exclusive range missing `{$max}` + .label = this range doesn't match `{$max}` because `..` is an exclusive range + .suggestion = use an inclusive range instead + pattern_analysis_non_exhaustive_omitted_pattern = some variants are not matched explicitly .help = ensure that all variants are matched explicitly by adding the suggested match arms .note = the matched value is of type `{$scrut_ty}` and the `non_exhaustive_omitted_patterns` attribute was found diff --git a/compiler/rustc_pattern_analysis/src/errors.rs b/compiler/rustc_pattern_analysis/src/errors.rs index d0f56f0268d..e471b8abd73 100644 --- a/compiler/rustc_pattern_analysis/src/errors.rs +++ b/compiler/rustc_pattern_analysis/src/errors.rs @@ -76,6 +76,57 @@ fn add_to_diagnostic_with>( } } +#[derive(LintDiagnostic)] +#[diag(pattern_analysis_excluside_range_missing_max)] +pub struct ExclusiveRangeMissingMax<'tcx> { + #[label] + #[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")] + /// This is an exclusive range that looks like `lo..max` (i.e. doesn't match `max`). + pub first_range: Span, + /// Suggest `lo..=max` instead. + pub suggestion: String, + pub max: Pat<'tcx>, +} + +#[derive(LintDiagnostic)] +#[diag(pattern_analysis_excluside_range_missing_gap)] +pub struct ExclusiveRangeMissingGap<'tcx> { + #[label] + #[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")] + /// This is an exclusive range that looks like `lo..gap` (i.e. doesn't match `gap`). + pub first_range: Span, + pub gap: Pat<'tcx>, + /// Suggest `lo..=gap` instead. + pub suggestion: String, + #[subdiagnostic] + /// All these ranges skipped over `gap` which we think is probably a mistake. + pub gap_with: Vec>, +} + +pub struct GappedRange<'tcx> { + pub span: Span, + pub gap: Pat<'tcx>, + pub first_range: Pat<'tcx>, +} + +impl<'tcx> AddToDiagnostic for GappedRange<'tcx> { + fn add_to_diagnostic_with>( + self, + diag: &mut Diag<'_, G>, + _: F, + ) { + let GappedRange { span, gap, first_range } = self; + + // FIXME(mejrs) unfortunately `#[derive(LintDiagnostic)]` + // does not support `#[subdiagnostic(eager)]`... + let message = format!( + "this could appear to continue range `{first_range}`, but `{gap}` isn't matched by \ + either of them" + ); + diag.span_label(span, message); + } +} + #[derive(LintDiagnostic)] #[diag(pattern_analysis_non_exhaustive_omitted_pattern)] #[help] diff --git a/compiler/rustc_pattern_analysis/src/lib.rs b/compiler/rustc_pattern_analysis/src/lib.rs index 4b0955699fc..f632eaf7ea4 100644 --- a/compiler/rustc_pattern_analysis/src/lib.rs +++ b/compiler/rustc_pattern_analysis/src/lib.rs @@ -70,14 +70,8 @@ pub fn insert(&mut self, elem: T) { use rustc_span::ErrorGuaranteed; use crate::constructor::{Constructor, ConstructorSet, IntRange}; -#[cfg(feature = "rustc")] -use crate::lints::lint_nonexhaustive_missing_variants; use crate::pat::DeconstructedPat; use crate::pat_column::PatternColumn; -#[cfg(feature = "rustc")] -use crate::rustc::RustcMatchCheckCtxt; -#[cfg(feature = "rustc")] -use crate::usefulness::{compute_match_usefulness, ValidityConstraint}; pub trait Captures<'a> {} impl<'a, T: ?Sized> Captures<'a> for T {} @@ -145,6 +139,18 @@ fn lint_overlapping_range_endpoints( /// The maximum pattern complexity limit was reached. fn complexity_exceeded(&self) -> Result<(), Self::Error>; + + /// Lint that there is a gap `gap` between `pat` and all of `gapped_with` such that the gap is + /// not matched by another range. If `gapped_with` is empty, then `gap` is `T::MAX`. We only + /// detect singleton gaps. + /// The default implementation does nothing. + fn lint_non_contiguous_range_endpoints( + &self, + _pat: &DeconstructedPat, + _gap: IntRange, + _gapped_with: &[&DeconstructedPat], + ) { + } } /// The arm of a match expression. @@ -167,11 +173,14 @@ 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>, pattern_complexity_limit: Option, ) -> Result, ErrorGuaranteed> { + use lints::lint_nonexhaustive_missing_variants; + use usefulness::{compute_match_usefulness, ValidityConstraint}; + let scrut_ty = tycx.reveal_opaque_ty(scrut_ty); let scrut_validity = ValidityConstraint::from_bool(tycx.known_valid_scrutinee); let report = diff --git a/compiler/rustc_pattern_analysis/src/rustc.rs b/compiler/rustc_pattern_analysis/src/rustc.rs index a36c6dda433..0085f0ab656 100644 --- a/compiler/rustc_pattern_analysis/src/rustc.rs +++ b/compiler/rustc_pattern_analysis/src/rustc.rs @@ -8,7 +8,7 @@ use rustc_middle::middle::stability::EvalResult; use rustc_middle::mir::interpret::Scalar; use rustc_middle::mir::{self, Const}; -use rustc_middle::thir::{FieldPat, Pat, PatKind, PatRange, PatRangeBoundary}; +use rustc_middle::thir::{self, FieldPat, Pat, PatKind, PatRange, PatRangeBoundary}; use rustc_middle::ty::layout::IntegerExt; use rustc_middle::ty::{self, FieldDef, OpaqueTypeKey, Ty, TyCtxt, TypeVisitableExt, VariantDef}; use rustc_session::lint; @@ -900,6 +900,70 @@ fn complexity_exceeded(&self) -> Result<(), Self::Error> { let span = self.whole_match_span.unwrap_or(self.scrut_span); Err(self.tcx.dcx().span_err(span, "reached pattern complexity limit")) } + + fn lint_non_contiguous_range_endpoints( + &self, + pat: &crate::pat::DeconstructedPat, + gap: IntRange, + gapped_with: &[&crate::pat::DeconstructedPat], + ) { + let Some(&thir_pat) = pat.data() else { return }; + let thir::PatKind::Range(range) = &thir_pat.kind else { return }; + // Only lint when the left range is an exclusive range. + if range.end != rustc_hir::RangeEnd::Excluded { + return; + } + // `pat` is an exclusive range like `lo..gap`. `gapped_with` contains ranges that start with + // `gap+1`. + let suggested_range: thir::Pat<'_> = { + // Suggest `lo..=gap` instead. + let mut suggested_range = thir_pat.clone(); + let thir::PatKind::Range(range) = &mut suggested_range.kind else { unreachable!() }; + range.end = rustc_hir::RangeEnd::Included; + suggested_range + }; + let gap_as_pat = self.hoist_pat_range(&gap, *pat.ty()); + if gapped_with.is_empty() { + // If `gapped_with` is empty, `gap == T::MAX`. + self.tcx.emit_node_span_lint( + lint::builtin::NON_CONTIGUOUS_RANGE_ENDPOINTS, + self.match_lint_level, + thir_pat.span, + errors::ExclusiveRangeMissingMax { + // Point at this range. + first_range: thir_pat.span, + // That's the gap that isn't covered. + max: gap_as_pat.clone(), + // Suggest `lo..=max` instead. + suggestion: suggested_range.to_string(), + }, + ); + } else { + self.tcx.emit_node_span_lint( + lint::builtin::NON_CONTIGUOUS_RANGE_ENDPOINTS, + self.match_lint_level, + thir_pat.span, + errors::ExclusiveRangeMissingGap { + // Point at this range. + first_range: thir_pat.span, + // That's the gap that isn't covered. + gap: gap_as_pat.clone(), + // Suggest `lo..=gap` instead. + suggestion: suggested_range.to_string(), + // All these ranges skipped over `gap` which we think is probably a + // mistake. + gap_with: gapped_with + .iter() + .map(|pat| errors::GappedRange { + span: pat.data().unwrap().span, + gap: gap_as_pat.clone(), + first_range: thir_pat.clone(), + }) + .collect(), + }, + ); + } + } } /// Recursively expand this pattern into its subpatterns. Only useful for or-patterns. diff --git a/compiler/rustc_pattern_analysis/src/usefulness.rs b/compiler/rustc_pattern_analysis/src/usefulness.rs index 5c4b3752a51..a067bf1f0c2 100644 --- a/compiler/rustc_pattern_analysis/src/usefulness.rs +++ b/compiler/rustc_pattern_analysis/src/usefulness.rs @@ -1489,7 +1489,7 @@ fn extend(&mut self, other: Self) { /// 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>( - mcx: &mut UsefulnessCtxt<'_, Cx>, + cx: &Cx, overlap_range: IntRange, matrix: &Matrix<'p, Cx>, specialized_matrix: &Matrix<'p, Cx>, @@ -1522,7 +1522,7 @@ fn collect_overlapping_range_endpoints<'p, Cx: TypeCx>( .map(|&(_, pat)| pat) .collect(); if !overlaps_with.is_empty() { - mcx.tycx.lint_overlapping_range_endpoints(pat, overlap_range, &overlaps_with); + cx.lint_overlapping_range_endpoints(pat, overlap_range, &overlaps_with); } } suffixes.push((child_row_id, pat)) @@ -1538,7 +1538,7 @@ fn collect_overlapping_range_endpoints<'p, Cx: TypeCx>( .map(|&(_, pat)| pat) .collect(); if !overlaps_with.is_empty() { - mcx.tycx.lint_overlapping_range_endpoints(pat, overlap_range, &overlaps_with); + cx.lint_overlapping_range_endpoints(pat, overlap_range, &overlaps_with); } } prefixes.push((child_row_id, pat)) @@ -1546,6 +1546,33 @@ fn collect_overlapping_range_endpoints<'p, Cx: TypeCx>( } } +/// Collect ranges that have a singleton gap between them. +fn collect_non_contiguous_range_endpoints<'p, Cx: TypeCx>( + cx: &Cx, + gap_range: &IntRange, + matrix: &Matrix<'p, Cx>, +) { + let gap = gap_range.lo; + // Ranges that look like `lo..gap`. + let mut onebefore: SmallVec<[_; 1]> = Default::default(); + // Ranges that start on `gap+1` or singletons `gap+1`. + let mut oneafter: SmallVec<[_; 1]> = Default::default(); + // Look through the column for ranges near the gap. + for pat in matrix.heads() { + let PatOrWild::Pat(pat) = pat else { continue }; + let Constructor::IntRange(this_range) = pat.ctor() else { continue }; + if gap == this_range.hi { + onebefore.push(pat) + } else if gap.plus_one() == Some(this_range.lo) { + oneafter.push(pat) + } + } + + for pat_before in onebefore { + cx.lint_non_contiguous_range_endpoints(pat_before, *gap_range, oneafter.as_slice()); + } +} + /// The core of the algorithm. /// /// This recursively computes witnesses of the non-exhaustiveness of `matrix` (if any). Also tracks @@ -1626,13 +1653,24 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>( && spec_matrix.rows.len() >= 2 && spec_matrix.rows.iter().any(|row| !row.intersects.is_empty()) { - collect_overlapping_range_endpoints(mcx, overlap_range, matrix, &spec_matrix); + collect_overlapping_range_endpoints(mcx.tycx, overlap_range, matrix, &spec_matrix); } } matrix.unspecialize(spec_matrix); } + // Detect singleton gaps between ranges. + if missing_ctors.iter().any(|c| matches!(c, Constructor::IntRange(..))) { + for missing in &missing_ctors { + if let Constructor::IntRange(gap) = missing { + if gap.is_singleton() { + collect_non_contiguous_range_endpoints(mcx.tycx, gap, matrix); + } + } + } + } + // Record usefulness in the patterns. for row in matrix.rows() { if row.useful { diff --git a/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.rs b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.rs new file mode 100644 index 00000000000..78849d7e143 --- /dev/null +++ b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.rs @@ -0,0 +1,117 @@ +#![feature(exclusive_range_pattern)] +#![deny(non_contiguous_range_endpoints)] + +macro_rules! m { + ($s:expr, $t1:pat, $t2:pat) => { + match $s { + $t1 => {} + $t2 => {} + _ => {} + } + }; +} + +fn main() { + match 0u8 { + 20..30 => {} //~ ERROR multiple ranges are one apart + 31..=40 => {} + _ => {} + } + match 0u8 { + 20..30 => {} //~ ERROR multiple ranges are one apart + 31 => {} + _ => {} + } + + m!(0u8, 20..30, 31..=40); //~ ERROR multiple ranges are one apart + m!(0u8, 31..=40, 20..30); //~ ERROR multiple ranges are one apart + m!(0u8, 20..30, 29..=40); //~ WARN multiple patterns overlap on their endpoints + m!(0u8, 20..30, 30..=40); + m!(0u8, 20..30, 31..=40); //~ ERROR multiple ranges are one apart + m!(0u8, 20..30, 32..=40); + m!(0u8, 20..30, 31..=32); //~ ERROR multiple ranges are one apart + // Don't lint two singletons. + m!(0u8, 30, 32); + // Don't lint on the equivalent inclusive range + m!(0u8, 20..=29, 31..=40); + // Don't lint if the lower one is a singleton. + m!(0u8, 30, 32..=40); + + // Catch the `lo..MAX` case. + match 0u8 { + 0..255 => {} //~ ERROR exclusive range missing `u8::MAX` + _ => {} + } + // Except for `usize`, since `0..=usize::MAX` isn't exhaustive either. + match 0usize { + 0..usize::MAX => {} + _ => {} + } + + // Don't lint if the gap is caught by another range. + match 0u8 { + 0..10 => {} + 11..20 => {} + 10 => {} + _ => {} + } + match 0u8 { + 0..10 => {} + 11..20 => {} + 5..15 => {} + _ => {} + } + match 0u8 { + 0..255 => {} + 255 => {} + } + + // Test multiple at once + match 0u8 { + 0..10 => {} //~ ERROR multiple ranges are one apart + 11..20 => {} + 11..30 => {} + _ => {} + } + match 0u8 { + 0..10 => {} //~ ERROR multiple ranges are one apart + 11..20 => {} //~ ERROR multiple ranges are one apart + 21..30 => {} + _ => {} + } + match 0u8 { + 00..20 => {} //~ ERROR multiple ranges are one apart + 10..20 => {} //~ ERROR multiple ranges are one apart + 21..30 => {} + 21..40 => {} + _ => {} + } + + // Test nested + match (0u8, true) { + (0..10, true) => {} //~ ERROR multiple ranges are one apart + (11..20, true) => {} + _ => {} + } + match (true, 0u8) { + (true, 0..10) => {} //~ ERROR multiple ranges are one apart + (true, 11..20) => {} + _ => {} + } + // Asymmetry due to how exhaustiveness is computed. + match (0u8, true) { + (0..10, true) => {} //~ ERROR multiple ranges are one apart + (11..20, false) => {} + _ => {} + } + match (true, 0u8) { + (true, 0..10) => {} + (false, 11..20) => {} + _ => {} + } + match Some(0u8) { + Some(0..10) => {} //~ ERROR multiple ranges are one apart + Some(11..20) => {} + _ => {} + } +} diff --git a/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.stderr b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.stderr new file mode 100644 index 00000000000..e5c2d788ba4 --- /dev/null +++ b/tests/ui/pattern/usefulness/integer-ranges/gap_between_ranges.stderr @@ -0,0 +1,193 @@ +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:16:9 + | +LL | 20..30 => {} + | ^^^^^^ + | | + | this range doesn't match `30_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `20_u8..=30_u8` +LL | 31..=40 => {} + | ------- this could appear to continue range `20_u8..30_u8`, but `30_u8` isn't matched by either of them + | +note: the lint level is defined here + --> $DIR/gap_between_ranges.rs:2:9 + | +LL | #![deny(non_contiguous_range_endpoints)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:21:9 + | +LL | 20..30 => {} + | ^^^^^^ + | | + | this range doesn't match `30_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `20_u8..=30_u8` +LL | 31 => {} + | -- this could appear to continue range `20_u8..30_u8`, but `30_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:26:13 + | +LL | m!(0u8, 20..30, 31..=40); + | ^^^^^^ ------- this could appear to continue range `20_u8..30_u8`, but `30_u8` isn't matched by either of them + | | + | this range doesn't match `30_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `20_u8..=30_u8` + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:27:22 + | +LL | m!(0u8, 31..=40, 20..30); + | ------- ^^^^^^ + | | | + | | this range doesn't match `30_u8` because `..` is an exclusive range + | | help: use an inclusive range instead: `20_u8..=30_u8` + | this could appear to continue range `20_u8..30_u8`, but `30_u8` isn't matched by either of them + +warning: multiple patterns overlap on their endpoints + --> $DIR/gap_between_ranges.rs:28:21 + | +LL | m!(0u8, 20..30, 29..=40); + | ------ ^^^^^^^ ... with this range + | | + | this range overlaps on `29_u8`... + | + = note: you likely meant to write mutually exclusive ranges + = note: `#[warn(overlapping_range_endpoints)]` on by default + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:30:13 + | +LL | m!(0u8, 20..30, 31..=40); + | ^^^^^^ ------- this could appear to continue range `20_u8..30_u8`, but `30_u8` isn't matched by either of them + | | + | this range doesn't match `30_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `20_u8..=30_u8` + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:32:13 + | +LL | m!(0u8, 20..30, 31..=32); + | ^^^^^^ ------- this could appear to continue range `20_u8..30_u8`, but `30_u8` isn't matched by either of them + | | + | this range doesn't match `30_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `20_u8..=30_u8` + +error: exclusive range missing `u8::MAX` + --> $DIR/gap_between_ranges.rs:42:9 + | +LL | 0..255 => {} + | ^^^^^^ + | | + | this range doesn't match `u8::MAX` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=u8::MAX` + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:71:9 + | +LL | 0..10 => {} + | ^^^^^ + | | + | this range doesn't match `10_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=10_u8` +LL | 11..20 => {} + | ------ this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them +LL | 11..30 => {} + | ------ this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:77:9 + | +LL | 0..10 => {} + | ^^^^^ + | | + | this range doesn't match `10_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=10_u8` +LL | 11..20 => {} + | ------ this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:78:9 + | +LL | 11..20 => {} + | ^^^^^^ + | | + | this range doesn't match `20_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `11_u8..=20_u8` +LL | 21..30 => {} + | ------ this could appear to continue range `11_u8..20_u8`, but `20_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:83:9 + | +LL | 00..20 => {} + | ^^^^^^ + | | + | this range doesn't match `20_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=20_u8` +LL | 10..20 => {} +LL | 21..30 => {} + | ------ this could appear to continue range `0_u8..20_u8`, but `20_u8` isn't matched by either of them +LL | 21..40 => {} + | ------ this could appear to continue range `0_u8..20_u8`, but `20_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:84:9 + | +LL | 10..20 => {} + | ^^^^^^ + | | + | this range doesn't match `20_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `10_u8..=20_u8` +LL | 21..30 => {} + | ------ this could appear to continue range `10_u8..20_u8`, but `20_u8` isn't matched by either of them +LL | 21..40 => {} + | ------ this could appear to continue range `10_u8..20_u8`, but `20_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:92:10 + | +LL | (0..10, true) => {} + | ^^^^^ + | | + | this range doesn't match `10_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=10_u8` +LL | (11..20, true) => {} + | ------ this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:97:16 + | +LL | (true, 0..10) => {} + | ^^^^^ + | | + | this range doesn't match `10_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=10_u8` +LL | (true, 11..20) => {} + | ------ this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:103:10 + | +LL | (0..10, true) => {} + | ^^^^^ + | | + | this range doesn't match `10_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=10_u8` +LL | (11..20, false) => {} + | ------ this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them + +error: multiple ranges are one apart + --> $DIR/gap_between_ranges.rs:113:14 + | +LL | Some(0..10) => {} + | ^^^^^ + | | + | this range doesn't match `10_u8` because `..` is an exclusive range + | help: use an inclusive range instead: `0_u8..=10_u8` +LL | Some(11..20) => {} + | ------ this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them + +error: aborting due to 16 previous errors; 1 warning emitted +