use of Result::unwrap_or_else over Result::map_or_else

[Valentine-Mario]

This PR is the lint suggested in #7328

[rust-highfive]

r? @llogiq

(rust-highfive has picked a reviewer for you, use r? to override)

[llogiq]

Hey there! I see you have put the basic structure together, now you may want to start fleshing out the test, then implement the actual lint.

Let me know if you need any help.

[Valentine-Mario]

Hey there! I see you have put the basic structure together, now you may want to start fleshing out the test, then implement the actual lint.

Let me know if you need any help.

Thank you. I definitely would reach out when I need help

[Valentine-Mario]

@llogiq can you please help me with review

[llogiq]

I feel the example suggestion is worse than the code it replaces. Ultimately I don't understand the reasoning behind this lint.

Both map_or_else and unwrap_or_else are about getting the return value, yet your examples only ever return (). Worse, process::exit for control flow is not generally applicable and in most cases misguided.

[llogiq]

Having read the underlying issue, I think the lint was meant to catch an id-implementing closure in the map part of the map_or_else.

[Valentine-Mario]

I feel the example suggestion is worse than the code it replaces. Ultimately I don't understand the reasoning behind this lint.

Both map_or_else and unwrap_or_else are about getting the return value, yet your examples only ever return (). Worse, process::exit for control flow is not generally applicable and in most cases misguided.

I've updated the doc and test to give a clearer picture to the lint usage

[llogiq]

I'm still not happy about the example.

Things I would consider lint-worthy:

a.map_or_else(|_| 0, |n| n) (but only in the absence of type adjustments) or b.map_or_else(|x| something_else(x), std::convert::identity).

Note that both are expressions that return some value (of the Ok-part of the type of a and b, respectively).

As soon as the map part does some operation, I would consider linting the expression a false positive.

[Valentine-Mario]

I'm still not happy about the example.

Things I would consider lint-worthy:

a.map_or_else(|_| 0, |n| n) (but only in the absence of type adjustments) or b.map_or_else(|x| something_else(x), std::convert::identity).

Note that both are expressions that return some value (of the Ok-part of the type of a and b, respectively).

As soon as the map part does some operation, I would consider linting the expression a false positive.

Thanks for that. I'm kind of new to contributing, so how do I check if the map part performs any form of operation?

[llogiq]

For that you need to match on the second argument expression of the map_or_else method call (args[2], because [0] is the receiver). Either it is an ExprPath, then you check if that matches std::convert::identity. Or it is an ExprClosure then you need to get the Body and see if it is an ExprPath with just one segment and the same ident as the closure's sole argument. In this case, check no type adjustments took place (see eta_reduction.rs as a reference).

[flip1995]

how do I check if the map part performs any form of operation?

With #7330 you can do this with the function clippy_utils::is_expr_identity_function

[camsteffen]

Any thoughts on enhancing map_identity instead of creating a new lint? We could also include Result::map_or, Option::map_or, Option::map_or_else.

[Valentine-Mario]

For that you need to match on the second argument expression of the map_or_else method call (args[2], because [0] is the receiver). Either it is an ExprPath, then you check if that matches std::convert::identity. Or it is an ExprClosure then you need to get the Body and see if it is an ExprPath with just one segment and the same ident as the closure's sole argument. In this case, check no type adjustments took place (see eta_reduction.rs as a reference).

Thanks for this. In my case, I checked if it was an ExprKind::Lit, then I checked the adjstments

[llogiq]

As @flip1995 said, we already have a utils function to find identities. You can simply use that one.

And @camsteffen is right, we should extend existing lints that fit before creating a new one.

[Valentine-Mario]

As @flip1995 said, we already have a utils function to find identities. You can simply use that one.

And @camsteffen is right, we should extend existing lints that fit before creating a new one.

Not sure how the extending should work. Since map_identity matches for just map method name

[camsteffen]

Not sure how the extending should work. Since map_identity matches for just map method name

You can just edit the implementation of map_identity to match map_or_else. Ultimately you just need to do span_lint_and_help(cx, MAP_IDENTITY, ..).

[giraffate]

ping from triage @Valentine-Mario. Can you have any update on this?

[bors]

☔ The latest upstream changes (presumably #7516) made this pull request unmergeable. Please resolve the merge conflicts.

[giraffate]

ping from triage @Valentine-Mario. According to the triage procedure, I'm closing this because 2 weeks have passed with no activity. If you have more time to work on this, feel free to reopen this.