Skip to content

Commit b918434

Browse files
borsbors
committed
Auto merge of #11974 - y21:if_let_true, r=llogiq
[`redundant_pattern_matching`]: lint `if let true`, `while let true`, `matches!(.., true)` Closes #11917 This could also lint e.g. `if let (true, true, false) = (a, b, c)` and suggest `if a && b && c`, but that's a change in semantics (going from eager to lazy, so I just left it out for now. Could be a future improvement. changelog: [`redundant_pattern_matching`]: lint `if let true`, `while let true`, `matches!(.., true)`
2 parents a859e5c + bc22407 commit b918434

28 files changed

+312
-70
lines changed

‎clippy_lints/src/format_push_string.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ fn is_format(cx: &LateContext<'_>, e: &Expr<'_>) -> bool {
5757
Some(higher::IfLetOrMatch::Match(_, arms, MatchSource::Normal)) => {
5858
arms.iter().any(|arm| is_format(cx, arm.body))
5959
},
60-
Some(higher::IfLetOrMatch::IfLet(_, _, then, r#else)) => {
60+
Some(higher::IfLetOrMatch::IfLet(_, _, then, r#else, _)) => {
6161
is_format(cx, then) || r#else.is_some_and(|e| is_format(cx, e))
6262
},
6363
_ => false,

‎clippy_lints/src/loops/manual_flatten.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ pub(super) fn check<'tcx>(
2020
span: Span,
2121
) {
2222
let inner_expr = peel_blocks_with_stmt(body);
23-
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: None })
23+
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: None, .. })
2424
= higher::IfLet::hir(cx, inner_expr)
2525
// Ensure match_expr in `if let` statement is the same as the pat from the for-loop
2626
&& let PatKind::Binding(_, pat_hir_id, _, _) = pat.kind

‎clippy_lints/src/loops/while_let_on_iterator.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ use rustc_span::symbol::sym;
1414
use rustc_span::Symbol;
1515

1616
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
17-
if let Some(higher::WhileLet { if_then, let_pat, let_expr }) = higher::WhileLet::hir(expr)
17+
if let Some(higher::WhileLet { if_then, let_pat, let_expr, .. }) = higher::WhileLet::hir(expr)
1818
// check for `Some(..)` pattern
1919
&& let PatKind::TupleStruct(ref pat_path, some_pat, _) = let_pat.kind
2020
&& is_res_lang_ctor(cx, cx.qpath_res(pat_path, let_pat.hir_id), LangItem::OptionSome)

‎clippy_lints/src/manual_let_else.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ impl<'tcx> QuestionMark {
6161
&& let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init)
6262
{
6363
match if_let_or_match {
64-
IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => {
64+
IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else, ..) => {
6565
if let Some(ident_map) = expr_simple_identity_map(local.pat, let_pat, if_then)
6666
&& let Some(if_else) = if_else
6767
&& is_never_expr(cx, if_else).is_some()

‎clippy_lints/src/matches/collapsible_match.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ fn check_arm<'tcx>(
4141
let inner_expr = peel_blocks_with_stmt(outer_then_body);
4242
if let Some(inner) = IfLetOrMatch::parse(cx, inner_expr)
4343
&& let Some((inner_scrutinee, inner_then_pat, inner_else_body)) = match inner {
44-
IfLetOrMatch::IfLet(scrutinee, pat, _, els) => Some((scrutinee, pat, els)),
44+
IfLetOrMatch::IfLet(scrutinee, pat, _, els, _) => Some((scrutinee, pat, els)),
4545
IfLetOrMatch::Match(scrutinee, arms, ..) => if arms.len() == 2 && arms.iter().all(|a| a.guard.is_none())
4646
// if there are more than two arms, collapsing would be non-trivial
4747
// one of the arms must be "wild-like"
@@ -75,7 +75,7 @@ fn check_arm<'tcx>(
7575
)
7676
// ...or anywhere in the inner expression
7777
&& match inner {
78-
IfLetOrMatch::IfLet(_, _, body, els) => {
78+
IfLetOrMatch::IfLet(_, _, body, els, _) => {
7979
!is_local_used(cx, body, binding_id) && els.map_or(true, |e| !is_local_used(cx, e, binding_id))
8080
},
8181
IfLetOrMatch::Match(_, arms, ..) => !arms.iter().any(|arm| is_local_used(cx, arm, binding_id)),

‎clippy_lints/src/matches/mod.rs

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -469,15 +469,15 @@ declare_clippy_lint! {
469469
declare_clippy_lint! {
470470
/// ### What it does
471471
/// Lint for redundant pattern matching over `Result`, `Option`,
472-
/// `std::task::Poll` or `std::net::IpAddr`
472+
/// `std::task::Poll`, `std::net::IpAddr` or `bool`s
473473
///
474474
/// ### Why is this bad?
475475
/// It's more concise and clear to just use the proper
476-
/// utility function
476+
/// utility function or using the condition directly
477477
///
478478
/// ### Known problems
479-
/// This will change the drop order for the matched type. Both `if let` and
480-
/// `while let` will drop the value at the end of the block, both `if` and `while` will drop the
479+
/// For suggestions involving bindings in patterns, this will change the drop order for the matched type.
480+
/// Both `if let` and `while let` will drop the value at the end of the block, both `if` and `while` will drop the
481481
/// value before entering the block. For most types this change will not matter, but for a few
482482
/// types this will not be an acceptable change (e.g. locks). See the
483483
/// [reference](https://doc.rust-lang.org/reference/destructors.html#drop-scopes) for more about
@@ -499,6 +499,10 @@ declare_clippy_lint! {
499499
/// Ok(_) => true,
500500
/// Err(_) => false,
501501
/// };
502+
///
503+
/// let cond = true;
504+
/// if let true = cond {}
505+
/// matches!(cond, true);
502506
/// ```
503507
///
504508
/// The more idiomatic use would be:
@@ -515,6 +519,10 @@ declare_clippy_lint! {
515519
/// if IpAddr::V4(Ipv4Addr::LOCALHOST).is_ipv4() {}
516520
/// if IpAddr::V6(Ipv6Addr::LOCALHOST).is_ipv6() {}
517521
/// Ok::<i32, i32>(42).is_ok();
522+
///
523+
/// let cond = true;
524+
/// if cond {}
525+
/// cond;
518526
/// ```
519527
#[clippy::version = "1.31.0"]
520528
pub REDUNDANT_PATTERN_MATCHING,
@@ -1019,8 +1027,11 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
10191027
let from_expansion = expr.span.from_expansion();
10201028

10211029
if let ExprKind::Match(ex, arms, source) = expr.kind {
1022-
if is_direct_expn_of(expr.span, "matches").is_some() {
1030+
if is_direct_expn_of(expr.span, "matches").is_some()
1031+
&& let [arm, _] = arms
1032+
{
10231033
redundant_pattern_match::check_match(cx, expr, ex, arms);
1034+
redundant_pattern_match::check_matches_true(cx, expr, arm, ex);
10241035
}
10251036

10261037
if source == MatchSource::Normal && !is_span_match(cx, expr.span) {
@@ -1104,6 +1115,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
11041115
if_let.let_pat,
11051116
if_let.let_expr,
11061117
if_let.if_else.is_some(),
1118+
if_let.let_span,
11071119
);
11081120
needless_match::check_if_let(cx, expr, &if_let);
11091121
}

‎clippy_lints/src/matches/redundant_pattern_match.rs

Lines changed: 78 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use super::REDUNDANT_PATTERN_MATCHING;
22
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
33
use clippy_utils::source::{snippet, walk_span_to_context};
4-
use clippy_utils::sugg::Sugg;
4+
use clippy_utils::sugg::{make_unop, Sugg};
55
use clippy_utils::ty::{is_type_diagnostic_item, needs_ordered_drop};
66
use clippy_utils::visitors::{any_temporaries_need_ordered_drop, for_each_expr};
77
use clippy_utils::{higher, is_expn_of, is_trait_method};
@@ -12,13 +12,20 @@ use rustc_hir::LangItem::{self, OptionNone, OptionSome, PollPending, PollReady,
1212
use rustc_hir::{Arm, Expr, ExprKind, Guard, Node, Pat, PatKind, QPath, UnOp};
1313
use rustc_lint::LateContext;
1414
use rustc_middle::ty::{self, GenericArgKind, Ty};
15-
use rustc_span::{sym, Symbol};
15+
use rustc_span::{sym, Span, Symbol};
1616
use std::fmt::Write;
1717
use std::ops::ControlFlow;
1818

1919
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
20-
if let Some(higher::WhileLet { let_pat, let_expr, .. }) = higher::WhileLet::hir(expr) {
21-
find_sugg_for_if_let(cx, expr, let_pat, let_expr, "while", false);
20+
if let Some(higher::WhileLet {
21+
let_pat,
22+
let_expr,
23+
let_span,
24+
..
25+
}) = higher::WhileLet::hir(expr)
26+
{
27+
find_method_sugg_for_if_let(cx, expr, let_pat, let_expr, "while", false);
28+
find_if_let_true(cx, let_pat, let_expr, let_span);
2229
}
2330
}
2431

@@ -28,8 +35,73 @@ pub(super) fn check_if_let<'tcx>(
2835
pat: &'tcx Pat<'_>,
2936
scrutinee: &'tcx Expr<'_>,
3037
has_else: bool,
38+
let_span: Span,
3139
) {
32-
find_sugg_for_if_let(cx, expr, pat, scrutinee, "if", has_else);
40+
find_if_let_true(cx, pat, scrutinee, let_span);
41+
find_method_sugg_for_if_let(cx, expr, pat, scrutinee, "if", has_else);
42+
}
43+
44+
/// Looks for:
45+
/// * `matches!(expr, true)`
46+
pub fn check_matches_true<'tcx>(
47+
cx: &LateContext<'tcx>,
48+
expr: &'tcx Expr<'_>,
49+
arm: &'tcx Arm<'_>,
50+
scrutinee: &'tcx Expr<'_>,
51+
) {
52+
find_match_true(
53+
cx,
54+
arm.pat,
55+
scrutinee,
56+
expr.span.source_callsite(),
57+
"using `matches!` to pattern match a bool",
58+
);
59+
}
60+
61+
/// Looks for any of:
62+
/// * `if let true = ...`
63+
/// * `if let false = ...`
64+
/// * `while let true = ...`
65+
fn find_if_let_true<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, scrutinee: &'tcx Expr<'_>, let_span: Span) {
66+
find_match_true(cx, pat, scrutinee, let_span, "using `if let` to pattern match a bool");
67+
}
68+
69+
/// Common logic between `find_if_let_true` and `check_matches_true`
70+
fn find_match_true<'tcx>(
71+
cx: &LateContext<'tcx>,
72+
pat: &'tcx Pat<'_>,
73+
scrutinee: &'tcx Expr<'_>,
74+
span: Span,
75+
message: &str,
76+
) {
77+
if let PatKind::Lit(lit) = pat.kind
78+
&& let ExprKind::Lit(lit) = lit.kind
79+
&& let LitKind::Bool(pat_is_true) = lit.node
80+
{
81+
let mut applicability = Applicability::MachineApplicable;
82+
83+
let mut sugg = Sugg::hir_with_context(
84+
cx,
85+
scrutinee,
86+
scrutinee.span.source_callsite().ctxt(),
87+
"..",
88+
&mut applicability,
89+
);
90+
91+
if !pat_is_true {
92+
sugg = make_unop("!", sugg);
93+
}
94+
95+
span_lint_and_sugg(
96+
cx,
97+
REDUNDANT_PATTERN_MATCHING,
98+
span,
99+
message,
100+
"consider using the condition directly",
101+
sugg.to_string(),
102+
applicability,
103+
);
104+
}
33105
}
34106

35107
// Extract the generic arguments out of a type
@@ -100,7 +172,7 @@ fn find_method_and_type<'tcx>(
100172
}
101173
}
102174

103-
fn find_sugg_for_if_let<'tcx>(
175+
fn find_method_sugg_for_if_let<'tcx>(
104176
cx: &LateContext<'tcx>,
105177
expr: &'tcx Expr<'_>,
106178
let_pat: &Pat<'_>,

‎clippy_lints/src/methods/filter_map.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ impl<'tcx> OffendingFilterExpr<'tcx> {
186186
match higher::IfLetOrMatch::parse(cx, map_body.value) {
187187
// For `if let` we want to check that the variant matching arm references the local created by
188188
// its pattern
189-
Some(higher::IfLetOrMatch::IfLet(sc, pat, then, Some(else_)))
189+
Some(higher::IfLetOrMatch::IfLet(sc, pat, then, Some(else_), ..))
190190
if let Some((ident, span)) = expr_uses_local(pat, then) =>
191191
{
192192
(sc, else_, ident, span)

‎clippy_lints/src/option_if_let_else.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@ fn detect_option_if_let_else<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) ->
238238
let_expr,
239239
if_then,
240240
if_else: Some(if_else),
241+
..
241242
}) = higher::IfLet::hir(cx, expr)
242243
&& !cx.typeck_results().expr_ty(expr).is_unit()
243244
&& !is_else_clause(cx.tcx, expr)

‎clippy_lints/src/question_mark.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ impl QuestionMark {
256256
let_expr,
257257
if_then,
258258
if_else,
259+
..
259260
}) = higher::IfLet::hir(cx, expr)
260261
&& !is_else_clause(cx.tcx, expr)
261262
&& let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind

‎clippy_lints/src/utils/author.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,7 @@ impl<'a, 'tcx> PrintVisitor<'a, 'tcx> {
350350
let_pat,
351351
let_expr,
352352
if_then,
353+
..
353354
}) = higher::WhileLet::hir(expr.value)
354355
{
355356
bind!(self, let_pat, let_expr, if_then);

‎clippy_utils/src/higher.rs

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,9 @@ pub struct IfLet<'hir> {
9191
pub if_then: &'hir Expr<'hir>,
9292
/// `if let` else expression
9393
pub if_else: Option<&'hir Expr<'hir>>,
94+
/// `if let PAT = EXPR`
95+
/// ^^^^^^^^^^^^^^
96+
pub let_span: Span,
9497
}
9598

9699
impl<'hir> IfLet<'hir> {
@@ -99,9 +102,10 @@ impl<'hir> IfLet<'hir> {
99102
if let ExprKind::If(
100103
Expr {
101104
kind:
102-
ExprKind::Let(hir::Let {
105+
ExprKind::Let(&hir::Let {
103106
pat: let_pat,
104107
init: let_expr,
108+
span: let_span,
105109
..
106110
}),
107111
..
@@ -129,6 +133,7 @@ impl<'hir> IfLet<'hir> {
129133
let_expr,
130134
if_then,
131135
if_else,
136+
let_span,
132137
});
133138
}
134139
None
@@ -146,6 +151,9 @@ pub enum IfLetOrMatch<'hir> {
146151
&'hir Pat<'hir>,
147152
&'hir Expr<'hir>,
148153
Option<&'hir Expr<'hir>>,
154+
/// `if let PAT = EXPR`
155+
/// ^^^^^^^^^^^^^^
156+
Span,
149157
),
150158
}
151159

@@ -160,7 +168,8 @@ impl<'hir> IfLetOrMatch<'hir> {
160168
let_pat,
161169
if_then,
162170
if_else,
163-
}| { Self::IfLet(let_expr, let_pat, if_then, if_else) },
171+
let_span,
172+
}| { Self::IfLet(let_expr, let_pat, if_then, if_else, let_span) },
164173
),
165174
}
166175
}
@@ -353,6 +362,9 @@ pub struct WhileLet<'hir> {
353362
pub let_expr: &'hir Expr<'hir>,
354363
/// `while let` loop body
355364
pub if_then: &'hir Expr<'hir>,
365+
/// `while let PAT = EXPR`
366+
/// ^^^^^^^^^^^^^^
367+
pub let_span: Span,
356368
}
357369

358370
impl<'hir> WhileLet<'hir> {
@@ -367,9 +379,10 @@ impl<'hir> WhileLet<'hir> {
367379
ExprKind::If(
368380
Expr {
369381
kind:
370-
ExprKind::Let(hir::Let {
382+
ExprKind::Let(&hir::Let {
371383
pat: let_pat,
372384
init: let_expr,
385+
span: let_span,
373386
..
374387
}),
375388
..
@@ -390,6 +403,7 @@ impl<'hir> WhileLet<'hir> {
390403
let_pat,
391404
let_expr,
392405
if_then,
406+
let_span,
393407
});
394408
}
395409
None

‎tests/ui/author/loop.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
#![feature(stmt_expr_attributes)]
2-
#![allow(clippy::never_loop, clippy::while_immutable_condition)]
2+
#![allow(
3+
clippy::never_loop,
4+
clippy::while_immutable_condition,
5+
clippy::redundant_pattern_matching
6+
)]
37

48
fn main() {
59
#[clippy::author]

‎tests/ui/branches_sharing_code/shared_at_bottom.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
#![deny(clippy::if_same_then_else, clippy::branches_sharing_code)]
2-
#![allow(dead_code)]
3-
#![allow(clippy::equatable_if_let, clippy::uninlined_format_args)]
2+
#![allow(
3+
clippy::equatable_if_let,
4+
clippy::uninlined_format_args,
5+
clippy::redundant_pattern_matching,
6+
dead_code
7+
)]
48
//@no-rustfix
59
// This tests the branches_sharing_code lint at the end of blocks
610

0 commit comments

Comments
 (0)