diff options
5 files changed, 339 insertions, 45 deletions
diff --git a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs index e2c56df6014..8a3a46c1190 100644 --- a/compiler/rustc_mir_build/src/thir/pattern/check_match.rs +++ b/compiler/rustc_mir_build/src/thir/pattern/check_match.rs @@ -158,7 +158,7 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> { self.check_patterns(pat, Refutable); let mut cx = self.new_cx(scrutinee.hir_id); let tpat = self.lower_pattern(&mut cx, pat, &mut false); - check_let_reachability(&mut cx, pat.hir_id, tpat, span); + self.check_let_reachability(&mut cx, pat.hir_id, tpat, span); } fn check_match( @@ -176,7 +176,7 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> { if let Some(hir::Guard::IfLet(ref pat, _)) = arm.guard { self.check_patterns(pat, Refutable); let tpat = self.lower_pattern(&mut cx, pat, &mut false); - check_let_reachability(&mut cx, pat.hir_id, tpat, tpat.span()); + self.check_let_reachability(&mut cx, pat.hir_id, tpat, tpat.span()); } } @@ -224,6 +224,157 @@ impl<'p, 'tcx> MatchVisitor<'_, 'p, 'tcx> { } } + fn check_let_reachability( + &mut self, + cx: &mut MatchCheckCtxt<'p, 'tcx>, + pat_id: HirId, + pat: &'p DeconstructedPat<'p, 'tcx>, + span: Span, + ) { + if self.check_let_chain(cx, pat_id) { + return; + } + + if is_let_irrefutable(cx, pat_id, pat) { + irrefutable_let_pattern(cx.tcx, pat_id, span); + } + } + + fn check_let_chain(&mut self, cx: &mut MatchCheckCtxt<'p, 'tcx>, pat_id: HirId) -> bool { + let hir = self.tcx.hir(); + let parent = hir.get_parent_node(pat_id); + + // First, figure out if the given pattern is part of a let chain, + // and if so, obtain the top node of the chain. + let mut top = parent; + let mut part_of_chain = false; + loop { + let new_top = hir.get_parent_node(top); + if let hir::Node::Expr( + hir::Expr { + kind: hir::ExprKind::Binary(Spanned { node: hir::BinOpKind::And, .. }, lhs, rhs), + .. + }, + .., + ) = hir.get(new_top) + { + // If this isn't the first iteration, we need to check + // if there is a let expr before us in the chain, so + // that we avoid doubly checking the let chain. + + // The way a chain of &&s is encoded is ((let ... && let ...) && let ...) && let ... + // as && is left-to-right associative. Thus, we need to check rhs. + if part_of_chain && matches!(rhs.kind, hir::ExprKind::Let(..)) { + return true; + } + // If there is a let at the lhs, and we provide the rhs, we don't do any checking either. + if !part_of_chain && matches!(lhs.kind, hir::ExprKind::Let(..)) && rhs.hir_id == top + { + return true; + } + } else { + // We've reached the top. + break; + } + + // Since this function is called within a let context, it is reasonable to assume that any parent + // `&&` infers a let chain + part_of_chain = true; + top = new_top; + } + if !part_of_chain { + return false; + } + + // Second, obtain the refutabilities of all exprs in the chain, + // and record chain members that aren't let exprs. + let mut chain_refutabilities = Vec::new(); + let hir::Node::Expr(top_expr) = hir.get(top) else { + // We ensure right above that it's an Expr + unreachable!() + }; + let mut cur_expr = top_expr; + loop { + let mut add = |expr: &hir::Expr<'tcx>| { + let refutability = match expr.kind { + hir::ExprKind::Let(hir::Let { pat, init, span, .. }) => { + let mut ncx = self.new_cx(init.hir_id); + let tpat = self.lower_pattern(&mut ncx, pat, &mut false); + + let refutable = !is_let_irrefutable(&mut ncx, pat.hir_id, tpat); + Some((*span, refutable)) + } + _ => None, + }; + chain_refutabilities.push(refutability); + }; + if let hir::Expr { + kind: hir::ExprKind::Binary(Spanned { node: hir::BinOpKind::And, .. }, lhs, rhs), + .. + } = cur_expr + { + add(rhs); + cur_expr = lhs; + } else { + add(cur_expr); + break; + } + } + chain_refutabilities.reverse(); + + // Third, emit the actual warnings. + + if chain_refutabilities.iter().all(|r| matches!(*r, Some((_, false)))) { + // The entire chain is made up of irrefutable `let` statements + let let_source = let_source_parent(self.tcx, top, None); + irrefutable_let_patterns( + cx.tcx, + top, + let_source, + chain_refutabilities.len(), + top_expr.span, + ); + return true; + } + let lint_affix = |affix: &[Option<(Span, bool)>], kind, suggestion| { + let span_start = affix[0].unwrap().0; + let span_end = affix.last().unwrap().unwrap().0; + let span = span_start.to(span_end); + let cnt = affix.len(); + cx.tcx.struct_span_lint_hir(IRREFUTABLE_LET_PATTERNS, top, span, |lint| { + let s = pluralize!(cnt); + let mut diag = lint.build(&format!("{kind} irrefutable pattern{s} in let chain")); + diag.note(&format!( + "{these} pattern{s} will always match", + these = pluralize!("this", cnt), + )); + diag.help(&format!( + "consider moving {} {suggestion}", + if cnt > 1 { "them" } else { "it" } + )); + diag.emit() + }); + }; + if let Some(until) = chain_refutabilities.iter().position(|r| !matches!(*r, Some((_, false)))) && until > 0 { + // The chain has a non-zero prefix of irrefutable `let` statements. + + // Check if the let source is while, for there is no alternative place to put a prefix, + // and we shouldn't lint. + let let_source = let_source_parent(self.tcx, top, None); + if !matches!(let_source, LetSource::WhileLet) { + // Emit the lint + let prefix = &chain_refutabilities[..until]; + lint_affix(prefix, "leading", "outside of the construct"); + } + } + if let Some(from) = chain_refutabilities.iter().rposition(|r| !matches!(*r, Some((_, false)))) && from != (chain_refutabilities.len() - 1) { + // The chain has a non-empty suffix of irrefutable `let` statements + let suffix = &chain_refutabilities[from + 1..]; + lint_affix(suffix, "trailing", "into the body"); + } + true + } + fn check_irrefutable(&self, pat: &'tcx Pat<'tcx>, origin: &str, sp: Option<Span>) { let mut cx = self.new_cx(pat.hir_id); @@ -453,6 +604,17 @@ fn unreachable_pattern(tcx: TyCtxt<'_>, span: Span, id: HirId, catchall: Option< } fn irrefutable_let_pattern(tcx: TyCtxt<'_>, id: HirId, span: Span) { + let source = let_source(tcx, id); + irrefutable_let_patterns(tcx, id, source, 1, span); +} + +fn irrefutable_let_patterns( + tcx: TyCtxt<'_>, + id: HirId, + source: LetSource, + count: usize, + span: Span, +) { macro_rules! emit_diag { ( $lint:expr, @@ -460,14 +622,15 @@ fn irrefutable_let_pattern(tcx: TyCtxt<'_>, id: HirId, span: Span) { $note_sufix:expr, $help_sufix:expr ) => {{ - let mut diag = $lint.build(concat!("irrefutable ", $source_name, " pattern")); - diag.note(concat!("this pattern will always match, so the ", $note_sufix)); + let s = pluralize!(count); + let these = pluralize!("this", count); + let mut diag = $lint.build(&format!("irrefutable {} pattern{s}", $source_name)); + diag.note(&format!("{these} pattern{s} will always match, so the {}", $note_sufix)); diag.help(concat!("consider ", $help_sufix)); diag.emit() }}; } - let source = let_source(tcx, id); let span = match source { LetSource::LetElse(span) => span, _ => span, @@ -511,16 +674,11 @@ fn irrefutable_let_pattern(tcx: TyCtxt<'_>, id: HirId, span: Span) { }); } -fn check_let_reachability<'p, 'tcx>( +fn is_let_irrefutable<'p, 'tcx>( cx: &mut MatchCheckCtxt<'p, 'tcx>, pat_id: HirId, pat: &'p DeconstructedPat<'p, 'tcx>, - span: Span, -) { - if is_let_chain(cx.tcx, pat_id) { - return; - } - +) -> bool { let arms = [MatchArm { pat, hir_id: pat_id, has_guard: false }]; let report = compute_match_usefulness(&cx, &arms, pat_id, pat.ty()); @@ -529,10 +687,9 @@ fn check_let_reachability<'p, 'tcx>( // `is_uninhabited` check. report_arm_reachability(&cx, &report); - if report.non_exhaustiveness_witnesses.is_empty() { - // The match is exhaustive, i.e. the `if let` pattern is irrefutable. - irrefutable_let_pattern(cx.tcx, pat_id, span); - } + // 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 arms, if any. @@ -941,13 +1098,19 @@ fn let_source(tcx: TyCtxt<'_>, pat_id: HirId) -> LetSource { let hir = tcx.hir(); let parent = hir.get_parent_node(pat_id); + let_source_parent(tcx, parent, Some(pat_id)) +} + +fn let_source_parent(tcx: TyCtxt<'_>, parent: HirId, pat_id: Option<HirId>) -> LetSource { + let hir = tcx.hir(); + let parent_node = hir.get(parent); match parent_node { hir::Node::Arm(hir::Arm { guard: Some(hir::Guard::IfLet(&hir::Pat { hir_id, .. }, _)), .. - }) if hir_id == pat_id => { + }) if Some(hir_id) == pat_id => { return LetSource::IfLetGuard; } hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Let(..), span, .. }) => { @@ -980,21 +1143,3 @@ fn let_source(tcx: TyCtxt<'_>, pat_id: HirId) -> LetSource { LetSource::GenericLet } - -// Since this function is called within a let context, it is reasonable to assume that any parent -// `&&` infers a let chain -fn is_let_chain(tcx: TyCtxt<'_>, pat_id: HirId) -> bool { - let hir = tcx.hir(); - let parent = hir.get_parent_node(pat_id); - let parent_parent = hir.get_parent_node(parent); - matches!( - hir.get(parent_parent), - hir::Node::Expr( - hir::Expr { - kind: hir::ExprKind::Binary(Spanned { node: hir::BinOpKind::And, .. }, ..), - .. - }, - .. - ) - ) -} diff --git a/src/test/ui/mir/mir_let_chains_drop_order.rs b/src/test/ui/mir/mir_let_chains_drop_order.rs index 01f943c87dd..6498a519571 100644 --- a/src/test/ui/mir/mir_let_chains_drop_order.rs +++ b/src/test/ui/mir/mir_let_chains_drop_order.rs @@ -5,6 +5,7 @@ // See `mir_drop_order.rs` for more information #![feature(let_chains)] +#![allow(irrefutable_let_patterns)] use std::cell::RefCell; use std::panic; diff --git a/src/test/ui/rfc-2497-if-let-chains/ast-lowering-does-not-wrap-let-chains.rs b/src/test/ui/rfc-2497-if-let-chains/ast-lowering-does-not-wrap-let-chains.rs index 708bcdd0aef..d851fac8e64 100644 --- a/src/test/ui/rfc-2497-if-let-chains/ast-lowering-does-not-wrap-let-chains.rs +++ b/src/test/ui/rfc-2497-if-let-chains/ast-lowering-does-not-wrap-let-chains.rs @@ -1,6 +1,7 @@ // run-pass #![feature(let_chains)] +#![allow(irrefutable_let_patterns)] fn main() { let first = Some(1); diff --git a/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.disallowed.stderr b/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.disallowed.stderr new file mode 100644 index 00000000000..d1d5288aea3 --- /dev/null +++ b/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.disallowed.stderr @@ -0,0 +1,115 @@ +error: leading irrefutable pattern in let chain + --> $DIR/irrefutable-lets.rs:13:8 + | +LL | if let first = &opt && let Some(ref second) = first && let None = second.start {} + | ^^^^^^^^^^^^^^^^ + | +note: the lint level is defined here + --> $DIR/irrefutable-lets.rs:6:30 + | +LL | #![cfg_attr(disallowed, deny(irrefutable_let_patterns))] + | ^^^^^^^^^^^^^^^^^^^^^^^^ + = note: this pattern will always match + = help: consider moving it outside of the construct + +error: irrefutable `if let` patterns + --> $DIR/irrefutable-lets.rs:19:8 + | +LL | if let first = &opt && let (a, b) = (1, 2) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: these patterns will always match, so the `if let` is useless + = help: consider replacing the `if let` with a `let` + +error: leading irrefutable pattern in let chain + --> $DIR/irrefutable-lets.rs:22:8 + | +LL | if let first = &opt && let Some(ref second) = first && let None = second.start && let v = 0 {} + | ^^^^^^^^^^^^^^^^ + | + = note: this pattern will always match + = help: consider moving it outside of the construct + +error: trailing irrefutable pattern in let chain + --> $DIR/irrefutable-lets.rs:22:87 + | +LL | if let first = &opt && let Some(ref second) = first && let None = second.start && let v = 0 {} + | ^^^^^^^^^ + | + = note: this pattern will always match + = help: consider moving it into the body + +error: trailing irrefutable patterns in let chain + --> $DIR/irrefutable-lets.rs:26:37 + | +LL | if let Some(ref first) = opt && let second = first && let _third = second {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: these patterns will always match + = help: consider moving them into the body + +error: leading irrefutable pattern in let chain + --> $DIR/irrefutable-lets.rs:29:8 + | +LL | if let Range { start: local_start, end: _ } = (None..Some(1)) && let None = local_start {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this pattern will always match + = help: consider moving it outside of the construct + +error: leading irrefutable pattern in let chain + --> $DIR/irrefutable-lets.rs:32:8 + | +LL | if let (a, b, c) = (Some(1), Some(1), Some(1)) && let None = Some(1) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this pattern will always match + = help: consider moving it outside of the construct + +error: leading irrefutable pattern in let chain + --> $DIR/irrefutable-lets.rs:35:8 + | +LL | if let first = &opt && let None = Some(1) {} + | ^^^^^^^^^^^^^^^^ + | + = note: this pattern will always match + = help: consider moving it outside of the construct + +error: irrefutable `let` patterns + --> $DIR/irrefutable-lets.rs:44:28 + | +LL | Some(ref first) if let second = first && let _third = second && let v = 4 + 4 => {}, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: these patterns will always match, so the `let` is useless + = help: consider removing `let` + +error: leading irrefutable pattern in let chain + --> $DIR/irrefutable-lets.rs:50:28 + | +LL | Some(ref first) if let Range { start: local_start, end: _ } = first + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: this pattern will always match + = help: consider moving it outside of the construct + +error: irrefutable `while let` patterns + --> $DIR/irrefutable-lets.rs:59:11 + | +LL | while let first = &opt && let (a, b) = (1, 2) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: these patterns will always match, so the loop will never exit + = help: consider instead using a `loop { ... }` with a `let` inside it + +error: trailing irrefutable patterns in let chain + --> $DIR/irrefutable-lets.rs:62:40 + | +LL | while let Some(ref first) = opt && let second = first && let _third = second {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: these patterns will always match + = help: consider moving them into the body + +error: aborting due to 12 previous errors + diff --git a/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs b/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs index 945c665e35d..3d1626e8ffb 100644 --- a/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs +++ b/src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs @@ -1,35 +1,67 @@ -// check-pass +// revisions: allowed disallowed +//[allowed] check-pass #![feature(if_let_guard, let_chains)] +#![cfg_attr(allowed, allow(irrefutable_let_patterns))] +#![cfg_attr(disallowed, deny(irrefutable_let_patterns))] use std::ops::Range; fn main() { let opt = Some(None..Some(1)); - if let first = &opt && let Some(ref second) = first && let None = second.start { - } - if let Some(ref first) = opt && let second = first && let _third = second { - } + if let first = &opt && let Some(ref second) = first && let None = second.start {} + //[disallowed]~^ ERROR leading irrefutable pattern in let chain + + // No lint as the irrefutable pattern is surrounded by other stuff + if 4 * 2 == 0 && let first = &opt && let Some(ref second) = first && let None = second.start {} + + if let first = &opt && let (a, b) = (1, 2) {} + //[disallowed]~^ ERROR irrefutable `if let` patterns + + if let first = &opt && let Some(ref second) = first && let None = second.start && let v = 0 {} + //[disallowed]~^ ERROR leading irrefutable pattern in let chain + //[disallowed]~^^ ERROR trailing irrefutable pattern in let chain + + if let Some(ref first) = opt && let second = first && let _third = second {} + //[disallowed]~^ ERROR trailing irrefutable patterns in let chain + + if let Range { start: local_start, end: _ } = (None..Some(1)) && let None = local_start {} + //[disallowed]~^ ERROR leading irrefutable pattern in let chain + + if let (a, b, c) = (Some(1), Some(1), Some(1)) && let None = Some(1) {} + //[disallowed]~^ ERROR leading irrefutable pattern in let chain + + if let first = &opt && let None = Some(1) {} + //[disallowed]~^ ERROR leading irrefutable pattern in let chain + if let Some(ref first) = opt && let Range { start: local_start, end: _ } = first && let None = local_start { } match opt { - Some(ref first) if let second = first && let _third = second => {}, + Some(ref first) if let second = first && let _third = second && let v = 4 + 4 => {}, + //[disallowed]~^ ERROR irrefutable `let` patterns _ => {} } + match opt { Some(ref first) if let Range { start: local_start, end: _ } = first + //[disallowed]~^ ERROR leading irrefutable pattern in let chain && let None = local_start => {}, _ => {} } - while let first = &opt && let Some(ref second) = first && let None = second.start { - } - while let Some(ref first) = opt && let second = first && let _third = second { - } + // No error, despite the prefix being irrefutable + while let first = &opt && let Some(ref second) = first && let None = second.start {} + + while let first = &opt && let (a, b) = (1, 2) {} + //[disallowed]~^ ERROR irrefutable `while let` patterns + + while let Some(ref first) = opt && let second = first && let _third = second {} + //[disallowed]~^ ERROR trailing irrefutable patterns in let chain + while let Some(ref first) = opt && let Range { start: local_start, end: _ } = first && let None = local_start { |