redundant_pattern_matching fixit suggestion fails to apply

[matthiaskrgr]

Note: tools are broken since some time now which is why I'm using a fairly outdated version of clippy: clippy 0.0.212 (c3e9136 2019-07-30)

code:

fn main() {
    let foo = if let None = a() {
        println!("hi");
        a()
    } else {
        Some(3)
    };
}

fn a() -> Option<u32> {
    Some(3)
}

cargo fix fails to apply clippys suggestion:

warning: redundant pattern matching, consider using `is_none()`
 --> src/main.rs:2:22
  |
2 |       let foo = if let None = a() {
  |  _______________-      ^^^^
3 | |         println!("hi");
4 | |         a()
5 | |     } else {
6 | |         Some(3)
7 | |     };
  | |_____- help: try this: `if a().is_none()`
  |
  = note: `#[warn(clippy::redundant_pattern_matching)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching

If I run cargo fix -Z unstable-options --clippy, I get an error:

warning: failed to automatically apply fixes suggested by rustc to crate `b`

after fixes were automatically applied the compiler reported errors within these files:

  * src/main.rs

This likely indicates a bug in either rustc or cargo itself,
and we would appreciate a bug report! You're likely to see
a number of compiler warnings after this message which cargo
attempted to fix but failed. If you could open an issue at
https://github.com/rust-lang/rust/issues
quoting the full output of this command we'd be very appreciative!
Note that you may be able to make some more progress in the near-term
fixing code with the `--broken-code` flag

The following errors were reported:
error: expected `{`, found `;`
 --> src/main.rs:2:32
  |
2 |     let _foo = if a().is_none();
  |                --              ^ expected `{`
  |                |
  |                this `if` statement has a condition, but no block

error: aborting due to previous error

Original diagnostics will follow.

warning: unused variable: `foo`
 --> src/main.rs:2:9
  |
2 |     let foo = if let None = a() {
  |         ^^^ help: consider prefixing with an underscore: `_foo`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: redundant pattern matching, consider using `is_none()`
 --> src/main.rs:2:22
  |
2 |       let foo = if let None = a() {
  |  _______________-      ^^^^
3 | |         println!("hi");
4 | |         a()
5 | |     } else {
6 | |         Some(3)
7 | |     };
  | |_____- help: try this: `if a().is_none()`
  |
  = note: `#[warn(clippy::redundant_pattern_matching)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching

warning: use of a blacklisted/placeholder name `foo`
 --> src/main.rs:2:9
  |
2 |     let foo = if let None = a() {
  |         ^^^
  |
  = note: `#[warn(clippy::blacklisted_name)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name

warning: unused variable: `foo`
 --> src/main.rs:2:9
  |
2 |     let foo = if let None = a() {
  |         ^^^ help: consider prefixing with an underscore: `_foo`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: redundant pattern matching, consider using `is_none()`
 --> src/main.rs:2:22
  |
2 |       let foo = if let None = a() {
  |  _______________-      ^^^^
3 | |         println!("hi");
4 | |         a()
5 | |     } else {
6 | |         Some(3)
7 | |     };
  | |_____- help: try this: `if a().is_none()`
  |
  = note: `#[warn(clippy::redundant_pattern_matching)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_pattern_matching

warning: use of a blacklisted/placeholder name `foo`
 --> src/main.rs:2:9
  |
2 |     let foo = if let None = a() {
  |         ^^^
  |
  = note: `#[warn(clippy::blacklisted_name)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name

    Finished dev [unoptimized + debuginfo] target(s) in 0.68s

[matthiaskrgr]

I believe this is the same underlying error:

fn main() {
    let _ = has_flags();
}

fn has_flags() -> bool {
    let bla: Result<i32, i32> = Ok(4);
    if let Ok(_) = bla {
        true
    } else {
        false
    }
}

clippy suggests

warning: redundant pattern matching, consider using `is_ok()`
  --> src/main.rs:7:12
   |
7  |       if let Ok(_) = bla {
   |  _____-      ^^^^^
8  | |         true
9  | |     } else {
10 | |         false
11 | |     }
   | |_____- help: try this: `if bla.is_ok()`
   |
   = note: `#[warn(clippy::redundant_pattern_matching)]` on by default

It seems that clippy replaces the entire span with bla.is_ok() which does not work, we probably want to remove the preceeding "if" as well?

fn has_flags() -> bool {
    let bla: Result<i32, i32> = Ok(4);
    if bla.is_ok()
}

The code above does not compile for obvious reasons... :/

[phansch]

Thanks, these are good examples and actually show two problems with the suggestions:

1. The suggestion currently drops the body of the conditional. (as in the opening post)
2. The suggestion should make use of the result by appending {} to the suggestion, to prevent triggering unused_must_use (as in the first comment)

I have a fix coming up for 2), but not yet for 1)

A simple solution for 1) could be to skip the suggestion if there's any statements in the conditional bodies?

[flip1995]

The suggestions were improved in #4352, but there is still room for more improvements:

1. (https://github.com/rust-lang/rust-clippy/pull/4352/files#r312354699) applying the suggestions results in a syntax error, since the ; is missing sometimes. There are 3 cases that I can think of:
   • on a let _ = if ...; the ; is already there,
   • on a returning if the ; shouldn't be added,
   • and on a stray if, it has to be added.
2. (https://github.com/rust-lang/rust-clippy/pull/4352/files#r312355415) The suggestion is semantically wrong in some cases and the returned type of the if/match statement has to be accounted for.