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.

Sign up for GitHub

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

Jump to bottom

Add new manual_unwrap_or_default lint #12440

Merged
merged 4 commits into from Mar 10, 2024
Merged

Add new manual_unwrap_or_default lint #12440

merged 4 commits into from Mar 10, 2024

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 8, 2024 •
edited by llogiq

This adds a new lint checking if a match or a if let can be replaced with unwrap_or_default.

changelog: Add new [manual_unwrap_or_default] lint

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2024

r? @llogiq

rustbot has assigned @llogiq.
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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 8, 2024
@GuillaumeGomez
Copy link
Member Author

Also I'm not a big fan of the lint name but couldn't find anything better. If anyone has a suggestion, it'd be very appreciated. :)

@GuillaumeGomez GuillaumeGomez force-pushed the add-match_option_and_default branch 2 times, most recently from c1863a9 to 890051a Compare March 8, 2024 21:32
@bors
Copy link
Collaborator

bors commented Mar 9, 2024

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

@llogiq
Copy link
Contributor

llogiq commented Mar 9, 2024

How about manual_unwrap_or_default?

llogiq
llogiq reviewed Mar 9, 2024
View reviewed changes
clippy_lints/src/match_option_and_default.rs Outdated
&& local_id == binding_id
// We now check the `None` arm is calling a method equivalent to `Default::default`.
&& let body_none = body_none.peel_blocks()
&& let ExprKind::Call(_, &[]) = body_none.kind
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that really needed here, when in the next line we check for defaultness?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just checked the is_default_equivalent implementation and you're absolutely right! Even worse: this check can actually be pretty bad as it excludes some potential cases I don't cover from what I saw. Good catch!

@GuillaumeGomez
Copy link
Member Author

Renamed and applied the suggestion. Thanks for both! :)

@llogiq llogiq changed the title Add new match_option_and_default lint Add new manual_unwrap_or_default lint Mar 10, 2024
@llogiq
Copy link
Contributor

llogiq commented Mar 10, 2024

Thank you for writing this lint!

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 10, 2024

📌 Commit 98ac5f1 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 10, 2024

⌛ Testing commit 98ac5f1 with merge 81debac...

@bors
Copy link
Collaborator

bors commented Mar 10, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 81debac to master...

@bors bors merged commit 81debac into rust-lang:master Mar 10, 2024
8 checks passed
This was referenced Mar 10, 2024
Merged
Merged
@GuillaumeGomez GuillaumeGomez deleted the add-match_option_and_default branch March 10, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@llogiq llogiq llogiq left review comments

Assignees

@llogiq llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@GuillaumeGomez @rustbot @bors @llogiq