Rust-clippy: Suggest Option::ok_or{,_else}

[JarredAllen]

What it does

This lint should suggest calling Option::ok_or or Option::ok_or_else when this functionality is reimplemented using the Option::map_or or Option::map_or_else functions, respectively.

Categories (optional)

• Kind: clippy::complexity

Makes it clearer that this is converting from an Option to a Result, and produces shorter code.

Drawbacks

None.

Example

optional.map_or(
    Err(0),
    |x| Ok(x),
)

Could be written as:

optional.ok_or(0)

[tnielens]

Taking a stab at this one.

I'm thinking of naming it conciser_alternative and be an umbrella lint for other similar cases such as

    match None {
        Some(i) => i,
        None => 1,
    };

has a conciser alternative in: None.unwrap_or(1).

Let me know if that's a bad idea and you'd prefer one separate lint for every case.

Alternative naming scheme would be a series like:

• conciser_with_option_ok_or
• conciser_with_option_unwrap_or
• ..

[flip1995]

The problem with the name is, that it describes the code _after_ the lint is applied. This means allow(more_concise_alternative) (I think it's "more concise" instead of "conciser", not a native speaker though) doesn't really make sense, since this kind of code is already allowed.

[tnielens]

(I think it's "more concise" instead of "conciser", not a native speaker though)

Correct, -er suffix only applies to one-syllable adjectives. In German, would have been fine no? :sweat_smile:

The intent of conciser_alternative was has_more_concise_alternative. But it isn't that clear given your comment. I found the long form has_more_concise_alternative a bit bloated and tried to shorten it (as recommended in the lint naming convention).
I'm open to suggestions.

[JarredAllen]

I think it might make more sense to make a lint to capture cases where a method that exists in the standard library is being reimplemented, and then call it standard_library_reimplementation or something like that, since all of the proposed cases to lint are the programmer writing out some function which already exists.

It is also worth pointing out that there already are several lints that handle cases of this (including map_unwrap_or, match_as_ref, ok_expect, filter_map) currently in clippy, which could be made a part of this lint if we do generalize it to be more broad.

[tnielens]

Is there a mechanism in clippy to deprecate lint names and advertise new names? In that case, we could rename these lints into a common name if that is deemed worthwhile.

standard_library_reimplementation doesn't describe well cases that don't verbatim duplicate implementation but rather encode a pattern in a less direct way than what a std method offers. bind_instead_of_map is such an example.

[JarredAllen]

There are some lints which have been deprecated and whose deprecation notices say that they were replaced by a new lint (into_iter_on_array, invalid_ref, and unused_label). All of those were deprecated because their functionality got moved into rustc lints, but I see no reason why we couldn't do the same thing but mention a different lint within clippy, instead. It would cause a bit of work for people who allowed one of the original lints being replaced (they'd have to change the allow statement manually), but I think that should be minimal because there's not really any reason to allow those lints in the first place.

That's a good point that we might want to extend it beyond just the specifics that standard_library_reimplementation captures. Perhaps it could be better to just take the original name and flip it to the antonym, for something like verbose_alternative (or maybe a different antonym to concise than that one. I'm not 100% on the word "verbose", but vocabulary isn't my forte).

[flip1995]

I'm hesitant on combining lints from the methods module into one lint, since some people might like the map_unwrap_or lint, but dislike the filter_map lint, so they only enable/disable one of them. This wouldn't be possible when they are combined into one lint.

[tnielens]

I'll keep it to one name per case so the user can disable them independently. I'm going for less_concise_than_option_ok_or and less_concise_than_option_unwrap_or.

[flip1995]

You can open a PR about this, but expect some discussion on that PR, if lints should _really_ get merged. I definitely think some lints can get merged, but wouldn't want to go through those lints myself. I'm happy to review your suggestion though!