user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix expect_fun_call producing invalid suggestions (#15122)

Browse source 
Previously expect_fun_call would too eagerly convert cases like
`foo.expect(if | block | match)` into `foo.unwrap_or_else`.
Additionally, it would also add to_string() even though the argument is
being passed into format!() which can accept a &str. I also discovered
some other cases where this lint would either produce invalid results,
or be triggered unnecessarily:

- Clippy would suggest changing expect to unwrap_or_else even if the
expression inside expect contains a return statement
- opt.expect(const_fn()) no longer triggers the lint
- The lint would always add braces to the closure body, even if the body
of expect is a single expression
- opt.expect({"literal"}) used to get turned into
```opt.unwrap_or_else(|| panic!("{}", {"literal"}.to_string()))```

Fixes rust-lang/rust-clippy#15056

changelog: [`expect_fun_call`]: fix expect_fun_call producing invalid
suggestions
This commit is contained in:
Samuel Tardieu 2025-07-14 15:05:59 +00:00 • committed by GitHub
commit 8d6de0b82e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 149 additions and 149 deletions
Download patch file Download diff file Expand all files Collapse all files

188
clippy_lints/src/methods/expect_fun_call.rs
View file

@ -2,13 +2,15 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
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::visitors::for_each_expr;
use clippy_utils::{contains_return, is_inside_always_const_context, 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;
use std::ops::ControlFlow;
use super::EXPECT_FUN_CALL;
@ -23,10 +25,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 +49,68 @@ 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;
fn contains_call<'a>(cx: &LateContext<'a>, arg: &'a hir::Expr<'a>) -> bool {
for_each_expr(cx, arg, |expr| {
if matches!(expr.kind, hir::ExprKind::MethodCall { .. } | hir::ExprKind::Call { .. })
&& !is_inside_always_const_context(cx.tcx, expr.hir_id)
{
ControlFlow::Break(())
} else {
ControlFlow::Continue(())
}
})
.is_some()
}
if name == sym::expect
&& let [arg] = args
&& let arg_root = get_arg_root(cx, arg)
&& contains_call(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;
};
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,
);
}
return;
}
if let ty::Ref(_, ty, ..) = arg_ty.kind()
&& ty.is_str()
&& can_be_static_str(cx, arg)
{
return false;
}
true
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,
);
}
// 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,
}
}
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,
}
}
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,
);
}
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()");
}
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,
);
}

34
tests/ui/expect_fun_call.fixed
View file

@ -90,17 +90,30 @@ 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").unwrap_or_else(|| panic!("{}", const_evaluable()));
//~^ expect_fun_call
const {
Some("foo").expect(const_evaluable());
}
Some("foo").expect(const { const_evaluable() });
}
//Issue #3839
@ -122,4 +135,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;
});
}

24
tests/ui/expect_fun_call.rs
View file

@ -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,15 @@ fn main() {
//~^ expect_fun_call
Some("foo").expect(get_non_static_str(&0));
//~^ expect_fun_call
Some("foo").expect(const_evaluable());
//~^ expect_fun_call
const {
Some("foo").expect(const_evaluable());
}
Some("foo").expect(const { const_evaluable() });
}
//Issue #3839
@ -122,4 +135,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;
});
}

52
tests/ui/expect_fun_call.stderr
View file

@ -37,59 +37,65 @@ error: function call inside of `expect`
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
|
LL | Some("foo").expect(&get_string());
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
error: function call inside of `expect`
--> tests/ui/expect_fun_call.rs:95:21
|
LL | Some("foo").expect(get_string().as_ref());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
error: function call inside of `expect`
--> tests/ui/expect_fun_call.rs:97:21
|
LL | Some("foo").expect(get_string().as_str());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
LL | Some("foo").expect(&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:99:21
|
LL | Some("foo").expect(get_string().as_ref());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
error: function call inside of `expect`
--> tests/ui/expect_fun_call.rs:101:21
|
LL | Some("foo").expect(get_string().as_str());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
error: function call inside of `expect`
--> 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:109:21
|
LL | Some("foo").expect(const_evaluable());
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", const_evaluable()))`
error: function call inside of `expect`
--> tests/ui/expect_fun_call.rs:120: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:127: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:132: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:136:30
|
LL | format_capture_and_value.expect(&format!("{error_code}, {}", 1));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{error_code}, {}", 1))`
error: aborting due to 15 previous errors
error: aborting due to 16 previous errors