Skip to content

Commit 8d6de0b

Browse files
samueltardieusamueltardieu
authored
Fix expect_fun_call producing invalid suggestions (#15122)
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
2 parents 1c64211 + fcd064d commit 8d6de0b

File tree

4 files changed

+135
-135
lines changed

4 files changed

+135
-135
lines changed

‎clippy_lints/src/methods/expect_fun_call.rs

Lines changed: 61 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,15 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
22
use clippy_utils::macros::{FormatArgsStorage, format_args_inputs_span, root_macro_call_first_node};
33
use clippy_utils::source::snippet_with_applicability;
44
use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item};
5+
use clippy_utils::visitors::for_each_expr;
6+
use clippy_utils::{contains_return, is_inside_always_const_context, peel_blocks};
57
use rustc_errors::Applicability;
68
use rustc_hir as hir;
79
use rustc_lint::LateContext;
8-
use rustc_middle::ty;
910
use rustc_span::symbol::sym;
1011
use rustc_span::{Span, Symbol};
1112
use std::borrow::Cow;
13+
use std::ops::ControlFlow;
1214

1315
use super::EXPECT_FUN_CALL;
1416

@@ -23,10 +25,10 @@ pub(super) fn check<'tcx>(
2325
receiver: &'tcx hir::Expr<'tcx>,
2426
args: &'tcx [hir::Expr<'tcx>],
2527
) {
26-
// Strip `&`, `as_ref()` and `as_str()` off `arg` until we're left with either a `String` or
28+
// Strip `{}`, `&`, `as_ref()` and `as_str()` off `arg` until we're left with either a `String` or
2729
// `&str`
2830
fn get_arg_root<'a>(cx: &LateContext<'_>, arg: &'a hir::Expr<'a>) -> &'a hir::Expr<'a> {
29-
let mut arg_root = arg;
31+
let mut arg_root = peel_blocks(arg);
3032
loop {
3133
arg_root = match &arg_root.kind {
3234
hir::ExprKind::AddrOf(hir::BorrowKind::Ref, _, expr) => expr,
@@ -47,124 +49,68 @@ pub(super) fn check<'tcx>(
4749
arg_root
4850
}
4951

50-
// Only `&'static str` or `String` can be used directly in the `panic!`. Other types should be
51-
// converted to string.
52-
fn requires_to_string(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool {
53-
let arg_ty = cx.typeck_results().expr_ty(arg);
54-
if is_type_lang_item(cx, arg_ty, hir::LangItem::String) {
55-
return false;
56-
}
57-
if let ty::Ref(_, ty, ..) = arg_ty.kind()
58-
&& ty.is_str()
59-
&& can_be_static_str(cx, arg)
60-
{
61-
return false;
62-
}
63-
true
52+
fn contains_call<'a>(cx: &LateContext<'a>, arg: &'a hir::Expr<'a>) -> bool {
53+
for_each_expr(cx, arg, |expr| {
54+
if matches!(expr.kind, hir::ExprKind::MethodCall { .. } | hir::ExprKind::Call { .. })
55+
&& !is_inside_always_const_context(cx.tcx, expr.hir_id)
56+
{
57+
ControlFlow::Break(())
58+
} else {
59+
ControlFlow::Continue(())
60+
}
61+
})
62+
.is_some()
6463
}
6564

66-
// Check if an expression could have type `&'static str`, knowing that it
67-
// has type `&str` for some lifetime.
68-
fn can_be_static_str(cx: &LateContext<'_>, arg: &hir::Expr<'_>) -> bool {
69-
match arg.kind {
70-
hir::ExprKind::Lit(_) => true,
71-
hir::ExprKind::Call(fun, _) => {
72-
if let hir::ExprKind::Path(ref p) = fun.kind {
73-
match cx.qpath_res(p, fun.hir_id) {
74-
hir::def::Res::Def(hir::def::DefKind::Fn | hir::def::DefKind::AssocFn, def_id) => matches!(
75-
cx.tcx.fn_sig(def_id).instantiate_identity().output().skip_binder().kind(),
76-
ty::Ref(re, ..) if re.is_static(),
77-
),
78-
_ => false,
79-
}
80-
} else {
81-
false
82-
}
83-
},
84-
hir::ExprKind::MethodCall(..) => {
85-
cx.typeck_results()
86-
.type_dependent_def_id(arg.hir_id)
87-
.is_some_and(|method_id| {
88-
matches!(
89-
cx.tcx.fn_sig(method_id).instantiate_identity().output().skip_binder().kind(),
90-
ty::Ref(re, ..) if re.is_static()
91-
)
92-
})
93-
},
94-
hir::ExprKind::Path(ref p) => matches!(
95-
cx.qpath_res(p, arg.hir_id),
96-
hir::def::Res::Def(hir::def::DefKind::Const | hir::def::DefKind::Static { .. }, _)
97-
),
98-
_ => false,
99-
}
100-
}
65+
if name == sym::expect
66+
&& let [arg] = args
67+
&& let arg_root = get_arg_root(cx, arg)
68+
&& contains_call(cx, arg_root)
69+
&& !contains_return(arg_root)
70+
{
71+
let receiver_type = cx.typeck_results().expr_ty_adjusted(receiver);
72+
let closure_args = if is_type_diagnostic_item(cx, receiver_type, sym::Option) {
73+
"||"
74+
} else if is_type_diagnostic_item(cx, receiver_type, sym::Result) {
75+
"|_|"
76+
} else {
77+
return;
78+
};
10179

102-
fn is_call(node: &hir::ExprKind<'_>) -> bool {
103-
match node {
104-
hir::ExprKind::AddrOf(hir::BorrowKind::Ref, _, expr) => {
105-
is_call(&expr.kind)
106-
},
107-
hir::ExprKind::Call(..)
108-
| hir::ExprKind::MethodCall(..)
109-
// These variants are debatable or require further examination
110-
| hir::ExprKind::If(..)
111-
| hir::ExprKind::Match(..)
112-
| hir::ExprKind::Block{ .. } => true,
113-
_ => false,
114-
}
115-
}
80+
let span_replace_word = method_span.with_hi(expr.span.hi());
11681

117-
if args.len() != 1 || name != sym::expect || !is_call(&args[0].kind) {
118-
return;
119-
}
82+
let mut applicability = Applicability::MachineApplicable;
12083

121-
let receiver_type = cx.typeck_results().expr_ty_adjusted(receiver);
122-
let closure_args = if is_type_diagnostic_item(cx, receiver_type, sym::Option) {
123-
"||"
124-
} else if is_type_diagnostic_item(cx, receiver_type, sym::Result) {
125-
"|_|"
126-
} else {
127-
return;
128-
};
129-
130-
let arg_root = get_arg_root(cx, &args[0]);
131-
132-
let span_replace_word = method_span.with_hi(expr.span.hi());
133-
134-
let mut applicability = Applicability::MachineApplicable;
135-
136-
// Special handling for `format!` as arg_root
137-
if let Some(macro_call) = root_macro_call_first_node(cx, arg_root) {
138-
if cx.tcx.is_diagnostic_item(sym::format_macro, macro_call.def_id)
139-
&& let Some(format_args) = format_args_storage.get(cx, arg_root, macro_call.expn)
140-
{
141-
let span = format_args_inputs_span(format_args);
142-
let sugg = snippet_with_applicability(cx, span, "..", &mut applicability);
143-
span_lint_and_sugg(
144-
cx,
145-
EXPECT_FUN_CALL,
146-
span_replace_word,
147-
format!("function call inside of `{name}`"),
148-
"try",
149-
format!("unwrap_or_else({closure_args} panic!({sugg}))"),
150-
applicability,
151-
);
84+
// Special handling for `format!` as arg_root
85+
if let Some(macro_call) = root_macro_call_first_node(cx, arg_root) {
86+
if cx.tcx.is_diagnostic_item(sym::format_macro, macro_call.def_id)
87+
&& let Some(format_args) = format_args_storage.get(cx, arg_root, macro_call.expn)
88+
{
89+
let span = format_args_inputs_span(format_args);
90+
let sugg = snippet_with_applicability(cx, span, "..", &mut applicability);
91+
span_lint_and_sugg(
92+
cx,
93+
EXPECT_FUN_CALL,
94+
span_replace_word,
95+
format!("function call inside of `{name}`"),
96+
"try",
97+
format!("unwrap_or_else({closure_args} panic!({sugg}))"),
98+
applicability,
99+
);
100+
}
101+
return;
152102
}
153-
return;
154-
}
155103

156-
let mut arg_root_snippet: Cow<'_, _> = snippet_with_applicability(cx, arg_root.span, "..", &mut applicability);
157-
if requires_to_string(cx, arg_root) {
158-
arg_root_snippet.to_mut().push_str(".to_string()");
159-
}
104+
let arg_root_snippet: Cow<'_, _> = snippet_with_applicability(cx, arg_root.span, "..", &mut applicability);
160105

161-
span_lint_and_sugg(
162-
cx,
163-
EXPECT_FUN_CALL,
164-
span_replace_word,
165-
format!("function call inside of `{name}`"),
166-
"try",
167-
format!("unwrap_or_else({closure_args} {{ panic!(\"{{}}\", {arg_root_snippet}) }})"),
168-
applicability,
169-
);
106+
span_lint_and_sugg(
107+
cx,
108+
EXPECT_FUN_CALL,
109+
span_replace_word,
110+
format!("function call inside of `{name}`"),
111+
"try",
112+
format!("unwrap_or_else({closure_args} panic!(\"{{}}\", {arg_root_snippet}))"),
113+
applicability,
114+
);
115+
}
170116
}

‎tests/ui/expect_fun_call.fixed

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -90,17 +90,30 @@ fn main() {
9090
"foo"
9191
}
9292

93-
Some("foo").unwrap_or_else(|| { panic!("{}", get_string()) });
93+
const fn const_evaluable() -> &'static str {
94+
"foo"
95+
}
96+
97+
Some("foo").unwrap_or_else(|| panic!("{}", get_string()));
9498
//~^ expect_fun_call
95-
Some("foo").unwrap_or_else(|| { panic!("{}", get_string()) });
99+
Some("foo").unwrap_or_else(|| panic!("{}", get_string()));
96100
//~^ expect_fun_call
97-
Some("foo").unwrap_or_else(|| { panic!("{}", get_string()) });
101+
Some("foo").unwrap_or_else(|| panic!("{}", get_string()));
98102
//~^ expect_fun_call
99103

100-
Some("foo").unwrap_or_else(|| { panic!("{}", get_static_str()) });
104+
Some("foo").unwrap_or_else(|| panic!("{}", get_static_str()));
101105
//~^ expect_fun_call
102-
Some("foo").unwrap_or_else(|| { panic!("{}", get_non_static_str(&0).to_string()) });
106+
Some("foo").unwrap_or_else(|| panic!("{}", get_non_static_str(&0)));
107+
//~^ expect_fun_call
108+
109+
Some("foo").unwrap_or_else(|| panic!("{}", const_evaluable()));
103110
//~^ expect_fun_call
111+
112+
const {
113+
Some("foo").expect(const_evaluable());
114+
}
115+
116+
Some("foo").expect(const { const_evaluable() });
104117
}
105118

106119
//Issue #3839
@@ -122,4 +135,15 @@ fn main() {
122135
let format_capture_and_value: Option<i32> = None;
123136
format_capture_and_value.unwrap_or_else(|| panic!("{error_code}, {}", 1));
124137
//~^ expect_fun_call
138+
139+
// Issue #15056
140+
let a = false;
141+
Some(5).expect(if a { "a" } else { "b" });
142+
143+
let return_in_expect: Option<i32> = None;
144+
return_in_expect.expect(if true {
145+
"Error"
146+
} else {
147+
return;
148+
});
125149
}

‎tests/ui/expect_fun_call.rs

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,10 @@ fn main() {
9090
"foo"
9191
}
9292

93+
const fn const_evaluable() -> &'static str {
94+
"foo"
95+
}
96+
9397
Some("foo").expect(&get_string());
9498
//~^ expect_fun_call
9599
Some("foo").expect(get_string().as_ref());
@@ -101,6 +105,15 @@ fn main() {
101105
//~^ expect_fun_call
102106
Some("foo").expect(get_non_static_str(&0));
103107
//~^ expect_fun_call
108+
109+
Some("foo").expect(const_evaluable());
110+
//~^ expect_fun_call
111+
112+
const {
113+
Some("foo").expect(const_evaluable());
114+
}
115+
116+
Some("foo").expect(const { const_evaluable() });
104117
}
105118

106119
//Issue #3839
@@ -122,4 +135,15 @@ fn main() {
122135
let format_capture_and_value: Option<i32> = None;
123136
format_capture_and_value.expect(&format!("{error_code}, {}", 1));
124137
//~^ expect_fun_call
138+
139+
// Issue #15056
140+
let a = false;
141+
Some(5).expect(if a { "a" } else { "b" });
142+
143+
let return_in_expect: Option<i32> = None;
144+
return_in_expect.expect(if true {
145+
"Error"
146+
} else {
147+
return;
148+
});
125149
}

‎tests/ui/expect_fun_call.stderr

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -38,58 +38,64 @@ LL | Some("foo").expect(format!("{} {}", 1, 2).as_ref());
3838
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{} {}", 1, 2))`
3939

4040
error: function call inside of `expect`
41-
--> tests/ui/expect_fun_call.rs:93:21
41+
--> tests/ui/expect_fun_call.rs:97:21
4242
|
4343
LL | Some("foo").expect(&get_string());
44-
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
44+
| ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
4545

4646
error: function call inside of `expect`
47-
--> tests/ui/expect_fun_call.rs:95:21
47+
--> tests/ui/expect_fun_call.rs:99:21
4848
|
4949
LL | Some("foo").expect(get_string().as_ref());
50-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
50+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
5151

5252
error: function call inside of `expect`
53-
--> tests/ui/expect_fun_call.rs:97:21
53+
--> tests/ui/expect_fun_call.rs:101:21
5454
|
5555
LL | Some("foo").expect(get_string().as_str());
56-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_string()) })`
56+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_string()))`
5757

5858
error: function call inside of `expect`
59-
--> tests/ui/expect_fun_call.rs:100:21
59+
--> tests/ui/expect_fun_call.rs:104:21
6060
|
6161
LL | Some("foo").expect(get_static_str());
62-
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_static_str()) })`
62+
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_static_str()))`
6363

6464
error: function call inside of `expect`
65-
--> tests/ui/expect_fun_call.rs:102:21
65+
--> tests/ui/expect_fun_call.rs:106:21
6666
|
6767
LL | Some("foo").expect(get_non_static_str(&0));
68-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| { panic!("{}", get_non_static_str(&0).to_string()) })`
68+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", get_non_static_str(&0)))`
69+
70+
error: function call inside of `expect`
71+
--> tests/ui/expect_fun_call.rs:109:21
72+
|
73+
LL | Some("foo").expect(const_evaluable());
74+
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", const_evaluable()))`
6975

7076
error: function call inside of `expect`
71-
--> tests/ui/expect_fun_call.rs:107:16
77+
--> tests/ui/expect_fun_call.rs:120:16
7278
|
7379
LL | Some(true).expect(&format!("key {}, {}", 1, 2));
7480
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("key {}, {}", 1, 2))`
7581

7682
error: function call inside of `expect`
77-
--> tests/ui/expect_fun_call.rs:114:17
83+
--> tests/ui/expect_fun_call.rs:127:17
7884
|
7985
LL | opt_ref.expect(&format!("{:?}", opt_ref));
8086
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{:?}", opt_ref))`
8187

8288
error: function call inside of `expect`
83-
--> tests/ui/expect_fun_call.rs:119:20
89+
--> tests/ui/expect_fun_call.rs:132:20
8490
|
8591
LL | format_capture.expect(&format!("{error_code}"));
8692
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{error_code}"))`
8793

8894
error: function call inside of `expect`
89-
--> tests/ui/expect_fun_call.rs:123:30
95+
--> tests/ui/expect_fun_call.rs:136:30
9096
|
9197
LL | format_capture_and_value.expect(&format!("{error_code}, {}", 1));
9298
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{error_code}, {}", 1))`
9399

94-
error: aborting due to 15 previous errors
100+
error: aborting due to 16 previous errors
95101

0 commit comments

Comments
 (0)