Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: unnecessary_map_or #11796

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

Jacherr
Copy link
Contributor

@Jacherr Jacherr commented Nov 12, 2023

Closes #10118

This lint checks map_or method calls to check if they can be consolidated down to something simpler and/or more readable.

For example, the code

let x = Some(5);
x.map_or(false, |n| n == 5)

can be rewritten as

let x = Some(5);
x == Some(5)

In addition, when the closure is more complex, the code can be altered from, say,

let x = Ok::<Vec<i32>, i32>(vec![5]);
x.map_or(false, |n| n == [5])

into

let x = Ok::<Vec<i32>, i32>(vec![5]);
x.is_some_and(|n| n == [5])

This lint also considers cases where the map_or can be chained with other method calls, and accommodates accordingly by adding extra parentheses as needed to the suggestion.

changelog: add new lint unnecessary_map_or

@rustbot
Copy link
Collaborator

rustbot commented Nov 12, 2023

r? @Centri3

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 12, 2023
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
@WaffleLapkin
Copy link
Member

@Jacherr this seems to be waiting on you, since Catherine left you a review.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Mar 2, 2024
@Jacherr
Copy link
Contributor Author

Jacherr commented Mar 7, 2024

Hi, apologies for the lack of movement on this. I’ll make changes based on the review now, and once everything is up to standard I’ll go through the conflicts.

@Jacherr
Copy link
Contributor Author

Jacherr commented Mar 8, 2024

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Mar 8, 2024
@bors
Copy link
Collaborator

bors commented Mar 9, 2024

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

@bors
Copy link
Collaborator

bors commented Mar 22, 2024

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

@xFrednet
Copy link
Member

Hey @Jacherr Sorry for the delay. @Centri3 was sadly busy, but now she indicated that she'll have time for reviews again. Would you mind rebasing again? Next time there is a long delay, you can also pick a new reviewer with r? clippy.

@Jacherr

This comment was marked as resolved.

@bors
Copy link
Collaborator

bors commented Jun 27, 2024

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

Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple more changes then this is good <3

tests/ui/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

xFrednet commented Jul 2, 2024

Never mind: My GH view was outdated

@xFrednet
Copy link
Member

Hey, this is a ping from triage. @Centri3 can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@rustbot ready

@bors
Copy link
Collaborator

bors commented Jul 27, 2024

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

clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/unnecessary_map_or.rs Outdated Show resolved Hide resolved
@bors
Copy link
Collaborator

bors commented Oct 7, 2024

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

@Jacherr
Copy link
Contributor Author

Jacherr commented Oct 17, 2024

Apologies for the slow turnover again. I hope this is the last of the issues now and this can finally be merged 😅

@samueltardieu
Copy link
Contributor

samueltardieu commented Oct 17, 2024

It looks like you marked the comment in #11796 (comment) as resolved but it is not. The example:

fn main() {
    struct S;
    let r: Result<i32, S> = Ok(3);
    let _ = r.map_or(false, |x| x == 7);
}

will incorrectly suggest using (r == Ok(7)) (also, note the extra parentheses, but this is not the main problem, which is that Result<i32, S> does not implement PartialEq). A proposed fix is given in the linked comment.

Could you also add this as a testcase? As well as one where S is a type which implements PartialEq (such as i32), to check for the extra parentheses?

let _ = Some(5) == Some(5);
let _ = Some(5) != Some(5);
let _ = Some(5) == Some(5);
let _ = (Some(5) == Some(5));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ( should be removed, shouldn't they?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is just an effect of maybe_par deciding they should be present. While I'd usually agree, I'm not sure there is a better way to tackle that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

== check instead of opt.map_or(false, |val| val == n) or if let Some(n) = opt
7 participants