Overhaul `unnecessary_result_or_else`

[ada4a]

I originally wanted to suggest a new lint which would check the following pattern:

fn foo(opt: Option<i32>, default: i32) -> i32 {
    opt.map_or(default, |o| o)
}

and would rewrite it as:

fn foo(opt: Option<i32>, default: i32) -> i32 {
    opt.unwrap_or(default)
}

and same with Option::map_or_else.

But it turns out that there's already a pretty similar lint, unnecessary_result_map_or_else, which does the same for Results. Therefore, I'd instead suggest revamping that lint, by adding support both for Result::map_or and Option::map_or{,_else}.

That would of course require a name change, and I'd intuitively go with unnecessary_map_or_else. But there's already an unnecessary_map_or -- and I kind of feel like this new lint is more deserving of that name; maybe unnecessary_map_or could be renamed to something like map_or_for_partialeq?..

Also, currently unnecessary_result_map_or_else uses some bespoke algorithm to find out whether the "map closure" (as they call it) is an identity closure -- I think we should just switch to is_expr_untyped_identity_function instead. I guess this could be done separately from the rest.

@rustbot label C-enhancement

[teofr]

@rustbot claim

[teofr]

I've been thinking how to best approach this issue, and I think there's a lot of improvements we can make, but also some questions we need to answer first. Some things I think we can improve on regardless of design desicions:

• Renaming unnecessary_result_map_or_else to unnecessary_map_or_else and applying it to Option::map_or_else as well. Should be straightforward and no reason not to do it.
• Improving the applicability of the unnecessary_map_or_else lint, for example suggesting to use map_or when the None/Err closure is a constant. Clippy does something similar suggesting unwrap_or from unwrap_or_else. (See the transitive closure question below)

But also, I have some discussion points:

• Why do you propose to have a single lint for both map_or and map_or_else? From the naming guidelines, it seems we want to have lints that describe what items or patterns they work on, rather than the logic or the suggestions.
• In a similar line, why do you want to rename the current unnecessary_map_or to something else? Following the naming guidelines, it seems to apply to map_or. I guess is a very specific kind of map_or, one with an equality check inside.
• For the current unnecessary_map_or, even if we decide to rename it, there's some bits that I think should stay in the new unnecessary_map_or, in particular suggesting is_none_or(...) from map_or(true, ...)
• (transitive closure) Out of the working scope of this issue, but has there been any discussions about how suggestions can be closured? For example, from opt.map_or_else(|_| default, |o| o) clippy will suggest opt.unwrap_or_else(|_| default), which will trigger again and suggest opt.unwrap_or(default)? I just think it's an interesting question.
  • Within the scope of this issue, I'd suggest trying to take this closure, ie going straight to suggesting unwrap_or

Let me know what you think @ada4a

[ada4a]

Why do you propose to have a single lint for both map_or and map_or_else? From the naming guidelines, it seems we want to have lints that describe what items or patterns they work on, rather than the logic or the suggestions.

Fair point -- that would make the lint names less awkward at the very least. Though there are a few existing lints which cover both map_or and map_or_else, or both unwrap_or and unwrap_or_else, for example map_uwnrap_or

In a similar line, why do you want to rename the current unnecessary_map_or to something else? Following the naming guidelines, it seems to apply to map_or. I guess is a very specific kind of map_or, one with an equality check inside.

Exactly because it applies to a very specific kind of unnecessary map_or, while the pattern I've described above makes the very map_or unnecessary, as unwrap_or could be used instead. But after thinking about it for a while... I don't know, I guess they both would deserve to be called that, so since that lint has already taken the name, we should just admit defeat and look for another one -- I came up with map_identity_or_else.

For the current unnecessary_map_or, even if we decide to rename it, there's some bits that I think should stay in the new unnecessary_map_or, in particular suggesting is_none_or(...) from map_or(true, ...)

Oh, it covers that pattern as well?.. Those could honestly be two separate lints imo.

(transitive closure) Out of the working scope of this issue, but has there been any discussions about how suggestions can be closured? For example, from opt.map_or_else(|_| default, |o| o) clippy will suggest opt.unwrap_or_else(|_| default), which will trigger again and suggest opt.unwrap_or(default)? I just think it's an interesting question.

• Within the scope of this issue, I'd suggest trying to take this closure, ie going straight to suggesting unwrap_or

This is a pretty common pattern in Clippy, yes. The "transitive closure" is achieved by running cargo clippy --fix -- it will first apply all the suggestions, then rerun Clippy and apply the newly created suggestion, etc. until there are no more suggestions. Doing it that way is nice, because it allows each lint's logic to remain simple, because it only needs to think about its particular pattern.

[teofr]

Thanks, all very clear. I'll start working on it and keeping them sort of separate, renaming/combining them should be easy.

Right now I see the following (separate) lints, some of them may be better off together:

Currently	New name	logic
unnecessary_map_or	unnecessary_map_or_predicate	x.map_or(true, ...) => x.is_none_or( ... )
unnecessary_map_or	unnecessary_map_or_partial_eq	x.map_or(false, |n| n == 5) => x == Some(5)
unnecessary_result_map_or_else	unnecessary_map_or_else(_identity)	x.map_or_else( ... , |n| n) => x.unwrap_or_else( ... )
-	unnecessary_map_or_else(_constant)	x.map_or_else( c, ... ) => x.map_or(c, ... )
-	unnecessary_map_or(_default)	x.map_or( Default::default(), ... ) => x.map_or_default( ... )
-	unnecessary_map_or	x.map_or( ... , |n| n) => x.unwrap_or( ... )

* All of the xs can be options or results

[teofr]

I completely missed this one earlier, unclear to me whether we want to merge them or not.

[ada4a]

First of all, thank you for thinking about all this so thoroughly! Love the table:)

Secondly, I think there's an argument to be made that all of these should be called map_x_or instead of map_or_x, as we lint (at least in the examples you show) only the "map" closure.

And finally, here are some thoughts about all the individual proposed lints:

unnecessary_map_or_predicate

Intuitively, either unnecessary_map_or or map_or_predicate would be enough by themselves -- the first one pointing out that it's unnecessary, the second one pointing out what's unnecessary; but since the former name is taken, I think the latter is fine. The latter naming style is seen in map_unwrap_or, ok_expect etc.

unnecessary_map_or_partial_eq

I think this should be two lints, actually:

1. "map_or_predicate", which is currently called manual_is_variant_and
   • map_or(false, <pred>) => is_some_and(<pred>)
   • map_or(true, <pred>) => is_none_or(<pred>)
2. "is_variant_and_eq"
   • is_some_and(|n| n == m) => == Some(m)

That's because the second pattern does come up on its own, and is worth linting -- there's even an feature request for it: #14713

unnecessary_map_or_else(_identity)

Again, I'd vote for map_or_else_identity^^

unnecessary_map_or_else(_constant)

This one is covered by unnecessary_lazy_evaluations -- well, will be, after someone tackles #15829 ^^

unnecessary_map_or(_default)

Keep in mind that map_or_default isn't stable quite yet

unnecessary_map_or

map_or_identity

I completely missed this one [(unnecessary_result_map_or_else)] earlier

That's the one this issue started with^^

[ada4a]

Also, implementing all of this at once might be daunting -- if you want, we might later create a tracking issue for each individual lint we've decided to move/split/rename/whatever

[teofr]

Thanks for the blazingly fast replies!

I'll reply quickly below, and try to better organize the work when I find more free time

Completely agree about map_or_predicate, dropping the unnecessary_ (and the other similar suggestions)

"map_or_predicate", which is currently called manual_is_variant_and

Missed that lint, maybe then we should increase the scope of that one. There's a big overlap with unnecessary_map_or.

Maybe splitting the current unnecessary_map_or, sending the map_or({true,false}, into manual_is_variant_and. And implementing #14713 as its own simpler lint, dealing with the special case x.map_or(false, |n| n == 5) in two steps.

That's the one this issue started with^^

Of course I copied the wrong link, I meant this one

we might later create a tracking issue for each individual lint

Yeah, that might be the best

- Naming is hard

[ada4a]

Maybe splitting the current unnecessary_map_or, sending the map_or({true,false}, into manual_is_variant_and. And implementing #14713 as its own simpler lint, dealing with the special case x.map_or(false, |n| n == 5) in two steps.

Sounds good to me:)

Though keep in mind that x.map_or(true, can be transformed as well, namely into x.is_none_or( (but only if x: Option)

Of course I copied the wrong link, I meant this one

Oh, I've missed that one as well! To me, this is an argument for having only one lint for both Option and Result -- makes it a bit easier to keep track of them.

I'll [..] try to better organize the work when I find more free time

Of course:)

[teofr]

Back with some more time :)

I'll try to describe a bit of where we are, and then maybe start creating individual issues where appropriate, so we can discuss them better.

1. Refactor bespoke algorithm and merge implementations

Opened #15916 and working on it

2. Rename (and combine) unnecessary_option_map_or_else and unnecessary_result_map_or_else

Issue: #15997

I think these should be combined into a single lint, the options for naming could be:

• unnecessary_map_or_else, most similar to the historical name
• map_identity_or_else, semantic naming
• map_or_else_identity, item + case naming

I do like the semantic naming, but I also see lint names more like unique identifiers, and I do think the discoverability of having map_or_else unbroken is important (the power of Ctrl+F). Then again, if names are unique ids a lot of this discussion is pointless.

3. New lint for x.map_or( ... , |o| o)

Issue: #15801 (this one)

This would be an actual new lint (your original concern). The same naming questions as (2) remain, but it's something that could be discussed in a PR and easy to change.

4. Splitting unnecessary_map_or

Issue: #15999

unnecessary_map_or is a bit complicated so, we should get rid of it and:

4A. Add x.map_or(true, |n| n > 5) to manual_is_variant_and

Issue: #15998

4B. Lift equal checks out of is_some_and( |x| x == c )

Issue: #14713

5. Remove lazy evaluations from map_or_else

Issue: #15829

6. The default cases

x.map_or( Default::default(), ... ) => x.map_or_default( ... )

We could create an issue, but maybe worth waiting untils it's stable.

[ada4a]

Once again, great structuring:) Honestly we could create a tracking issue just from this comment

map_*_or_else vs `map_or_else_*:

• There's also the fact that the identity closure does in fact come second (I think this is some functional programming tradition, with e.g. Haskell defining its version of Result with the Err variant coming first), so map_or_else_identity is kind of correct as well.
• Calling the lints map_or_else_* would help sort them together, which is nice.

All in all, both options would be fine by me. I think a poll over on Zulip would be a good idea -- I would probably use that opportunity to hear people's opinions on all this refactoring in general^^

We could create an issue, but maybe worth waiting until it's stable.

Yeah