clippy::option_if_let_else suggests hard code

[PierreAntoineGuillaume]

trafficstars

Summary

I guess clippy::option_if_let_else should not be suggested here, because the branch yields an Output variant, and not a string slice as suggested.

Lint Name

clippy::option_if_let_else

Reproducer

I tried this code in rust playground:

#![deny(clippy::option_if_let_else)]

fn main() {
    println!("{:?}", run("ok:yay"));
    println!("{:?}", run("ko:there was an error"));
    println!("{:?}", run("crash:oh no, it crashed"));
}


#[derive(Debug)]
pub enum Output {
    Success(String, String),
    JobError(String, String),
    ProcessError(String),
}
fn run(job: &str) -> Output {
    if let Some(stripped) = job.strip_prefix("ok:") {
        Output::Success(stripped.into(), "".into())
    } else if let Some(stripped) = job.strip_prefix("ko:") {
        Output::JobError(stripped.into(), "".into())
    } else if let Some(stripped) = job.strip_prefix("crash:") {
        Output::ProcessError(stripped.into())
    } else {
        unreachable!(
            "Job should begin with ok:, ko, or crash: (actual: '{}')",
            job
        )
    }
}

I saw this happen:

error: use Option::map_or_else instead of an if let/else
   --> src/ci/job/schedule.rs:176:13
    |
176 | /             if let Some(stripped) = job.strip_prefix("ok:") {
177 | |                 Output::Success(stripped.into(), "".into())
178 | |             } else if let Some(stripped) = job.strip_prefix("ko:") {
179 | |                 Output::JobError(stripped.into(), "".into())
...   |
186 | |                 )
187 | |             }
    | |_____________^
    |
note: the lint level is defined here
   --> src/main.rs:3:9
    |
3   | #![deny(clippy::nursery)]
    |         ^^^^^^^^^^^^^^^
    = note: `#[deny(clippy::option_if_let_else)]` implied by `#[deny(clippy::nursery)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else
help: try
    |
176 ~             job.strip_prefix("ok:").map_or_else(|| if let Some(stripped) = job.strip_prefix("ko:") {
177 +                 Output::JobError(stripped.into(), "".into())
178 +             } else if let Some(stripped) = job.strip_prefix("crash:") {
179 +                 Output::ProcessError(stripped.into())
180 +             } else {
181 +                 unreachable!(
  ...

I expected to see this happen: ???

Version

cargo clippy --version
clippy 0.1.62 (88860d5 2022-05-09)
$ rustc -Vv
rustc 1.62.0-nightly (88860d547 2022-05-09)
binary: rustc
commit-hash: 88860d5474a32f507dde8fba8df35fd2064f11b9
commit-date: 2022-05-09
host: x86_64-unknown-linux-gnu
release: 1.62.0-nightly
LLVM version: 14.0.1

Additional Labels

No response

[ghost]

You can't see the mapping argument in the suggestion because it's cut off.

I think it wants you to do this:

fn run(job: &str) -> Output {
    job.strip_prefix("ok:").map_or_else(|| 
        if let Some(stripped) = job.strip_prefix("ko:") {
             Output::JobError(stripped.into(), "".into())
        } else if let Some(stripped) = job.strip_prefix("crash:") {
             Output::ProcessError(stripped.into())
        } else {
            unreachable!(
                "Job should begin with ok:, ko, or crash: (actual: '{}')",
                job
            )
        },
        |stripped| Output::Success(stripped.into(), "".into()))
}

Once that's done it will start warning about the left over if lets.

[PierreAntoineGuillaume]

So this lint suggests

fn run(&self, job: &str) -> Output {
    job.strip_prefix("ok:").map_or_else(
        || {
            job.strip_prefix("ko:").map_or_else(
                || {
                    job.strip_prefix("crash:").map_or_else(
                        || {
                            unreachable!(
                                "Job should begin with ok:, ko, or crash: (actual: '{}')",
                                job
                            )
                        },
                        |stripped| Output::ProcessError(stripped.to_string()),
                    )
                },
                |stripped| Output::JobError(stripped.to_string(), String::default()),
            )
        },
        |stripped| Output::Success(stripped.to_string(), String::default()),
    )
}

over

fn run(&self, job: &str) -> Output {
    if let Some(stripped) = job.strip_prefix("ok:") {
        Output::Success(stripped.to_string(), String::new())
    } else if let Some(stripped) = job.strip_prefix("ko:") {
        Output::JobError(stripped.to_string(), String::new())
    } else if let Some(stripped) = job.strip_prefix("crash:") {
        Output::ProcessError(stripped.to_string())
    } else {
        panic!(
            "Job should begin with ok:, ko, or crash: (actual: '{}')",
            job
        )
    }
}

And I must say I don't find the first gist nice or readable, because :

• the second solution use 2 layers of identation, and the suggested 7
• the second solution allows direct usage of extracted options, where the suggested forces much higher cognitive load.

First, is there a more idiomatic way of doing what I did ? (this gist is a test impl).

Second, is there a way to improve clippy ? How could I help ?

[PierreAntoineGuillaume]

The problem is not about a false positive, so I renamed and retag, but I don't know how to tag this

[softmoth]

Here's a similar example, also involving strip_prefix:

        /* Clippy's suggested version, 13 lines, hard to follow, more nested */
        line.strip_prefix("fold along ").map_or_else(
            || {
                if !line.is_empty() {
                    let (a, b) = line
                        .split_once(',')
                        .expect("comma-separated pair of integers");
                    grid.insert((a.parse::<u16>().unwrap(), b.parse::<u16>().unwrap()));
                }
            },
            |fold| {
                folds.push(fold.split_once('=').unwrap());
            },
        );

        /* Original code, 8 lines, easier to follow, less indentation */
        if let Some(fold) = line.strip_prefix("fold along ") {
            folds.push(fold.split_once('=').unwrap());
        } else if !line.is_empty() {
            let (a, b) = line
                .split_once(',')
                .expect("comma-separated pair of integers");
            grid.insert((a.parse::<u16>().unwrap(), b.parse::<u16>().unwrap()));
        }

[fxdave]

I also agree

fn handle_resolve(limits: &limits::Limits, limiter_id: String, user_id: String) -> Response {
    if let Some(limiter) = limits.get_limiter(&limiter_id) {
        limiter.resolve(user_id);
        Response::ResolveDone
    } else {
        Response::ErrorLimiterNotFound
    }
}

fn handle_resolve(limits: &limits::Limits, limiter_id: String, user_id: String) -> Response {
    limits.get_limiter(&limiter_id).map_or(Response::ErrorLimiterNotFound, |limiter| {
        limiter.resolve(user_id);
        Response::ResolveDone
    })
}

The first one has less cognitive load.

However, the lint's example is good: https://rust-lang.github.io/rust-clippy/master/index.html#option_if_let_else Somehow it should decide whether it is visually better or not.

[tgross35]

map_or is usually a solid improvement, but I feel like map_or_else has a tendancy to decrease readability for anything that isn't trivial - and this is kind of true for any combinators that accept two closures. Part of this is because it's not immediately clear which position is for which variant, but it also just seems to always lead to ugly formatting - all the above examples are map_or_else

Imho, it would be better to never suggest map_or_else if it means using closures, only if both arguments are function names/pointers (as suggested by redundant_closure). Or even spllit the map_or_else suggestion into restriction / not suggested at all.

The |foo| foo pattern is the suggestion is also kind of weird even though it's the most simple case:

let _ = if let Some(foo) = optional {
    foo
} else {
    let y = do_complicated_function();
    y*y
};

// suggested in the docs:
let _ = optional.map_or_else(||{
    let y = do_complicated_function();
    y*y
}, |foo| foo);

// suggested as rustfmt actually formats it - one line more than the `if let ... else`
let _ = optional.map_or_else(
    || {
        let y = do_complicated_function();
        y * y
    },
    |foo| foo,
);

The original if let ... else is definitely better. Or a match, which I personally find easier to read for simple cases over let else (adds an extra line & indent though so not true for larger block bodies)

// maybe better...?
let _ = match optional {
    Some(foo) => foo,
    None => {
        let y = do_complicated_function();
        y*y
    }
}

[HadrienG2]

In general, I would hesitate to use map_or_else when either branch is side-effectful (assertions, I/O, etc), because I that feel that obscures the control flow too much and control flow matters a lot in the presence of such operations.

[ronnodas]

map_or_else with |foo| foo is just unwrap_or_else:

// suggested in the docs:
let _ = optional.map_or_else(||{
    let y = do_complicated_function();
    y*y
}, |foo| foo);

should just be

let _ = optional.unwrap_or_else(|| {
    let y = do_complicated_function();
    y * y
});

(as formatted by rustfmt). So that's not a great example to be in the docs. Especially since there isn't a follow up lint that suggests this simplification.