Skip to content

Commit

Permalink
Rollup merge of rust-lang#5408 - dtolnay:matchbool, r=flip1995
Browse files Browse the repository at this point in the history
Downgrade match_bool to pedantic

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`:

```rust
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:

```rust
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:

```rust
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:

```rust
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
  • Loading branch information
flip1995 committed Apr 25, 2020
2 parents 6ffe725 + ef28361 commit 0f00a7c
Show file tree
Hide file tree
Showing 10 changed files with 30 additions and 27 deletions.
3 changes: 1 addition & 2 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&loops::EXPLICIT_INTO_ITER_LOOP),
LintId::of(&loops::EXPLICIT_ITER_LOOP),
LintId::of(&macro_use::MACRO_USE_IMPORTS),
LintId::of(&matches::MATCH_BOOL),
LintId::of(&matches::SINGLE_MATCH_ELSE),
LintId::of(&methods::FILTER_MAP),
LintId::of(&methods::FILTER_MAP_NEXT),
Expand Down Expand Up @@ -1279,7 +1280,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&map_unit_fn::RESULT_MAP_UNIT_FN),
LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),
LintId::of(&matches::MATCH_AS_REF),
LintId::of(&matches::MATCH_BOOL),
LintId::of(&matches::MATCH_OVERLAPPING_ARM),
LintId::of(&matches::MATCH_REF_PATS),
LintId::of(&matches::MATCH_SINGLE_BINDING),
Expand Down Expand Up @@ -1470,7 +1470,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&main_recursion::MAIN_RECURSION),
LintId::of(&map_clone::MAP_CLONE),
LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),
LintId::of(&matches::MATCH_BOOL),
LintId::of(&matches::MATCH_OVERLAPPING_ARM),
LintId::of(&matches::MATCH_REF_PATS),
LintId::of(&matches::MATCH_WILD_ERR_ARM),
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/matches.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ declare_clippy_lint! {
/// }
/// ```
pub MATCH_BOOL,
style,
pedantic,
"a `match` on a boolean expression instead of an `if..else` block"
}

Expand Down
2 changes: 1 addition & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1139,7 +1139,7 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
},
Lint {
name: "match_bool",
group: "style",
group: "pedantic",
desc: "a `match` on a boolean expression instead of an `if..else` block",
deprecation: None,
module: "matches",
Expand Down
3 changes: 1 addition & 2 deletions tests/ui/implicit_return.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ fn test_if_block() -> bool {
}
}

#[allow(clippy::match_bool)]
#[rustfmt::skip]
fn test_match(x: bool) -> bool {
match x {
Expand All @@ -30,7 +29,7 @@ fn test_match(x: bool) -> bool {
}
}

#[allow(clippy::match_bool, clippy::needless_return)]
#[allow(clippy::needless_return)]
fn test_match_with_unreachable(x: bool) -> bool {
match x {
true => return false,
Expand Down
3 changes: 1 addition & 2 deletions tests/ui/implicit_return.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ fn test_if_block() -> bool {
}
}

#[allow(clippy::match_bool)]
#[rustfmt::skip]
fn test_match(x: bool) -> bool {
match x {
Expand All @@ -30,7 +29,7 @@ fn test_match(x: bool) -> bool {
}
}

#[allow(clippy::match_bool, clippy::needless_return)]
#[allow(clippy::needless_return)]
fn test_match_with_unreachable(x: bool) -> bool {
match x {
true => return false,
Expand Down
16 changes: 8 additions & 8 deletions tests/ui/implicit_return.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -19,49 +19,49 @@ LL | false
| ^^^^^ help: add `return` as shown: `return false`

error: missing `return` statement
--> $DIR/implicit_return.rs:28:17
--> $DIR/implicit_return.rs:27:17
|
LL | true => false,
| ^^^^^ help: add `return` as shown: `return false`

error: missing `return` statement
--> $DIR/implicit_return.rs:29:20
--> $DIR/implicit_return.rs:28:20
|
LL | false => { true },
| ^^^^ help: add `return` as shown: `return true`

error: missing `return` statement
--> $DIR/implicit_return.rs:44:9
--> $DIR/implicit_return.rs:43:9
|
LL | break true;
| ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`

error: missing `return` statement
--> $DIR/implicit_return.rs:52:13
--> $DIR/implicit_return.rs:51:13
|
LL | break true;
| ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`

error: missing `return` statement
--> $DIR/implicit_return.rs:61:13
--> $DIR/implicit_return.rs:60:13
|
LL | break true;
| ^^^^^^^^^^ help: change `break` to `return` as shown: `return true`

error: missing `return` statement
--> $DIR/implicit_return.rs:79:18
--> $DIR/implicit_return.rs:78:18
|
LL | let _ = || { true };
| ^^^^ help: add `return` as shown: `return true`

error: missing `return` statement
--> $DIR/implicit_return.rs:80:16
--> $DIR/implicit_return.rs:79:16
|
LL | let _ = || true;
| ^^^^ help: add `return` as shown: `return true`

error: missing `return` statement
--> $DIR/implicit_return.rs:88:5
--> $DIR/implicit_return.rs:87:5
|
LL | format!("test {}", "test")
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add `return` as shown: `return format!("test {}", "test")`
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/match_bool.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#![deny(clippy::match_bool)]

fn match_bool() {
let test: bool = true;

Expand Down
22 changes: 13 additions & 9 deletions tests/ui/match_bool.stderr
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
error: this boolean expression can be simplified
--> $DIR/match_bool.rs:29:11
--> $DIR/match_bool.rs:31:11
|
LL | match test && test {
| ^^^^^^^^^^^^ help: try: `test`
|
= note: `-D clippy::nonminimal-bool` implied by `-D warnings`

error: you seem to be trying to match on a boolean expression
--> $DIR/match_bool.rs:4:5
--> $DIR/match_bool.rs:6:5
|
LL | / match test {
LL | | true => 0,
LL | | false => 42,
LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if test { 0 } else { 42 }`
|
= note: `-D clippy::match-bool` implied by `-D warnings`
note: the lint level is defined here
--> $DIR/match_bool.rs:1:9
|
LL | #![deny(clippy::match_bool)]
| ^^^^^^^^^^^^^^^^^^

error: you seem to be trying to match on a boolean expression
--> $DIR/match_bool.rs:10:5
--> $DIR/match_bool.rs:12:5
|
LL | / match option == 1 {
LL | | true => 1,
Expand All @@ -27,7 +31,7 @@ LL | | };
| |_____^ help: consider using an `if`/`else` expression: `if option == 1 { 1 } else { 0 }`

error: you seem to be trying to match on a boolean expression
--> $DIR/match_bool.rs:15:5
--> $DIR/match_bool.rs:17:5
|
LL | / match test {
LL | | true => (),
Expand All @@ -45,7 +49,7 @@ LL | };
|

error: you seem to be trying to match on a boolean expression
--> $DIR/match_bool.rs:22:5
--> $DIR/match_bool.rs:24:5
|
LL | / match test {
LL | | false => {
Expand All @@ -63,7 +67,7 @@ LL | };
|

error: you seem to be trying to match on a boolean expression
--> $DIR/match_bool.rs:29:5
--> $DIR/match_bool.rs:31:5
|
LL | / match test && test {
LL | | false => {
Expand All @@ -81,15 +85,15 @@ LL | };
|

error: equal expressions as operands to `&&`
--> $DIR/match_bool.rs:29:11
--> $DIR/match_bool.rs:31:11
|
LL | match test && test {
| ^^^^^^^^^^^^
|
= note: `#[deny(clippy::eq_op)]` on by default

error: you seem to be trying to match on a boolean expression
--> $DIR/match_bool.rs:36:5
--> $DIR/match_bool.rs:38:5
|
LL | / match test {
LL | | false => {
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/needless_return.fixed
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// run-rustfix

#![allow(unused, clippy::needless_bool, clippy::match_bool)]
#![allow(unused, clippy::needless_bool)]
#![allow(clippy::if_same_then_else, clippy::single_match)]
#![warn(clippy::needless_return)]

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/needless_return.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// run-rustfix

#![allow(unused, clippy::needless_bool, clippy::match_bool)]
#![allow(unused, clippy::needless_bool)]
#![allow(clippy::if_same_then_else, clippy::single_match)]
#![warn(clippy::needless_return)]

Expand Down

0 comments on commit 0f00a7c

Please sign in to comment.