Skip to content

Commit bc22407

Browse files
y21y21
committed
add tests, lint on while let true and matches!(.., true) 
1 parent 850d77e commit bc22407

21 files changed

+272
-68
lines changed

‎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/matches/mod.rs‎

Lines changed: 16 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) {

‎clippy_lints/src/matches/redundant_pattern_match.rs‎

Lines changed: 58 additions & 12 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};
@@ -17,8 +17,15 @@ 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) {
20+
if let Some(higher::WhileLet {
21+
let_pat,
22+
let_expr,
23+
let_span,
24+
..
25+
}) = higher::WhileLet::hir(expr)
26+
{
2127
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

@@ -34,26 +41,65 @@ pub(super) fn check_if_let<'tcx>(
3441
find_method_sugg_for_if_let(cx, expr, pat, scrutinee, "if", has_else);
3542
}
3643

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 = ...`
3765
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+
) {
3877
if let PatKind::Lit(lit) = pat.kind
3978
&& let ExprKind::Lit(lit) = lit.kind
40-
&& let LitKind::Bool(is_true) = lit.node
79+
&& let LitKind::Bool(pat_is_true) = lit.node
4180
{
42-
let mut snip = snippet(cx, scrutinee.span, "..").into_owned();
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+
);
4390

44-
if !is_true {
45-
// Invert condition for `if let false = ...`
46-
snip.insert(0, '!');
91+
if !pat_is_true {
92+
sugg = make_unop("!", sugg);
4793
}
4894

4995
span_lint_and_sugg(
5096
cx,
5197
REDUNDANT_PATTERN_MATCHING,
52-
let_span,
53-
"using `if let` to pattern match a boolean",
54-
"consider using a regular `if` expression",
55-
snip,
56-
Applicability::MachineApplicable,
98+
span,
99+
message,
100+
"consider using the condition directly",
101+
sugg.to_string(),
102+
applicability,
57103
);
58104
}
59105
}

‎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: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,9 @@ pub struct WhileLet<'hir> {
362362
pub let_expr: &'hir Expr<'hir>,
363363
/// `while let` loop body
364364
pub if_then: &'hir Expr<'hir>,
365+
/// `while let PAT = EXPR`
366+
/// ^^^^^^^^^^^^^^
367+
pub let_span: Span,
365368
}
366369

367370
impl<'hir> WhileLet<'hir> {
@@ -376,9 +379,10 @@ impl<'hir> WhileLet<'hir> {
376379
ExprKind::If(
377380
Expr {
378381
kind:
379-
ExprKind::Let(hir::Let {
382+
ExprKind::Let(&hir::Let {
380383
pat: let_pat,
381384
init: let_expr,
385+
span: let_span,
382386
..
383387
}),
384388
..
@@ -399,6 +403,7 @@ impl<'hir> WhileLet<'hir> {
399403
let_pat,
400404
let_expr,
401405
if_then,
406+
let_span,
402407
});
403408
}
404409
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

‎tests/ui/branches_sharing_code/shared_at_bottom.stderr‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
error: all if blocks contain the same code at the end
2-
--> $DIR/shared_at_bottom.rs:31:5
2+
--> $DIR/shared_at_bottom.rs:35:5
33
|
44
LL | / let result = false;
55
LL | |
@@ -26,7 +26,7 @@ LL ~ result;
2626
|
2727

2828
error: all if blocks contain the same code at the end
29-
--> $DIR/shared_at_bottom.rs:51:5
29+
--> $DIR/shared_at_bottom.rs:55:5
3030
|
3131
LL | / println!("Same end of block");
3232
LL | |
@@ -40,7 +40,7 @@ LL + println!("Same end of block");
4040
|
4141

4242
error: all if blocks contain the same code at the end
43-
--> $DIR/shared_at_bottom.rs:69:5
43+
--> $DIR/shared_at_bottom.rs:73:5
4444
|
4545
LL | / println!(
4646
LL | |
@@ -61,7 +61,7 @@ LL + );
6161
|
6262

6363
error: all if blocks contain the same code at the end
64-
--> $DIR/shared_at_bottom.rs:82:9
64+
--> $DIR/shared_at_bottom.rs:86:9
6565
|
6666
LL | / println!("Hello World");
6767
LL | |
@@ -75,7 +75,7 @@ LL + println!("Hello World");
7575
|
7676

7777
error: all if blocks contain the same code at the end
78-
--> $DIR/shared_at_bottom.rs:99:5
78+
--> $DIR/shared_at_bottom.rs:103:5
7979
|
8080
LL | / let later_used_value = "A string value";
8181
LL | |
@@ -94,7 +94,7 @@ LL + println!("{}", later_used_value);
9494
|
9595

9696
error: all if blocks contain the same code at the end
97-
--> $DIR/shared_at_bottom.rs:113:5
97+
--> $DIR/shared_at_bottom.rs:117:5
9898
|
9999
LL | / let simple_examples = "I now identify as a &str :)";
100100
LL | |
@@ -112,7 +112,7 @@ LL + println!("This is the new simple_example: {}", simple_examples);
112112
|
113113

114114
error: all if blocks contain the same code at the end
115-
--> $DIR/shared_at_bottom.rs:179:5
115+
--> $DIR/shared_at_bottom.rs:183:5
116116
|
117117
LL | / x << 2
118118
LL | |
@@ -128,7 +128,7 @@ LL ~ x << 2;
128128
|
129129

130130
error: all if blocks contain the same code at the end
131-
--> $DIR/shared_at_bottom.rs:188:5
131+
--> $DIR/shared_at_bottom.rs:192:5
132132
|
133133
LL | / x * 4
134134
LL | |
@@ -144,7 +144,7 @@ LL + x * 4
144144
|
145145

146146
error: all if blocks contain the same code at the end
147-
--> $DIR/shared_at_bottom.rs:202:44
147+
--> $DIR/shared_at_bottom.rs:206:44
148148
|
149149
LL | if x == 17 { b = 1; a = 0x99; } else { a = 0x99; }
150150
| ^^^^^^^^^^^

‎tests/ui/collapsible_if.fixed‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
clippy::equatable_if_let,
44
clippy::needless_if,
55
clippy::nonminimal_bool,
6-
clippy::eq_op
6+
clippy::eq_op,
7+
clippy::redundant_pattern_matching
78
)]
89

910
#[rustfmt::skip]

‎tests/ui/collapsible_if.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
clippy::equatable_if_let,
44
clippy::needless_if,
55
clippy::nonminimal_bool,
6-
clippy::eq_op
6+
clippy::eq_op,
7+
clippy::redundant_pattern_matching
78
)]
89

910
#[rustfmt::skip]

0 commit comments

Comments
 (0)