Don't lint manual_let_else in cases where the question mark operator would work

Also, lint question_mark for `let...else` clauses that can be simplified to use `?`.
This lint isn't perfect as it doesn't support the unstable try blocks.

5 files changed, 245 insertions, 2 deletions

diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs
index 59e421c1622..dcfc365a348 100644
--- a/clippy_lints/src/manual_let_else.rs
+++ b/clippy_lints/src/manual_let_else.rs
@@ -1,14 +1,15 @@
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::higher::IfLetOrMatch;
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::peel_blocks;
 use clippy_utils::source::snippet_with_context;
 use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::visitors::{Descend, Visitable};
+use clippy_utils::{is_refutable, is_res_lang_ctor, peel_blocks};
 use if_chain::if_chain;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{walk_expr, Visitor};
+use rustc_hir::LangItem::{OptionNone, OptionSome};
 use rustc_hir::{Expr, ExprKind, HirId, ItemId, Local, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind, Ty};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
@@ -85,6 +86,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
                     if let Some(ident_map) = expr_simple_identity_map(local.pat, let_pat, if_then);
                     if let Some(if_else) = if_else;
                     if expr_diverges(cx, if_else);
+                    if pat_and_expr_can_be_question_mark(cx, let_pat, if_else).is_none();
                     then {
                         emit_manual_let_else(cx, stmt.span, if_let_expr, &ident_map, let_pat, if_else);
                     }
@@ -167,6 +169,50 @@ fn emit_manual_let_else(
     );
 }
 
+/// Returns whether the given let pattern and else body can be turned into a question mark
+///
+/// For this example:
+/// ```ignore
+/// let FooBar { a, b } = if let Some(a) = ex { a } else { return None };
+/// ```
+/// We get as parameters:
+/// ```ignore
+/// pat: Some(a)
+/// else_body: return None
+/// ```
+
+/// And for this example:
+/// ```ignore
+/// let Some(FooBar { a, b }) = ex else { return None };
+/// ```
+/// We get as parameters:
+/// ```ignore
+/// pat: Some(FooBar { a, b })
+/// else_body: return None
+/// ```
+
+/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because
+/// the question mark operator is applicable here. Callers have to check whether we are in a
+/// constant or not.
+pub fn pat_and_expr_can_be_question_mark<'a, 'hir>(
+    cx: &LateContext<'_>,
+    pat: &'a Pat<'hir>,
+    else_body: &Expr<'_>,
+) -> Option<&'a Pat<'hir>> {
+    if let PatKind::TupleStruct(pat_path, [inner_pat], _) = pat.kind &&
+        is_res_lang_ctor(cx, cx.qpath_res(&pat_path, pat.hir_id), OptionSome) &&
+        !is_refutable(cx, inner_pat) &&
+        let else_body = peel_blocks(else_body) &&
+        let ExprKind::Ret(Some(ret_val)) = else_body.kind &&
+        let ExprKind::Path(ret_path) = ret_val.kind &&
+        is_res_lang_ctor(cx, cx.qpath_res(&ret_path, ret_val.hir_id), OptionNone)
+    {
+        Some(inner_pat)
+    } else {
+        None
+    }
+}
+
 /// Replaces the locals in the pattern
 ///
 /// For this example:
diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs
index e3d940ad2a4..b1eeedc26bd 100644
--- a/clippy_lints/src/question_mark.rs
+++ b/clippy_lints/src/question_mark.rs
@@ -1,3 +1,4 @@
+use crate::manual_let_else::pat_and_expr_can_be_question_mark;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::is_type_diagnostic_item;
@@ -10,7 +11,9 @@ use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
 use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk};
-use rustc_hir::{BindingAnnotation, ByRef, Expr, ExprKind, Node, PatKind, PathSegment, QPath};
+use rustc_hir::{
+    BindingAnnotation, Block, ByRef, Expr, ExprKind, Local, Node, PatKind, PathSegment, QPath, Stmt, StmtKind,
+};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::Ty;
 use rustc_session::declare_tool_lint;
@@ -78,6 +81,29 @@ enum IfBlockType<'hir> {
     ),
 }
 
+fn check_let_some_else_return_none(cx: &LateContext<'_>, stmt: &Stmt<'_>) {
+    if let StmtKind::Local(Local { pat, init: Some(init_expr), els: Some(els), .. }) = stmt.kind &&
+        let Block { stmts: &[], expr: Some(els), .. } = els &&
+        let Some(inner_pat) = pat_and_expr_can_be_question_mark(cx, pat, els)
+    {
+        let mut applicability = Applicability::MaybeIncorrect;
+        let init_expr_str = snippet_with_applicability(cx, init_expr.span, "..", &mut applicability);
+        let receiver_str = snippet_with_applicability(cx, inner_pat.span, "..", &mut applicability);
+        let sugg = format!(
+            "let {receiver_str} = {init_expr_str}?;",
+        );
+        span_lint_and_sugg(
+            cx,
+            QUESTION_MARK,
+            stmt.span,
+            "this `let...else` may be rewritten with the `?` operator",
+            "replace it with",
+            sugg,
+            applicability,
+        );
+    }
+}
+
 fn is_early_return(smbl: Symbol, cx: &LateContext<'_>, if_block: &IfBlockType<'_>) -> bool {
     match *if_block {
         IfBlockType::IfIs(caller, caller_ty, call_sym, if_then, _) => {
@@ -259,6 +285,11 @@ fn is_try_block(cx: &LateContext<'_>, bl: &rustc_hir::Block<'_>) -> bool {
 }
 
 impl<'tcx> LateLintPass<'tcx> for QuestionMark {
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) {
+        if !in_constant(cx, stmt.hir_id) {
+            check_let_some_else_return_none(cx, stmt);
+        }
+    }
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if !in_constant(cx, expr.hir_id) {
             self.check_is_none_or_err_and_early_return(cx, expr);
diff --git a/tests/ui/manual_let_else_question_mark.fixed b/tests/ui/manual_let_else_question_mark.fixed
new file mode 100644
index 00000000000..617f127b9a8
--- /dev/null
+++ b/tests/ui/manual_let_else_question_mark.fixed
@@ -0,0 +1,56 @@
+//@run-rustfix
+#![allow(unused_braces, unused_variables, dead_code)]
+#![allow(
+    clippy::collapsible_else_if,
+    clippy::unused_unit,
+    clippy::let_unit_value,
+    clippy::match_single_binding,
+    clippy::never_loop
+)]
+#![warn(clippy::manual_let_else, clippy::question_mark)]
+
+enum Variant {
+    A(usize, usize),
+    B(usize),
+    C,
+}
+
+fn g() -> Option<(u8, u8)> {
+    None
+}
+
+fn e() -> Variant {
+    Variant::A(0, 0)
+}
+
+fn main() {}
+
+fn foo() -> Option<()> {
+    // Fire here, normal case
+    let v = g()?;
+
+    // Don't fire here, the pattern is refutable
+    let Variant::A(v, w) = e() else { return None };
+
+    // Fire here, the pattern is irrefutable
+    let (v, w) = g()?;
+
+    // Don't fire manual_let_else in this instance: question mark can be used instead.
+    let v = g()?;
+
+    // Do fire manual_let_else in this instance: question mark cannot be used here due to the return
+    // body.
+    let Some(v) = g() else {
+        return Some(());
+    };
+
+    // Here we could also fire the question_mark lint, but we don't (as it's a match and not an if let).
+    // So we still emit manual_let_else here. For the *resulting* code, we *do* emit the question_mark
+    // lint, so for rustfix reasons, we allow the question_mark lint here.
+    #[allow(clippy::question_mark)]
+    {
+        let Some(v) = g() else { return None };
+    }
+
+    Some(())
+}
diff --git a/tests/ui/manual_let_else_question_mark.rs b/tests/ui/manual_let_else_question_mark.rs
new file mode 100644
index 00000000000..3cf1eca8673
--- /dev/null
+++ b/tests/ui/manual_let_else_question_mark.rs
@@ -0,0 +1,61 @@
+//@run-rustfix
+#![allow(unused_braces, unused_variables, dead_code)]
+#![allow(
+    clippy::collapsible_else_if,
+    clippy::unused_unit,
+    clippy::let_unit_value,
+    clippy::match_single_binding,
+    clippy::never_loop
+)]
+#![warn(clippy::manual_let_else, clippy::question_mark)]
+
+enum Variant {
+    A(usize, usize),
+    B(usize),
+    C,
+}
+
+fn g() -> Option<(u8, u8)> {
+    None
+}
+
+fn e() -> Variant {
+    Variant::A(0, 0)
+}
+
+fn main() {}
+
+fn foo() -> Option<()> {
+    // Fire here, normal case
+    let Some(v) = g() else { return None };
+
+    // Don't fire here, the pattern is refutable
+    let Variant::A(v, w) = e() else { return None };
+
+    // Fire here, the pattern is irrefutable
+    let Some((v, w)) = g() else { return None };
+
+    // Don't fire manual_let_else in this instance: question mark can be used instead.
+    let v = if let Some(v_some) = g() { v_some } else { return None };
+
+    // Do fire manual_let_else in this instance: question mark cannot be used here due to the return
+    // body.
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        return Some(());
+    };
+
+    // Here we could also fire the question_mark lint, but we don't (as it's a match and not an if let).
+    // So we still emit manual_let_else here. For the *resulting* code, we *do* emit the question_mark
+    // lint, so for rustfix reasons, we allow the question_mark lint here.
+    #[allow(clippy::question_mark)]
+    {
+        let v = match g() {
+            Some(v_some) => v_some,
+            _ => return None,
+        };
+    }
+
+    Some(())
+}
diff --git a/tests/ui/manual_let_else_question_mark.stderr b/tests/ui/manual_let_else_question_mark.stderr
new file mode 100644
index 00000000000..31052acf75c
--- /dev/null
+++ b/tests/ui/manual_let_else_question_mark.stderr
@@ -0,0 +1,49 @@
+error: this `let...else` may be rewritten with the `?` operator
+  --> $DIR/manual_let_else_question_mark.rs:30:5
+   |
+LL |     let Some(v) = g() else { return None };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `let v = g()?;`
+   |
+   = note: `-D clippy::question-mark` implied by `-D warnings`
+
+error: this `let...else` may be rewritten with the `?` operator
+  --> $DIR/manual_let_else_question_mark.rs:36:5
+   |
+LL |     let Some((v, w)) = g() else { return None };
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `let (v, w) = g()?;`
+
+error: this block may be rewritten with the `?` operator
+  --> $DIR/manual_let_else_question_mark.rs:39:13
+   |
+LL |     let v = if let Some(v_some) = g() { v_some } else { return None };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `g()?`
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else_question_mark.rs:43:5
+   |
+LL | /     let v = if let Some(v_some) = g() {
+LL | |         v_some
+LL | |     } else {
+LL | |         return Some(());
+LL | |     };
+   | |______^
+   |
+   = note: `-D clippy::manual-let-else` implied by `-D warnings`
+help: consider writing
+   |
+LL ~     let Some(v) = g() else {
+LL +         return Some(());
+LL +     };
+   |
+
+error: this could be rewritten as `let...else`
+  --> $DIR/manual_let_else_question_mark.rs:54:9
+   |
+LL | /         let v = match g() {
+LL | |             Some(v_some) => v_some,
+LL | |             _ => return None,
+LL | |         };
+   | |__________^ help: consider writing: `let Some(v) = g() else { return None };`
+
+error: aborting due to 5 previous errors
+