rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.5k stars 1.55k forks source link

`expect_fun_call` recommends code which is not equivalent #2928

Open sgrif opened 6 years ago

sgrif commented 6 years ago

Clippy will suggest replacing:

Err::<(), _>("foo").expect(&format!("bar"))

with

Err:<(), _>("foo").unwrap_or_else(|_| panic!("bar"))

However, these are not equivalent. The first outputs foo: "bar", while the second will only output bar. Clippy could recommend doing the same formatting as expect, now we're literally recommending replacing expect with the exact body of that function. While it does of course avoid a single string allocation, it seems like a very aggressive lint to have on by default to avoid a case that is insignificant performance-wise for the vast majority of programs. format! should be white listed here IMO

pravic commented 3 years ago

Also, now it recommends the example for Option::unwrap_or_else() however Result::unwrap_or_else() takes an additional parameter.

So, the docs should mention the distinction. Also they should give the full equivalent for the expect which

Panics if the value is an Err, with a panic message including the passed message, and the content of the Err.

let bar = "bar";
foo.expect(&format!("expected {}", bar));
foo.unwrap_or_else(|e| panic!("expected {}: {}", bar, e));
chrisduerr commented 2 years ago

I've just found this lint while updating some things to the Rust 2021 edition and have to say that while I generally agree with almost all Clippy lints, this one does not seem fit to be a standard lint to me.

Even when changing the formatting to get back the expect output, the code will then still be significantly more complex to read. All for the value of optimizing a format! call which is something I'm very likely to just accept for the sake of nicely handling the error.

I also have many projects where I basically have no panic! anywhere, in that scenario searching for expect or unwrap() to validate error handling is relatively easy. Having an unwrap_or_else just run straight into a panic makes it significantly more difficult to identify at a glance what this code does (imo).

I'd suggest making this an opt-in lint.