Skip to content

Coalesce unreachable_patterns lint messages #107262

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
12 changes: 9 additions & 3 deletions compiler/rustc_error_messages/locales/en-US/mir_build.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,15 @@ mir_build_const_param_in_pattern = const parameters cannot be referenced in patt

mir_build_non_const_path = runtime values cannot be referenced in patterns

mir_build_unreachable_pattern = unreachable pattern
.label = unreachable pattern
.catchall_label = matches any value
mir_build_unreachable_patterns = {$count ->
[one] unreachable pattern
*[more] multiple unreachable patterns
}
.catchall_label = this pattern is irrefutable; subsequent arms are never executed

mir_build_never_executed = this arm is never executed

mir_build_unreachable_pattern = this pattern is unreachable

mir_build_const_pattern_depends_on_generic_parameter =
constant pattern depends on a generic parameter
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ pub fn explain_lint_level_source(
/// // ^^^^^^^^^^^^^^^^^^^^^ returns `&mut DiagnosticBuilder` by default
/// )
/// ```
#[track_caller]
pub fn struct_lint_level(
sess: &Session,
lint: &'static Lint,
Expand All @@ -311,6 +312,7 @@ pub fn struct_lint_level(
) {
// Avoid codegen bloat from monomorphization by immediately doing dyn dispatch of `decorate` to
// the "real" work.
#[track_caller]
fn struct_lint_level_impl(
sess: &Session,
lint: &'static Lint,
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2075,6 +2075,7 @@ impl<'tcx> TyCtxt<'tcx> {

/// Emit a lint at `span` from a lint struct (some type that implements `DecorateLint`,
/// typically generated by `#[derive(LintDiagnostic)]`).
#[track_caller]
pub fn emit_spanned_lint(
self,
lint: &'static Lint,
Expand Down Expand Up @@ -2111,6 +2112,7 @@ impl<'tcx> TyCtxt<'tcx> {

/// Emit a lint from a lint struct (some type that implements `DecorateLint`, typically
/// generated by `#[derive(LintDiagnostic)]`).
#[track_caller]
pub fn emit_lint(
self,
lint: &'static Lint,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/build/custom/parse.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ macro_rules! parse_by_kind {
let expr = &$self.thir[expr_id];
debug!("Trying to parse {:?} as {}", expr.kind, $expected);
let $expr_name = expr;
#[allow(unreachable_patterns)]
match &expr.kind {
$(
ExprKind::Call { ty, fun: _, args: $args, .. } if {
Expand All @@ -50,7 +51,6 @@ macro_rules! parse_by_kind {
$(
$pat => $expr,
)*
#[allow(unreachable_patterns)]
_ => return Err($self.expr_error(expr_id, $expected))
}
}};
Expand Down
23 changes: 19 additions & 4 deletions compiler/rustc_mir_build/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,14 +465,29 @@ pub struct NonConstPath {
}

#[derive(LintDiagnostic)]
#[diag(mir_build_unreachable_pattern)]
pub struct UnreachablePattern {
#[label]
pub span: Option<Span>,
#[diag(mir_build_unreachable_patterns)]
pub struct UnreachablePatterns {
#[subdiagnostic]
pub unreachable_arms: Vec<UnreachableArm>,
pub count: usize,
#[label(catchall_label)]
pub catchall: Option<Span>,
}

#[derive(Subdiagnostic)]
pub enum UnreachableArm {
#[label(mir_build_never_executed)]
NeverExecuted {
#[primary_span]
span: Span,
},
#[label(mir_build_unreachable_pattern)]
PatternUnreachable {
#[primary_span]
span: Span,
},
}

#[derive(Diagnostic)]
#[diag(mir_build_const_pattern_depends_on_generic_parameter)]
pub struct ConstPatternDependsOnGenericParameter {
Expand Down
142 changes: 112 additions & 30 deletions compiler/rustc_mir_build/src/thir/pattern/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,17 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> {
match source {
// Don't report arm reachability of desugared `match $iter.into_iter() { iter => .. }`
// when the iterator is an uninhabited type. unreachable_code will trigger instead.
hir::MatchSource::ForLoopDesugar if arms.len() == 1 => {}
hir::MatchSource::ForLoopDesugar
| hir::MatchSource::Normal
| hir::MatchSource::FormatArgs => report_arm_reachability(&cx, &report),
hir::MatchSource::ForLoopDesugar if arms.len() == 1 => {},
hir::MatchSource::ForLoopDesugar | hir::MatchSource::FormatArgs => {
report_let_reachability(scrut.hir_id, &cx, &report)
},
hir::MatchSource::Normal => {
if let ty::Adt(def, _) = scrut_ty.kind() && def.is_enum() && def.variants().is_empty() {
report_empty_enum_arms(scrut.hir_id, hir_arms, &cx)
} else {
report_arm_reachability(scrut.hir_id, hir_arms, &cx, &report)
}
}
// Unreachable patterns in try and await expressions occur when one of
// the arms are an uninhabited type. Which is OK.
hir::MatchSource::AwaitDesugar | hir::MatchSource::TryDesugar => {}
Expand Down Expand Up @@ -520,15 +527,6 @@ fn pat_is_catchall(pat: &DeconstructedPat<'_, '_>) -> bool {
}
}

fn unreachable_pattern(tcx: TyCtxt<'_>, span: Span, id: HirId, catchall: Option<Span>) {
tcx.emit_spanned_lint(
UNREACHABLE_PATTERNS,
id,
span,
UnreachablePattern { span: if catchall.is_some() { Some(span) } else { None }, catchall },
);
}

fn irrefutable_let_pattern(tcx: TyCtxt<'_>, id: HirId, span: Span) {
let source = let_source(tcx, id);
irrefutable_let_patterns(tcx, id, source, 1, span);
Expand Down Expand Up @@ -561,43 +559,127 @@ fn is_let_irrefutable<'p, 'tcx>(
pat_id: HirId,
pat: &'p DeconstructedPat<'p, 'tcx>,
) -> bool {
let arms = [MatchArm { pat, hir_id: pat_id, has_guard: false }];
let report = compute_match_usefulness(&cx, &arms, pat_id, pat.ty());
let arms = MatchArm { pat, hir_id: pat_id, has_guard: false };
let report = compute_match_usefulness(&cx, &[arms], pat_id, pat.ty());

// Report if the pattern is unreachable, which can only occur when the type is uninhabited.
// This also reports unreachable sub-patterns though, so we can't just replace it with an
// `is_uninhabited` check.
report_arm_reachability(&cx, &report);
report_let_reachability(pat_id, &cx, &report);

// If the list of witnesses is empty, the match is exhaustive,
// i.e. the `if let` pattern is irrefutable.
report.non_exhaustiveness_witnesses.is_empty()
}

/// Report unreachable let branches, if any.
fn report_let_reachability<'p, 'tcx>(
scrut: HirId,
cx: &MatchCheckCtxt<'p, 'tcx>,
report: &UsefulnessReport<'p, 'tcx>,
) {
let mut unreachable_arms = Vec::new();
let mut spans = Vec::new();

for (arm, is_useful) in &report.arm_usefulness {
match is_useful {
Reachability::Unreachable => {
spans.push(arm.pat.span());
unreachable_arms.push(UnreachableArm::PatternUnreachable { span: arm.pat.span() });
}
// The arm is reachable, but contains unreachable subpatterns (from or-patterns).
Reachability::Reachable(unreachables) => {
for &span in unreachables {
spans.push(span);
unreachable_arms.push(UnreachableArm::PatternUnreachable { span });
}
}
}
}

if !unreachable_arms.is_empty() {
cx.tcx.emit_spanned_lint(
UNREACHABLE_PATTERNS,
scrut,
spans,
UnreachablePatterns { count: unreachable_arms.len(), unreachable_arms, catchall: None },
);
}
}

/// Report unreachable arms, if any.
fn report_arm_reachability<'p, 'tcx>(
scrut: HirId,
hir_arms: &'tcx [hir::Arm<'tcx>],
cx: &MatchCheckCtxt<'p, 'tcx>,
report: &UsefulnessReport<'p, 'tcx>,
) {
use Reachability::*;
let mut catchall = None;
for (arm, is_useful) in report.arm_usefulness.iter() {
let mut unreachable_arms = Vec::new();

let mut spans = Vec::new();
for ((_, is_useful), hir::Arm { span, .. }) in report.arm_usefulness.iter().zip(hir_arms) {
match is_useful {
Unreachable => unreachable_pattern(cx.tcx, arm.pat.span(), arm.hir_id, catchall),
Reachable(unreachables) if unreachables.is_empty() => {}
Reachability::Unreachable => {
spans.push(*span);
unreachable_arms.push(UnreachableArm::NeverExecuted { span: *span });
}
// The arm is reachable, but contains unreachable subpatterns (from or-patterns).
Reachable(unreachables) => {
let mut unreachables = unreachables.clone();
// Emit lints in the order in which they occur in the file.
unreachables.sort_unstable();
for span in unreachables {
unreachable_pattern(cx.tcx, span, arm.hir_id, None);
Reachability::Reachable(unreachables) => {
for &span in unreachables {
spans.push(span);
unreachable_arms.push(UnreachableArm::PatternUnreachable { span });
}
}
}
if !arm.has_guard && catchall.is_none() && pat_is_catchall(arm.pat) {
catchall = Some(arm.pat.span());
}
}

// Mention it if there's a catch-all arm, but not if it's the last one.
let catchall = if let [arms @ .., _last] = &*report.arm_usefulness {
arms.iter()
.filter_map(|(arm, _)| {
(!arm.has_guard && pat_is_catchall(arm.pat)).then(|| arm.pat.span())
})
.next()
} else {
None
};
if let Some(span) = catchall {
spans.push(span);
}

if !unreachable_arms.is_empty() {
cx.tcx.emit_spanned_lint(
UNREACHABLE_PATTERNS,
scrut,
spans,
UnreachablePatterns { count: unreachable_arms.len(), unreachable_arms, catchall },
);
}
}

/// Report every arm matching on , if any.
fn report_empty_enum_arms<'p, 'tcx>(
scrut: HirId,
hir_arms: &'tcx [hir::Arm<'tcx>],
cx: &MatchCheckCtxt<'p, 'tcx>,
) {
let mut spans = Vec::new();

let unreachable_arms = hir_arms
.iter()
.map(|arm| {
spans.push(arm.span);
UnreachableArm::NeverExecuted { span: arm.span }
})
.collect::<Vec<_>>();

if !unreachable_arms.is_empty() {
cx.tcx.emit_spanned_lint(
UNREACHABLE_PATTERNS,
scrut,
spans,
UnreachablePatterns { count: unreachable_arms.len(), unreachable_arms, catchall: None },
);
}
}

Expand Down
6 changes: 3 additions & 3 deletions tests/ui/consts/const_in_pattern/issue-78057.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ LL | FOO => {},
| ^^^

error: unreachable pattern
--> $DIR/issue-78057.rs:14:9
--> $DIR/issue-78057.rs:12:9
|
LL | FOO => {},
| --- matches any value
| ^^^ this pattern is irrefutable; subsequent arms are never executed
LL |
LL | _ => {}
| ^ unreachable pattern
| ^^^^^^^ this arm is never executed
Copy link
Contributor

Choose a reason for hiding this comment

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

this may get very verbose for longer arms, maybe keep pointing to the pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the other diagnostic changes I think we should just keep the previous unreachable pattern, considering the other message is so much more helpful now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean with "the other message"? I'm afraid I'm not sure what you are proposing.

Copy link
Contributor Author

@mejrs mejrs Jan 27, 2023

Choose a reason for hiding this comment

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

Anyway: newbies tend to be more experienced with switch...case structures than pattern matching so I prefer saying "this arm is never executed" over "pattern is unreachable". As for the verbosity it'd be kinda weird to say "this arm is never executed" and just point at the pattern rather than the pattern and the entire code block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that argument, but it is orthogonal to the "make this diagnostic less spammy" change, so I would prefer to discuss it separately with wg-diagnostics, as it seems like a major divergence from existing diagnostics and we'd need to adjust other diagnostics, too.

Copy link
Contributor

@estebank estebank Feb 1, 2023

Choose a reason for hiding this comment

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

I'm in agreement with oli's points. We want to make the underlines as short as possible across the board. To address your concern around wording, you could say "this pattern cannot be reached, so its arm is never executed", while pointing only at the pattern. Otherwise, what you can do is have a primary span pointing at the pattern, and a secondary span pointing at the arm. This would be useful for some of the tests cases that involve macros, while still keeping the red underline in VSCode as short as possible.

|
note: the lint level is defined here
--> $DIR/issue-78057.rs:1:9
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/consts/packed_pattern.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ warning: unreachable pattern
--> $DIR/packed_pattern.rs:16:9
|
LL | FOO => unreachable!(),
| ^^^
| ^^^^^^^^^^^^^^^^^^^^^ this arm is never executed
|
= note: `#[warn(unreachable_patterns)]` on by default

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/consts/packed_pattern2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ warning: unreachable pattern
--> $DIR/packed_pattern2.rs:24:9
|
LL | FOO => unreachable!(),
| ^^^
| ^^^^^^^^^^^^^^^^^^^^^ this arm is never executed
|
= note: `#[warn(unreachable_patterns)]` on by default

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/error-codes/E0001.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: unreachable pattern
--> $DIR/E0001.rs:8:9
|
LL | _ => {/* ... */}
| ^
| ^^^^^^^^^^^^^^^^ this arm is never executed
|
note: the lint level is defined here
--> $DIR/E0001.rs:1:9
Expand Down
6 changes: 3 additions & 3 deletions tests/ui/lint/issue-30302.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@ LL | Nil => true,
= note: `#[deny(bindings_with_variant_name)]` on by default

error: unreachable pattern
--> $DIR/issue-30302.rs:15:9
--> $DIR/issue-30302.rs:13:9
|
LL | Nil => true,
| --- matches any value
| ^^^ this pattern is irrefutable; subsequent arms are never executed
LL |
LL | _ => false
| ^ unreachable pattern
| ^^^^^^^^^^ this arm is never executed
|
note: the lint level is defined here
--> $DIR/issue-30302.rs:4:9
Expand Down
Loading