Auto merge of rust-lang#11001 - y21:issue8628, r=Jarcho

bors · bors · commit 10ce1a6d5e7c · 2023-06-28T18:38:44.000Z
[`question_mark`]: don't lint inside of `try` block Fixes rust-lang#8628. Diff looks a bit noisy because I had to move the two functions into an impl, because they now need to access the structs `try_block_depth` field to see if they're inside a try block. changelog: [`question_mark`]: don't lint inside of `try` block
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -771,7 +771,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
@@ -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_stack: Vec<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,147 @@ 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_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_stack
+                .last_mut()
+                .expect("blocks are always part of bodies and must have a depth") += 1;
+        }
+    }
+
+    fn check_body(&mut self, _: &LateContext<'tcx>, _: &'tcx rustc_hir::Body<'tcx>) {
+        self.try_block_depth_stack.push(0);
+    }
+
+    fn check_body_post(&mut self, _: &LateContext<'tcx>, _: &'tcx rustc_hir::Body<'tcx>) {
+        self.try_block_depth_stack.pop();
+    }
+
+    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_stack
+                .last_mut()
+                .expect("blocks are always part of bodies and must have a depth") -= 1;
         }
     }
 }
diff --git 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,27 @@ 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))
+}
+
+fn issue6828_nested_body() -> Option<u32> {
+    try {
+        fn f2(a: Option<i32>) -> Option<i32> {
+            a?;
+            Some(32)
+        }
+        123
+    }
+}
+
 // 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
@@ -1,4 +1,5 @@
 //@run-rustfix
+#![feature(try_blocks)]
 #![allow(unreachable_code)]
 #![allow(dead_code)]
 #![allow(clippy::unnecessary_wraps)]
@@ -263,6 +264,31 @@ 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))
+}
+
+fn issue6828_nested_body() -> Option<u32> {
+    try {
+        fn f2(a: Option<i32>) -> Option<i32> {
+            if a.is_none() {
+                return None;
+                // do lint here, the outer `try` is not relevant here
+                // https://github.com/rust-lang/rust-clippy/pull/11001#issuecomment-1610636867
+            }
+            Some(32)
+        }
+        123
+    }
+}
+
 // 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
