From 2909b33a243591cec6a5d6d51dc6e1684809aeac Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 16 Mar 2022 16:26:56 +0800 Subject: [PATCH 1/4] quick fix of issue#8542 for lint `[needless_match]` remove `ref`/`ref mut` check --- clippy_lints/src/matches/needless_match.rs | 61 ++++----- tests/ui/needless_match.fixed | 116 ++++++++++++---- tests/ui/needless_match.rs | 148 +++++++++++++++------ tests/ui/needless_match.stderr | 83 ++++++------ 4 files changed, 257 insertions(+), 151 deletions(-) diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index 76131d307d7..d9544e18b18 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -5,7 +5,7 @@ use clippy_utils::{eq_expr_value, get_parent_expr, higher, is_else_clause, is_lang_ctor, peel_blocks_with_stmt}; use rustc_errors::Applicability; use rustc_hir::LangItem::OptionNone; -use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, Pat, PatKind, Path, PathSegment, QPath, UnOp}; +use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, Pat, PatKind, Path, PathSegment, QPath}; use rustc_lint::LateContext; use rustc_span::sym; @@ -21,7 +21,7 @@ pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) if !eq_expr_value(cx, ex, ret_expr) { return; } - } else if !pat_same_as_expr(arm.pat, arm.body) { + } else if !pat_same_as_expr(arm.pat, peel_blocks_with_stmt(arm.body)) { return; } } @@ -92,6 +92,9 @@ fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { if matches!(if_else.kind, ExprKind::Block(..)) { let else_expr = peel_blocks_with_stmt(if_else); + if matches!(else_expr.kind, ExprKind::Block(..)) { + return false; + } let ret = strip_return(else_expr); let let_expr_ty = cx.typeck_results().expr_ty(if_let.let_expr); if is_type_diagnostic_item(cx, let_expr_ty, sym::Option) { @@ -120,40 +123,25 @@ fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { let expr = strip_return(expr); match (&pat.kind, &expr.kind) { // Example: `Some(val) => Some(val)` - ( - PatKind::TupleStruct(QPath::Resolved(_, path), [first_pat, ..], _), - ExprKind::Call(call_expr, [first_param, ..]), - ) => { + (PatKind::TupleStruct(QPath::Resolved(_, path), tuple_params, _), ExprKind::Call(call_expr, call_params)) => { if let ExprKind::Path(QPath::Resolved(_, call_path)) = call_expr.kind { - if has_identical_segments(path.segments, call_path.segments) - && has_same_non_ref_symbol(first_pat, first_param) - { - return true; - } + return has_identical_segments(path.segments, call_path.segments) + && has_same_non_ref_symbols(tuple_params, call_params); } }, - // Example: `val => val`, or `ref val => *val` - (PatKind::Binding(annot, _, pat_ident, _), _) => { - let new_expr = if let ( - BindingAnnotation::Ref | BindingAnnotation::RefMut, - ExprKind::Unary(UnOp::Deref, operand_expr), - ) = (annot, &expr.kind) - { - operand_expr - } else { - expr - }; - - if let ExprKind::Path(QPath::Resolved( + // Example: `val => val` + ( + PatKind::Binding(annot, _, pat_ident, _), + ExprKind::Path(QPath::Resolved( _, Path { segments: [first_seg, ..], .. }, - )) = new_expr.kind - { - return pat_ident.name == first_seg.ident.name; - } + )), + ) => { + return !matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut) + && pat_ident.name == first_seg.ident.name; }, // Example: `Custom::TypeA => Custom::TypeB`, or `None => None` (PatKind::Path(QPath::Resolved(_, p_path)), ExprKind::Path(QPath::Resolved(_, e_path))) => { @@ -183,15 +171,16 @@ fn has_identical_segments(left_segs: &[PathSegment<'_>], right_segs: &[PathSegme 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 {segments: [first_seg, ..], .. })) = expr.kind; - then { - return pat_ident.name == first_seg.ident.name; +fn has_same_non_ref_symbols(pats: &[Pat<'_>], exprs: &[Expr<'_>]) -> bool { + if pats.len() != exprs.len() { + return false; + } + + for i in 0..pats.len() { + if !pat_same_as_expr(&pats[i], &exprs[i]) { + return false; } } - false + true } diff --git a/tests/ui/needless_match.fixed b/tests/ui/needless_match.fixed index ece18ad737f..6a518f83925 100644 --- a/tests/ui/needless_match.fixed +++ b/tests/ui/needless_match.fixed @@ -4,38 +4,35 @@ #![allow(dead_code)] #[derive(Clone, Copy)] -enum Choice { +enum Simple { A, B, C, D, } -#[allow(unused_mut)] fn useless_match() { - let mut i = 10; + let i = 10; let _: i32 = i; - let _: i32 = i; - let mut _i_mut = i; - let s = "test"; let _: &str = s; } -fn custom_type_match(se: Choice) { - let _: Choice = se; +fn custom_type_match() { + let se = Simple::A; + let _: Simple = se; // Don't trigger - let _: Choice = match se { - Choice::A => Choice::A, - Choice::B => Choice::B, - _ => Choice::C, + let _: Simple = match se { + Simple::A => Simple::A, + Simple::B => Simple::B, + _ => Simple::C, }; // Mingled, don't trigger - let _: Choice = match se { - Choice::A => Choice::B, - Choice::B => Choice::C, - Choice::C => Choice::D, - Choice::D => Choice::A, + let _: Simple = match se { + Simple::A => Simple::B, + Simple::B => Simple::C, + Simple::C => Simple::D, + Simple::D => Simple::A, }; } @@ -55,29 +52,96 @@ fn func_ret_err(err: T) -> Result { fn result_match() { let _: Result = Ok(1); let _: Result = func_ret_err(0_i32); + // as ref, don't trigger + let res = &func_ret_err(0_i32); + let _: Result<&i32, &i32> = match *res { + Ok(ref x) => Ok(x), + Err(ref x) => Err(x), + }; } fn if_let_option() -> Option { Some(1) } -fn if_let_result(x: Result<(), i32>) { - let _: Result<(), i32> = x; - let _: Result<(), i32> = x; +fn if_let_result() { + let x: Result = Ok(1); + let _: Result = x; + let _: Result = x; // Input type mismatch, don't trigger - let _: Result<(), i32> = if let Err(e) = Ok(1) { Err(e) } else { x }; + let _: Result = if let Err(e) = Ok(1) { Err(e) } else { x }; } -fn if_let_custom_enum(x: Choice) { - let _: Choice = x; +fn if_let_custom_enum(x: Simple) { + let _: Simple = x; + // Don't trigger - let _: Choice = if let Choice::A = x { - Choice::A + let _: Simple = if let Simple::A = x { + Simple::A } else if true { - Choice::B + Simple::B } else { x }; } +mod issue8542 { + #[derive(Clone, Copy)] + enum E { + VariantA(u8, u8), + VariantB(u8, bool), + } + + enum Complex { + A(u8), + B(u8, bool), + C(u8, i32, f64), + D(E, bool), + } + + fn match_test() { + let ce = Complex::B(8, false); + let aa = 0_u8; + let bb = false; + + let _: Complex = ce; + + // Don't trigger + let _: Complex = match ce { + Complex::A(_) => Complex::A(aa), + Complex::B(_, b) => Complex::B(aa, b), + Complex::C(_, b, _) => Complex::C(aa, b, 64_f64), + Complex::D(e, b) => Complex::D(e, b), + }; + + // Don't trigger + let _: Complex = match ce { + Complex::A(a) => Complex::A(a), + Complex::B(a, _) => Complex::B(a, bb), + Complex::C(a, _, _) => Complex::C(a, 32_i32, 64_f64), + _ => ce, + }; + } + + fn if_let_test() { + fn do_something() {} + + // Don't trigger + let _ = if let Some(a) = Some(1) { + Some(a) + } else { + do_something(); + None + }; + + // Don't trigger + let _ = if let Some(a) = Some(1) { + do_something(); + Some(a) + } else { + None + }; + } +} + fn main() {} diff --git a/tests/ui/needless_match.rs b/tests/ui/needless_match.rs index 36649de35a6..ffc6a291a15 100644 --- a/tests/ui/needless_match.rs +++ b/tests/ui/needless_match.rs @@ -4,33 +4,21 @@ #![allow(dead_code)] #[derive(Clone, Copy)] -enum Choice { +enum Simple { A, B, C, D, } -#[allow(unused_mut)] fn useless_match() { - let mut i = 10; + let i = 10; let _: i32 = match i { 0 => 0, 1 => 1, 2 => 2, _ => i, }; - let _: i32 = match i { - 0 => 0, - 1 => 1, - ref i => *i, - }; - let mut _i_mut = match i { - 0 => 0, - 1 => 1, - ref mut i => *i, - }; - let s = "test"; let _: &str = match s { "a" => "a", @@ -39,25 +27,26 @@ fn useless_match() { }; } -fn custom_type_match(se: Choice) { - let _: Choice = match se { - Choice::A => Choice::A, - Choice::B => Choice::B, - Choice::C => Choice::C, - Choice::D => Choice::D, +fn custom_type_match() { + let se = Simple::A; + let _: Simple = match se { + Simple::A => Simple::A, + Simple::B => Simple::B, + Simple::C => Simple::C, + Simple::D => Simple::D, }; // Don't trigger - let _: Choice = match se { - Choice::A => Choice::A, - Choice::B => Choice::B, - _ => Choice::C, + let _: Simple = match se { + Simple::A => Simple::A, + Simple::B => Simple::B, + _ => Simple::C, }; // Mingled, don't trigger - let _: Choice = match se { - Choice::A => Choice::B, - Choice::B => Choice::C, - Choice::C => Choice::D, - Choice::D => Choice::A, + let _: Simple = match se { + Simple::A => Simple::B, + Simple::B => Simple::C, + Simple::C => Simple::D, + Simple::D => Simple::A, }; } @@ -86,37 +75,110 @@ fn result_match() { Err(err) => Err(err), Ok(a) => Ok(a), }; + // as ref, don't trigger + let res = &func_ret_err(0_i32); + let _: Result<&i32, &i32> = match *res { + Ok(ref x) => Ok(x), + Err(ref x) => Err(x), + }; } fn if_let_option() -> Option { if let Some(a) = Some(1) { Some(a) } else { None } } -fn if_let_result(x: Result<(), i32>) { - let _: Result<(), i32> = if let Err(e) = x { Err(e) } else { x }; - let _: Result<(), i32> = if let Ok(val) = x { Ok(val) } else { x }; +fn if_let_result() { + let x: Result = Ok(1); + let _: Result = if let Err(e) = x { Err(e) } else { x }; + let _: Result = if let Ok(val) = x { Ok(val) } else { x }; // Input type mismatch, don't trigger - let _: Result<(), i32> = if let Err(e) = Ok(1) { Err(e) } else { x }; + let _: Result = if let Err(e) = Ok(1) { Err(e) } else { x }; } -fn if_let_custom_enum(x: Choice) { - let _: Choice = if let Choice::A = x { - Choice::A - } else if let Choice::B = x { - Choice::B - } else if let Choice::C = x { - Choice::C +fn if_let_custom_enum(x: Simple) { + let _: Simple = if let Simple::A = x { + Simple::A + } else if let Simple::B = x { + Simple::B + } else if let Simple::C = x { + Simple::C } else { x }; + // Don't trigger - let _: Choice = if let Choice::A = x { - Choice::A + let _: Simple = if let Simple::A = x { + Simple::A } else if true { - Choice::B + Simple::B } else { x }; } +mod issue8542 { + #[derive(Clone, Copy)] + enum E { + VariantA(u8, u8), + VariantB(u8, bool), + } + + enum Complex { + A(u8), + B(u8, bool), + C(u8, i32, f64), + D(E, bool), + } + + fn match_test() { + let ce = Complex::B(8, false); + let aa = 0_u8; + let bb = false; + + let _: Complex = match ce { + Complex::A(a) => Complex::A(a), + Complex::B(a, b) => Complex::B(a, b), + Complex::C(a, b, c) => Complex::C(a, b, c), + Complex::D(E::VariantA(ea, eb), b) => Complex::D(E::VariantA(ea, eb), b), + Complex::D(E::VariantB(ea, eb), b) => Complex::D(E::VariantB(ea, eb), b), + }; + + // Don't trigger + let _: Complex = match ce { + Complex::A(_) => Complex::A(aa), + Complex::B(_, b) => Complex::B(aa, b), + Complex::C(_, b, _) => Complex::C(aa, b, 64_f64), + Complex::D(e, b) => Complex::D(e, b), + }; + + // Don't trigger + let _: Complex = match ce { + Complex::A(a) => Complex::A(a), + Complex::B(a, _) => Complex::B(a, bb), + Complex::C(a, _, _) => Complex::C(a, 32_i32, 64_f64), + _ => ce, + }; + } + + fn if_let_test() { + fn do_something() {} + + // Don't trigger + let _ = if let Some(a) = Some(1) { + Some(a) + } else { + do_something(); + None + }; + + // Don't trigger + let _ = if let Some(a) = Some(1) { + do_something(); + Some(a) + } else { + None + }; + } +} + fn main() {} diff --git a/tests/ui/needless_match.stderr b/tests/ui/needless_match.stderr index ad1525406ad..67bd84d6bbc 100644 --- a/tests/ui/needless_match.stderr +++ b/tests/ui/needless_match.stderr @@ -1,5 +1,5 @@ error: this match expression is unnecessary - --> $DIR/needless_match.rs:17:18 + --> $DIR/needless_match.rs:16:18 | LL | let _: i32 = match i { | __________________^ @@ -13,29 +13,7 @@ LL | | }; = note: `-D clippy::needless-match` implied by `-D warnings` error: this match expression is unnecessary - --> $DIR/needless_match.rs:23:18 - | -LL | let _: i32 = match i { - | __________________^ -LL | | 0 => 0, -LL | | 1 => 1, -LL | | ref i => *i, -LL | | }; - | |_____^ help: replace it with: `i` - -error: this match expression is unnecessary - --> $DIR/needless_match.rs:28:22 - | -LL | let mut _i_mut = match i { - | ______________________^ -LL | | 0 => 0, -LL | | 1 => 1, -LL | | ref mut i => *i, -LL | | }; - | |_____^ help: replace it with: `i` - -error: this match expression is unnecessary - --> $DIR/needless_match.rs:35:19 + --> $DIR/needless_match.rs:23:19 | LL | let _: &str = match s { | ___________________^ @@ -46,19 +24,19 @@ LL | | }; | |_____^ help: replace it with: `s` error: this match expression is unnecessary - --> $DIR/needless_match.rs:43:21 + --> $DIR/needless_match.rs:32:21 | -LL | let _: Choice = match se { +LL | let _: Simple = match se { | _____________________^ -LL | | Choice::A => Choice::A, -LL | | Choice::B => Choice::B, -LL | | Choice::C => Choice::C, -LL | | Choice::D => Choice::D, +LL | | Simple::A => Simple::A, +LL | | Simple::B => Simple::B, +LL | | Simple::C => Simple::C, +LL | | Simple::D => Simple::D, LL | | }; | |_____^ help: replace it with: `se` error: this match expression is unnecessary - --> $DIR/needless_match.rs:65:26 + --> $DIR/needless_match.rs:54:26 | LL | let _: Option = match x { | __________________________^ @@ -68,7 +46,7 @@ LL | | }; | |_____^ help: replace it with: `x` error: this match expression is unnecessary - --> $DIR/needless_match.rs:81:31 + --> $DIR/needless_match.rs:70:31 | LL | let _: Result = match Ok(1) { | _______________________________^ @@ -78,7 +56,7 @@ LL | | }; | |_____^ help: replace it with: `Ok(1)` error: this match expression is unnecessary - --> $DIR/needless_match.rs:85:31 + --> $DIR/needless_match.rs:74:31 | LL | let _: Result = match func_ret_err(0_i32) { | _______________________________^ @@ -88,35 +66,48 @@ LL | | }; | |_____^ help: replace it with: `func_ret_err(0_i32)` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:92:5 + --> $DIR/needless_match.rs:87:5 | LL | if let Some(a) = Some(1) { Some(a) } else { None } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `Some(1)` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:96:30 + --> $DIR/needless_match.rs:92:31 | -LL | let _: Result<(), i32> = if let Err(e) = x { Err(e) } else { x }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x` +LL | let _: Result = if let Err(e) = x { Err(e) } else { x }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:97:30 + --> $DIR/needless_match.rs:93:31 | -LL | let _: Result<(), i32> = if let Ok(val) = x { Ok(val) } else { x }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x` +LL | let _: Result = if let Ok(val) = x { Ok(val) } else { x }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:103:21 + --> $DIR/needless_match.rs:99:21 | -LL | let _: Choice = if let Choice::A = x { +LL | let _: Simple = if let Simple::A = x { | _____________________^ -LL | | Choice::A -LL | | } else if let Choice::B = x { -LL | | Choice::B +LL | | Simple::A +LL | | } else if let Simple::B = x { +LL | | Simple::B ... | LL | | x LL | | }; | |_____^ help: replace it with: `x` -error: aborting due to 12 previous errors +error: this match expression is unnecessary + --> $DIR/needless_match.rs:138:26 + | +LL | let _: Complex = match ce { + | __________________________^ +LL | | Complex::A(a) => Complex::A(a), +LL | | Complex::B(a, b) => Complex::B(a, b), +LL | | Complex::C(a, b, c) => Complex::C(a, b, c), +LL | | Complex::D(E::VariantA(ea, eb), b) => Complex::D(E::VariantA(ea, eb), b), +LL | | Complex::D(E::VariantB(ea, eb), b) => Complex::D(E::VariantB(ea, eb), b), +LL | | }; + | |_________^ help: replace it with: `ce` + +error: aborting due to 11 previous errors From 4b128624ed44223b7d0b61438b0d02fc91419e43 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Thu, 17 Mar 2022 23:06:31 +0800 Subject: [PATCH 2/4] fix #8551, add test cases, and some code improvement --- clippy_lints/src/matches/mod.rs | 2 +- clippy_lints/src/matches/needless_match.rs | 108 ++++++++++++++------- tests/ui/needless_match.fixed | 66 ++++++++++--- tests/ui/needless_match.rs | 66 ++++++++++--- tests/ui/needless_match.stderr | 14 +-- 5 files changed, 185 insertions(+), 71 deletions(-) diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index ff85623acf4..e93b494653f 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -667,7 +667,7 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { overlapping_arms::check(cx, ex, arms); match_wild_enum::check(cx, ex, arms); match_as_ref::check(cx, ex, arms, expr); - needless_match::check_match(cx, ex, arms); + needless_match::check_match(cx, ex, arms, expr); if self.infallible_destructuring_match_linted { self.infallible_destructuring_match_linted = false; diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index d9544e18b18..2830ae1f61d 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -1,37 +1,25 @@ use super::NEEDLESS_MATCH; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; -use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{eq_expr_value, get_parent_expr, higher, is_else_clause, is_lang_ctor, peel_blocks_with_stmt}; +use clippy_utils::ty::{is_type_diagnostic_item, same_type_and_consts}; +use clippy_utils::{ + eq_expr_value, get_parent_expr_for_hir, get_parent_node, higher, is_else_clause, is_lang_ctor, + peel_blocks_with_stmt, +}; use rustc_errors::Applicability; use rustc_hir::LangItem::OptionNone; -use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, Pat, PatKind, Path, PathSegment, QPath}; +use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, FnRetTy, Node, Pat, PatKind, Path, PathSegment, QPath}; use rustc_lint::LateContext; use rustc_span::sym; +use rustc_typeck::hir_ty_to_ty; -pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) { - // 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, peel_blocks_with_stmt(arm.body)) { - return; - } - } - - if let Some(match_expr) = get_parent_expr(cx, ex) { +pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) { + if arms.len() > 1 && !is_coercion_casting(cx, ex, expr) && check_all_arms(cx, ex, arms) { let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, NEEDLESS_MATCH, - match_expr.span, + expr.span, "this match expression is unnecessary", "replace it with", snippet_with_applicability(cx, ex.span, "..", &mut applicability).to_string(), @@ -60,11 +48,8 @@ pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) /// } /// ``` pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { - if_chain! { - if let Some(ref if_let) = higher::IfLet::hir(cx, ex); - if !is_else_clause(cx.tcx, ex); - if check_if_let(cx, if_let); - then { + if let Some(ref if_let) = higher::IfLet::hir(cx, ex) { + if !is_else_clause(cx.tcx, ex) && !is_coercion_casting(cx, if_let.let_expr, ex) && check_if_let(cx, if_let) { let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, @@ -79,6 +64,19 @@ pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { } } +fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>]) -> bool { + for arm in arms { + let arm_expr = peel_blocks_with_stmt(arm.body); + if let PatKind::Wild = arm.pat.kind { + return eq_expr_value(cx, match_expr, strip_return(arm_expr)); + } else if !pat_same_as_expr(arm.pat, arm_expr) { + return false; + } + } + + true +} + fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { if let Some(if_else) = if_let.if_else { if !pat_same_as_expr(if_let.let_pat, peel_blocks_with_stmt(if_let.if_then)) { @@ -101,12 +99,12 @@ fn check_if_let(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool { if let ExprKind::Path(ref qpath) = ret.kind { return is_lang_ctor(cx, qpath, OptionNone) || eq_expr_value(cx, if_let.let_expr, ret); } - } else { - return eq_expr_value(cx, if_let.let_expr, ret); + return true; } - return true; + return eq_expr_value(cx, if_let.let_expr, ret); } } + false } @@ -119,14 +117,52 @@ fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { } } +/// Manually check for coercion casting by checking if the type of the match operand or let expr +/// differs with the assigned local variable or the funtion return type. +fn is_coercion_casting(cx: &LateContext<'_>, match_expr: &Expr<'_>, expr: &Expr<'_>) -> bool { + if let Some(p_node) = get_parent_node(cx.tcx, expr.hir_id) { + match p_node { + // Compare match_expr ty with local in `let local = match match_expr {..}` + Node::Local(local) => { + let results = cx.typeck_results(); + return !same_type_and_consts(results.node_type(local.hir_id), results.expr_ty(match_expr)); + }, + // compare match_expr ty with RetTy in `fn foo() -> RetTy` + Node::Item(..) => { + if let Some(fn_decl) = p_node.fn_decl() { + if let FnRetTy::Return(ret_ty) = fn_decl.output { + return !same_type_and_consts( + hir_ty_to_ty(cx.tcx, ret_ty), + cx.typeck_results().expr_ty(match_expr), + ); + } + } + }, + // check the parent expr for this whole block `{ match match_expr {..} }` + Node::Block(block) => { + if let Some(block_parent_expr) = get_parent_expr_for_hir(cx, block.hir_id) { + return is_coercion_casting(cx, match_expr, block_parent_expr); + } + }, + // recursively call on `if xxx {..}` etc. + Node::Expr(p_expr) => { + return is_coercion_casting(cx, match_expr, p_expr); + }, + _ => {}, + } + } + + false +} + fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { let expr = strip_return(expr); match (&pat.kind, &expr.kind) { // Example: `Some(val) => Some(val)` (PatKind::TupleStruct(QPath::Resolved(_, path), tuple_params, _), ExprKind::Call(call_expr, call_params)) => { if let ExprKind::Path(QPath::Resolved(_, call_path)) = call_expr.kind { - return has_identical_segments(path.segments, call_path.segments) - && has_same_non_ref_symbols(tuple_params, call_params); + return same_segments(path.segments, call_path.segments) + && same_non_ref_symbols(tuple_params, call_params); } }, // Example: `val => val` @@ -145,7 +181,7 @@ fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { }, // Example: `Custom::TypeA => Custom::TypeB`, or `None => None` (PatKind::Path(QPath::Resolved(_, p_path)), ExprKind::Path(QPath::Resolved(_, e_path))) => { - return has_identical_segments(p_path.segments, e_path.segments); + return same_segments(p_path.segments, e_path.segments); }, // Example: `5 => 5` (PatKind::Lit(pat_lit_expr), ExprKind::Lit(expr_spanned)) => { @@ -159,19 +195,21 @@ fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { false } -fn has_identical_segments(left_segs: &[PathSegment<'_>], right_segs: &[PathSegment<'_>]) -> bool { +fn same_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_symbols(pats: &[Pat<'_>], exprs: &[Expr<'_>]) -> bool { +fn same_non_ref_symbols(pats: &[Pat<'_>], exprs: &[Expr<'_>]) -> bool { if pats.len() != exprs.len() { return false; } diff --git a/tests/ui/needless_match.fixed b/tests/ui/needless_match.fixed index 6a518f83925..d55dd589a54 100644 --- a/tests/ui/needless_match.fixed +++ b/tests/ui/needless_match.fixed @@ -60,8 +60,26 @@ fn result_match() { }; } -fn if_let_option() -> Option { - Some(1) +fn if_let_option() { + let _ = Some(1); + + fn do_something() {} + + // Don't trigger + let _ = if let Some(a) = Some(1) { + Some(a) + } else { + do_something(); + None + }; + + // Don't trigger + let _ = if let Some(a) = Some(1) { + do_something(); + Some(a) + } else { + None + }; } fn if_let_result() { @@ -122,25 +140,45 @@ mod issue8542 { _ => ce, }; } +} - fn if_let_test() { - fn do_something() {} +/// Lint triggered when type coercions happen. +/// Do NOT trigger on any of these. +mod issue8551 { + trait Trait {} + struct Struct; + impl Trait for Struct {} - // Don't trigger - let _ = if let Some(a) = Some(1) { - Some(a) - } else { - do_something(); - None + fn optmap(s: Option<&Struct>) -> Option<&dyn Trait> { + match s { + Some(s) => Some(s), + None => None, + } + } + + fn lint_tests() { + let option: Option<&Struct> = None; + let _: Option<&dyn Trait> = match option { + Some(s) => Some(s), + None => None, }; - // Don't trigger - let _ = if let Some(a) = Some(1) { - do_something(); - Some(a) + let _: Option<&dyn Trait> = if true { + match option { + Some(s) => Some(s), + None => None, + } } else { None }; + + let result: Result<&Struct, i32> = Err(0); + let _: Result<&dyn Trait, i32> = match result { + Ok(s) => Ok(s), + Err(e) => Err(e), + }; + + let _: Option<&dyn Trait> = if let Some(s) = option { Some(s) } else { None }; } } diff --git a/tests/ui/needless_match.rs b/tests/ui/needless_match.rs index ffc6a291a15..f3c7ee01a47 100644 --- a/tests/ui/needless_match.rs +++ b/tests/ui/needless_match.rs @@ -83,8 +83,26 @@ fn result_match() { }; } -fn if_let_option() -> Option { - if let Some(a) = Some(1) { Some(a) } else { None } +fn if_let_option() { + let _ = if let Some(a) = Some(1) { Some(a) } else { None }; + + fn do_something() {} + + // Don't trigger + let _ = if let Some(a) = Some(1) { + Some(a) + } else { + do_something(); + None + }; + + // Don't trigger + let _ = if let Some(a) = Some(1) { + do_something(); + Some(a) + } else { + None + }; } fn if_let_result() { @@ -159,25 +177,45 @@ fn match_test() { _ => ce, }; } +} - fn if_let_test() { - fn do_something() {} +/// Lint triggered when type coercions happen. +/// Do NOT trigger on any of these. +mod issue8551 { + trait Trait {} + struct Struct; + impl Trait for Struct {} - // Don't trigger - let _ = if let Some(a) = Some(1) { - Some(a) - } else { - do_something(); - None + fn optmap(s: Option<&Struct>) -> Option<&dyn Trait> { + match s { + Some(s) => Some(s), + None => None, + } + } + + fn lint_tests() { + let option: Option<&Struct> = None; + let _: Option<&dyn Trait> = match option { + Some(s) => Some(s), + None => None, }; - // Don't trigger - let _ = if let Some(a) = Some(1) { - do_something(); - Some(a) + let _: Option<&dyn Trait> = if true { + match option { + Some(s) => Some(s), + None => None, + } } else { None }; + + let result: Result<&Struct, i32> = Err(0); + let _: Result<&dyn Trait, i32> = match result { + Ok(s) => Ok(s), + Err(e) => Err(e), + }; + + let _: Option<&dyn Trait> = if let Some(s) = option { Some(s) } else { None }; } } diff --git a/tests/ui/needless_match.stderr b/tests/ui/needless_match.stderr index 67bd84d6bbc..34c5226f060 100644 --- a/tests/ui/needless_match.stderr +++ b/tests/ui/needless_match.stderr @@ -66,25 +66,25 @@ LL | | }; | |_____^ help: replace it with: `func_ret_err(0_i32)` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:87:5 + --> $DIR/needless_match.rs:87:13 | -LL | if let Some(a) = Some(1) { Some(a) } else { None } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `Some(1)` +LL | let _ = if let Some(a) = Some(1) { Some(a) } else { None }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `Some(1)` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:92:31 + --> $DIR/needless_match.rs:110:31 | LL | let _: Result = if let Err(e) = x { Err(e) } else { x }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:93:31 + --> $DIR/needless_match.rs:111:31 | LL | let _: Result = if let Ok(val) = x { Ok(val) } else { x }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x` error: this if-let expression is unnecessary - --> $DIR/needless_match.rs:99:21 + --> $DIR/needless_match.rs:117:21 | LL | let _: Simple = if let Simple::A = x { | _____________________^ @@ -97,7 +97,7 @@ LL | | }; | |_____^ help: replace it with: `x` error: this match expression is unnecessary - --> $DIR/needless_match.rs:138:26 + --> $DIR/needless_match.rs:156:26 | LL | let _: Complex = match ce { | __________________________^ From 448a26d6960d9a4902db59a57b61ed0586933dfd Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Tue, 29 Mar 2022 15:23:19 +0800 Subject: [PATCH 3/4] improve parent expr check --- clippy_lints/src/matches/needless_match.rs | 20 ++++++++------------ tests/ui/needless_match.fixed | 12 ++++++++++++ tests/ui/needless_match.rs | 12 ++++++++++++ 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index 2830ae1f61d..3703488632b 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -14,7 +14,7 @@ use rustc_typeck::hir_ty_to_ty; pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) { - if arms.len() > 1 && !is_coercion_casting(cx, ex, expr) && check_all_arms(cx, ex, arms) { + if arms.len() > 1 && expr_ty_matches_p_ty(cx, ex, expr) && check_all_arms(cx, ex, arms) { let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, @@ -49,7 +49,7 @@ pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], /// ``` pub(crate) fn check(cx: &LateContext<'_>, ex: &Expr<'_>) { if let Some(ref if_let) = higher::IfLet::hir(cx, ex) { - if !is_else_clause(cx.tcx, ex) && !is_coercion_casting(cx, if_let.let_expr, ex) && check_if_let(cx, if_let) { + if !is_else_clause(cx.tcx, ex) && expr_ty_matches_p_ty(cx, if_let.let_expr, ex) && check_if_let(cx, if_let) { let mut applicability = Applicability::MachineApplicable; span_lint_and_sugg( cx, @@ -119,39 +119,35 @@ fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { /// Manually check for coercion casting by checking if the type of the match operand or let expr /// differs with the assigned local variable or the funtion return type. -fn is_coercion_casting(cx: &LateContext<'_>, match_expr: &Expr<'_>, expr: &Expr<'_>) -> bool { - if let Some(p_node) = get_parent_node(cx.tcx, expr.hir_id) { +fn expr_ty_matches_p_ty(cx: &LateContext<'_>, expr: &Expr<'_>, p_expr: &Expr<'_>) -> bool { + if let Some(p_node) = get_parent_node(cx.tcx, p_expr.hir_id) { match p_node { // Compare match_expr ty with local in `let local = match match_expr {..}` Node::Local(local) => { let results = cx.typeck_results(); - return !same_type_and_consts(results.node_type(local.hir_id), results.expr_ty(match_expr)); + return same_type_and_consts(results.node_type(local.hir_id), results.expr_ty(expr)); }, // compare match_expr ty with RetTy in `fn foo() -> RetTy` Node::Item(..) => { if let Some(fn_decl) = p_node.fn_decl() { if let FnRetTy::Return(ret_ty) = fn_decl.output { - return !same_type_and_consts( - hir_ty_to_ty(cx.tcx, ret_ty), - cx.typeck_results().expr_ty(match_expr), - ); + return same_type_and_consts(hir_ty_to_ty(cx.tcx, ret_ty), cx.typeck_results().expr_ty(expr)); } } }, // check the parent expr for this whole block `{ match match_expr {..} }` Node::Block(block) => { if let Some(block_parent_expr) = get_parent_expr_for_hir(cx, block.hir_id) { - return is_coercion_casting(cx, match_expr, block_parent_expr); + return expr_ty_matches_p_ty(cx, expr, block_parent_expr); } }, // recursively call on `if xxx {..}` etc. Node::Expr(p_expr) => { - return is_coercion_casting(cx, match_expr, p_expr); + return expr_ty_matches_p_ty(cx, expr, p_expr); }, _ => {}, } } - false } diff --git a/tests/ui/needless_match.fixed b/tests/ui/needless_match.fixed index d55dd589a54..9ccccaa1725 100644 --- a/tests/ui/needless_match.fixed +++ b/tests/ui/needless_match.fixed @@ -182,4 +182,16 @@ mod issue8551 { } } +trait Tr { + fn as_mut(&mut self) -> Result<&mut i32, &mut i32>; +} +impl Tr for Result { + fn as_mut(&mut self) -> Result<&mut i32, &mut i32> { + match self { + Ok(x) => Ok(x), + Err(e) => Err(e), + } + } +} + fn main() {} diff --git a/tests/ui/needless_match.rs b/tests/ui/needless_match.rs index f3c7ee01a47..d210ecff7f1 100644 --- a/tests/ui/needless_match.rs +++ b/tests/ui/needless_match.rs @@ -219,4 +219,16 @@ fn lint_tests() { } } +trait Tr { + fn as_mut(&mut self) -> Result<&mut i32, &mut i32>; +} +impl Tr for Result { + fn as_mut(&mut self) -> Result<&mut i32, &mut i32> { + match self { + Ok(x) => Ok(x), + Err(e) => Err(e), + } + } +} + fn main() {} From 85b081b65aa36e0d7cbdfba7cd705517563c0bd4 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 6 Apr 2022 20:44:54 +0800 Subject: [PATCH 4/4] code refractor for `[needless_match]` --- clippy_lints/src/matches/needless_match.rs | 27 +++++++--------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/matches/needless_match.rs b/clippy_lints/src/matches/needless_match.rs index 3703488632b..2105a03e03a 100644 --- a/clippy_lints/src/matches/needless_match.rs +++ b/clippy_lints/src/matches/needless_match.rs @@ -3,12 +3,12 @@ use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::{is_type_diagnostic_item, same_type_and_consts}; use clippy_utils::{ - eq_expr_value, get_parent_expr_for_hir, get_parent_node, higher, is_else_clause, is_lang_ctor, + eq_expr_value, get_parent_expr_for_hir, get_parent_node, higher, is_else_clause, is_lang_ctor, over, peel_blocks_with_stmt, }; use rustc_errors::Applicability; use rustc_hir::LangItem::OptionNone; -use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, FnRetTy, Node, Pat, PatKind, Path, PathSegment, QPath}; +use rustc_hir::{Arm, BindingAnnotation, Expr, ExprKind, FnRetTy, Node, Pat, PatKind, Path, QPath}; use rustc_lint::LateContext; use rustc_span::sym; use rustc_typeck::hir_ty_to_ty; @@ -157,8 +157,9 @@ fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { // Example: `Some(val) => Some(val)` (PatKind::TupleStruct(QPath::Resolved(_, path), tuple_params, _), ExprKind::Call(call_expr, call_params)) => { if let ExprKind::Path(QPath::Resolved(_, call_path)) = call_expr.kind { - return same_segments(path.segments, call_path.segments) - && same_non_ref_symbols(tuple_params, call_params); + return over(path.segments, call_path.segments, |pat_seg, call_seg| { + pat_seg.ident.name == call_seg.ident.name + }) && same_non_ref_symbols(tuple_params, call_params); } }, // Example: `val => val` @@ -177,7 +178,9 @@ fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { }, // Example: `Custom::TypeA => Custom::TypeB`, or `None => None` (PatKind::Path(QPath::Resolved(_, p_path)), ExprKind::Path(QPath::Resolved(_, e_path))) => { - return same_segments(p_path.segments, e_path.segments); + return over(p_path.segments, e_path.segments, |p_seg, e_seg| { + p_seg.ident.name == e_seg.ident.name + }); }, // Example: `5 => 5` (PatKind::Lit(pat_lit_expr), ExprKind::Lit(expr_spanned)) => { @@ -191,20 +194,6 @@ fn pat_same_as_expr(pat: &Pat<'_>, expr: &Expr<'_>) -> bool { false } -fn same_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 same_non_ref_symbols(pats: &[Pat<'_>], exprs: &[Expr<'_>]) -> bool { if pats.len() != exprs.len() { return false;