Downgrade match_bool to pedantic

[dtolnay]

I don't quite buy the justification in https://rust-lang.github.io/rust-clippy/. The justification is:

It makes the code less readable.

In the Rust codebases I've worked in, I have found people were comfortable using match bool (selectively) to make code more readable. For example, initializing struct fields is a place where the indentation of match can work better than the indentation of if:

let _ = Struct {
    v: {
        ...
    },
    w: match doing_w {
        true => ...,
        false => ...,
    },
    x: Nested {
        c: ...,
        b: ...,
        a: ...,
    },
    y: if doing_y {
        ...
    } else { // :(
        ...
    },
    z: ...,
};

Or sometimes people prefer something a bit less pithy than if when the meaning of the bool doesn't read off clearly from the condition:

if set.insert(...) {
    ... // ???
} else {
    ...
}

match set.insert(...) {
    // set.insert returns false if already present
    false => ...,
    true => ...,
}

Or match can be a better fit when the bool is playing the role more of a value than a branch condition:

impl ErrorCodes {
    pub fn from(b: bool) -> Self {
        match b {
            true => ErrorCodes::Yes,
            false => ErrorCodes::No,
        }
    }
}

And then there's plain old it's-1-line-shorter, which means we get 25% more content on a screen when stacking a sequence of conditions:

let old_noun = match old_binding.is_import() {
    true => "import",
    false => "definition",
};
let new_participle = match new_binding.is_import() {
    true => "imported",
    false => "defined",
};

Bottom line is I think this lint fits the bill better as a pedantic lint; I don't think linting on this by default is justified.

changelog: Remove match_bool from default set of enabled lints

[dtolnay]

Build failure appears unrelated.

[flip1995]

I think the reasoning

It makes the code less readable.

is a little too weak for this lint.

---

In your first code example I'd prefer the if over the match, since I don't think that it is less readable. If the code is short, this can be formatted as a one-liner if x { a } else { b }. If the if/else is longer it should be moved into a binding and the binding should be used to initialize the struct.

For your second example, you could still put the comment on top of the if-block. If it is not about the comment you could also write if set.insert() == false, which doesn't clarify the intent though and is bad style IMO.

The enum-from-bool example is a special case, where you could allow this lint once in the code base. After that you probably only use the enum and don't match on the bool anymore.

I can see the "it's shorter" argument.

---

Overall, this lint is opinionated (which is fair for style group lints), so marking it as C-needs-discussion. cc @rust-lang/clippy

[djc]

FWIW, I'm also not a fan of this lint, for similar reasons as @dtolnay.

[tesuji]

I'm for one to downgrade this lint.

[flip1995]

@dtolnay This needs a rebase. After that, we can merge this.

[dtolnay]

Rebased.

[flip1995]

@bors r+

[bors]

📌 Commit ef28361 has been approved by flip1995

[bors]

🌲 The tree is currently closed for pull requests below priority 1, this pull request will be tested once the tree is reopened