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

match_wild_err_arm: Err(_) is not wild #3688

Closed
tspiteri opened this issue Jan 23, 2019 · 6 comments · Fixed by #5622
Closed

match_wild_err_arm: Err(_) is not wild #3688

tspiteri opened this issue Jan 23, 2019 · 6 comments · Fixed by #5622
Assignees
Labels
A-documentation Area: Adding or improving documentation C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages good-first-issue These issues are a good way to get started with Clippy

Comments

@tspiteri
Copy link
Contributor

The match_wild_err_arm lint documentation compares Err(_) to Java with catch(Exception), but it is nothing like it. While the Java version does catch all exceptions, the Rust version can only catch one error type.

Even the warning message provided is misguided. It recommends matching each error separately, but there is only one error to match.

This makes this lint not really helpful. Or am I missing something?

@ghost
Copy link

ghost commented Jan 24, 2019

I don't get it either. Based on the logic in this lint, we should also warn on expect and unwrap calls since they do the same thing.

@flip1995
Copy link
Member

That Result<T, E> can only have one error type is true. But the error type can be an enum. For example

enum Error {
    InternalError,
    WrongArguments,
    FileNotFound,
    ...,
}

fn main() {
    match run_my_program() {
        Ok(_) => println!("yay"),
        Err(_) => panic!("nay"),
    }
}

Would result in the error message "nay" no matter what the error was. If you have only one error Type you can handle this by an exhaustive match:

match run_my_program() {
    Ok(_) => println!("yay"),
    Err(Error::InternalError) => panic!("oops"),
    Err(Error::WrongArguments) => panic!("{}", help),
    Err(Error::FileNotFound) => panic!("File {} does not exist", file),
    ...
}

This is better, because it gives a custom error message dependent on the type of the error. Panicking on a known error is still probably something you don't want to do and you may want to handle errors with crates like failure or similar.


That said, the documentation should be more specific about what the lint wants from the programmer.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy A-documentation Area: Adding or improving documentation labels Jan 24, 2019
@flip1995
Copy link
Member

we should also warn on expect and unwrap calls

IMHO this isn't quite the same. If you write expect or unwrap somewhere you are (should be) sure, that the call never panics. If you write a match over Ok and Err you expect that something can go wrong and want to deal with this. But then you can do it right and be more specific in the error messages.

Maybe this lint can mention something like:
"If you're sure that this doesn't error, use .expect(msg)"


On expect vs unwrap see #1921.

@flip1995 flip1995 added the C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages label Jan 24, 2019
@tspiteri
Copy link
Contributor Author

I can see that when the error is an enum it can be helpful. If that is the case, there are some false positives. For example

enum Radix {
    Digits(i32),
    DigitsLetters(i32),
    AltDigits(i32),
    AltDigitsLetters(i32),
}
fn classify(radix: i32) -> Result<Radix, i32> {
    match radix {
        2..=10 => Ok(Radix::Digits(radix)),
        11..=36 => Ok(Radix::DigitsLetters(radix)),
        -10..=-2 => Ok(Radix::AltDigits(-radix)),
        -36..=-11 => Ok(Radix::AltDigitsLetters(-radix)),
        _ => Err(radix),
    }
}
// Panics if magnitude of radix < 2 or > 36
fn foo(n: u32, radix: i32) {
    match classify(radix) {
        Ok(Radix::Digits(radix)) => print_digits(n, radix),
        Ok(Radix::DigitsLetters(radix)) => print_digits_letters(n, radix),
        Ok(Radix::AltDigits(radix)) => alt_print_digits(n, radix),
        Ok(Radix::AltDigitsLetters(radix)) => alt_print_digits_letters(n, radix),
        Err(_) => panic!("wrong argument"),
    }
}

In this case I'd say the warning is a false positive.

So the only time the warning is helpful is when the error type is an enum. Why not warn only in that case then? It would make it easier to give a helpful message too, because then it would make sense to ask for matching each error variant separately.

@yaahc yaahc self-assigned this Dec 19, 2019
@Manishearth
Copy link
Member

Actually, I'd go further and say that this lint should be pedantic, in addition to making it only warn for public enums.

hawkw added a commit to tokio-rs/tracing that referenced this issue Mar 13, 2020
## Motivation

Looks like Clippy is once again complaining about the
`match_wild_err_arm` lint, presumably as a result of the Rust 1.42
release. This lint triggers on the `try_lock!` macro that
`tracing-subscriber` uses to avoid double panics when a mutex is
poisoned. In this case, the lint is something of a false positive here,
since we _do_ actually have two different `Err(...)` arms; the
differentiation between the two arms is not in the match pattern but in
a guard. See rust-lang/rust-clippy#3688 for
details on the lint.

## Solution

I've refactored the code in question to use `if`/`else`, avoiding the
lint.

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@elichai
Copy link
Contributor

elichai commented May 20, 2020

Agreed, this lint just failed my CI(https://travis-ci.org/github/elichai/stdio-override/jobs/689175682) and the code there is reasonable, I don't always want to use expect for various esthetic reasons.
and as people said before me Java's catch is really different, because the exception could come from anywhere and new types of exceptions can be added in the future.
in Rust the "exceptions"(aka Errors) are safely typed. so if a function returns for example Result<T, ()> there's nothing smart to "match on" and no new errors can be added without a breaking change (the same is also for error structs, or non-exhuastive error enums)

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 good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants