Properly check that an expression might be the one returned

The `TyCtxt::hir_get_fn_id_for_return_block()` function was too broad,
as it will return positively even when given part of an expression that can
be used as a return value. A new `potential_return_of_enclosing_body()`
utility function has been made to represent the fact that an expression
might be directly returned from its enclosing body.

Author: samueltardieu

clippy_lints/src/methods/return_and_then.rs

@@ -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::{get_parent_expr, 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;
     }
 

clippy_utils/src/lib.rs

@@ -3497,3 +3497,46 @@ 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
+///
+/// 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;
+    for (hir_id, node) in cx.tcx.hir_parent_iter(expr.hir_id) {
+        if hir_id == enclosing_body_owner {
+            return true;
+        }
+        match node {
+            Node::Block(Block { expr, .. }) if expr.is_some_and(|expr| expr.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.body.hir_id == prev_id) => {},
+                ExprKind::Block(block, _) if block.hir_id == prev_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

@@ -124,3 +124,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

@@ -115,3 +115,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 _ = ();
+            }
+        },
+    };
+}