Skip to content

Commit 75b616e

Browse files
committed
Auto merge of rust-lang#8471 - J-ZhengLi:master-issue7040, r=llogiq
new lint that detects useless match expression fixes rust-lang#7040 changelog: Add new lint [`needless_match`] under complexity lint group
2 parents 8ae74da + 086b045 commit 75b616e

11 files changed

+577
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3331,6 +3331,7 @@ Released 2018-09-13
33313331
[`needless_for_each`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_for_each
33323332
[`needless_late_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_late_init
33333333
[`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
3334+
[`needless_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_match
33343335
[`needless_option_as_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_as_deref
33353336
[`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
33363337
[`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark

clippy_lints/src/lib.register_all.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
138138
LintId::of(matches::MATCH_OVERLAPPING_ARM),
139139
LintId::of(matches::MATCH_REF_PATS),
140140
LintId::of(matches::MATCH_SINGLE_BINDING),
141+
LintId::of(matches::NEEDLESS_MATCH),
141142
LintId::of(matches::REDUNDANT_PATTERN_MATCHING),
142143
LintId::of(matches::SINGLE_MATCH),
143144
LintId::of(matches::WILDCARD_IN_OR_PATTERNS),

clippy_lints/src/lib.register_complexity.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec!
3030
LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN),
3131
LintId::of(matches::MATCH_AS_REF),
3232
LintId::of(matches::MATCH_SINGLE_BINDING),
33+
LintId::of(matches::NEEDLESS_MATCH),
3334
LintId::of(matches::WILDCARD_IN_OR_PATTERNS),
3435
LintId::of(methods::BIND_INSTEAD_OF_MAP),
3536
LintId::of(methods::CLONE_ON_COPY),

clippy_lints/src/lib.register_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,7 @@ store.register_lints(&[
260260
matches::MATCH_SINGLE_BINDING,
261261
matches::MATCH_WILDCARD_FOR_SINGLE_VARIANTS,
262262
matches::MATCH_WILD_ERR_ARM,
263+
matches::NEEDLESS_MATCH,
263264
matches::REDUNDANT_PATTERN_MATCHING,
264265
matches::REST_PAT_IN_FULLY_BOUND_STRUCTS,
265266
matches::SINGLE_MATCH,

clippy_lints/src/matches/mod.rs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ mod match_same_arms;
1616
mod match_single_binding;
1717
mod match_wild_enum;
1818
mod match_wild_err_arm;
19+
mod needless_match;
1920
mod overlapping_arms;
2021
mod redundant_pattern_match;
2122
mod rest_pat_in_fully_bound_struct;
@@ -566,6 +567,49 @@ declare_clippy_lint! {
566567
"`match` with identical arm bodies"
567568
}
568569

570+
declare_clippy_lint! {
571+
/// ### What it does
572+
/// Checks for unnecessary `match` or match-like `if let` returns for `Option` and `Result`
573+
/// when function signatures are the same.
574+
///
575+
/// ### Why is this bad?
576+
/// This `match` block does nothing and might not be what the coder intended.
577+
///
578+
/// ### Example
579+
/// ```rust,ignore
580+
/// fn foo() -> Result<(), i32> {
581+
/// match result {
582+
/// Ok(val) => Ok(val),
583+
/// Err(err) => Err(err),
584+
/// }
585+
/// }
586+
///
587+
/// fn bar() -> Option<i32> {
588+
/// if let Some(val) = option {
589+
/// Some(val)
590+
/// } else {
591+
/// None
592+
/// }
593+
/// }
594+
/// ```
595+
///
596+
/// Could be replaced as
597+
///
598+
/// ```rust,ignore
599+
/// fn foo() -> Result<(), i32> {
600+
/// result
601+
/// }
602+
///
603+
/// fn bar() -> Option<i32> {
604+
/// option
605+
/// }
606+
/// ```
607+
#[clippy::version = "1.61.0"]
608+
pub NEEDLESS_MATCH,
609+
complexity,
610+
"`match` or match-like `if let` that are unnecessary"
611+
}
612+
569613
#[derive(Default)]
570614
pub struct Matches {
571615
msrv: Option<RustcVersion>,
@@ -599,6 +643,7 @@ impl_lint_pass!(Matches => [
599643
REDUNDANT_PATTERN_MATCHING,
600644
MATCH_LIKE_MATCHES_MACRO,
601645
MATCH_SAME_ARMS,
646+
NEEDLESS_MATCH,
602647
]);
603648

604649
impl<'tcx> LateLintPass<'tcx> for Matches {
@@ -622,6 +667,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
622667
overlapping_arms::check(cx, ex, arms);
623668
match_wild_enum::check(cx, ex, arms);
624669
match_as_ref::check(cx, ex, arms, expr);
670+
needless_match::check_match(cx, ex, arms);
625671

626672
if self.infallible_destructuring_match_linted {
627673
self.infallible_destructuring_match_linted = false;
@@ -640,6 +686,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
640686
match_like_matches::check(cx, expr);
641687
}
642688
redundant_pattern_match::check(cx, expr);
689+
needless_match::check(cx, expr);
643690
}
644691
}
645692

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
use super::NEEDLESS_MATCH;
2+
use clippy_utils::diagnostics::span_lint_and_sugg;
3+
use clippy_utils::source::snippet_with_applicability;
4+
use clippy_utils::ty::is_type_diagnostic_item;
5+
use clippy_utils::{eq_expr_value, get_parent_expr, higher, is_else_clause, is_lang_ctor, peel_blocks_with_stmt};
6+
use rustc_errors::Applicability;
7+
use rustc_hir::LangItem::OptionNone;
8+
use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, Pat, PatKind, Path, PathSegment, QPath, UnOp};
9+
use rustc_lint::LateContext;
10+
use rustc_span::sym;
11+
12+
pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {
13+
// This is for avoiding collision with `match_single_binding`.
14+
if arms.len() < 2 {
15+
return;
16+
}
17+
18+
for arm in arms {
19+
if let PatKind::Wild = arm.pat.kind {
20+
let ret_expr = strip_return(arm.body);
21+
if !eq_expr_value(cx, ex, ret_expr) {
22+
return;
23+
}
24+
} else if !pat_same_as_expr(arm.pat, arm.body) {
25+
return;
26+
}
27+
}
28+
29+
if let Some(match_expr) = get_parent_expr(cx, ex) {
30+
let mut applicability = Applicability::MachineApplicable;
31+
span_lint_and_sugg(
32+
cx,
33+
NEEDLESS_MATCH,
34+
match_expr.span,
35+
"this match expression is unnecessary",
36+
"replace it with",
37+
snippet_with_applicability(cx, ex.span, "..", &mut applicability).to_string(),
38+
applicability,
39+
);
40+
}
41+
}
42+
43+
/// Check for nop `if let` expression that assembled as unnecessary match
44+
///
45+
/// ```rust,ignore
46+
/// if let Some(a) = option {
47+
/// Some(a)
48+
/// } else {
49+
/// None
50+
/// }
51+
/// ```
52+
/// OR
53+
/// ```rust,ignore
54+
/// if let SomeEnum::A = some_enum {
55+
/// SomeEnum::A
56+
/// } else if let SomeEnum::B = some_enum {
57+
/// SomeEnum::B
58+
/// } else {
59+
/// some_enum
60+
/// }
61+
/// ```
62+
pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) {
63+
if_chain! {
64+
if let Some(ref if_let) = higher::IfLet::hir(cx, ex);
65+
if !is_else_clause(cx.tcx, ex);
66+
if check_if_let(cx, if_let);
67+
then {
68+
let mut applicability = Applicability::MachineApplicable;
69+
span_lint_and_sugg(
70+
cx,
71+
NEEDLESS_MATCH,
72+
ex.span,
73+
"this if-let expression is unnecessary",
74+
"replace it with",
75+
snippet_with_applicability(cx, if_let.let_expr.span, "..", &mut applicability).to_string(),
76+
applicability,
77+
);
78+
}
79+
}
80+
}
81+
82+
fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool {
83+
if let Some(if_else) = if_let.if_else {
84+
if !pat_same_as_expr(if_let.let_pat, peel_blocks_with_stmt(if_let.if_then)) {
85+
return false;
86+
}
87+
88+
// Recurrsively check for each `else if let` phrase,
89+
if let Some(ref nested_if_let) = higher::IfLet::hir(cx, if_else) {
90+
return check_if_let(cx, nested_if_let);
91+
}
92+
93+
if matches!(if_else.kind, ExprKind::Block(..)) {
94+
let else_expr = peel_blocks_with_stmt(if_else);
95+
let ret = strip_return(else_expr);
96+
let let_expr_ty = cx.typeck_results().expr_ty(if_let.let_expr);
97+
if is_type_diagnostic_item(cx, let_expr_ty, sym::Option) {
98+
if let ExprKind::Path(ref qpath) = ret.kind {
99+
return is_lang_ctor(cx, qpath, OptionNone) || eq_expr_value(cx, if_let.let_expr, ret);
100+
}
101+
} else {
102+
return eq_expr_value(cx, if_let.let_expr, ret);
103+
}
104+
return true;
105+
}
106+
}
107+
false
108+
}
109+
110+
/// Strip `return` keyword if the expression type is `ExprKind::Ret`.
111+
fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> {
112+
if let ExprKind::Ret(Some(ret)) = expr.kind {
113+
ret
114+
} else {
115+
expr
116+
}
117+
}
118+
119+
fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool {
120+
let expr = strip_return(expr);
121+
match (&pat.kind, &expr.kind) {
122+
// Example: `Some(val) => Some(val)`
123+
(
124+
PatKind::TupleStruct(QPath::Resolved(_, path), [first_pat, ..], _),
125+
ExprKind::Call(call_expr, [first_param, ..]),
126+
) => {
127+
if let ExprKind::Path(QPath::Resolved(_, call_path)) = call_expr.kind {
128+
if has_identical_segments(path.segments, call_path.segments)
129+
&& has_same_non_ref_symbol(first_pat, first_param)
130+
{
131+
return true;
132+
}
133+
}
134+
},
135+
// Example: `val => val`, or `ref val => *val`
136+
(PatKind::Binding(annot, _, pat_ident, _), _) => {
137+
let new_expr = if let (
138+
BindingAnnotation::Ref | BindingAnnotation::RefMut,
139+
ExprKind::Unary(UnOp::Deref, operand_expr),
140+
) = (annot, &expr.kind)
141+
{
142+
operand_expr
143+
} else {
144+
expr
145+
};
146+
147+
if let ExprKind::Path(QPath::Resolved(
148+
_,
149+
Path {
150+
segments: [first_seg, ..],
151+
..
152+
},
153+
)) = new_expr.kind
154+
{
155+
return pat_ident.name == first_seg.ident.name;
156+
}
157+
},
158+
// Example: `Custom::TypeA => Custom::TypeB`, or `None => None`
159+
(PatKind::Path(QPath::Resolved(_, p_path)), ExprKind::Path(QPath::Resolved(_, e_path))) => {
160+
return has_identical_segments(p_path.segments, e_path.segments);
161+
},
162+
// Example: `5 => 5`
163+
(PatKind::Lit(pat_lit_expr), ExprKind::Lit(expr_spanned)) => {
164+
if let ExprKind::Lit(pat_spanned) = &pat_lit_expr.kind {
165+
return pat_spanned.node == expr_spanned.node;
166+
}
167+
},
168+
_ => {},
169+
}
170+
171+
false
172+
}
173+
174+
fn has_identical_segments(left_segs: &[PathSegment<'_>], right_segs: &[PathSegment<'_>]) -> bool {
175+
if left_segs.len() != right_segs.len() {
176+
return false;
177+
}
178+
for i in 0..left_segs.len() {
179+
if left_segs[i].ident.name != right_segs[i].ident.name {
180+
return false;
181+
}
182+
}
183+
true
184+
}
185+
186+
fn has_same_non_ref_symbol(pat: &Pat<'_>, expr: &Expr<'_>) -> bool {
187+
if_chain! {
188+
if let PatKind::Binding(annot, _, pat_ident, _) = pat.kind;
189+
if !matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
190+
if let ExprKind::Path(QPath::Resolved(_, Path {segments: [first_seg, ..], .. })) = expr.kind;
191+
then {
192+
return pat_ident.name == first_seg.ident.name;
193+
}
194+
}
195+
196+
false
197+
}

tests/ui/manual_map_option.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ fn main() {
148148

149149
// #7077
150150
let s = &String::new();
151+
#[allow(clippy::needless_match)]
151152
let _: Option<&str> = match Some(s) {
152153
Some(s) => Some(s),
153154
None => None,

tests/ui/manual_map_option.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,7 @@ fn main() {
214214

215215
// #7077
216216
let s = &String::new();
217+
#[allow(clippy::needless_match)]
217218
let _: Option<&str> = match Some(s) {
218219
Some(s) => Some(s),
219220
None => None,

0 commit comments

Comments
 (0)