Auto merge of #13085 - J-ZhengLi:issue12973, r=llogiq

bors · bors · commit 1aac7783ef6c · 2024-07-13T15:38:23.000Z
make [`or_fun_call`] recursive. fixes: #12973 --- changelog: make [`or_fun_call`] recursive.
diff --git a/clippy_lints/src/methods/or_fun_call.rs b/clippy_lints/src/methods/or_fun_call.rs
@@ -1,8 +1,13 @@
+use std::ops::ControlFlow;
+
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::eager_or_lazy::switch_to_lazy_eval;
 use clippy_utils::source::snippet_with_context;
 use clippy_utils::ty::{expr_type_is_certain, implements_trait, is_type_diagnostic_item};
-use clippy_utils::{contains_return, is_default_equivalent, is_default_equivalent_call, last_path_segment};
+use clippy_utils::visitors::for_each_expr;
+use clippy_utils::{
+    contains_return, is_default_equivalent, is_default_equivalent_call, last_path_segment, peel_blocks,
+};
 use rustc_errors::Applicability;
 use rustc_lint::LateContext;
 use rustc_middle::ty;
@@ -13,7 +18,7 @@ use {rustc_ast as ast, rustc_hir as hir};
 use super::{OR_FUN_CALL, UNWRAP_OR_DEFAULT};
 
 /// Checks for the `OR_FUN_CALL` lint.
-#[allow(clippy::too_many_lines)]
+#[expect(clippy::too_many_lines)]
 pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &hir::Expr<'_>,
@@ -26,7 +31,6 @@ pub(super) fn check<'tcx>(
     /// `or_insert(T::new())` or `or_insert(T::default())`.
     /// Similarly checks for `unwrap_or_else(T::new)`, `unwrap_or_else(T::default)`,
     /// `or_insert_with(T::new)` or `or_insert_with(T::default)`.
-    #[allow(clippy::too_many_arguments)]
     fn check_unwrap_or_default(
         cx: &LateContext<'_>,
         name: &str,
@@ -123,8 +127,8 @@ pub(super) fn check<'tcx>(
     }
 
     /// Checks for `*or(foo())`.
-    #[allow(clippy::too_many_arguments)]
-    fn check_general_case<'tcx>(
+    #[expect(clippy::too_many_arguments)]
+    fn check_or_fn_call<'tcx>(
         cx: &LateContext<'tcx>,
         name: &str,
         method_span: Span,
@@ -135,7 +139,7 @@ pub(super) fn check<'tcx>(
         span: Span,
         // None if lambda is required
         fun_span: Option<Span>,
-    ) {
+    ) -> bool {
         // (path, fn_has_argument, methods, suffix)
         const KNOW_TYPES: [(Symbol, bool, &[&str], &str); 4] = [
             (sym::BTreeEntry, false, &["or_insert"], "with"),
@@ -185,54 +189,68 @@ pub(super) fn check<'tcx>(
                 format!("{name}_{suffix}({sugg})"),
                 app,
             );
-        }
-    }
-
-    let extract_inner_arg = |arg: &'tcx hir::Expr<'_>| {
-        if let hir::ExprKind::Block(
-            hir::Block {
-                stmts: [],
-                expr: Some(expr),
-                ..
-            },
-            _,
-        ) = arg.kind
-        {
-            expr
+            true
         } else {
-            arg
+            false
         }
-    };
+    }
 
     if let [arg] = args {
-        let inner_arg = extract_inner_arg(arg);
-        match inner_arg.kind {
-            hir::ExprKind::Call(fun, or_args) => {
-                let or_has_args = !or_args.is_empty();
-                if or_has_args
-                    || !check_unwrap_or_default(cx, name, receiver, fun, Some(inner_arg), expr.span, method_span)
-                {
-                    let fun_span = if or_has_args { None } else { Some(fun.span) };
-                    check_general_case(cx, name, method_span, receiver, arg, None, expr.span, fun_span);
-                }
-            },
-            hir::ExprKind::Path(..) | hir::ExprKind::Closure(..) => {
-                check_unwrap_or_default(cx, name, receiver, inner_arg, None, expr.span, method_span);
-            },
-            hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => {
-                check_general_case(cx, name, method_span, receiver, arg, None, expr.span, None);
-            },
-            _ => (),
-        }
+        let inner_arg = peel_blocks(arg);
+        for_each_expr(cx, inner_arg, |ex| {
+            // `or_fun_call` lint needs to take nested expr into account,
+            // but `unwrap_or_default` lint doesn't, we don't want something like:
+            // `opt.unwrap_or(Foo { inner: String::default(), other: 1 })` to get replaced by
+            // `opt.unwrap_or_default()`.
+            let is_nested_expr = ex.hir_id != inner_arg.hir_id;
+
+            let is_triggered = match ex.kind {
+                hir::ExprKind::Call(fun, fun_args) => {
+                    let inner_fun_has_args = !fun_args.is_empty();
+                    let fun_span = if inner_fun_has_args || is_nested_expr {
+                        None
+                    } else {
+                        Some(fun.span)
+                    };
+                    (!inner_fun_has_args
+                        && !is_nested_expr
+                        && check_unwrap_or_default(cx, name, receiver, fun, Some(ex), expr.span, method_span))
+                        || check_or_fn_call(cx, name, method_span, receiver, arg, None, expr.span, fun_span)
+                },
+                hir::ExprKind::Path(..) | hir::ExprKind::Closure(..) if !is_nested_expr => {
+                    check_unwrap_or_default(cx, name, receiver, ex, None, expr.span, method_span)
+                },
+                hir::ExprKind::Index(..) | hir::ExprKind::MethodCall(..) => {
+                    check_or_fn_call(cx, name, method_span, receiver, arg, None, expr.span, None)
+                },
+                _ => false,
+            };
+
+            if is_triggered {
+                ControlFlow::Break(())
+            } else {
+                ControlFlow::Continue(())
+            }
+        });
     }
 
     // `map_or` takes two arguments
     if let [arg, lambda] = args {
-        let inner_arg = extract_inner_arg(arg);
-        if let hir::ExprKind::Call(fun, or_args) = inner_arg.kind {
-            let fun_span = if or_args.is_empty() { Some(fun.span) } else { None };
-            check_general_case(cx, name, method_span, receiver, arg, Some(lambda), expr.span, fun_span);
-        }
+        let inner_arg = peel_blocks(arg);
+        for_each_expr(cx, inner_arg, |ex| {
+            let is_top_most_expr = ex.hir_id == inner_arg.hir_id;
+            if let hir::ExprKind::Call(fun, fun_args) = ex.kind {
+                let fun_span = if fun_args.is_empty() && is_top_most_expr {
+                    Some(fun.span)
+                } else {
+                    None
+                };
+                if check_or_fn_call(cx, name, method_span, receiver, arg, Some(lambda), expr.span, fun_span) {
+                    return ControlFlow::Break(());
+                }
+            }
+            ControlFlow::Continue(())
+        });
     }
 }
 
diff --git a/tests/ui/or_fun_call.fixed b/tests/ui/or_fun_call.fixed
@@ -330,4 +330,39 @@ mod issue_10228 {
     }
 }
 
+// issue #12973
+fn fn_call_in_nested_expr() {
+    struct Foo {
+        val: String,
+    }
+
+    fn f() -> i32 {
+        1
+    }
+    let opt: Option<i32> = Some(1);
+
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt.unwrap_or_else(f); // suggest `.unwrap_or_else(f)`
+    //
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt.unwrap_or_else(|| f() + 1); // suggest `.unwrap_or_else(|| f() + 1)`
+    //
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt.unwrap_or_else(|| {
+        let x = f();
+        x + 1
+    });
+    //~v ERROR: use of `map_or` followed by a function call
+    let _ = opt.map_or_else(|| f() + 1, |v| v); // suggest `.map_or_else(|| f() + 1, |v| v)`
+    //
+    //~v ERROR: use of `unwrap_or` to construct default value
+    let _ = opt.unwrap_or_default();
+
+    let opt_foo = Some(Foo {
+        val: String::from("123"),
+    });
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt_foo.unwrap_or_else(|| Foo { val: String::default() });
+}
+
 fn main() {}
diff --git a/tests/ui/or_fun_call.rs b/tests/ui/or_fun_call.rs
@@ -330,4 +330,39 @@ mod issue_10228 {
     }
 }
 
+// issue #12973
+fn fn_call_in_nested_expr() {
+    struct Foo {
+        val: String,
+    }
+
+    fn f() -> i32 {
+        1
+    }
+    let opt: Option<i32> = Some(1);
+
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt.unwrap_or({ f() }); // suggest `.unwrap_or_else(f)`
+    //
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt.unwrap_or(f() + 1); // suggest `.unwrap_or_else(|| f() + 1)`
+    //
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt.unwrap_or({
+        let x = f();
+        x + 1
+    });
+    //~v ERROR: use of `map_or` followed by a function call
+    let _ = opt.map_or(f() + 1, |v| v); // suggest `.map_or_else(|| f() + 1, |v| v)`
+    //
+    //~v ERROR: use of `unwrap_or` to construct default value
+    let _ = opt.unwrap_or({ i32::default() });
+
+    let opt_foo = Some(Foo {
+        val: String::from("123"),
+    });
+    //~v ERROR: use of `unwrap_or` followed by a function call
+    let _ = opt_foo.unwrap_or(Foo { val: String::default() });
+}
+
 fn main() {}
diff --git a/tests/ui/or_fun_call.stderr b/tests/ui/or_fun_call.stderr
@@ -196,5 +196,53 @@ error: use of `unwrap_or_else` to construct default value
 LL |         let _ = stringy.unwrap_or_else(String::new);
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
 
-error: aborting due to 32 previous errors
+error: use of `unwrap_or` followed by a function call
+  --> tests/ui/or_fun_call.rs:345:17
+   |
+LL |     let _ = opt.unwrap_or({ f() }); // suggest `.unwrap_or_else(f)`
+   |                 ^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(f)`
+
+error: use of `unwrap_or` followed by a function call
+  --> tests/ui/or_fun_call.rs:348:17
+   |
+LL |     let _ = opt.unwrap_or(f() + 1); // suggest `.unwrap_or_else(|| f() + 1)`
+   |                 ^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| f() + 1)`
+
+error: use of `unwrap_or` followed by a function call
+  --> tests/ui/or_fun_call.rs:351:17
+   |
+LL |       let _ = opt.unwrap_or({
+   |  _________________^
+LL | |         let x = f();
+LL | |         x + 1
+LL | |     });
+   | |______^
+   |
+help: try
+   |
+LL ~     let _ = opt.unwrap_or_else(|| {
+LL +         let x = f();
+LL +         x + 1
+LL ~     });
+   |
+
+error: use of `map_or` followed by a function call
+  --> tests/ui/or_fun_call.rs:356:17
+   |
+LL |     let _ = opt.map_or(f() + 1, |v| v); // suggest `.map_or_else(|| f() + 1, |v| v)`
+   |                 ^^^^^^^^^^^^^^^^^^^^^^ help: try: `map_or_else(|| f() + 1, |v| v)`
+
+error: use of `unwrap_or` to construct default value
+  --> tests/ui/or_fun_call.rs:359:17
+   |
+LL |     let _ = opt.unwrap_or({ i32::default() });
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
+
+error: use of `unwrap_or` followed by a function call
+  --> tests/ui/or_fun_call.rs:365:21
+   |
+LL |     let _ = opt_foo.unwrap_or(Foo { val: String::default() });
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| Foo { val: String::default() })`
+
+error: aborting due to 38 previous errors
 
