Fix expect_fun_call producing invalid suggestions

[sethdusek]

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 #15056

changelog: [expect_fun_call]: fix expect_fun_call producing invalid suggestions

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

* opt.expect(const_fn()) no longer triggers the lint

What's the rationale behind this? const_fn() is not evaluated at compile time here.

[samueltardieu]

Note: this doesn't fix #15195 (nor does it pretend to)

[sethdusek]

* opt.expect(const_fn()) no longer triggers the lint

What's the rationale behind this? const_fn() is not evaluated at compile time here.

* opt.expect(const_fn()) no longer triggers the lint

What's the rationale behind this? const_fn() is not evaluated at compile time here.

This behavior is inherited from switch_to_lazy_eval() which is also used by the or_fun_call lint, and opt.unwrap_or(const_fn()) doesn't trigger that lint either. But this is definitely a bug since const_call would still get evaluated at runtime:

#![warn(clippy::or_fun_call)]
fn call() -> i32 {
    0
}
const fn const_call() -> i32 {
    0
}
fn opt() -> Option<i32> {
    None
}
fn main() {
    let a: Option<i32> = opt();
    a.unwrap_or(call()); // triggers the lint
    a.unwrap_or(const_call()); // doesn't trigger
}

Playground link

Should I try to address this in this same PR or handle it separately?

[samueltardieu]

Note: this doesn't fix #15195 (nor does it pretend to)

This PR turns the snippet into:

error: function call inside of `expect`
 --> /tmp/foo.rs:6:11
  |
6 |     _ = x.expect(std::convert::identity(s));
  |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_else(|| panic!("{}", std::convert::identity(s)))`

which compiles

As far as I can see while testing, it doesn't allow s to be typed anymore, even with this PR.

[samueltardieu]

Should I try to address this in this same PR or handle it separately?

A separate PR (prerequisite for this one) would be preferable as to evaluate the effects on other callers of the eagerness evaluator.

[samueltardieu]

After the experiment in #15211 we have to choose whether we want to lint on const fn calls or not:

• on the one hand, the lint name suggests that one should not use function calls in expect()
• on the other hand, the lint is in the perf category, which is supposed to only give performance increase suggestions

Note that if the call to the const fn function was in a const block, it would not have any performance impact.

Would you be wanting to try looking for function or method calls which are made outside an always-const context (see clippy_utils::is_inside_always_const_context()) instead of looking for expression eagerness? That may give better results.

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[sethdusek]

@rustbot ready

[samueltardieu]

Thanks