Add match-based manual try to clippy::question_mark

GnomedDev · GnomedDev · commit 8ca1274a1c49 · 2024-10-29T23:29:46.000Z
diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs
@@ -8,18 +8,18 @@ use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::{implements_trait, is_type_diagnostic_item};
 use clippy_utils::{
     eq_expr_value, higher, is_else_clause, is_in_const_context, is_lint_allowed, is_path_lang_item, is_res_lang_ctor,
-    pat_and_expr_can_be_question_mark, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt,
+    pat_and_expr_can_be_question_mark, path_res, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt,
     span_contains_comment,
 };
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk};
 use rustc_hir::def::Res;
 use rustc_hir::{
-    BindingMode, Block, Body, ByRef, Expr, ExprKind, LetStmt, Mutability, Node, PatKind, PathSegment, QPath, Stmt,
-    StmtKind,
+    Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, HirId, LetStmt, MatchSource, Mutability, Node, Pat, PatKind,
+    PathSegment, QPath, Stmt, StmtKind,
 };
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::Ty;
+use rustc_middle::ty::{self, Ty};
 use rustc_session::impl_lint_pass;
 use rustc_span::sym;
 use rustc_span::symbol::Symbol;
@@ -271,6 +271,140 @@ fn check_is_none_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Ex
     }
 }
 
+#[derive(Clone, Copy, Debug)]
+enum TryMode {
+    Result,
+    Option,
+}
+
+fn find_try_mode<'tcx>(cx: &LateContext<'tcx>, scrutinee: &Expr<'tcx>) -> Option<TryMode> {
+    let scrutinee_ty = cx.typeck_results().expr_ty_adjusted(scrutinee);
+    let ty::Adt(scrutinee_adt_def, _) = scrutinee_ty.kind() else {
+        return None;
+    };
+
+    match cx.tcx.get_diagnostic_name(scrutinee_adt_def.did())? {
+        sym::Result => Some(TryMode::Result),
+        sym::Option => Some(TryMode::Option),
+        _ => None,
+    }
+}
+
+// Check that `pat` is `{ctor_lang_item}(val)`, returning `val`.
+fn extract_ctor_call<'a, 'tcx>(
+    cx: &LateContext<'tcx>,
+    expected_ctor: LangItem,
+    pat: &'a Pat<'tcx>,
+) -> Option<&'a Pat<'tcx>> {
+    if let PatKind::TupleStruct(variant_path, [val_binding], _) = &pat.kind
+        && is_res_lang_ctor(cx, cx.qpath_res(variant_path, pat.hir_id), expected_ctor)
+    {
+        Some(val_binding)
+    } else {
+        None
+    }
+}
+
+// Extracts the local ID of a plain `val` pattern.
+fn extract_binding_pat(pat: &Pat<'_>) -> Option<HirId> {
+    if let PatKind::Binding(BindingMode::NONE, binding, _, None) = pat.kind {
+        Some(binding)
+    } else {
+        None
+    }
+}
+
+// Check `arm` is `Ok(val) => val` or `Some(val) => val`.
+fn check_arm_is_happy<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm: &Arm<'tcx>) -> bool {
+    let arm_body = arm.body.peel_blocks().peel_drop_temps();
+    let happy_ctor = match mode {
+        TryMode::Result => ResultOk,
+        TryMode::Option => OptionSome,
+    };
+
+    // Check for `Ok(val)` or `Some(val)`
+    if arm.guard.is_none()
+        && let Some(val_binding) = extract_ctor_call(cx, happy_ctor, arm.pat)
+        // Extract out `val`
+        && let Some(binding) = extract_binding_pat(val_binding)
+        // Check body is just `=> val`
+        && let ExprKind::Path(body_path) = &arm_body.kind
+        && let Res::Local(body_local_ref) = cx.qpath_res(body_path, arm_body.hir_id)
+        && binding == body_local_ref
+    {
+        true
+    } else {
+        false
+    }
+}
+
+// Check `arm` is `Err(err) => return Err(err)` or `None => return None`.
+fn check_arm_is_sad<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm: &Arm<'tcx>) -> bool {
+    if arm.guard.is_some() {
+        return false;
+    }
+
+    let arm_body = arm.body.peel_blocks().peel_drop_temps();
+    match mode {
+        TryMode::Result => {
+            // Check that pat is Err(val)
+            if let Some(ok_pat) = extract_ctor_call(cx, ResultErr, arm.pat)
+                && let Some(ok_val) = extract_binding_pat(ok_pat)
+                // check `=> return Err(...)`
+                && let ExprKind::Ret(Some(wrapped_ret_expr)) = arm_body.kind
+                && let ExprKind::Call(ok_ctor, [ret_expr]) = wrapped_ret_expr.kind
+                && is_res_lang_ctor(cx, path_res(cx, ok_ctor), ResultErr)
+                // check `...` is `val` from binding
+                && let Res::Local(ret_local_ref) = path_res(cx, ret_expr)
+                && ok_val == ret_local_ref
+            {
+                true
+            } else {
+                false
+            }
+        },
+        TryMode::Option => {
+            // Check the pat is `None`
+            if is_res_lang_ctor(cx, path_res(cx, arm.pat), OptionNone)
+                // Check `=> return None`
+                && let ExprKind::Ret(Some(ret_expr)) = arm_body.kind
+                && is_res_lang_ctor(cx, path_res(cx, ret_expr), OptionNone)
+            {
+                true
+            } else {
+                false
+            }
+        },
+    }
+}
+
+fn check_arms_are_try<'tcx>(cx: &LateContext<'tcx>, mode: TryMode, arm1: &Arm<'tcx>, arm2: &Arm<'tcx>) -> bool {
+    (check_arm_is_happy(cx, mode, arm1) && check_arm_is_sad(cx, mode, arm2))
+        || (check_arm_is_happy(cx, mode, arm2) && check_arm_is_sad(cx, mode, arm1))
+}
+
+fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
+    if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal | MatchSource::Postfix) = expr.kind
+        && !expr.span.from_expansion()
+        && let Some(mode) = find_try_mode(cx, scrutinee)
+        && check_arms_are_try(cx, mode, arm1, arm2)
+    {
+        let mut applicability = Applicability::MachineApplicable;
+        let mut snippet = snippet_with_applicability(cx, scrutinee.span, "...", &mut applicability).into_owned();
+        snippet.push('?');
+
+        span_lint_and_sugg(
+            cx,
+            QUESTION_MARK,
+            expr.span,
+            "this `match` expression can be replaced with `?`",
+            "try instead",
+            snippet,
+            applicability,
+        );
+    }
+}
+
 fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
     if let Some(higher::IfLet {
         let_pat,
@@ -350,11 +484,15 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark {
         }
         self.check_manual_let_else(cx, stmt);
     }
+
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if !self.inside_try_block() && !is_in_const_context(cx) && is_lint_allowed(cx, QUESTION_MARK_USED, expr.hir_id)
-        {
-            check_is_none_or_err_and_early_return(cx, expr);
-            check_if_let_some_or_err_and_early_return(cx, expr);
+        if !is_in_const_context(cx) && is_lint_allowed(cx, QUESTION_MARK_USED, expr.hir_id) {
+            if !self.inside_try_block() {
+                check_is_none_or_err_and_early_return(cx, expr);
+                check_if_let_some_or_err_and_early_return(cx, expr);
+            }
+
+            check_if_try_match(cx, expr);
         }
     }
 
diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed
@@ -102,6 +102,10 @@ fn func() -> Option<i32> {
 
     f()?;
 
+    let _val = f()?;
+
+    f()?;
+
     Some(0)
 }
 
@@ -114,6 +118,10 @@ fn result_func(x: Result<i32, i32>) -> Result<i32, i32> {
 
     x?;
 
+    let _val = func_returning_result()?;
+
+    func_returning_result()?;
+
     // No warning
     let y = if let Ok(x) = x {
         x
diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
@@ -132,6 +132,16 @@ fn func() -> Option<i32> {
         return None;
     }
 
+    let _val = match f() {
+        Some(val) => val,
+        None => return None,
+    };
+
+    match f() {
+        Some(val) => val,
+        None => return None,
+    };
+
     Some(0)
 }
 
@@ -146,6 +156,16 @@ fn result_func(x: Result<i32, i32>) -> Result<i32, i32> {
         return x;
     }
 
+    let _val = match func_returning_result() {
+        Ok(val) => val,
+        Err(err) => return Err(err),
+    };
+
+    match func_returning_result() {
+        Ok(val) => val,
+        Err(err) => return Err(err),
+    };
+
     // No warning
     let y = if let Ok(x) = x {
         x
diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr
@@ -101,38 +101,76 @@ LL | |         return None;
 LL | |     }
    | |_____^ help: replace it with: `f()?;`
 
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:135:16
+   |
+LL |       let _val = match f() {
+   |  ________________^
+LL | |         Some(val) => val,
+LL | |         None => return None,
+LL | |     };
+   | |_____^ help: try instead: `f()?`
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:140:5
+   |
+LL | /     match f() {
+LL | |         Some(val) => val,
+LL | |         None => return None,
+LL | |     };
+   | |_____^ help: try instead: `f()?`
+
 error: this block may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:143:13
+  --> tests/ui/question_mark.rs:153:13
    |
 LL |     let _ = if let Ok(x) = x { x } else { return x };
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?`
 
 error: this block may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:145:5
+  --> tests/ui/question_mark.rs:155:5
    |
 LL | /     if x.is_err() {
 LL | |         return x;
 LL | |     }
    | |_____^ help: replace it with: `x?;`
 
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:159:16
+   |
+LL |       let _val = match func_returning_result() {
+   |  ________________^
+LL | |         Ok(val) => val,
+LL | |         Err(err) => return Err(err),
+LL | |     };
+   | |_____^ help: try instead: `func_returning_result()?`
+
+error: this `match` expression can be replaced with `?`
+  --> tests/ui/question_mark.rs:164:5
+   |
+LL | /     match func_returning_result() {
+LL | |         Ok(val) => val,
+LL | |         Err(err) => return Err(err),
+LL | |     };
+   | |_____^ help: try instead: `func_returning_result()?`
+
 error: this block may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:213:5
+  --> tests/ui/question_mark.rs:233:5
    |
 LL | /     if let Err(err) = func_returning_result() {
 LL | |         return Err(err);
 LL | |     }
    | |_____^ help: replace it with: `func_returning_result()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:220:5
+  --> tests/ui/question_mark.rs:240:5
    |
 LL | /     if let Err(err) = func_returning_result() {
 LL | |         return Err(err);
 LL | |     }
    | |_____^ help: replace it with: `func_returning_result()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:297:13
+  --> tests/ui/question_mark.rs:317:13
    |
 LL | /             if a.is_none() {
 LL | |                 return None;
@@ -142,12 +180,12 @@ LL | |             }
    | |_____________^ help: replace it with: `a?;`
 
 error: this `let...else` may be rewritten with the `?` operator
-  --> tests/ui/question_mark.rs:357:5
+  --> tests/ui/question_mark.rs:377:5
    |
 LL | /     let Some(v) = bar.foo.owned.clone() else {
 LL | |         return None;
 LL | |     };
    | |______^ help: replace it with: `let v = bar.foo.owned.clone()?;`
 
-error: aborting due to 17 previous errors
+error: aborting due to 21 previous errors
 
