Skip to content

New lint: concealed_obvious_default #15037

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

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

Conversation

nik-rev
Copy link

@nik-rev nik-rev commented Jun 12, 2025

This PR introduces a complexity lint concealed_obvious_default which checks usages of Option::<T>::unwrap_or_default() on a type with an obvious default and suggests using Option::<T>::unwrap_or(<default>) instead. It checks similar methods on Result and Entry

for example, it suggests to replace x.unwrap_or_default() with x.unwrap_or(0) where x: Option<u8>.

Closes #14779

changelog: add [concealed_obvious_default] lint

@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2025

r? @samueltardieu

rustbot has assigned @samueltardieu.
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 Jun 12, 2025
@nik-rev nik-rev changed the title feat: Implement concealed_obvious_default lint New lint: concealed_obvious_default Jun 12, 2025
@nik-rev nik-rev marked this pull request as draft June 12, 2025 15:45
@nik-rev nik-rev marked this pull request as ready for review June 12, 2025 16:03
Copy link
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

Thanks, this is interesting. I've added some comments.

Also, could you split this into two commits: one with the lint applied (auto-fixed) to Clippy sources, then one with the lint itself. The order between both commits is not mandatory, but auto-fixing Clippy sources first allows the repository to pass the dogfood at every step, in case we need to bisect it.

Also, should the lint move to a allow-by-default category once we have discussed about it, removing the application commit would be easy.

@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 Jun 12, 2025
@nik-rev nik-rev force-pushed the or-default branch 2 times, most recently from 14d8112 to 4f7953e Compare June 13, 2025 15:06
@nik-rev nik-rev marked this pull request as draft June 13, 2025 15:10
@nik-rev nik-rev force-pushed the or-default branch 2 times, most recently from f2f906f to bb6c0d4 Compare June 13, 2025 15:19
@nik-rev nik-rev marked this pull request as ready for review June 13, 2025 15:26
@nik-rev
Copy link
Author

nik-rev commented Jun 13, 2025

Thanks, this is interesting. I've added some comments.

Also, could you split this into two commits: one with the lint applied (auto-fixed) to Clippy sources, then one with the lint itself. The order between both commits is not mandatory, but auto-fixing Clippy sources first allows the repository to pass the dogfood at every step, in case we need to bisect it.

Also, should the lint move to a allow-by-default category once we have discussed about it, removing the application commit would be easy.

Thanks for the review. I've addressed your comments and added tests to check for macro expansion both on the receiver and the method

@nik-rev nik-rev requested a review from samueltardieu June 13, 2025 15:32
@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 Jun 13, 2025
Copy link
Contributor

@samueltardieu samueltardieu left a comment

Choose a reason for hiding this comment

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

Thanks. I've started a FCP thread on Zulip to discuss about the lint inclusion in Clippy.

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.

New lint: Prefer .unwrap_or(0) over .unwrap_or_default()
3 participants