fix rust-lang#8551 and add some tests

J-ZhengLi · Mar 17, 2022 · 9dc1465 · 9dc1465
1 parent 2909b33
commit 9dc1465
Show file tree

Hide file tree

Showing 5 changed files with 173 additions and 51 deletions.
diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs
@@ -667,7 +667,7 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
                     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
@@ -1,15 +1,19 @@
 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<'_>]) {
+pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>], expr: &Expr<'_>) {
     // This is for avoiding collision with `match_single_binding`.
     if arms.len() < 2 {
         return;
@@ -26,18 +30,20 @@ pub(crate) fn check_match(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>])
         }
     }
 
-    if let Some(match_expr) = get_parent_expr(cx, ex) {
-        let mut applicability = Applicability::MachineApplicable;
-        span_lint_and_sugg(
-            cx,
-            NEEDLESS_MATCH,
-            match_expr.span,
-            "this match expression is unnecessary",
-            "replace it with",
-            snippet_with_applicability(cx, ex.span, "..", &mut applicability).to_string(),
-            applicability,
-        );
+    if is_type_differ(cx, ex, expr) {
+        return;
     }
+
+    let mut applicability = Applicability::MachineApplicable;
+    span_lint_and_sugg(
+        cx,
+        NEEDLESS_MATCH,
+        expr.span,
+        "this match expression is unnecessary",
+        "replace it with",
+        snippet_with_applicability(cx, ex.span, "..", &mut applicability).to_string(),
+        applicability,
+    );
 }
 
 /// Check for nop `if let` expression that assembled as unnecessary match
@@ -63,6 +69,7 @@ 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 !is_type_differ(cx, if_let.let_expr, ex);
         if check_if_let(cx, if_let);
         then {
             let mut applicability = Applicability::MachineApplicable;
@@ -119,6 +126,45 @@ fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> {
     }
 }
 
+/// Check if the type of the match operand differs with the assigned local or function return type
+///
+/// Can also be used to check for the `let_expr` in `IfLet` as well.
+fn is_type_differ(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),
+                        );
+                    }
+                }
+            },
+            // get the parent expr for this whole block `{ match match_expr {..} }`
+            Node::Block(block) => {
+                // This is where I lost track
+                if let Some(block_parent_expr) = get_parent_expr_for_hir(cx, block.hir_id) {
+                    return is_type_differ(cx, match_expr, block_parent_expr);
+                }
+            },
+            // recursively call on `if xxx {..}` etc.
+            Node::Expr(p_expr) => {
+                return is_type_differ(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) {

diff --git a/tests/ui/needless_match.fixed b/tests/ui/needless_match.fixed
@@ -60,8 +60,26 @@ fn result_match() {
     };
 }
 
-fn if_let_option() -> Option<i32> {
-    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 {}
+
+    fn optmap(s: Option<&Struct>) -> Option<&dyn Trait> {
+        match s {
+            Some(s) => Some(s),
+            None => None,
+        }
+    }
 
-        // Don't trigger
-        let _ = if let Some(a) = Some(1) {
-            Some(a)
-        } else {
-            do_something();
-            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
@@ -83,8 +83,26 @@ fn result_match() {
     };
 }
 
-fn if_let_option() -> Option<i32> {
-    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 @@ 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 {}
+
+    fn optmap(s: Option<&Struct>) -> Option<&dyn Trait> {
+        match s {
+            Some(s) => Some(s),
+            None => None,
+        }
+    }
 
-        // Don't trigger
-        let _ = if let Some(a) = Some(1) {
-            Some(a)
-        } else {
-            do_something();
-            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
@@ -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<i32, i32> = 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<i32, i32> = 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 {
    |  __________________________^