Skip to content
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.

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

expect_fun_call recommends code which is not equivalent #2928

Open
sgrif opened this issue Jul 17, 2018 · 2 comments
Open

expect_fun_call recommends code which is not equivalent #2928

sgrif opened this issue Jul 17, 2018 · 2 comments
Labels
A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@sgrif
Copy link

sgrif commented Jul 17, 2018

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
Copy link

pravic commented Dec 3, 2020

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));

@flip1995 flip1995 added L-suggestion Lint: Improving, adding or fixing lint suggestions A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages labels Dec 3, 2020
@chrisduerr
Copy link
Contributor

chrisduerr commented Jan 16, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

No branches or pull requests

4 participants