feat(question_mark): lint if let Some(_) = _ { ..; _ } else { None } as final expr

clippy_lints/src/question_mark.rs

@@ -2,10 +2,10 @@ use crate::manual_let_else::MANUAL_LET_ELSE;
 use crate::question_mark_used::QUESTION_MARK_USED;
 use clippy_config::Conf;
 use clippy_config::types::MatchLintBehaviour;
-use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
-use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::{implements_trait, is_copy};
 use clippy_utils::usage::local_used_after_expr;
@@ -17,13 +17,15 @@ use clippy_utils::{
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk};
 use rustc_hir::def::Res;
+use rustc_hir::def_id::LocalDefId;
 use rustc_hir::{
-    Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnRetTy, HirId, LetStmt, MatchSource, Mutability, Node, Pat,
-    PatKind, PathSegment, QPath, Stmt, StmtKind,
+    Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnDecl, FnRetTy, HirId, LetStmt, MatchSource, Mutability,
+    Node, Pat, PatKind, PathSegment, QPath, Stmt, StmtKind, intravisit,
 };
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::ty::{self, Ty};
 use rustc_session::impl_lint_pass;
+use rustc_span::Span;
 use rustc_span::symbol::Symbol;
 
 declare_clippy_lint! {
@@ -524,6 +526,63 @@ fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr:
     }
 }
 
+fn check_if_let_some_as_return_val<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
+    if !expr.span.in_external_macro(cx.sess().source_map())
+        && let Some(higher::IfLet {
+            let_pat,
+            let_expr,
+            if_then,
+            if_else,
+            ..
+        }) = higher::IfLet::hir(cx, expr)
+        && let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind
+        && ddpos.as_opt_usize().is_none()
+        && let PatKind::Binding(BindingMode(by_ref, _), _, ident, None) = field.kind
+        && let caller_ty = cx.typeck_results().expr_ty(let_expr)
+        && let if_block = IfBlockType::IfLet(
+            cx.qpath_res(path1, let_pat.hir_id),
+            caller_ty,
+            ident.name,
+            let_expr,
+            if_then,
+            if_else,
+        )
+        && let ExprKind::Block(if_then_block, _) = if_then.kind
+        // Don't consider case where if-then branch has only one statement/expression
+        && (if_then_block.stmts.len() >= 2 || if_then_block.stmts.len() == 1 && if_then_block.expr.is_some())
+        && is_early_return(sym::Option, cx, &if_block)
+    {
+        let mut applicability = Applicability::MachineApplicable;
+        let receiver_str = snippet_with_context(cx, let_expr.span, expr.span.ctxt(), "..", &mut applicability).0;
+        let method_call_str = match by_ref {
+            ByRef::Yes(Mutability::Mut) => ".as_mut()",
+            ByRef::Yes(Mutability::Not) => ".as_ref()",
+            ByRef::No => "",
+        };
+        let indent = snippet_indent(cx, if_then.span).unwrap_or_default();
+        let body_str = {
+            let snippet = snippet_with_applicability(cx, if_then.span, "..", &mut applicability);
+            let Some(snippet) = snippet.strip_prefix('{').and_then(|s| s.strip_suffix('}')) else {
+                return;
+            };
+            // There probably was a newline before the `}`, and it adds an extra line to the suggestion
+            // -- remove it
+            let snippet = snippet.trim_end();
+            reindent_multiline(snippet, true, Some(indent.len()))
+        };
+        let sugg = format!("let {ident} = {receiver_str}{method_call_str}?;{body_str}");
+        span_lint_and_then(
+            cx,
+            QUESTION_MARK,
+            expr.span,
+            "this block may be rewritten with the `?` operator",
+            |diag| {
+                diag.span_suggestion_verbose(expr.span, "replace it with", sugg, applicability);
+            },
+        );
+    }
+}
+
 impl QuestionMark {
     fn inside_try_block(&self) -> bool {
         self.try_block_depth_stack.last() > Some(&0)
@@ -603,6 +662,42 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark {
         self.try_block_depth_stack.push(0);
     }
 
+    // Defining `check_fn` instead of using `check_body` to avoid accidentally triggering on
+    // const expressions
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: intravisit::FnKind<'tcx>,
+        _: &'tcx FnDecl<'tcx>,
+        body: &'tcx Body<'tcx>,
+        _: Span,
+        local_def_id: LocalDefId,
+    ) {
+        if !is_in_const_context(cx)
+            && is_lint_allowed(cx, QUESTION_MARK_USED, HirId::make_owner(local_def_id))
+            && self.msrv.meets(cx, msrvs::QUESTION_MARK_OPERATOR)
+            && let Some(return_expr) = match body.value.kind {
+                #[expect(
+                    clippy::unnecessary_lazy_evaluations,
+                    reason = "there are a bunch of if-let's, why should we evaluate them eagerly? Arguably an FP"
+                )]
+                ExprKind::Block(block, _) => block.expr.or_else(|| {
+                    if let [.., stmt] = block.stmts
+                        && let StmtKind::Semi(expr) = stmt.kind
+                        && let ExprKind::Ret(Some(ret)) = expr.kind
+                    {
+                        Some(ret)
+                    } else {
+                        None
+                    }
+                }),
+                _ => None,
+            }
+        {
+            check_if_let_some_as_return_val(cx, return_expr);
+        }
+    }
+
     fn check_body_post(&mut self, _: &LateContext<'tcx>, _: &Body<'tcx>) {
         self.try_block_depth_stack.pop();
     }

tests/ui/question_mark.fixed

@@ -1,5 +1,5 @@
 #![feature(try_blocks)]
-#![allow(clippy::unnecessary_wraps)]
+#![allow(clippy::unnecessary_wraps, clippy::let_unit_value)]
 
 use std::sync::MutexGuard;
 
@@ -500,3 +500,91 @@ mod issue14894 {
         Ok(())
     }
 }
+
+mod issue13626 {
+    fn basic(x: Option<u32>) -> Option<u32> {
+        let x = x?;
+        //~^ question_mark
+        dbg!(x);
+        Some(x * 2)
+    }
+
+    fn mut_ref(mut x: Option<u32>) -> Option<u32> {
+        let x = x.as_mut()?;
+        //~^ question_mark
+        dbg!(*x);
+        Some(*x * 2)
+    }
+
+    #[allow(clippy::needless_return)]
+    fn explicit_return(x: Option<u32>) -> Option<u32> {
+        let x = x?;
+        //~^ question_mark
+        dbg!(x);
+        return Some(x * 2);
+    }
+
+    #[deny(clippy::question_mark_used)]
+    fn question_mark_forbidden(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            dbg!(x);
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    #[clippy::msrv = "1.12"]
+    fn msrv_1_12(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            dbg!(x);
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    #[clippy::msrv = "1.13"]
+    fn msrv_1_13(x: Option<u32>) -> Option<u32> {
+        let x = x?;
+        //~^ question_mark
+        dbg!(x);
+        Some(x * 2)
+    }
+
+    const fn no_lint_in_const(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            let x = x + 2;
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    fn with_macro() -> Option<()> {
+        macro_rules! m {
+            () => {
+                Some(())
+            };
+        };
+
+        let x = m!()?;
+        //~^ question_mark
+        dbg!(x);
+        Some(x)
+    }
+
+    #[rustfmt::skip]
+    mod bad_formatting{
+        fn t1(x: Option<u32>) -> Option<u32> {
+            let x = x?; dbg!(x); Some(x * 2)
+            //~^ question_mark
+        }
+        fn t2(x: Option<u32>) -> Option<u32> {
+            let x = x?;
+            //~^^ question_mark
+            dbg!(x);
+            Some(x * 2)
+        }
+    }
+}

tests/ui/question_mark.rs

@@ -1,5 +1,5 @@
 #![feature(try_blocks)]
-#![allow(clippy::unnecessary_wraps)]
+#![allow(clippy::unnecessary_wraps, clippy::let_unit_value)]
 
 use std::sync::MutexGuard;
 
@@ -615,3 +615,112 @@ mod issue14894 {
         Ok(())
     }
 }
+
+mod issue13626 {
+    fn basic(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            //~^ question_mark
+            dbg!(x);
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    fn mut_ref(mut x: Option<u32>) -> Option<u32> {
+        if let Some(ref mut x) = x {
+            //~^ question_mark
+            dbg!(*x);
+            Some(*x * 2)
+        } else {
+            None
+        }
+    }
+
+    #[allow(clippy::needless_return)]
+    fn explicit_return(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            //~^ question_mark
+            dbg!(x);
+            return Some(x * 2);
+        } else {
+            return None;
+        }
+    }
+
+    #[deny(clippy::question_mark_used)]
+    fn question_mark_forbidden(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            dbg!(x);
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    #[clippy::msrv = "1.12"]
+    fn msrv_1_12(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            dbg!(x);
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    #[clippy::msrv = "1.13"]
+    fn msrv_1_13(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            //~^ question_mark
+            dbg!(x);
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    const fn no_lint_in_const(x: Option<u32>) -> Option<u32> {
+        if let Some(x) = x {
+            let x = x + 2;
+            Some(x * 2)
+        } else {
+            None
+        }
+    }
+
+    fn with_macro() -> Option<()> {
+        macro_rules! m {
+            () => {
+                Some(())
+            };
+        };
+
+        if let Some(x) = m!() {
+            //~^ question_mark
+            dbg!(x);
+            Some(x)
+        } else {
+            None
+        }
+    }
+
+    #[rustfmt::skip]
+    mod bad_formatting{
+        fn t1(x: Option<u32>) -> Option<u32> {
+            if let Some(x) = x { dbg!(x); Some(x * 2) } else { None }
+            //~^ question_mark
+        }
+        fn t2(x: Option<u32>) -> Option<u32> {
+            if let Some(x) = x
+            {
+                //~^^ question_mark
+                dbg!(x);
+                Some(x * 2)
+            }
+            else
+            {
+                None
+            }
+        }
+    }
+}