Skip to content

New lint: concealed_obvious_default#15037

Open
nik-rev wants to merge 2 commits intorust-lang:masterfrom
nik-contrib:or-default
Open

New lint: concealed_obvious_default#15037
nik-rev wants to merge 2 commits intorust-lang:masterfrom
nik-contrib:or-default

Conversation

@nik-rev
Copy link

@nik-rev nik-rev commented Jun 12, 2025 •
edited
Loading

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
samueltardieu
samueltardieu requested changes Jun 12, 2025
View reviewed changes
Copy link
Member

@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
samueltardieu
samueltardieu approved these changes Jun 13, 2025
View reviewed changes
Copy link
Member

@samueltardieu samueltardieu left a comment •
edited
Loading

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.

@samueltardieu samueltardieu added the A-lint Area: New lints label Jul 11, 2025
nik-rev added a commit to nik-contrib/helix that referenced this pull request Jul 27, 2025
@nik-rev
refactor: Do not use unwrap_or_default on simple integer
1c0b99d 
See rust-lang/rust-clippy#15037
@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2025

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

@samueltardieu samueltardieu mentioned this pull request Sep 18, 2025
Merged
@samueltardieu
Copy link
Member

samueltardieu commented Sep 18, 2025

Could you please rebase your PR?

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 18, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2025

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

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 18, 2025
github-merge-queue bot pushed a commit that referenced this pull request Sep 18, 2025
@Jarcho
Do not replace .unwrap_or(vec![]) by .unwrap_or_default() (#15699)
4a8b7ea 
`.unwrap_or(vec![])` is as readable as `.unwrap_or_default()`.

Also, this ensures by adding tests that expressions such as
`.unwrap_or(DEFAULT_LITERAL)` (`0`, `""`, etc.) are not replaced by
`.unwrap_or_default()` either.

Related to the discussion in the [Zulip
discussion](https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.20concealed_obvious_default)
about PR #15037.

changelog: [`unwrap_or_default`]: do not replace `.unwrap_or(vec![])` by
`.unwrap_or_default()`
@samueltardieu samueltardieu added the needs-fcp PRs that add, remove, or rename lints and need an FCP label Oct 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samueltardieu samueltardieu samueltardieu approved these changes

Assignees

@samueltardieu samueltardieu

Labels

A-lint Area: New lints needs-fcp PRs that add, remove, or rename lints and need an FCP S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@nik-rev @rustbot @samueltardieu