Auto merge of #11994 - y21:issue11993-fn, r=xFrednet

[`question_mark`]: also trigger on `return` statements

This fixes the false negative mentioned in #11993: the lint only used to check for `return` expressions, and not a statement containing a `return` expression (doesn't close the issue tho since there's still a useful suggestion that we could make, which is to suggest `.ok_or()?`/`.ok_or_else()?` for `else { return Err(..) }`)

changelog: [`question_mark`]: also trigger on `return` statements

Author: bors

clippy_lints/src/matches/manual_utils.rs

@@ -63,9 +63,7 @@ where
         return None;
     }
 
-    let Some(some_expr) = get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt) else {
-        return None;
-    };
+    let some_expr = get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt)?;
 
     // These two lints will go back and forth with each other.
     if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit

clippy_lints/src/matches/redundant_pattern_match.rs

@@ -128,9 +128,7 @@ fn find_method_and_type<'tcx>(
 
             if is_wildcard || is_rest {
                 let res = cx.typeck_results().qpath_res(qpath, check_pat.hir_id);
-                let Some(id) = res.opt_def_id().map(|ctor_id| cx.tcx.parent(ctor_id)) else {
-                    return None;
-                };
+                let id = res.opt_def_id().map(|ctor_id| cx.tcx.parent(ctor_id))?;
                 let lang_items = cx.tcx.lang_items();
                 if Some(id) == lang_items.result_ok_variant() {
                     Some(("is_ok()", try_get_generic_ty(op_ty, 0).unwrap_or(op_ty)))

clippy_lints/src/question_mark.rs

@@ -8,7 +8,7 @@ use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::{
     eq_expr_value, get_parent_node, higher, in_constant, is_else_clause, 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,
+    peel_blocks_with_stmt, span_contains_comment,
 };
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
@@ -96,19 +96,34 @@ enum IfBlockType<'hir> {
     ),
 }
 
+fn find_let_else_ret_expression<'hir>(block: &'hir Block<'hir>) -> Option<&'hir Expr<'hir>> {
+    if let Block {
+        stmts: &[],
+        expr: Some(els),
+        ..
+    } = block
+    {
+        Some(els)
+    } else if let [stmt] = block.stmts
+        && let StmtKind::Semi(expr) = stmt.kind
+        && let ExprKind::Ret(..) = expr.kind
+    {
+        Some(expr)
+    } else {
+        None
+    }
+}
+
 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 Some(ret) = find_let_else_ret_expression(els)
+        && let Some(inner_pat) = pat_and_expr_can_be_question_mark(cx, pat, ret)
+        && !span_contains_comment(cx.tcx.sess.source_map(), els.span)
     {
         let mut applicability = Applicability::MaybeIncorrect;
         let init_expr_str = snippet_with_applicability(cx, init_expr.span, "..", &mut applicability);

clippy_lints/src/undocumented_unsafe_blocks.rs

@@ -680,9 +680,7 @@ fn text_has_safety_comment(src: &str, line_starts: &[RelativeBytePos], start_pos
         })
         .filter(|(_, text)| !text.is_empty());
 
-    let Some((line_start, line)) = lines.next() else {
-        return None;
-    };
+    let (line_start, line) = lines.next()?;
     // Check for a sequence of line comments.
     if line.starts_with("//") {
         let (mut line, mut line_start) = (line, line_start);

tests/ui/manual_let_else_question_mark.fixed

@@ -60,3 +60,24 @@ fn foo() -> Option<()> {
 
     Some(())
 }
+
+// lint not just `return None`, but also `return None;` (note the semicolon)
+fn issue11993(y: Option<i32>) -> Option<i32> {
+    let x = y?;
+
+    // don't lint: more than one statement in the else body
+    let Some(x) = y else {
+        todo!();
+        return None;
+    };
+
+    let Some(x) = y else {
+        // Roses are red,
+        // violets are blue,
+        // please keep this comment,
+        // it's art, you know?
+        return None;
+    };
+
+    None
+}

tests/ui/manual_let_else_question_mark.rs

@@ -65,3 +65,26 @@ fn foo() -> Option<()> {
 
     Some(())
 }
+
+// lint not just `return None`, but also `return None;` (note the semicolon)
+fn issue11993(y: Option<i32>) -> Option<i32> {
+    let Some(x) = y else {
+        return None;
+    };
+
+    // don't lint: more than one statement in the else body
+    let Some(x) = y else {
+        todo!();
+        return None;
+    };
+
+    let Some(x) = y else {
+        // Roses are red,
+        // violets are blue,
+        // please keep this comment,
+        // it's art, you know?
+        return None;
+    };
+
+    None
+}

tests/ui/manual_let_else_question_mark.stderr

@@ -53,5 +53,13 @@ error: this could be rewritten as `let...else`
 LL |         let v = if let Some(v_some) = g() { v_some } else { return None };
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return None };`
 
-error: aborting due to 6 previous errors
+error: this `let...else` may be rewritten with the `?` operator
+  --> $DIR/manual_let_else_question_mark.rs:71:5
+   |
+LL | /     let Some(x) = y else {
+LL | |         return None;
+LL | |     };
+   | |______^ help: replace it with: `let x = y?;`
+
+error: aborting due to 7 previous errors