[`question_mark`]: don't lint inside of `try` block

5 files changed, 178 insertions, 114 deletions

diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 3fb4e6c8fa5..876fcd0f197 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -770,7 +770,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::<useless_conversion::UselessConversion>::default());
     store.register_late_pass(|_| Box::new(implicit_hasher::ImplicitHasher));
     store.register_late_pass(|_| Box::new(fallible_impl_from::FallibleImplFrom));
-    store.register_late_pass(|_| Box::new(question_mark::QuestionMark));
+    store.register_late_pass(|_| Box::<question_mark::QuestionMark>::default());
     store.register_late_pass(|_| Box::new(question_mark_used::QuestionMarkUsed));
     store.register_early_pass(|| Box::new(suspicious_operation_groupings::SuspiciousOperationGroupings));
     store.register_late_pass(|_| Box::new(suspicious_trait_impl::SuspiciousImpl));
diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs
index 5269bbd1f1a..036ad3e0d15 100644
--- a/clippy_lints/src/question_mark.rs
+++ b/clippy_lints/src/question_mark.rs
@@ -1,19 +1,20 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::higher;
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::{
     eq_expr_value, get_parent_node, in_constant, is_else_clause, is_res_lang_ctor, path_to_local, path_to_local_id,
     peel_blocks, peel_blocks_with_stmt,
 };
+use clippy_utils::{higher, is_path_lang_item};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::def::Res;
-use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
+use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk};
 use rustc_hir::{BindingAnnotation, ByRef, Expr, ExprKind, Node, PatKind, PathSegment, QPath};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::ty::Ty;
-use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_session::declare_tool_lint;
+use rustc_session::impl_lint_pass;
 use rustc_span::{sym, symbol::Symbol};
 
 declare_clippy_lint! {
@@ -41,7 +42,16 @@ declare_clippy_lint! {
     "checks for expressions that could be replaced by the question mark operator"
 }
 
-declare_lint_pass!(QuestionMark => [QUESTION_MARK]);
+#[derive(Default)]
+pub struct QuestionMark {
+    /// Keeps track of how many try blocks we are in at any point during linting.
+    /// This allows us to answer the question "are we inside of a try block"
+    /// very quickly, without having to walk up the parent chain, by simply checking
+    /// if it is greater than zero.
+    /// As for why we need this in the first place: <https://github.com/rust-lang/rust-clippy/issues/8628>
+    try_block_depth: u32,
+}
+impl_lint_pass!(QuestionMark => [QUESTION_MARK]);
 
 enum IfBlockType<'hir> {
     /// An `if x.is_xxx() { a } else { b } ` expression.
@@ -68,98 +78,6 @@ enum IfBlockType<'hir> {
     ),
 }
 
-/// 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_chain! {
-        if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr);
-        if !is_else_clause(cx.tcx, expr);
-        if 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, r#else);
-        if is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block);
-        then {
-            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;
-                }
-            } 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,
-            );
-        }
-    }
-}
-
-fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
-    if_chain! {
-        if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else }) = higher::IfLet::hir(cx, expr);
-        if !is_else_clause(cx.tcx, expr);
-        if let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind;
-        if ddpos.as_opt_usize().is_none();
-        if 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
-        );
-        if (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 if_else.map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))).filter(|e| *e).is_none();
-        then {
-            let mut applicability = Applicability::MachineApplicable;
-            let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
-            let requires_semi = matches!(get_parent_node(cx.tcx, expr.hir_id), Some(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 is_early_return(smbl: Symbol, cx: &LateContext<'_>, if_block: &IfBlockType<'_>) -> bool {
     match *if_block {
         IfBlockType::IfIs(caller, caller_ty, call_sym, if_then, _) => {
@@ -230,11 +148,133 @@ fn expr_return_none_or_err(
     }
 }
 
+impl QuestionMark {
+    fn inside_try_block(&self) -> bool {
+        self.try_block_depth > 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_chain! {
+            if !self.inside_try_block();
+            if let Some(higher::If { cond, then, r#else }) = higher::If::hir(expr);
+            if !is_else_clause(cx.tcx, expr);
+            if 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, r#else);
+            if is_early_return(sym::Option, cx, &if_block) || is_early_return(sym::Result, cx, &if_block);
+            then {
+                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;
+                    }
+                } 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,
+                );
+            }
+        }
+    }
+
+    fn check_if_let_some_or_err_and_early_return<'tcx>(&self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
+        if_chain! {
+            if !self.inside_try_block();
+            if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else }) = higher::IfLet::hir(cx, expr);
+            if !is_else_clause(cx.tcx, expr);
+            if let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind;
+            if ddpos.as_opt_usize().is_none();
+            if 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
+            );
+            if (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 if_else.map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))).filter(|e| *e).is_none();
+            then {
+                let mut applicability = Applicability::MachineApplicable;
+                let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
+                let requires_semi = matches!(get_parent_node(cx.tcx, expr.hir_id), Some(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 is_try_block(cx: &LateContext<'_>, bl: &rustc_hir::Block<'_>) -> bool {
+    if let Some(expr) = bl.expr
+        && let rustc_hir::ExprKind::Call(callee, _) = expr.kind
+    {
+        is_path_lang_item(cx, callee, LangItem::TryTraitFromOutput)
+    } else {
+        false
+    }
+}
+
 impl<'tcx> LateLintPass<'tcx> for QuestionMark {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if !in_constant(cx, expr.hir_id) {
-            check_is_none_or_err_and_early_return(cx, expr);
-            check_if_let_some_or_err_and_early_return(cx, expr);
+            self.check_is_none_or_err_and_early_return(cx, expr);
+            self.check_if_let_some_or_err_and_early_return(cx, expr);
+        }
+    }
+
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx rustc_hir::Block<'tcx>) {
+        if is_try_block(cx, block) {
+            self.try_block_depth += 1;
+        }
+    }
+
+    fn check_block_post(&mut self, cx: &LateContext<'tcx>, block: &'tcx rustc_hir::Block<'tcx>) {
+        if is_try_block(cx, block) {
+            self.try_block_depth -= 1;
         }
     }
 }
diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed
index 7f1660f31fa..f257378842b 100644
--- a/tests/ui/question_mark.fixed
+++ b/tests/ui/question_mark.fixed
@@ -1,4 +1,5 @@
 //@run-rustfix
+#![feature(try_blocks)]
 #![allow(unreachable_code)]
 #![allow(dead_code)]
 #![allow(clippy::unnecessary_wraps)]
@@ -227,6 +228,17 @@ fn pattern() -> Result<(), PatternedError> {
 
 fn main() {}
 
+// `?` is not the same as `return None;` if inside of a try block
+fn issue8628(a: Option<u32>) -> Option<u32> {
+    let b: Option<u32> = try {
+        if a.is_none() {
+            return None;
+        }
+        32
+    };
+    b.or(Some(128))
+}
+
 // should not lint, `?` operator not available in const context
 const fn issue9175(option: Option<()>) -> Option<()> {
     if option.is_none() {
diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
index a90eae50ed4..29d79b078ac 100644
--- a/tests/ui/question_mark.rs
+++ b/tests/ui/question_mark.rs
@@ -1,4 +1,5 @@
 //@run-rustfix
+#![feature(try_blocks)]
 #![allow(unreachable_code)]
 #![allow(dead_code)]
 #![allow(clippy::unnecessary_wraps)]
@@ -263,6 +264,17 @@ fn pattern() -> Result<(), PatternedError> {
 
 fn main() {}
 
+// `?` is not the same as `return None;` if inside of a try block
+fn issue8628(a: Option<u32>) -> Option<u32> {
+    let b: Option<u32> = try {
+        if a.is_none() {
+            return None;
+        }
+        32
+    };
+    b.or(Some(128))
+}
+
 // should not lint, `?` operator not available in const context
 const fn issue9175(option: Option<()>) -> Option<()> {
     if option.is_none() {
diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr
index 23172d7e535..86d89942cc0 100644
--- a/tests/ui/question_mark.stderr
+++ b/tests/ui/question_mark.stderr
@@ -1,5 +1,5 @@
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:7:5
+  --> $DIR/question_mark.rs:8:5
    |
 LL | /     if a.is_none() {
 LL | |         return None;
@@ -9,7 +9,7 @@ LL | |     }
    = note: `-D clippy::question-mark` implied by `-D warnings`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:52:9
+  --> $DIR/question_mark.rs:53:9
    |
 LL | /         if (self.opt).is_none() {
 LL | |             return None;
@@ -17,7 +17,7 @@ LL | |         }
    | |_________^ help: replace it with: `(self.opt)?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:56:9
+  --> $DIR/question_mark.rs:57:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None
@@ -25,7 +25,7 @@ LL | |         }
    | |_________^ help: replace it with: `self.opt?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:60:17
+  --> $DIR/question_mark.rs:61:17
    |
 LL |           let _ = if self.opt.is_none() {
    |  _________________^
@@ -36,7 +36,7 @@ LL | |         };
    | |_________^ help: replace it with: `Some(self.opt?)`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:66:17
+  --> $DIR/question_mark.rs:67:17
    |
 LL |           let _ = if let Some(x) = self.opt {
    |  _________________^
@@ -47,7 +47,7 @@ LL | |         };
    | |_________^ help: replace it with: `self.opt?`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:83:9
+  --> $DIR/question_mark.rs:84:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
@@ -55,7 +55,7 @@ LL | |         }
    | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:91:9
+  --> $DIR/question_mark.rs:92:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
@@ -63,7 +63,7 @@ LL | |         }
    | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:99:9
+  --> $DIR/question_mark.rs:100:9
    |
 LL | /         if self.opt.is_none() {
 LL | |             return None;
@@ -71,7 +71,7 @@ LL | |         }
    | |_________^ help: replace it with: `self.opt.as_ref()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:106:26
+  --> $DIR/question_mark.rs:107:26
    |
 LL |           let v: &Vec<_> = if let Some(ref v) = self.opt {
    |  __________________________^
@@ -82,7 +82,7 @@ LL | |         };
    | |_________^ help: replace it with: `self.opt.as_ref()?`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:116:17
+  --> $DIR/question_mark.rs:117:17
    |
 LL |           let v = if let Some(v) = self.opt {
    |  _________________^
@@ -93,7 +93,7 @@ LL | |         };
    | |_________^ help: replace it with: `self.opt?`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:131:5
+  --> $DIR/question_mark.rs:132:5
    |
 LL | /     if f().is_none() {
 LL | |         return None;
@@ -101,13 +101,13 @@ LL | |     }
    | |_____^ help: replace it with: `f()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:143:13
+  --> $DIR/question_mark.rs:144: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
-  --> $DIR/question_mark.rs:145:5
+  --> $DIR/question_mark.rs:146:5
    |
 LL | /     if x.is_err() {
 LL | |         return x;
@@ -115,7 +115,7 @@ LL | |     }
    | |_____^ help: replace it with: `x?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:196:5
+  --> $DIR/question_mark.rs:197:5
    |
 LL | /     if let Err(err) = func_returning_result() {
 LL | |         return Err(err);
@@ -123,7 +123,7 @@ LL | |     }
    | |_____^ help: replace it with: `func_returning_result()?;`
 
 error: this block may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:203:5
+  --> $DIR/question_mark.rs:204:5
    |
 LL | /     if let Err(err) = func_returning_result() {
 LL | |         return Err(err);