Skip to content

Fix expect_fun_call producing invalid suggestions #15122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+102 −136
Open

Fix expect_fun_call producing invalid suggestions #15122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 49 additions & 117 deletions clippy_lints/src/methods/expect_fun_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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,
Expand All @@ -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,
);
}
}
27 changes: 22 additions & 5 deletions tests/ui/expect_fun_call.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
});
}
17 changes: 17 additions & 0 deletions tests/ui/expect_fun_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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
Expand All @@ -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;
});
}
28 changes: 14 additions & 14 deletions tests/ui/expect_fun_call.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))`
Expand Down
Loading