mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Extend the irrefutable_let_patterns lint to let chains

Browse Source 
Only look for complete suffixes or prefixes of irrefutable let patterns, so
that an irrefutable let pattern in a chain surrounded by refutable ones is
not linted, as it is an useful pattern.
This commit is contained in:
est31 2022-03-15 03:48:53 +01:00
parent 83460d5e62
commit 0f4c81a1a9
5 changed files with 339 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

215
compiler/rustc_mir_build/src/thir/pattern/check_match.rs
View File

@ -158,7 +158,7 @@ fn check_let(&mut self, pat: &'tcx hir::Pat<'tcx>, scrutinee: &hir::Expr<'_>, sp
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 @@ fn check_match(
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 @@ fn check_match(
}
}
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 @@ macro_rules! emit_diag {
$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 @@ macro_rules! emit_diag {
});
}
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, .. }, ..),
..
},
..
)
)
}

1
src/test/ui/mir/mir_let_chains_drop_order.rs
View File

@ -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;

1
src/test/ui/rfc-2497-if-let-chains/ast-lowering-does-not-wrap-let-chains.rs
View File

@ -1,6 +1,7 @@
// run-pass
#![feature(let_chains)]
#![allow(irrefutable_let_patterns)]
fn main() {
let first = Some(1);

115
src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.disallowed.stderr Normal file
View File

@ -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

52
src/test/ui/rfc-2497-if-let-chains/irrefutable-lets.rs
View File

@ -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 {