Properly check that an expression might be the one returned

clippy_lints/src/methods/return_and_then.rs

@@ -1,5 +1,5 @@
 use rustc_errors::Applicability;
-use rustc_hir as hir;
+use rustc_hir::{self as hir, Node};
 use rustc_lint::LateContext;
 use rustc_middle::ty::{self, GenericArg, Ty};
 use rustc_span::sym;
@@ -9,7 +9,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_applicability};
 use clippy_utils::ty::get_type_diagnostic_name;
 use clippy_utils::visitors::for_each_unconsumed_temporary;
-use clippy_utils::{get_parent_expr, peel_blocks};
+use clippy_utils::{peel_blocks, potential_return_of_enclosing_body};
 
 use super::RETURN_AND_THEN;
 
@@ -21,7 +21,7 @@ pub(super) fn check<'tcx>(
     recv: &'tcx hir::Expr<'tcx>,
     arg: &'tcx hir::Expr<'_>,
 ) {
-    if cx.tcx.hir_get_fn_id_for_return_block(expr.hir_id).is_none() {
+    if !potential_return_of_enclosing_body(cx, expr) {
         return;
     }
 
@@ -55,15 +55,28 @@ pub(super) fn check<'tcx>(
         None => &body_snip,
     };
 
-    // If suggestion is going to get inserted as part of a `return` expression, it must be blockified.
-    let sugg = if let Some(parent_expr) = get_parent_expr(cx, expr) {
-        let base_indent = indent_of(cx, parent_expr.span);
+    // If suggestion is going to get inserted as part of a `return` expression or as a match expression
+    // arm, it must be blockified.
+    let (parent_span_for_indent, opening_paren, closing_paren) = match cx.tcx.parent_hir_node(expr.hir_id) {
+        Node::Expr(parent_expr) if matches!(parent_expr.kind, hir::ExprKind::Break(..)) => {
+            (Some(parent_expr.span), "(", ")")
+        },
+        Node::Expr(parent_expr) => (Some(parent_expr.span), "", ""),
+        Node::Arm(match_arm) => (Some(match_arm.span), "", ""),
+        _ => (None, "", ""),
+    };
+    let sugg = if let Some(span) = parent_span_for_indent {
+        let base_indent = indent_of(cx, span);
         let inner_indent = base_indent.map(|i| i + 4);
         format!(
             "{}\n{}\n{}",
-            reindent_multiline(&format!("{{\nlet {arg_snip} = {recv_snip}?;"), true, inner_indent),
+            reindent_multiline(
+                &format!("{opening_paren}{{\nlet {arg_snip} = {recv_snip}?;"),
+                true,
+                inner_indent
+            ),
             reindent_multiline(inner, false, inner_indent),
-            reindent_multiline("}", false, base_indent),
+            reindent_multiline(&format!("}}{closing_paren}"), false, base_indent),
         )
     } else {
         format!(

clippy_utils/src/lib.rs

@@ -3497,3 +3497,64 @@ pub fn is_expr_default<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) ->
         false
     }
 }
+
+/// Checks if `expr` may be directly used as the return value of its enclosing body.
+/// The following cases are covered:
+/// - `expr` as the last expression of the body, or of a block that can be used as the return value
+/// - `return expr`
+/// - then or else part of a `if` in return position
+/// - arm body of a `match` in a return position
+/// - `break expr` or `break 'label expr` if the loop or block being exited is used as a return
+///   value
+///
+/// Contrary to [`TyCtxt::hir_get_fn_id_for_return_block()`], if `expr` is part of a
+/// larger expression, for example a field expression of a `struct`, it will not be
+/// considered as matching the condition and will return `false`.
+///
+/// Also, even if `expr` is assigned to a variable which is later returned, this function
+/// will still return `false` because `expr` is not used *directly* as the return value
+/// as it goes through the intermediate variable.
+pub fn potential_return_of_enclosing_body(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    let enclosing_body_owner = cx
+        .tcx
+        .local_def_id_to_hir_id(cx.tcx.hir_enclosing_body_owner(expr.hir_id));
+    let mut prev_id = expr.hir_id;
+    let mut skip_until_id = None;
+    for (hir_id, node) in cx.tcx.hir_parent_iter(expr.hir_id) {
+        if hir_id == enclosing_body_owner {
+            return true;
+        }
+        if let Some(id) = skip_until_id {
+            prev_id = hir_id;
+            if id == hir_id {
+                skip_until_id = None;
+            }
+            continue;
+        }
+        match node {
+            Node::Block(Block { expr, .. }) if expr.is_some_and(|expr| expr.hir_id == prev_id) => {},
+            Node::Arm(arm) if arm.body.hir_id == prev_id => {},
+            Node::Expr(expr) => match expr.kind {
+                ExprKind::Ret(_) => return true,
+                ExprKind::If(_, then, opt_else)
+                    if then.hir_id == prev_id || opt_else.is_some_and(|els| els.hir_id == prev_id) => {},
+                ExprKind::Match(_, arms, _) if arms.iter().any(|arm| arm.hir_id == prev_id) => {},
+                ExprKind::Block(block, _) if block.hir_id == prev_id => {},
+                ExprKind::Break(
+                    Destination {
+                        target_id: Ok(target_id),
+                        ..
+                    },
+                    _,
+                ) => skip_until_id = Some(target_id),
+                _ => break,
+            },
+            _ => break,
+        }
+        prev_id = hir_id;
+    }
+
+    // `expr` is used as part of "something" and is not returned directly from its
+    // enclosing body.
+    false
+}

tests/ui/return_and_then.fixed

@@ -99,6 +99,92 @@ fn main() {
         };
         None
     }
+
+    #[expect(clippy::diverging_sub_expression)]
+    fn with_return_in_expression() -> Option<i32> {
+        _ = (
+            return {
+                let x = Some("")?;
+                if x.len() > 2 { Some(3) } else { None }
+            },
+            //~^ return_and_then
+            10,
+        );
+    }
+
+    fn inside_if(a: bool, i: Option<u32>) -> Option<u32> {
+        if a {
+            let i = i?;
+            if i > 3 { Some(i) } else { None }
+            //~^ return_and_then
+        } else {
+            Some(42)
+        }
+    }
+
+    fn inside_match(a: u32, i: Option<u32>) -> Option<u32> {
+        match a {
+            1 | 2 => {
+                let i = i?;
+                if i > 3 { Some(i) } else { None }
+            },
+            //~^ return_and_then
+            3 | 4 => Some(42),
+            _ => None,
+        }
+    }
+
+    fn inside_match_and_block_and_if(a: u32, i: Option<u32>) -> Option<u32> {
+        match a {
+            1 | 2 => {
+                let a = a * 3;
+                if a.is_multiple_of(2) {
+                    let i = i?;
+                    if i > 3 { Some(i) } else { None }
+                    //~^ return_and_then
+                } else {
+                    Some(10)
+                }
+            },
+            3 | 4 => Some(42),
+            _ => None,
+        }
+    }
+
+    #[expect(clippy::never_loop)]
+    fn with_break(i: Option<u32>) -> Option<u32> {
+        match i {
+            Some(1) => loop {
+                break ({
+                    let i = i?;
+                    if i > 3 { Some(i) } else { None }
+                });
+                //~^ return_and_then
+            },
+            Some(2) => 'foo: loop {
+                loop {
+                    break 'foo ({
+                        let i = i?;
+                        if i > 3 { Some(i) } else { None }
+                    });
+                    //~^ return_and_then
+                }
+            },
+            Some(3) => 'bar: {
+                break 'bar ({
+                    let i = i?;
+                    if i > 3 { Some(i) } else { None }
+                });
+                //~^ return_and_then
+            },
+            Some(4) => 'baz: loop {
+                _ = loop {
+                    break i.and_then(|i| if i > 3 { Some(i) } else { None });
+                };
+            },
+            _ => None,
+        }
+    }
 }
 
 fn gen_option(n: i32) -> Option<i32> {
@@ -124,3 +210,48 @@ mod issue14781 {
         Ok(())
     }
 }
+
+mod issue15111 {
+    #[derive(Debug)]
+    struct EvenOdd {
+        even: Option<u32>,
+        odd: Option<u32>,
+    }
+
+    impl EvenOdd {
+        fn new(i: Option<u32>) -> Self {
+            Self {
+                even: i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }),
+                odd: i.and_then(|i| if i.is_multiple_of(2) { None } else { Some(i) }),
+            }
+        }
+    }
+
+    fn with_if_let(i: Option<u32>) -> u32 {
+        if let Some(x) = i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }) {
+            x
+        } else {
+            std::hint::black_box(0)
+        }
+    }
+
+    fn main() {
+        let _ = EvenOdd::new(Some(2));
+    }
+}
+
+mod issue14927 {
+    use std::path::Path;
+    struct A {
+        pub func: fn(check: bool, a: &Path, b: Option<&Path>),
+    }
+    const MY_A: A = A {
+        func: |check, a, b| {
+            if check {
+                let _ = ();
+            } else if let Some(parent) = b.and_then(|p| p.parent()) {
+                let _ = ();
+            }
+        },
+    };
+}

tests/ui/return_and_then.rs

@@ -90,6 +90,75 @@ fn main() {
         };
         None
     }
+
+    #[expect(clippy::diverging_sub_expression)]
+    fn with_return_in_expression() -> Option<i32> {
+        _ = (
+            return Some("").and_then(|x| if x.len() > 2 { Some(3) } else { None }),
+            //~^ return_and_then
+            10,
+        );
+    }
+
+    fn inside_if(a: bool, i: Option<u32>) -> Option<u32> {
+        if a {
+            i.and_then(|i| if i > 3 { Some(i) } else { None })
+            //~^ return_and_then
+        } else {
+            Some(42)
+        }
+    }
+
+    fn inside_match(a: u32, i: Option<u32>) -> Option<u32> {
+        match a {
+            1 | 2 => i.and_then(|i| if i > 3 { Some(i) } else { None }),
+            //~^ return_and_then
+            3 | 4 => Some(42),
+            _ => None,
+        }
+    }
+
+    fn inside_match_and_block_and_if(a: u32, i: Option<u32>) -> Option<u32> {
+        match a {
+            1 | 2 => {
+                let a = a * 3;
+                if a.is_multiple_of(2) {
+                    i.and_then(|i| if i > 3 { Some(i) } else { None })
+                    //~^ return_and_then
+                } else {
+                    Some(10)
+                }
+            },
+            3 | 4 => Some(42),
+            _ => None,
+        }
+    }
+
+    #[expect(clippy::never_loop)]
+    fn with_break(i: Option<u32>) -> Option<u32> {
+        match i {
+            Some(1) => loop {
+                break i.and_then(|i| if i > 3 { Some(i) } else { None });
+                //~^ return_and_then
+            },
+            Some(2) => 'foo: loop {
+                loop {
+                    break 'foo i.and_then(|i| if i > 3 { Some(i) } else { None });
+                    //~^ return_and_then
+                }
+            },
+            Some(3) => 'bar: {
+                break 'bar i.and_then(|i| if i > 3 { Some(i) } else { None });
+                //~^ return_and_then
+            },
+            Some(4) => 'baz: loop {
+                _ = loop {
+                    break i.and_then(|i| if i > 3 { Some(i) } else { None });
+                };
+            },
+            _ => None,
+        }
+    }
 }
 
 fn gen_option(n: i32) -> Option<i32> {
@@ -115,3 +184,48 @@ mod issue14781 {
         Ok(())
     }
 }
+
+mod issue15111 {
+    #[derive(Debug)]
+    struct EvenOdd {
+        even: Option<u32>,
+        odd: Option<u32>,
+    }
+
+    impl EvenOdd {
+        fn new(i: Option<u32>) -> Self {
+            Self {
+                even: i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }),
+                odd: i.and_then(|i| if i.is_multiple_of(2) { None } else { Some(i) }),
+            }
+        }
+    }
+
+    fn with_if_let(i: Option<u32>) -> u32 {
+        if let Some(x) = i.and_then(|i| if i.is_multiple_of(2) { Some(i) } else { None }) {
+            x
+        } else {
+            std::hint::black_box(0)
+        }
+    }
+
+    fn main() {
+        let _ = EvenOdd::new(Some(2));
+    }
+}
+
+mod issue14927 {
+    use std::path::Path;
+    struct A {
+        pub func: fn(check: bool, a: &Path, b: Option<&Path>),
+    }
+    const MY_A: A = A {
+        func: |check, a, b| {
+            if check {
+                let _ = ();
+            } else if let Some(parent) = b.and_then(|p| p.parent()) {
+                let _ = ();
+            }
+        },
+    };
+}