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

Perhaps this match_bool case could be considered idiomatic? #3713

Closed
swfsql opened this issue Jan 28, 2019 · 3 comments
Closed

Perhaps this match_bool case could be considered idiomatic? #3713

swfsql opened this issue Jan 28, 2019 · 3 comments

Comments

@swfsql
Copy link

swfsql commented Jan 28, 2019

Hello, I have a suggestion for the match_bool lint
https://rust-lang.github.io/rust-clippy/master/index.html#match_bool

I'm not sure if this is idiomatic, buy I often attach (@) matched values into variables, eg.:

match some_hash_set.insert(some_key) {
        _previously_unset @ true => (),
        _previously_unset @ false => {
                 panic!("D'oh!")
        }
}

I actually prefer reading this interface like this, because I can kind of simultaneously see both branches (the "total" effect of each branch) and I kind of know beforehand that such value (_previously_unset) won't be used anywhere else, for anything else.


The "literal" translation for the if-else case would be:

{ // inner scope
        let previously_unset = some_hash_set.insert(some_key);
        if (!previously_unset) {
                panic!("D'oh!")
        }
}

But in this case, to see the "total" effect of previously_unset @ true, I'd have to look past the if statement and see where the variable would still be used (in which, in this case, would be nowhere.. since the scope ended).


Both cases have 6 lines, and the first one is slightly more verbose. But I actually am more comfortable at reasoning about the first case - it feels familiar, as if the function returned an enum of two cases..

so... this is a suggestion and perhaps you could agree with!
Thanks in advance,

@oli-obk
Copy link
Contributor

oli-obk commented Jan 29, 2019

I think we could prevent the lint from triggering if there are bindings bound on the arms.

Although I personally still prefer

if !some_hash_set.insert(some_key) {
    panic!("D'oh!");
}

@swfsql
Copy link
Author

swfsql commented Jan 31, 2019

Oh wait, I just noted that one can also do:

if let _previously_unset @ false = some_hash_set.insert(some_key) {
    panic!("D'oh!");
}

which has all the benefits!
(not that verbose; easy to infer effects; named values), (and passes on clippy!)


So perhaps this (if-let) could also be a suggestion instead (for someone matching on bools)?

Following the suggestion format (from https://rust-lang.github.io/rust-clippy/master/index.html#match_bool )

if let _condition @ true = true {
    foo();
} else {
    bar();
}

Thanks for the clear and quick response!

@camsteffen
Copy link
Contributor

I think it's quite unusual to use _name @ just for code clarity rather than just doing let name =.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants