Improve map_unwrap_or lint to support map(f).unwrap_or_default()

[qdot3]

View all comments

Option::map_or_default/Result::map_or_default was stabilized in Rust 1.98.0, so the map_unwrap_or lint should also cover map(f).unwrap_or_default(), suggesting map_or_default(f) instead.

changelog: [map_unwrap_or]: extend to support map(f).unwrap_or_default()

[rustbot]

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information.

[github-actions]

Lintcheck changes for 8788452

Lint	Added	Removed	Changed
clippy::map_unwrap_or	13	0	0

This comment will be updated if you push new changes

[CommanderStorm]

Community review: LGTM, could live with this. Would still prefer an better test that also coveres MSRV

View changes since this review

[DanielEScherzer]

Community review: lintcheck new hits look correct, some notes

View changes since this review

[DanielEScherzer]

community review: looks good to me

View changes since this review

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu for the project review.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: 9 candidates
• 9 candidates expanded to 9 candidates
• Random selection from 6 candidates

[samueltardieu]

That's a good start! I've added some suggestions.

Also, please add tests involving macros to ensure we don't trigger.

I'll start a FCP to determine whether we want to extend the map_unwrap_or lint or create a new map_unwrap_or_default one.

@rustbot label lint-nominated

View changes since this review

[rustbot]

This lint has been nominated for inclusion.

A FCP topic has been created on Zulip.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[qdot3]

These are new tests. The 5th case seems to be a false negative (to me) but filtered out by method_call, witch is very conservative abount macros. I prefer to keep that conservative behavior.

fn option(opt: Option<i32>) {
    let _ = opt.map(|x| x * x).unwrap_or_default(); //~ map_unwrap_or
    let _ = opt.map(|x| vec![x]).unwrap_or_default(); //~ map_unwrap_or
    let _ = identity!(opt.map(|x| x + 1).unwrap_or_default()); //~ map_unwrap_or
    let _ = identity!(opt.map(|x| x + 1)).unwrap_or_default();
    let _ = identity!(opt).map(|x| x + 1).unwrap_or_default();
}

[qdot3]

@rustbot ready

[samueltardieu]

At the end, before it can be merged, I'll ask you to squash the commits and split the changes into two commits:

• a first commit with the effect that these changes would have on Clippy sources;
• a second commit with the changes to the lint itself.

This way, at any stage (before the first commit, after the first commit, and after both commits) tests would pass without warning, in case we need to bisect an issue.

View changes since this review

[qdot3]

The merge-queue checks will be passed once Lokathor/tinyvec#226 is merged.

[qdot3]

Now new version of tinyvec is published, so could you try to add this PR to the merge queue again, @samueltardieu ?