New lint: manual_ok_err

[samueltardieu]

changelog: [manual_ok_err]: new lint

Detect manual implementations of .ok() or .err(), as in

let a = match func() {
    Ok(v) => Some(v),
    Err(_) => None,
};
let b = if let Err(v) = func() {
    Some(v)
} else {
    None
};

which can be replaced by

let a = func().ok();
let b = func().err();

This pattern was detected in the wild in the Rust reimplementation of coreutils: uutils/coreutils#6886 (review)

[rustbot]

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

Putting it to WIP as there is a case that is incorrectly handled.
Done

[samueltardieu]

(the latest push is a rebase on master)

[samueltardieu]

r? @y21

[samueltardieu]

More maintainer roulette

r? @blyxyas

[blyxyas]

Everything seems to be correct, and it seems to be a very useful lint. I think it has some bright future! (Some clarity nits were found)

[blyxyas]

A refactoring or comment would help the clarity of this fragment!

[samueltardieu]

Comment added

[blyxyas]

The FCP just opened, we'll wait a few days and check that everything's alright. I like the idea of this lint 👍

[pitaj]

Does the current implementation consider const code? It probably shouldn't lint in those cases.

[samueltardieu]

Does the current implementation consider const code? It probably shouldn't lint in those cases.

It doesn't lint in const code (checked in matches/mod.rs). I have added a new test for checking this because of your remark.

[y21]

Not sure if the lint logic accounts for this but I think it would be good to have a test for coercions in the Some match arm that wouldn't work with .ok(). Something like

let s: Option<&dyn Any> = match Ok::<_, ()>(&1) {
  Ok(v) => Some(v),
  _ => None
};

(Can't come up with a better case rn but this particular FP had been reported for similar lints in the past)

[samueltardieu]

Indeed. I've added a check that the type is the same inside the Result variant we keep and the Option. I've also rebased this patch because I added a new function to clippy_utils/src/ty/mod.rs which has been renamed from ty.rs in the meantime.

[y21]

Shouldn't PatKind::Binding be part of the first arm given that it is basically a wildcard pattern? E.g. this gets linted, but .ok() is not equivalent there

match Ok::<_, ()>(&1) {
  _x => None,
  Ok(v) => Some(v),
};

(Not sure why anyone would write code like this but there seems to be some logic for handling it in the lint and some similar looking tests, so thought I'd comment on that)

[samueltardieu]

Good catch. Fixed.

[y21]

Can you add a similar test case where the error type is infallible (so no | pattern is needed to create overlapping patterns)?

match Ok::<_, std::convert::Infallible>(1) {
  Ok(3) => None,
  Ok(v) => Some(v)
};

Reading through the current is_variant_or_wildcard implementation I think this gets linted even though it's not equivalent

[samueltardieu]

Good catch again, I hadn't thought of this one, fixed.

[blyxyas]

LGTM, thanks! ❤️