needless_match: do not pretend that return is not significant in an expression (#14757)

A `return` in an expression makes it divergent and cannot be removed
blindly. While this stripping might have been introduced as a way to
catch more cases, it was improperly used, and no tests exhibit a failure
when this special handling is removed.

changelog: [`needless_match`]: do not strip `return` as it might make
the `if let` or `match` divergent in some cases

Fixes #14754

Author: blyxyas

clippy_lints/src/matches/needless_match.rs

@@ -74,7 +74,7 @@ fn check_all_arms(cx: &LateContext<'_>, match_expr: &Expr<'_>, arms: &[Arm<'_>])
         }
 
         if let PatKind::Wild = arm.pat.kind {
-            if !eq_expr_value(cx, match_expr, strip_return(arm_expr)) {
+            if !eq_expr_value(cx, match_expr, arm_expr) {
                 return false;
             }
         } else if !pat_same_as_expr(arm.pat, arm_expr) {
@@ -103,27 +103,18 @@ fn check_if_let_inner(cx: &LateContext<'_>, if_let: &higher::IfLet<'_>) -> bool
             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) {
-                return is_res_lang_ctor(cx, path_res(cx, ret), OptionNone) || eq_expr_value(cx, if_let.let_expr, ret);
+                return is_res_lang_ctor(cx, path_res(cx, else_expr), OptionNone)
+                    || eq_expr_value(cx, if_let.let_expr, else_expr);
             }
-            return eq_expr_value(cx, if_let.let_expr, ret);
+            return eq_expr_value(cx, if_let.let_expr, else_expr);
         }
     }
 
     false
 }
 
-/// Strip `return` keyword if the expression type is `ExprKind::Ret`.
-fn strip_return<'hir>(expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> {
-    if let ExprKind::Ret(Some(ret)) = expr.kind {
-        ret
-    } else {
-        expr
-    }
-}
-
 /// 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 function return type.
 fn expr_ty_matches_p_ty(cx: &LateContext<'_>, expr: &Expr<'_>, p_expr: &Expr<'_>) -> bool {
@@ -161,7 +152,6 @@ fn expr_ty_matches_p_ty(cx: &LateContext<'_>, expr: &Expr<'_>, p_expr: &Expr<'_>
 }
 
 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)) => {

tests/ui/needless_match.fixed

@@ -301,4 +301,16 @@ pub fn issue13574() -> Option<()> {
     None
 }
 
+fn issue14754(t: Result<i32, &'static str>) -> Result<i32, &'static str> {
+    let _ = match t {
+        Ok(v) => Ok::<_, &'static str>(v),
+        err @ Err(_) => return err,
+    };
+    println!("Still here");
+    let x = t;
+    //~^^^^ needless_match
+    println!("Still here");
+    x
+}
+
 fn main() {}

tests/ui/needless_match.rs

@@ -364,4 +364,19 @@ pub fn issue13574() -> Option<()> {
     None
 }
 
+fn issue14754(t: Result<i32, &'static str>) -> Result<i32, &'static str> {
+    let _ = match t {
+        Ok(v) => Ok::<_, &'static str>(v),
+        err @ Err(_) => return err,
+    };
+    println!("Still here");
+    let x = match t {
+        Ok(v) => Ok::<_, &'static str>(v),
+        err @ Err(_) => err,
+    };
+    //~^^^^ needless_match
+    println!("Still here");
+    x
+}
+
 fn main() {}

tests/ui/needless_match.stderr

@@ -151,5 +151,15 @@ LL | |             None
 LL | |         }
    | |_________^ help: replace it with: `A`
 
-error: aborting due to 14 previous errors
+error: this match expression is unnecessary
+  --> tests/ui/needless_match.rs:373:13
+   |
+LL |       let x = match t {
+   |  _____________^
+LL | |         Ok(v) => Ok::<_, &'static str>(v),
+LL | |         err @ Err(_) => err,
+LL | |     };
+   | |_____^ help: replace it with: `t`
+
+error: aborting due to 15 previous errors