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

single_match: recongnize "Ok(_) => {}" or "Err(_bla) => {}" as "_ => {}" #2567

Open
matthiaskrgr opened this issue Mar 24, 2018 · 3 comments
Labels
I-false-negative Issue: The lint should have been triggered on code, but wasn't

Comments

@matthiaskrgr
Copy link
Member

matthiaskrgr commented Mar 24, 2018

If we match a Result<x,y> , and we see that one of the results is ignored via
Ok(_) => {} or Err(_bla) => {}, can we somehow simplify this to _ => {} ?
The idea is that since we know that Result will either return Ok or Error, no other values are possible, which is why I think

match x {
Ok(_) => {},
Err(a) => { do_something(a) },
}

is equivalent to

match x {
Err(a) => { do_something(a) },
_ => {},
}

however, the second example will trigger single_match lint while the first one will not.

code sample:

fn main() {
    test1();
    test2();
    test3();
    test4();
}

fn test1() {
    // false negative
    match is_ok(3) {
        Ok(_a) => {}
        Err(bad) => {
            println!("very bad: {}", bad);
        }
    }
}

fn test2() {
    // false negative
    match is_ok(3) {
        Ok(_) => {}
        Err(bad) => {
            println!("very bad: {}", bad);
        }
    }
}

fn test3() {
    // true positive
    match is_ok(3) {
        Err(bad) => {
            println!("very bad: {}", bad);
        }
        _ => {}
    }
}

fn test4() {
    // true positive
    match is_ok(3) {
        Ok(good) => {
            println!("very good: {}", good);
        }
        _ => {}
    }
}

fn is_ok(x: i32) -> Result<bool, String> {
    if x > 1 {
        Ok(true)
    } else {
        Err("Oh no".to_string())
    }
}

Does this make sense?

@lambda-fairy
Copy link

lambda-fairy commented Mar 27, 2018

This makes sense for Result, whose definition is guaranteed not to change. But it might not be an improvement for other enums.

When a new variant is added to an enum, the explicit match will fail to compile, whereas the catch-all match will pass silently. This can hide mistakes where you've forgotten to handle the new variant.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 27, 2018

I personally like Err(_) over _, because changes to the Ok pattern in the future will cause compiler errors

@clarfonthey
Copy link
Contributor

For the same reason as @oli-obk I actually would prefer the reverse lint; specifically _ to Ok(_) or Err(_).

@matthiaskrgr matthiaskrgr added the I-false-negative Issue: The lint should have been triggered on code, but wasn't label Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-false-negative Issue: The lint should have been triggered on code, but wasn't
Projects
None yet
Development

No branches or pull requests

4 participants