Skip to content

Commit

Permalink
Lint against empty match on not-known-valid place
Browse files Browse the repository at this point in the history
  • Loading branch information
Nadrieril committed Dec 19, 2023
1 parent 725702e commit 8587f11
Show file tree
Hide file tree
Showing 11 changed files with 486 additions and 41 deletions.
71 changes: 71 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ declare_lint_pass! {
DUPLICATE_MACRO_ATTRIBUTES,
ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT,
ELIDED_LIFETIMES_IN_PATHS,
EMPTY_MATCH_ON_UNSAFE_PLACE,
EXPORTED_PRIVATE_DEPENDENCIES,
FFI_UNWIND_CALLS,
FORBIDDEN_LINT_GROUPS,
Expand Down Expand Up @@ -4018,6 +4019,76 @@ declare_lint! {
@feature_gate = sym::non_exhaustive_omitted_patterns_lint;
}

declare_lint! {
/// The `empty_match_on_unsafe_place` lint detects uses of `match ... {}` on an empty type where
/// the matched place could contain invalid data in a well-defined program. These matches are
/// considered exhaustive for backwards-compatibility, but they shouldn't be since a `_` arm
/// would be reachable.
///
/// This will become an error in the future.
///
/// ### Example
///
/// ```compile_fail
/// #![feature(min_exhaustive_patterns)]
/// enum Void {}
/// let ptr: *const Void = ...;
/// unsafe {
/// match *ptr {}
/// }
/// ```
///
/// This will produce:
///
/// ```text
/// warning: empty match on potentially-invalid data
/// --> $DIR/empty-types.rs:157:9
/// |
/// LL | match *ptr {}
/// | ^^^^^^^^^^^^^
/// |
/// note: this place can hold invalid data, which would make the match reachable
/// --> $DIR/empty-types.rs:157:15
/// |
/// LL | match *ptr {}
/// | ^^^^
/// = note: `#[warn(empty_match_on_unsafe_place)]` on by default
/// help: consider forcing a read of the value
/// |
/// LL | match { *ptr } {}
/// | + +
/// ```
///
/// ### Explanation
///
/// Some place expressions (namely pointer dereferences, union field accesses, and
/// (conservatively) reference dereferences) can hold invalid data without causing UB. For
/// example, the following is a well-defined program that prints "reachable!".
///
/// ```rust
/// #[derive(Copy, Clone)]
/// enum Void {}
/// union Uninit<T: Copy> {
/// value: T,
/// uninit: (),
/// }
/// unsafe {
/// let x: Uninit<Void> = Uninit { uninit: () };
/// match x.value {
/// _ => println!("reachable!"),
/// }
/// }
/// ```
///
/// Therefore when the matched place can hold invalid data, a match with no arm should not be
/// considered exhaustive. For backwards-compatibility we consider them exhaustive but warn with
/// this lint. It will become an error in the future.
pub EMPTY_MATCH_ON_UNSAFE_PLACE,
Warn,
"warn about empty matches on a place with potentially-invalid data",
@feature_gate = sym::min_exhaustive_patterns;
}

declare_lint! {
/// The `text_direction_codepoint_in_comment` lint detects Unicode codepoints in comments that
/// change the visual representation of text on screen in a way that does not correspond to
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_pattern_analysis/messages.ftl
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
pattern_analysis_empty_match_on_unsafe_place =
empty match on potentially-invalid data
.note = this place can hold invalid data, which would make the match reachable
pattern_analysis_empty_match_on_unsafe_place_wrap_suggestion =
consider forcing a read of the value
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
Expand Down
21 changes: 21 additions & 0 deletions compiler/rustc_pattern_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,27 @@ impl<'tcx> Uncovered<'tcx> {
}
}

#[derive(LintDiagnostic)]
#[diag(pattern_analysis_empty_match_on_unsafe_place)]
pub struct EmptyMatchOnUnsafePlace {
#[note]
pub scrut_span: Span,
#[subdiagnostic]
pub suggestion: EmptyMatchOnUnsafePlaceWrapSuggestion,
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(
pattern_analysis_empty_match_on_unsafe_place_wrap_suggestion,
applicability = "maybe-incorrect"
)]
pub struct EmptyMatchOnUnsafePlaceWrapSuggestion {
#[suggestion_part(code = "{{ ")]
pub scrut_start: Span,
#[suggestion_part(code = " }}")]
pub scrut_end: Span,
}

#[derive(LintDiagnostic)]
#[diag(pattern_analysis_overlapping_range_endpoints)]
#[note]
Expand Down
57 changes: 43 additions & 14 deletions compiler/rustc_pattern_analysis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,7 @@ use rustc_index::Idx;
use rustc_middle::ty::Ty;

use crate::constructor::{Constructor, ConstructorSet};
#[cfg(feature = "rustc")]
use crate::lints::{
lint_nonexhaustive_missing_variants, lint_overlapping_range_endpoints, PatternColumn,
};
use crate::pat::DeconstructedPat;
#[cfg(feature = "rustc")]
use crate::rustc::RustcMatchCheckCtxt;
#[cfg(feature = "rustc")]
use crate::usefulness::{compute_match_usefulness, ValidityConstraint};

// It's not possible to only enable the `typed_arena` dependency when the `rustc` feature is off, so
// we use another feature instead. The crate won't compile if one of these isn't enabled.
Expand Down Expand Up @@ -110,26 +102,63 @@ 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>,
) -> rustc::UsefulnessReport<'p, 'tcx> {
use rustc_middle::ty;
use rustc_session::lint;

// Arena to store the extra wildcards we construct during analysis.
let wildcard_arena = tycx.pattern_arena;
let scrut_validity = ValidityConstraint::from_bool(tycx.known_valid_scrutinee);
let scrut_validity = usefulness::ValidityConstraint::from_bool(tycx.known_valid_scrutinee);
let cx = MatchCtxt { tycx, wildcard_arena };

let report = compute_match_usefulness(cx, arms, scrut_ty, scrut_validity);
if !tycx.known_valid_scrutinee && arms.iter().all(|arm| arm.has_guard) {
let is_directly_empty = match scrut_ty.kind() {
ty::Adt(def, ..) => {
def.is_enum()
&& def.variants().is_empty()
&& !tycx.is_foreign_non_exhaustive_enum(scrut_ty)
}
ty::Never => true,
_ => false,
};
if is_directly_empty {
if tycx.tcx.features().min_exhaustive_patterns {
tycx.tcx.emit_spanned_lint(
lint::builtin::EMPTY_MATCH_ON_UNSAFE_PLACE,
tycx.match_lint_level,
tycx.whole_match_span.unwrap_or(tycx.scrut_span),
errors::EmptyMatchOnUnsafePlace {
scrut_span: tycx.scrut_span,
suggestion: errors::EmptyMatchOnUnsafePlaceWrapSuggestion {
scrut_start: tycx.scrut_span.shrink_to_lo(),
scrut_end: tycx.scrut_span.shrink_to_hi(),
},
},
);
}

// For backwards compability we allow an empty match in this case.
return rustc::UsefulnessReport {
arm_usefulness: Vec::new(),
non_exhaustiveness_witnesses: Vec::new(),
};
}
}

let report = usefulness::compute_match_usefulness(cx, arms, scrut_ty, scrut_validity);

let pat_column = PatternColumn::new(arms);
let pat_column = lints::PatternColumn::new(arms);

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

// 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.
if tycx.refutable && report.non_exhaustiveness_witnesses.is_empty() {
lint_nonexhaustive_missing_variants(cx, arms, &pat_column, scrut_ty)
lints::lint_nonexhaustive_missing_variants(cx, arms, &pat_column, scrut_ty)
}

report
Expand Down
12 changes: 4 additions & 8 deletions compiler/rustc_pattern_analysis/src/usefulness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1199,23 +1199,19 @@ fn compute_exhaustiveness_and_usefulness<'a, 'p, Cx: TypeCx>(

debug!("ty: {ty:?}");
let pcx = &PlaceCtxt { mcx, ty, is_scrutinee: is_top_level };
let ctors_for_ty = pcx.ctors_for_ty();
let is_integers = matches!(ctors_for_ty, ConstructorSet::Integers { .. }); // For diagnostics.

// Whether the place/column we are inspecting is known to contain valid data.
let mut place_validity = matrix.place_validity[0];
if !mcx.tycx.is_min_exhaustive_patterns_feature_on()
|| (is_top_level && matches!(ctors_for_ty, ConstructorSet::NoConstructors))
{
// For backwards compability we allow omitting some empty arms that we ideally shouldn't.
if mcx.tycx.is_exhaustive_patterns_feature_on() {
// Under `exhaustive_patterns` we allow omitting empty arms even when they aren't redundant.
place_validity = place_validity.allow_omitting_side_effecting_arms();
}

// Analyze the constructors present in this column.
let ctors_for_ty = pcx.ctors_for_ty();
let is_integers = matches!(ctors_for_ty, ConstructorSet::Integers { .. }); // For diagnostics.
let ctors = matrix.heads().map(|p| p.ctor());
let split_set = ctors_for_ty.split(pcx, ctors);

// Decide what constructors to report.
let all_missing = split_set.present.is_empty();

// Build the set of constructors we will specialize with. It must cover the whole type.
Expand Down
Loading

0 comments on commit 8587f11

Please sign in to comment.