Skip to content

Commit 1b427b3

Browse files
borsbors
committed
Auto merge of #118879 - Nadrieril:lint-range-gap, r=estebank
Lint singleton gaps after exclusive ranges In the discussion to stabilize exclusive range patterns (#37854), it has often come up that they're likely to cause off-by-one mistakes. We already have the `overlapping_range_endpoints` lint, so I [proposed](#37854 (comment)) a lint to catch the complementary mistake. This PR adds a new `non_contiguous_range_endpoints` lint that catches likely off-by-one errors with exclusive range patterns. Here's the idea (see the test file for more examples): ```rust match x { 0..10 => ..., // WARN: this range doesn't match `10_u8` because `..` is an exclusive range 11..20 => ..., // this could appear to continue range `0_u8..10_u8`, but `10_u8` isn't matched by either of them _ => ..., } // help: use an inclusive range instead: `0_u8..=10_u8` ``` More precisely: for any exclusive range `lo..hi`, if `hi+1` is matched by another range but `hi` isn't, we suggest writing an inclusive range `lo..=hi` instead. We also catch `lo..T::MAX`.
2 parents 4d4bb49 + 8ac9a04 commit 1b427b3

21 files changed

+679
-157
lines changed

‎compiler/rustc_lint_defs/src/builtin.rs

+31
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ declare_lint_pass! {
6767
MISSING_FRAGMENT_SPECIFIER,
6868
MUST_NOT_SUSPEND,
6969
NAMED_ARGUMENTS_USED_POSITIONALLY,
70+
NON_CONTIGUOUS_RANGE_ENDPOINTS,
7071
NON_EXHAUSTIVE_OMITTED_PATTERNS,
7172
ORDER_DEPENDENT_TRAIT_OBJECTS,
7273
OVERLAPPING_RANGE_ENDPOINTS,
@@ -812,6 +813,36 @@ declare_lint! {
812813
"detects range patterns with overlapping endpoints"
813814
}
814815

816+
declare_lint! {
817+
/// The `non_contiguous_range_endpoints` lint detects likely off-by-one errors when using
818+
/// exclusive [range patterns].
819+
///
820+
/// [range patterns]: https://doc.rust-lang.org/nightly/reference/patterns.html#range-patterns
821+
///
822+
/// ### Example
823+
///
824+
/// ```rust
825+
/// # #![feature(exclusive_range_pattern)]
826+
/// let x = 123u32;
827+
/// match x {
828+
/// 0..100 => { println!("small"); }
829+
/// 101..1000 => { println!("large"); }
830+
/// _ => { println!("larger"); }
831+
/// }
832+
/// ```
833+
///
834+
/// {{produces}}
835+
///
836+
/// ### Explanation
837+
///
838+
/// It is likely a mistake to have range patterns in a match expression that miss out a single
839+
/// number. Check that the beginning and end values are what you expect, and keep in mind that
840+
/// with `..=` the right bound is inclusive, and with `..` it is exclusive.
841+
pub NON_CONTIGUOUS_RANGE_ENDPOINTS,
842+
Warn,
843+
"detects off-by-one errors with exclusive range patterns"
844+
}
845+
815846
declare_lint! {
816847
/// The `bindings_with_variant_name` lint detects pattern bindings with
817848
/// the same name as one of the matched variants.

‎compiler/rustc_pattern_analysis/messages.ftl

+8
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
1+
pattern_analysis_excluside_range_missing_gap = multiple ranges are one apart
2+
.label = this range doesn't match `{$gap}` because `..` is an exclusive range
3+
.suggestion = use an inclusive range instead
4+
5+
pattern_analysis_excluside_range_missing_max = exclusive range missing `{$max}`
6+
.label = this range doesn't match `{$max}` because `..` is an exclusive range
7+
.suggestion = use an inclusive range instead
8+
19
pattern_analysis_non_exhaustive_omitted_pattern = some variants are not matched explicitly
210
.help = ensure that all variants are matched explicitly by adding the suggested match arms
311
.note = the matched value is of type `{$scrut_ty}` and the `non_exhaustive_omitted_patterns` attribute was found

‎compiler/rustc_pattern_analysis/src/constructor.rs

+19-16
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ impl fmt::Display for RangeEnd {
192192
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
193193
pub enum MaybeInfiniteInt {
194194
NegInfinity,
195-
/// Encoded value. DO NOT CONSTRUCT BY HAND; use `new_finite`.
195+
/// Encoded value. DO NOT CONSTRUCT BY HAND; use `new_finite_{int,uint}`.
196196
#[non_exhaustive]
197197
Finite(u128),
198198
/// The integer after `u128::MAX`. We need it to represent `x..=u128::MAX` as an exclusive range.
@@ -229,25 +229,22 @@ impl MaybeInfiniteInt {
229229
}
230230

231231
/// Note: this will not turn a finite value into an infinite one or vice-versa.
232-
pub fn minus_one(self) -> Self {
232+
pub fn minus_one(self) -> Option<Self> {
233233
match self {
234-
Finite(n) => match n.checked_sub(1) {
235-
Some(m) => Finite(m),
236-
None => panic!("Called `MaybeInfiniteInt::minus_one` on 0"),
237-
},
238-
JustAfterMax => Finite(u128::MAX),
239-
x => x,
234+
Finite(n) => n.checked_sub(1).map(Finite),
235+
JustAfterMax => Some(Finite(u128::MAX)),
236+
x => Some(x),
240237
}
241238
}
242239
/// Note: this will not turn a finite value into an infinite one or vice-versa.
243-
pub fn plus_one(self) -> Self {
240+
pub fn plus_one(self) -> Option<Self> {
244241
match self {
245242
Finite(n) => match n.checked_add(1) {
246-
Some(m) => Finite(m),
247-
None => JustAfterMax,
243+
Some(m) => Some(Finite(m)),
244+
None => Some(JustAfterMax),
248245
},
249-
JustAfterMax => panic!("Called `MaybeInfiniteInt::plus_one` on u128::MAX+1"),
250-
x => x,
246+
JustAfterMax => None,
247+
x => Some(x),
251248
}
252249
}
253250
}
@@ -268,18 +265,24 @@ impl IntRange {
268265
pub fn is_singleton(&self) -> bool {
269266
// Since `lo` and `hi` can't be the same `Infinity` and `plus_one` never changes from finite
270267
// to infinite, this correctly only detects ranges that contain exacly one `Finite(x)`.
271-
self.lo.plus_one() == self.hi
268+
self.lo.plus_one() == Some(self.hi)
272269
}
273270

271+
/// Construct a singleton range.
272+
/// `x` must be a `Finite(_)` value.
274273
#[inline]
275274
pub fn from_singleton(x: MaybeInfiniteInt) -> IntRange {
276-
IntRange { lo: x, hi: x.plus_one() }
275+
// `unwrap()` is ok on a finite value
276+
IntRange { lo: x, hi: x.plus_one().unwrap() }
277277
}
278278

279+
/// Construct a range with these boundaries.
280+
/// `lo` must not be `PosInfinity` or `JustAfterMax`. `hi` must not be `NegInfinity`.
281+
/// If `end` is `Included`, `hi` must also not be `JustAfterMax`.
279282
#[inline]
280283
pub fn from_range(lo: MaybeInfiniteInt, mut hi: MaybeInfiniteInt, end: RangeEnd) -> IntRange {
281284
if end == RangeEnd::Included {
282-
hi = hi.plus_one();
285+
hi = hi.plus_one().unwrap();
283286
}
284287
if lo >= hi {
285288
// This should have been caught earlier by E0030.

‎compiler/rustc_pattern_analysis/src/errors.rs

+51
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,57 @@ impl<'tcx> AddToDiagnostic for Overlap<'tcx> {
7676
}
7777
}
7878

79+
#[derive(LintDiagnostic)]
80+
#[diag(pattern_analysis_excluside_range_missing_max)]
81+
pub struct ExclusiveRangeMissingMax<'tcx> {
82+
#[label]
83+
#[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")]
84+
/// This is an exclusive range that looks like `lo..max` (i.e. doesn't match `max`).
85+
pub first_range: Span,
86+
/// Suggest `lo..=max` instead.
87+
pub suggestion: String,
88+
pub max: Pat<'tcx>,
89+
}
90+
91+
#[derive(LintDiagnostic)]
92+
#[diag(pattern_analysis_excluside_range_missing_gap)]
93+
pub struct ExclusiveRangeMissingGap<'tcx> {
94+
#[label]
95+
#[suggestion(code = "{suggestion}", applicability = "maybe-incorrect")]
96+
/// This is an exclusive range that looks like `lo..gap` (i.e. doesn't match `gap`).
97+
pub first_range: Span,
98+
pub gap: Pat<'tcx>,
99+
/// Suggest `lo..=gap` instead.
100+
pub suggestion: String,
101+
#[subdiagnostic]
102+
/// All these ranges skipped over `gap` which we think is probably a mistake.
103+
pub gap_with: Vec<GappedRange<'tcx>>,
104+
}
105+
106+
pub struct GappedRange<'tcx> {
107+
pub span: Span,
108+
pub gap: Pat<'tcx>,
109+
pub first_range: Pat<'tcx>,
110+
}
111+
112+
impl<'tcx> AddToDiagnostic for GappedRange<'tcx> {
113+
fn add_to_diagnostic_with<G: EmissionGuarantee, F: SubdiagMessageOp<G>>(
114+
self,
115+
diag: &mut Diag<'_, G>,
116+
_: F,
117+
) {
118+
let GappedRange { span, gap, first_range } = self;
119+
120+
// FIXME(mejrs) unfortunately `#[derive(LintDiagnostic)]`
121+
// does not support `#[subdiagnostic(eager)]`...
122+
let message = format!(
123+
"this could appear to continue range `{first_range}`, but `{gap}` isn't matched by \
124+
either of them"
125+
);
126+
diag.span_label(span, message);
127+
}
128+
}
129+
79130
#[derive(LintDiagnostic)]
80131
#[diag(pattern_analysis_non_exhaustive_omitted_pattern)]
81132
#[help]

‎compiler/rustc_pattern_analysis/src/lib.rs

+16-7
Original file line numberDiff line numberDiff line change
@@ -70,14 +70,8 @@ use rustc_middle::ty::Ty;
7070
use rustc_span::ErrorGuaranteed;
7171

7272
use crate::constructor::{Constructor, ConstructorSet, IntRange};
73-
#[cfg(feature = "rustc")]
74-
use crate::lints::lint_nonexhaustive_missing_variants;
7573
use crate::pat::DeconstructedPat;
7674
use crate::pat_column::PatternColumn;
77-
#[cfg(feature = "rustc")]
78-
use crate::rustc::RustcMatchCheckCtxt;
79-
#[cfg(feature = "rustc")]
80-
use crate::usefulness::{compute_match_usefulness, ValidityConstraint};
8175

8276
pub trait Captures<'a> {}
8377
impl<'a, T: ?Sized> Captures<'a> for T {}
@@ -145,6 +139,18 @@ pub trait TypeCx: Sized + fmt::Debug {
145139

146140
/// The maximum pattern complexity limit was reached.
147141
fn complexity_exceeded(&self) -> Result<(), Self::Error>;
142+
143+
/// Lint that there is a gap `gap` between `pat` and all of `gapped_with` such that the gap is
144+
/// not matched by another range. If `gapped_with` is empty, then `gap` is `T::MAX`. We only
145+
/// detect singleton gaps.
146+
/// The default implementation does nothing.
147+
fn lint_non_contiguous_range_endpoints(
148+
&self,
149+
_pat: &DeconstructedPat<Self>,
150+
_gap: IntRange,
151+
_gapped_with: &[&DeconstructedPat<Self>],
152+
) {
153+
}
148154
}
149155

150156
/// The arm of a match expression.
@@ -167,11 +173,14 @@ impl<'p, Cx: TypeCx> Copy for MatchArm<'p, Cx> {}
167173
/// useful, and runs some lints.
168174
#[cfg(feature = "rustc")]
169175
pub fn analyze_match<'p, 'tcx>(
170-
tycx: &RustcMatchCheckCtxt<'p, 'tcx>,
176+
tycx: &rustc::RustcMatchCheckCtxt<'p, 'tcx>,
171177
arms: &[rustc::MatchArm<'p, 'tcx>],
172178
scrut_ty: Ty<'tcx>,
173179
pattern_complexity_limit: Option<usize>,
174180
) -> Result<rustc::UsefulnessReport<'p, 'tcx>, ErrorGuaranteed> {
181+
use lints::lint_nonexhaustive_missing_variants;
182+
use usefulness::{compute_match_usefulness, ValidityConstraint};
183+
175184
let scrut_ty = tycx.reveal_opaque_ty(scrut_ty);
176185
let scrut_validity = ValidityConstraint::from_bool(tycx.known_valid_scrutinee);
177186
let report =

‎compiler/rustc_pattern_analysis/src/rustc.rs

+68-4
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use rustc_index::{Idx, IndexVec};
88
use rustc_middle::middle::stability::EvalResult;
99
use rustc_middle::mir::interpret::Scalar;
1010
use rustc_middle::mir::{self, Const};
11-
use rustc_middle::thir::{FieldPat, Pat, PatKind, PatRange, PatRangeBoundary};
11+
use rustc_middle::thir::{self, FieldPat, Pat, PatKind, PatRange, PatRangeBoundary};
1212
use rustc_middle::ty::layout::IntegerExt;
1313
use rustc_middle::ty::{self, FieldDef, OpaqueTypeKey, Ty, TyCtxt, TypeVisitableExt, VariantDef};
1414
use rustc_session::lint;
@@ -718,12 +718,12 @@ impl<'p, 'tcx: 'p> RustcMatchCheckCtxt<'p, 'tcx> {
718718
let value = mir::Const::from_ty_const(c, cx.tcx);
719719
lo = PatRangeBoundary::Finite(value);
720720
}
721-
let hi = if matches!(range.hi, Finite(0)) {
721+
let hi = if let Some(hi) = range.hi.minus_one() {
722+
hi
723+
} else {
722724
// The range encodes `..ty::MIN`, so we can't convert it to an inclusive range.
723725
end = rustc_hir::RangeEnd::Excluded;
724726
range.hi
725-
} else {
726-
range.hi.minus_one()
727727
};
728728
let hi = cx.hoist_pat_range_bdy(hi, ty);
729729
PatKind::Range(Box::new(PatRange { lo, hi, end, ty: ty.inner() }))
@@ -900,6 +900,70 @@ impl<'p, 'tcx: 'p> TypeCx for RustcMatchCheckCtxt<'p, 'tcx> {
900900
let span = self.whole_match_span.unwrap_or(self.scrut_span);
901901
Err(self.tcx.dcx().span_err(span, "reached pattern complexity limit"))
902902
}
903+
904+
fn lint_non_contiguous_range_endpoints(
905+
&self,
906+
pat: &crate::pat::DeconstructedPat<Self>,
907+
gap: IntRange,
908+
gapped_with: &[&crate::pat::DeconstructedPat<Self>],
909+
) {
910+
let Some(&thir_pat) = pat.data() else { return };
911+
let thir::PatKind::Range(range) = &thir_pat.kind else { return };
912+
// Only lint when the left range is an exclusive range.
913+
if range.end != rustc_hir::RangeEnd::Excluded {
914+
return;
915+
}
916+
// `pat` is an exclusive range like `lo..gap`. `gapped_with` contains ranges that start with
917+
// `gap+1`.
918+
let suggested_range: thir::Pat<'_> = {
919+
// Suggest `lo..=gap` instead.
920+
let mut suggested_range = thir_pat.clone();
921+
let thir::PatKind::Range(range) = &mut suggested_range.kind else { unreachable!() };
922+
range.end = rustc_hir::RangeEnd::Included;
923+
suggested_range
924+
};
925+
let gap_as_pat = self.hoist_pat_range(&gap, *pat.ty());
926+
if gapped_with.is_empty() {
927+
// If `gapped_with` is empty, `gap == T::MAX`.
928+
self.tcx.emit_node_span_lint(
929+
lint::builtin::NON_CONTIGUOUS_RANGE_ENDPOINTS,
930+
self.match_lint_level,
931+
thir_pat.span,
932+
errors::ExclusiveRangeMissingMax {
933+
// Point at this range.
934+
first_range: thir_pat.span,
935+
// That's the gap that isn't covered.
936+
max: gap_as_pat.clone(),
937+
// Suggest `lo..=max` instead.
938+
suggestion: suggested_range.to_string(),
939+
},
940+
);
941+
} else {
942+
self.tcx.emit_node_span_lint(
943+
lint::builtin::NON_CONTIGUOUS_RANGE_ENDPOINTS,
944+
self.match_lint_level,
945+
thir_pat.span,
946+
errors::ExclusiveRangeMissingGap {
947+
// Point at this range.
948+
first_range: thir_pat.span,
949+
// That's the gap that isn't covered.
950+
gap: gap_as_pat.clone(),
951+
// Suggest `lo..=gap` instead.
952+
suggestion: suggested_range.to_string(),
953+
// All these ranges skipped over `gap` which we think is probably a
954+
// mistake.
955+
gap_with: gapped_with
956+
.iter()
957+
.map(|pat| errors::GappedRange {
958+
span: pat.data().unwrap().span,
959+
gap: gap_as_pat.clone(),
960+
first_range: thir_pat.clone(),
961+
})
962+
.collect(),
963+
},
964+
);
965+
}
966+
}
903967
}
904968

905969
/// Recursively expand this pattern into its subpatterns. Only useful for or-patterns.

0 commit comments

Comments
 (0)