Fix expect_fun_call producing invalid suggestions

sethdusek · sethdusek · commit 07e2d96effb3 · 2025-06-24T17:16:13.000+05:00
diff --git a/clippy_lints/src/methods/expect_fun_call.rs b/clippy_lints/src/methods/expect_fun_call.rs
@@ -1,11 +1,12 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::eager_or_lazy::switch_to_lazy_eval;
 use clippy_utils::macros::{FormatArgsStorage, format_args_inputs_span, root_macro_call_first_node};
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};
+use clippy_utils::{contains_return, peel_blocks};
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
-use rustc_middle::ty;
 use rustc_span::symbol::sym;
 use rustc_span::{Span, Symbol};
 use std::borrow::Cow;
@@ -23,10 +24,10 @@ pub(super) fn check<'tcx>(
     receiver: &'tcx hir::Expr<'tcx>,
     args: &'tcx [hir::Expr<'tcx>],
 ) {
-    // Strip `&`, `as_ref()` and `as_str()` off `arg` until we're left with either a `String` or
+    // Strip `{}`, `&`, `as_ref()` and `as_str()` off `arg` until we're left with either a `String` or
     // `&str`
     fn get_arg_root<'a>(cx: &LateContext<'_>, arg: &'a hir::Expr<'a>) -> &'a hir::Expr<'a> {
-        let mut arg_root = arg;
+        let mut arg_root = peel_blocks(arg);
         loop {
             arg_root = match &arg_root.kind {
                 hir::ExprKind::AddrOf(hir::BorrowKind::Ref, _, expr) => expr,
@@ -47,124 +48,55 @@ pub(super) fn check<'tcx>(
         arg_root
     }
 
-    // Only `&'static str` or `String` can be used directly in the `panic!`. Other types should be
-    // converted to string.
-    fn requires_to_string(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool {
-        let arg_ty = cx.typeck_results().expr_ty(arg);
-        if is_type_lang_item(cx, arg_ty, hir::LangItem::String) {
-            return false;
-        }
-        if let ty::Ref(_, ty, ..) = arg_ty.kind()
-            && ty.is_str()
-            && can_be_static_str(cx, arg)
-        {
-            return false;
-        }
-        true
-    }
+    if name == sym::expect
+        && let [arg] = args
+        && let arg_root = get_arg_root(cx, arg)
+        && switch_to_lazy_eval(cx, arg_root)
+        && !contains_return(arg_root)
+    {
+        let receiver_type = cx.typeck_results().expr_ty_adjusted(receiver);
+        let closure_args = if is_type_diagnostic_item(cx, receiver_type, sym::Option) {
+            "||"
+        } else if is_type_diagnostic_item(cx, receiver_type, sym::Result) {
+            "|_|"
+        } else {
+            return;
+        };
 
-    // Check if an expression could have type `&'static str`, knowing that it
-    // has type `&str` for some lifetime.
-    fn can_be_static_str(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool {
-        match arg.kind {
-            hir::ExprKind::Lit(_) => true,
-            hir::ExprKind::Call(fun, _) => {
-                if let hir::ExprKind::Path(ref p) = fun.kind {
-                    match cx.qpath_res(p, fun.hir_id) {
-                        hir::def::Res::Def(hir::def::DefKind::Fn | hir::def::DefKind::AssocFn, def_id) => matches!(
-                            cx.tcx.fn_sig(def_id).instantiate_identity().output().skip_binder().kind(),
-                            ty::Ref(re, ..) if re.is_static(),
-                        ),
-                        _ => false,
-                    }
-                } else {
-                    false
-                }
-            },
-            hir::ExprKind::MethodCall(..) => {
-                cx.typeck_results()
-                    .type_dependent_def_id(arg.hir_id)
-                    .is_some_and(|method_id| {
-                        matches!(
-                            cx.tcx.fn_sig(method_id).instantiate_identity().output().skip_binder().kind(),
-                            ty::Ref(re, ..) if re.is_static()
-                        )
-                    })
-            },
-            hir::ExprKind::Path(ref p) => matches!(
-                cx.qpath_res(p, arg.hir_id),
-                hir::def::Res::Def(hir::def::DefKind::Const | hir::def::DefKind::Static { .. }, _)
-            ),
-            _ => false,
-        }
-    }
+        let span_replace_word = method_span.with_hi(expr.span.hi());
 
-    fn is_call(node: &hir::ExprKind<'_>) -> bool {
-        match node {
-            hir::ExprKind::AddrOf(hir::BorrowKind::Ref, _, expr) => {
-                is_call(&expr.kind)
-            },
-            hir::ExprKind::Call(..)
-            | hir::ExprKind::MethodCall(..)
-            // These variants are debatable or require further examination
-            | hir::ExprKind::If(..)
-            | hir::ExprKind::Match(..)
-            | hir::ExprKind::Block{ .. } => true,
-            _ => false,
-        }
-    }
+        let mut applicability = Applicability::MachineApplicable;
 
-    if args.len() != 1 || name != sym::expect || !is_call(&args[0].kind) {
-        return;
-    }
-
-    let receiver_type = cx.typeck_results().expr_ty_adjusted(receiver);
-    let closure_args = if is_type_diagnostic_item(cx, receiver_type, sym::Option) {
-        "||"
-    } else if is_type_diagnostic_item(cx, receiver_type, sym::Result) {
-        "|_|"
-    } else {
-        return;
-    };
-
-    let arg_root = get_arg_root(cx, &args[0]);
-
-    let span_replace_word = method_span.with_hi(expr.span.hi());
-
-    let mut applicability = Applicability::MachineApplicable;
-
-    // Special handling for `format!` as arg_root
-    if let Some(macro_call) = root_macro_call_first_node(cx, arg_root) {
-        if cx.tcx.is_diagnostic_item(sym::format_macro, macro_call.def_id)
-            && let Some(format_args) = format_args_storage.get(cx, arg_root, macro_call.expn)
-        {
-            let span = format_args_inputs_span(format_args);
-            let sugg = snippet_with_applicability(cx, span, "..", &mut applicability);
-            span_lint_and_sugg(
-                cx,
-                EXPECT_FUN_CALL,
-                span_replace_word,
-                format!("function call inside of `{name}`"),
-                "try",
-                format!("unwrap_or_else({closure_args} panic!({sugg}))"),
-                applicability,
-            );
+        // Special handling for `format!` as arg_root
+        if let Some(macro_call) = root_macro_call_first_node(cx, arg_root) {
+            if cx.tcx.is_diagnostic_item(sym::format_macro, macro_call.def_id)
+                && let Some(format_args) = format_args_storage.get(cx, arg_root, macro_call.expn)
+            {
+                let span = format_args_inputs_span(format_args);
+                let sugg = snippet_with_applicability(cx, span, "..", &mut applicability);
+                span_lint_and_sugg(
+                    cx,
+                    EXPECT_FUN_CALL,
+                    span_replace_word,
+                    format!("function call inside of `{name}`"),
+                    "try",
+                    format!("unwrap_or_else({closure_args} panic!({sugg}))"),
+                    applicability,
+                );
+            }
+            return;
         }
-        return;
-    }
 
-    let mut arg_root_snippet: Cow<'_, _> = snippet_with_applicability(cx, arg_root.span, "..", &mut applicability);
-    if requires_to_string(cx, arg_root) {
-        arg_root_snippet.to_mut().push_str(".to_string()");
-    }
+        let arg_root_snippet: Cow<'_, _> = snippet_with_applicability(cx, arg_root.span, "..", &mut applicability);
 
-    span_lint_and_sugg(
-        cx,
-        EXPECT_FUN_CALL,
-        span_replace_word,
-        format!("function call inside of `{name}`"),
-        "try",
-        format!("unwrap_or_else({closure_args} {{ panic!(\"{{}}\", {arg_root_snippet}) }})"),
-        applicability,
-    );
+        span_lint_and_sugg(
+            cx,
+            EXPECT_FUN_CALL,
+            span_replace_word,
+            format!("function call inside of `{name}`"),
+            "try",
+            format!("unwrap_or_else({closure_args} panic!(\"{{}}\", {arg_root_snippet}))"),
+            applicability,
+        );
+    }
 }
diff --git a/tests/ui/expect_fun_call.fixed b/tests/ui/expect_fun_call.fixed
@@ -90,17 +90,23 @@ fn main() {
             "foo"
         }
 
-        Some("foo").unwrap_or_else(|| { panic!("{}", get_string()) });
+        const fn const_evaluable() -> &'static str {
+            "foo"
+        }
+
+        Some("foo").unwrap_or_else(|| panic!("{}", get_string()));
         //~^ expect_fun_call
-        Some("foo").unwrap_or_else(|| { panic!("{}", get_string()) });
+        Some("foo").unwrap_or_else(|| panic!("{}", get_string()));
         //~^ expect_fun_call
-        Some("foo").unwrap_or_else(|| { panic!("{}", get_string()) });
+        Some("foo").unwrap_or_else(|| panic!("{}", get_string()));
         //~^ expect_fun_call
 
-        Some("foo").unwrap_or_else(|| { panic!("{}", get_static_str()) });
+        Some("foo").unwrap_or_else(|| panic!("{}", get_static_str()));
         //~^ expect_fun_call
-        Some("foo").unwrap_or_else(|| { panic!("{}", get_non_static_str(&0).to_string()) });
+        Some("foo").unwrap_or_else(|| panic!("{}", get_non_static_str(&0)));
         //~^ expect_fun_call
+
+        Some("foo").expect(const_evaluable());
     }
 
     //Issue #3839
@@ -122,4 +128,15 @@ fn main() {
     let format_capture_and_value: Option<i32> = None;
     format_capture_and_value.unwrap_or_else(|| panic!("{error_code}, {}", 1));
     //~^ expect_fun_call
+
+    // Issue #15056
+    let a = false;
+    Some(5).expect(if a { "a" } else { "b" });
+
+    let return_in_expect: Option<i32> = None;
+    return_in_expect.expect(if true {
+        "Error"
+    } else {
+        return;
+    });
 }
diff --git a/tests/ui/expect_fun_call.rs b/tests/ui/expect_fun_call.rs
@@ -90,6 +90,10 @@ fn main() {
             "foo"
         }
 
+        const fn const_evaluable() -> &'static str {
+            "foo"
+        }
+
         Some("foo").expect(&get_string());
         //~^ expect_fun_call
         Some("foo").expect(get_string().as_ref());
@@ -101,6 +105,8 @@ fn main() {
         //~^ expect_fun_call
         Some("foo").expect(get_non_static_str(&0));
         //~^ expect_fun_call
+
+        Some("foo").expect(const_evaluable());
     }
 
     //Issue #3839
@@ -122,4 +128,15 @@ fn main() {
     let format_capture_and_value: Option<i32> = None;
     format_capture_and_value.expect(&format!("{error_code}, {}", 1));
     //~^ expect_fun_call
+
+    // Issue #15056
+    let a = false;
+    Some(5).expect(if a { "a" } else { "b" });
+
+    let return_in_expect: Option<i32> = None;
+    return_in_expect.expect(if true {
+        "Error"
+    } else {
+        return;
+    });
 }
diff --git a/tests/ui/expect_fun_call.stderr b/tests/ui/expect_fun_call.stderr
@@ -38,55 +38,55 @@ LL |     Some("foo").expect(format!("{} {}", 1, 2).as_ref());
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{} {}", 1, 2))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:93:21
+  --> tests/ui/expect_fun_call.rs:97:21
    |
 LL |         Some("foo").expect(&get_string());
-   |                     ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
+   |                     ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:95:21
+  --> tests/ui/expect_fun_call.rs:99:21
    |
 LL |         Some("foo").expect(get_string().as_ref());
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:97:21
+  --> tests/ui/expect_fun_call.rs:101:21
    |
 LL |         Some("foo").expect(get_string().as_str());
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:100:21
+  --> tests/ui/expect_fun_call.rs:104:21
    |
 LL |         Some("foo").expect(get_static_str());
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_static_str()) })`
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_static_str()))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:102:21
+  --> tests/ui/expect_fun_call.rs:106:21
    |
 LL |         Some("foo").expect(get_non_static_str(&0));
-   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_non_static_str(&0).to_string()) })`
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_non_static_str(&0)))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:107:16
+  --> tests/ui/expect_fun_call.rs:113:16
    |
 LL |     Some(true).expect(&format!("key {}, {}", 1, 2));
    |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("key {}, {}", 1, 2))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:114:17
+  --> tests/ui/expect_fun_call.rs:120:17
    |
 LL |         opt_ref.expect(&format!("{:?}", opt_ref));
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{:?}", opt_ref))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:119:20
+  --> tests/ui/expect_fun_call.rs:125:20
    |
 LL |     format_capture.expect(&format!("{error_code}"));
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{error_code}"))`
 
 error: function call inside of `expect`
-  --> tests/ui/expect_fun_call.rs:123:30
+  --> tests/ui/expect_fun_call.rs:129:30
    |
 LL |     format_capture_and_value.expect(&format!("{error_code}, {}", 1));
    |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{error_code}, {}", 1))`
