diff options
| -rw-r--r-- | clippy_lints/src/matches/mod.rs | 10 | ||||
| -rw-r--r-- | clippy_lints/src/matches/nop_match.rs | 98 | ||||
| -rw-r--r-- | tests/ui/manual_map_option.fixed | 1 | ||||
| -rw-r--r-- | tests/ui/manual_map_option.rs | 1 | ||||
| -rw-r--r-- | tests/ui/nop_match.fixed | 71 | ||||
| -rw-r--r-- | tests/ui/nop_match.rs | 77 | ||||
| -rw-r--r-- | tests/ui/nop_match.stderr | 53 |
7 files changed, 238 insertions, 73 deletions
diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index 48300931dc4..7c29df0dfb4 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -16,12 +16,12 @@ mod match_same_arms; mod match_single_binding; mod match_wild_enum; mod match_wild_err_arm; +mod nop_match; mod overlapping_arms; mod redundant_pattern_match; mod rest_pat_in_fully_bound_struct; mod single_match; mod wild_in_or_pats; -mod nop_match; declare_clippy_lint! { /// ### What it does @@ -569,7 +569,7 @@ declare_clippy_lint! { declare_clippy_lint! { /// ### What it does - /// Checks for unnecessary `match` or match-like `if let` returns for `Option` and `Result` + /// Checks for unnecessary `match` or match-like `if let` returns for `Option` and `Result` /// when function signatures are the same. /// /// ### Why is this bad? @@ -583,7 +583,7 @@ declare_clippy_lint! { /// Err(err) => Err(err), /// } /// } - /// + /// /// fn bar() -> Option<i32> { /// if let Some(val) = option { /// Some(val) @@ -594,12 +594,12 @@ declare_clippy_lint! { /// ``` /// /// Could be replaced as - /// + /// /// ```rust,ignore /// fn foo() -> Result<(), i32> { /// result /// } - /// + /// /// fn bar() -> Option<i32> { /// option /// } diff --git a/clippy_lints/src/matches/nop_match.rs b/clippy_lints/src/matches/nop_match.rs index d995caf6cfd..a0fff490cfa 100644 --- a/clippy_lints/src/matches/nop_match.rs +++ b/clippy_lints/src/matches/nop_match.rs @@ -1,9 +1,11 @@ #![allow(unused_variables)] +use super::NOP_MATCH; use clippy_utils::diagnostics::span_lint_and_sugg; -use rustc_lint::LateContext; -use rustc_hir::{Arm, Expr}; +use clippy_utils::source::snippet_with_applicability; +use clippy_utils::{eq_expr_value, get_parent_expr}; use rustc_errors::Applicability; -use super::NOP_MATCH; +use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, Pat, PatKind, PathSegment, QPath}; +use rustc_lint::LateContext; pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { if false { @@ -20,15 +22,95 @@ pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { } pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) { - if false { + // This is for avoiding collision with `match_single_binding`. + if arms.len() < 2 { + return; + } + + for arm in arms { + if let PatKind::Wild = arm.pat.kind { + let ret_expr = strip_return(arm.body); + if !eq_expr_value(cx, ex, ret_expr) { + return; + } + } else if !pat_same_as_expr(arm.pat, arm.body) { + return; + } + } + + if let Some(match_expr) = get_parent_expr(cx, ex) { + let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, NOP_MATCH, - ex.span, + match_expr.span, "this match expression is unnecessary", "replace it with", - "".to_string(), - Applicability::MachineApplicable, + snippet_with_applicability(cx, ex.span, "..", &mut applicability).to_string(), + applicability, ); } -} \ No newline at end of file +} + +fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { + if let ExprKind::Ret(Some(ret)) = expr.kind { + ret + } else { + expr + } +} + +fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { + let expr = strip_return(expr); + match (&pat.kind, &expr.kind) { + ( + PatKind::TupleStruct(QPath::Resolved(_, path), [first_pat, ..], _), + ExprKind::Call(call_expr, [first_param, ..]), + ) => { + if let ExprKind::Path(QPath::Resolved(_, call_path)) = call_expr.kind { + if is_identical_segments(path.segments, call_path.segments) + && has_same_non_ref_symbol(first_pat, first_param) + { + return true; + } + } + }, + (PatKind::Path(QPath::Resolved(_, p_path)), ExprKind::Path(QPath::Resolved(_, e_path))) => { + return is_identical_segments(p_path.segments, e_path.segments); + }, + (PatKind::Lit(pat_lit_expr), ExprKind::Lit(expr_spanned)) => { + if let ExprKind::Lit(pat_spanned) = &pat_lit_expr.kind { + return pat_spanned.node == expr_spanned.node; + } + }, + _ => {}, + } + + false +} + +fn is_identical_segments(left_segs: &[PathSegment<'_>], right_segs: &[PathSegment<'_>]) -> bool { + if left_segs.len() != right_segs.len() { + return false; + } + for i in 0..left_segs.len() { + if left_segs[i].ident.name != right_segs[i].ident.name { + return false; + } + } + true +} + +fn has_same_non_ref_symbol(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { + if_chain! { + if let PatKind::Binding(annot, _, pat_ident, _) = pat.kind; + if !matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut); + if let ExprKind::Path(QPath::Resolved(_, path)) = expr.kind; + if let Some(first_seg) = path.segments.first(); + then { + return pat_ident.name == first_seg.ident.name; + } + } + + false +} diff --git a/tests/ui/manual_map_option.fixed b/tests/ui/manual_map_option.fixed index 294d79abc04..6f71b5d3d32 100644 --- a/tests/ui/manual_map_option.fixed +++ b/tests/ui/manual_map_option.fixed @@ -148,6 +148,7 @@ fn main() { // #7077 let s = &String::new(); + #[allow(clippy::nop_match)] let _: Option<&str> = match Some(s) { Some(s) => Some(s), None => None, diff --git a/tests/ui/manual_map_option.rs b/tests/ui/manual_map_option.rs index d11bf5ecb82..f1b79f127a0 100644 --- a/tests/ui/manual_map_option.rs +++ b/tests/ui/manual_map_option.rs @@ -214,6 +214,7 @@ fn main() { // #7077 let s = &String::new(); + #[allow(clippy::nop_match)] let _: Option<&str> = match Some(s) { Some(s) => Some(s), None => None, diff --git a/tests/ui/nop_match.fixed b/tests/ui/nop_match.fixed index 0e37bb85439..eff03a464ce 100644 --- a/tests/ui/nop_match.fixed +++ b/tests/ui/nop_match.fixed @@ -4,26 +4,47 @@ #![allow(clippy::question_mark)] #![allow(dead_code)] -fn option_match() -> Option<i32> { - match Some(1) { - Some(a) => Some(a), - None => None +fn func_ret_err<T>(err: T) -> Result<(), T> { + Err(err) +} + +enum SampleEnum { + A, + B, + C, +} + +fn useless_prim_type_match(x: i32) -> i32 { + x +} + +fn useless_custom_type_match(se: SampleEnum) -> SampleEnum { + se +} + +// Don't trigger +fn mingled_custom_type(se: SampleEnum) -> SampleEnum { + match se { + SampleEnum::A => SampleEnum::B, + SampleEnum::B => SampleEnum::C, + SampleEnum::C => SampleEnum::A, } } +fn option_match() -> Option<i32> { + Some(1) +} + fn result_match() -> Result<i32, i32> { - match Ok(1) { - Ok(a) => Ok(a), - Err(err) => Err(err) - } + Ok(1) +} + +fn result_match_func_call() { + let _ = func_ret_err(0_i32); } fn option_check() -> Option<i32> { - if let Some(a) = Some(1) { - Some(a) - } else { - None - } + if let Some(a) = Some(1) { Some(a) } else { None } } fn option_check_no_else() -> Option<i32> { @@ -33,10 +54,6 @@ fn option_check_no_else() -> Option<i32> { None } -fn func_ret_err<T>(err: T) -> Result<(), T> { - Err(err) -} - fn result_check_no_else() -> Result<(), i32> { if let Err(e) = func_ret_err(0_i32) { return Err(e); @@ -54,30 +71,18 @@ fn result_check_a() -> Result<(), i32> { // Don't trigger fn result_check_b() -> Result<(), i32> { - if let Err(e) = Ok(1) { - Err(e) - } else { - Ok(()) - } + if let Err(e) = Ok(1) { Err(e) } else { Ok(()) } } fn result_check_c() -> Result<(), i32> { let example = Ok(()); - if let Err(e) = example { - Err(e) - } else { - example - } + if let Err(e) = example { Err(e) } else { example } } // Don't trigger fn result_check_d() -> Result<(), i32> { let example = Ok(1); - if let Err(e) = example { - Err(e) - } else { - Ok(()) - } + if let Err(e) = example { Err(e) } else { Ok(()) } } -fn main() { } \ No newline at end of file +fn main() {} diff --git a/tests/ui/nop_match.rs b/tests/ui/nop_match.rs index 0e37bb85439..4c4b47ce9c5 100644 --- a/tests/ui/nop_match.rs +++ b/tests/ui/nop_match.rs @@ -4,26 +4,65 @@ #![allow(clippy::question_mark)] #![allow(dead_code)] +fn func_ret_err<T>(err: T) -> Result<(), T> { + Err(err) +} + +enum SampleEnum { + A, + B, + C, +} + +fn useless_prim_type_match(x: i32) -> i32 { + match x { + 0 => 0, + 1 => 1, + 2 => 2, + _ => x, + } +} + +fn useless_custom_type_match(se: SampleEnum) -> SampleEnum { + match se { + SampleEnum::A => SampleEnum::A, + SampleEnum::B => SampleEnum::B, + SampleEnum::C => SampleEnum::C, + } +} + +// Don't trigger +fn mingled_custom_type(se: SampleEnum) -> SampleEnum { + match se { + SampleEnum::A => SampleEnum::B, + SampleEnum::B => SampleEnum::C, + SampleEnum::C => SampleEnum::A, + } +} + fn option_match() -> Option<i32> { match Some(1) { Some(a) => Some(a), - None => None + None => None, } } fn result_match() -> Result<i32, i32> { match Ok(1) { Ok(a) => Ok(a), - Err(err) => Err(err) + Err(err) => Err(err), } } +fn result_match_func_call() { + let _ = match func_ret_err(0_i32) { + Ok(a) => Ok(a), + Err(err) => Err(err), + }; +} + fn option_check() -> Option<i32> { - if let Some(a) = Some(1) { - Some(a) - } else { - None - } + if let Some(a) = Some(1) { Some(a) } else { None } } fn option_check_no_else() -> Option<i32> { @@ -33,10 +72,6 @@ fn option_check_no_else() -> Option<i32> { None } -fn func_ret_err<T>(err: T) -> Result<(), T> { - Err(err) -} - fn result_check_no_else() -> Result<(), i32> { if let Err(e) = func_ret_err(0_i32) { return Err(e); @@ -54,30 +89,18 @@ fn result_check_a() -> Result<(), i32> { // Don't trigger fn result_check_b() -> Result<(), i32> { - if let Err(e) = Ok(1) { - Err(e) - } else { - Ok(()) - } + if let Err(e) = Ok(1) { Err(e) } else { Ok(()) } } fn result_check_c() -> Result<(), i32> { let example = Ok(()); - if let Err(e) = example { - Err(e) - } else { - example - } + if let Err(e) = example { Err(e) } else { example } } // Don't trigger fn result_check_d() -> Result<(), i32> { let example = Ok(1); - if let Err(e) = example { - Err(e) - } else { - Ok(()) - } + if let Err(e) = example { Err(e) } else { Ok(()) } } -fn main() { } \ No newline at end of file +fn main() {} diff --git a/tests/ui/nop_match.stderr b/tests/ui/nop_match.stderr index e69de29bb2d..bb171295f0f 100644 --- a/tests/ui/nop_match.stderr +++ b/tests/ui/nop_match.stderr @@ -0,0 +1,53 @@ +error: this match expression is unnecessary + --> $DIR/nop_match.rs:18:5 + | +LL | / match x { +LL | | 0 => 0, +LL | | 1 => 1, +LL | | 2 => 2, +LL | | _ => x, +LL | | } + | |_____^ help: replace it with: `x` + | + = note: `-D clippy::nop-match` implied by `-D warnings` + +error: this match expression is unnecessary + --> $DIR/nop_match.rs:27:5 + | +LL | / match se { +LL | | SampleEnum::A => SampleEnum::A, +LL | | SampleEnum::B => SampleEnum::B, +LL | | SampleEnum::C => SampleEnum::C, +LL | | } + | |_____^ help: replace it with: `se` + +error: this match expression is unnecessary + --> $DIR/nop_match.rs:44:5 + | +LL | / match Some(1) { +LL | | Some(a) => Some(a), +LL | | None => None, +LL | | } + | |_____^ help: replace it with: `Some(1)` + +error: this match expression is unnecessary + --> $DIR/nop_match.rs:51:5 + | +LL | / match Ok(1) { +LL | | Ok(a) => Ok(a), +LL | | Err(err) => Err(err), +LL | | } + | |_____^ help: replace it with: `Ok(1)` + +error: this match expression is unnecessary + --> $DIR/nop_match.rs:58:13 + | +LL | let _ = match func_ret_err(0_i32) { + | _____________^ +LL | | Ok(a) => Ok(a), +LL | | Err(err) => Err(err), +LL | | }; + | |_____^ help: replace it with: `func_ret_err(0_i32)` + +error: aborting due to 5 previous errors + |