manual_filter wrongly suggesting filter when match has side-effects

[badboy]

Summary

The manual_filter lint's (#9451) suggestion can cause side-effect code to be removed.
While the code can be rewritten to use filter correctly, the current suggestion will change the meaning of the code and is therefore wrong.

Lint Name

manual_filter

Reproducer

I tried this code:

pub fn test() {
    let reason = Some("reason");
    let allowed = vec![
        "reason", "other"
    ];

    let corrected_reason = match reason {
        Some(reason) => {
            if allowed.contains(&reason) {
                Some(reason)
            } else {
                println!("Invalid reason: {reason:?}");
                None
            }
        }
        None => None,
    };

    println!("{corrected_reason:?}")
}

I saw this happen:

❯ cargo +beta clippy
    Checking clippy-manual-filter v0.1.0 (/Users/jer/projects/rust/clippy-manual-filter)
warning: manual implementation of `Option::filter`
  --> src/lib.rs:7:28
   |
7  |       let corrected_reason = match reason {
   |  ____________________________^
8  | |         Some(reason) => {
9  | |             if allowed.contains(&reason) {
10 | |                 Some(reason)
...  |
16 | |         None => None,
17 | |     };
   | |_____^ help: try this: `reason.filter(|&reason| allowed.contains(&reason))`

I expected to see this happen:

Either not triggering that lint or if it triggers suggesting something that doesn't remove the side-effect line (println).

Version

rustc 1.66.0-beta.1 (e080cc5a6 2022-11-01)
binary: rustc
commit-hash: e080cc5a659fb760c0bc561b722a790dad35b5e1
commit-date: 2022-11-01
host: aarch64-apple-darwin
release: 1.66.0-beta.1
LLVM version: 15.0.2

Additional Labels

No response

[badboy]

Actually a "trivial" rewrite like match reason.filter(|&reason| ... { would still be wrong.

[jcamiel]

Hi,
With the following code:

pub fn parse_options(matches: &ArgMatches) -> Result<CliOptions, CliError> {
    let cacert_file = match get::<String>(matches, "cacert_file") {
        None => None,
        Some(filename) => {
            if !Path::new(&filename).is_file() {
                let message = format!("File {} does not exist", filename);
                return Err(CliError { message });
            } else {
                Some(filename)
            }
        }
    };
    
   // .... end of the functions
}

clippy 0.1.67 (fc594f15 2023-01-24) gives the following hint:

warning: manual implementation of `Option::filter`
   --> packages/hurl/src/cli/options.rs:415:23
    |
415 |       let cacert_file = match get::<String>(matches, "cacert_file") {
    |  _______________________^
416 | |         None => None,
417 | |         Some(filename) => {
418 | |             if !Path::new(&filename).is_file() {
...   |
424 | |         }
425 | |     };
    | |_____^ help: try this: `get::<String>(matches, "cacert_file").filter(|filename| !(!Path::new(&filename).is_file()))`

The proposed fixes seems still erroneous to me as it take not into account the return Err(CliError { message }); side effect.