Auto merge of #12341 - y21:issue12337, r=dswij

Check for try blocks in `question_mark` more consistently

Fixes #12337

I split this PR up into two commits since this moves a method out of an `impl`, which makes for a pretty bad diff (the `&self` parameter is now unused, and there isn't a reason for that function to be part of the `impl` now).

The first commit is the actual relevant change and the 2nd commit just moves stuff (github's "hide whitespace" makes the diff easier to look at)

------------
Now for the actual issue:

`?` within `try {}` blocks desugars to a `break` to the block, rather than a `return`, so that changes behavior in those cases.

The lint has multiple patterns to look for and in *some* of them it already does correctly check whether we're in a try block, but this isn't done for all of its patterns.

We could add another `self.inside_try_block()` check to the function that looks for `let-else-return`, but I chose to actually just move those checks out and instead have them in `LintPass::check_{stmt,expr}`. This has the advantage that we can't (easily) accidentally forget to add that check in new patterns that might be added in the future.

(There's also a bit of a subtle interaction between two lints, where `question_mark`'s LintPass calls into `manual_let_else`, so I added a check to make sure we don't avoid linting for something that doesn't have anything to do with `?`)

changelog: [`question_mark`]: avoid linting on try blocks in more cases

Author: bors

clippy_lints/src/question_mark.rs

@@ -203,108 +203,106 @@ fn expr_return_none_or_err(
     }
 }
 
-impl QuestionMark {
-    fn inside_try_block(&self) -> bool {
-        self.try_block_depth_stack.last() > Some(&0)
-    }
-
-    /// Checks if the given expression on the given context matches the following structure:
-    ///
-    /// ```ignore
-    /// if option.is_none() {
-    ///    return None;
-    /// }
-    /// ```
-    ///
-    /// ```ignore
-    /// if result.is_err() {
-    ///     return result;
-    /// }
-    /// ```
-    ///
-    /// If it matches, it will suggest to use the question mark operator instead
-    fn check_is_none_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
-        if !self.inside_try_block()
-            && let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr)
-            && !is_else_clause(cx.tcx, expr)
-            && let ExprKind::MethodCall(segment, caller, ..) = &cond.kind
-            && let caller_ty = cx.typeck_results().expr_ty(caller)
-            && let if_block = IfBlockType::IfIs(caller, caller_ty, segment.ident.name, then)
-            && (is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block))
-        {
-            let mut applicability = Applicability::MachineApplicable;
-            let receiver_str = snippet_with_applicability(cx, caller.span, "..", &mut applicability);
-            let by_ref = !caller_ty.is_copy_modulo_regions(cx.tcx, cx.param_env)
-                && !matches!(caller.kind, ExprKind::Call(..) | ExprKind::MethodCall(..));
-            let sugg = if let Some(else_inner) = r#else {
-                if eq_expr_value(cx, caller, peel_blocks(else_inner)) {
-                    format!("Some({receiver_str}?)")
-                } else {
-                    return;
-                }
+/// Checks if the given expression on the given context matches the following structure:
+///
+/// ```ignore
+/// if option.is_none() {
+///    return None;
+/// }
+/// ```
+///
+/// ```ignore
+/// if result.is_err() {
+///     return result;
+/// }
+/// ```
+///
+/// If it matches, it will suggest to use the question mark operator instead
+fn check_is_none_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
+    if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr)
+        && !is_else_clause(cx.tcx, expr)
+        && let ExprKind::MethodCall(segment, caller, ..) = &cond.kind
+        && let caller_ty = cx.typeck_results().expr_ty(caller)
+        && let if_block = IfBlockType::IfIs(caller, caller_ty, segment.ident.name, then)
+        && (is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block))
+    {
+        let mut applicability = Applicability::MachineApplicable;
+        let receiver_str = snippet_with_applicability(cx, caller.span, "..", &mut applicability);
+        let by_ref = !caller_ty.is_copy_modulo_regions(cx.tcx, cx.param_env)
+            && !matches!(caller.kind, ExprKind::Call(..) | ExprKind::MethodCall(..));
+        let sugg = if let Some(else_inner) = r#else {
+            if eq_expr_value(cx, caller, peel_blocks(else_inner)) {
+                format!("Some({receiver_str}?)")
             } else {
-                format!("{receiver_str}{}?;", if by_ref { ".as_ref()" } else { "" })
-            };
+                return;
+            }
+        } else {
+            format!("{receiver_str}{}?;", if by_ref { ".as_ref()" } else { "" })
+        };
 
-            span_lint_and_sugg(
-                cx,
-                QUESTION_MARK,
-                expr.span,
-                "this block may be rewritten with the `?` operator",
-                "replace it with",
-                sugg,
-                applicability,
-            );
-        }
+        span_lint_and_sugg(
+            cx,
+            QUESTION_MARK,
+            expr.span,
+            "this block may be rewritten with the `?` operator",
+            "replace it with",
+            sugg,
+            applicability,
+        );
     }
+}
 
-    fn check_if_let_some_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
-        if !self.inside_try_block()
-            && let Some(higher::IfLet {
-                let_pat,
-                let_expr,
-                if_then,
-                if_else,
-                ..
-            }) = higher::IfLet::hir(cx, expr)
-            && !is_else_clause(cx.tcx, expr)
-            && let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind
-            && ddpos.as_opt_usize().is_none()
-            && let PatKind::Binding(BindingAnnotation(by_ref, _), bind_id, 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,
-            )
-            && ((is_early_return(sym::Option, cx, &if_block) && path_to_local_id(peel_blocks(if_then), bind_id))
-                || is_early_return(sym::Result, cx, &if_block))
-            && if_else
-                .map(|e| eq_expr_value(cx, let_expr, peel_blocks(e)))
-                .filter(|e| *e)
-                .is_none()
-        {
-            let mut applicability = Applicability::MachineApplicable;
-            let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
-            let requires_semi = matches!(cx.tcx.parent_hir_node(expr.hir_id), Node::Stmt(_));
-            let sugg = format!(
-                "{receiver_str}{}?{}",
-                if by_ref == ByRef::Yes { ".as_ref()" } else { "" },
-                if requires_semi { ";" } else { "" }
-            );
-            span_lint_and_sugg(
-                cx,
-                QUESTION_MARK,
-                expr.span,
-                "this block may be rewritten with the `?` operator",
-                "replace it with",
-                sugg,
-                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,
+        let_expr,
+        if_then,
+        if_else,
+        ..
+    }) = higher::IfLet::hir(cx, expr)
+        && !is_else_clause(cx.tcx, expr)
+        && let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind
+        && ddpos.as_opt_usize().is_none()
+        && let PatKind::Binding(BindingAnnotation(by_ref, _), bind_id, 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,
+        )
+        && ((is_early_return(sym::Option, cx, &if_block) && path_to_local_id(peel_blocks(if_then), bind_id))
+            || is_early_return(sym::Result, cx, &if_block))
+        && if_else
+            .map(|e| eq_expr_value(cx, let_expr, peel_blocks(e)))
+            .filter(|e| *e)
+            .is_none()
+    {
+        let mut applicability = Applicability::MachineApplicable;
+        let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
+        let requires_semi = matches!(cx.tcx.parent_hir_node(expr.hir_id), Node::Stmt(_));
+        let sugg = format!(
+            "{receiver_str}{}?{}",
+            if by_ref == ByRef::Yes { ".as_ref()" } else { "" },
+            if requires_semi { ";" } else { "" }
+        );
+        span_lint_and_sugg(
+            cx,
+            QUESTION_MARK,
+            expr.span,
+            "this block may be rewritten with the `?` operator",
+            "replace it with",
+            sugg,
+            applicability,
+        );
+    }
+}
+
+impl QuestionMark {
+    fn inside_try_block(&self) -> bool {
+        self.try_block_depth_stack.last() > Some(&0)
     }
 }
 
@@ -324,15 +322,18 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark {
             return;
         }
 
-        if !in_constant(cx, stmt.hir_id) {
+        if !self.inside_try_block() && !in_constant(cx, stmt.hir_id) {
             check_let_some_else_return_none(cx, stmt);
         }
         self.check_manual_let_else(cx, stmt);
     }
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
-        if !in_constant(cx, expr.hir_id) && is_lint_allowed(cx, QUESTION_MARK_USED, expr.hir_id) {
-            self.check_is_none_or_err_and_early_return(cx, expr);
-            self.check_if_let_some_or_err_and_early_return(cx, expr);
+        if !self.inside_try_block()
+            && !in_constant(cx, expr.hir_id)
+            && 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);
         }
     }
 

tests/ui/manual_let_else.rs

@@ -1,3 +1,4 @@
+#![feature(try_blocks)]
 #![allow(unused_braces, unused_variables, dead_code)]
 #![allow(
     clippy::collapsible_else_if,
@@ -446,3 +447,12 @@ struct U<T> {
     w: T,
     x: T,
 }
+
+fn issue12337() {
+    // We want to generally silence question_mark lints within try blocks, since `?` has different
+    // behavior to `return`, and question_mark calls into manual_let_else logic, so make sure that
+    // we still emit a lint for manual_let_else
+    let _: Option<()> = try {
+        let v = if let Some(v_some) = g() { v_some } else { return };
+    };
+}