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

Suggest Entry::or_default for Entry::or_insert(Default::default()) #9342

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Suggest Entry::or_default for Entry::or_insert(Default::default()) #9342

merged 1 commit into from
Sep 5, 2022

Conversation

relrelb
Copy link
Contributor

@relrelb relrelb commented Aug 16, 2022 •
edited by dswij
Loading

Unlike past similar work done in #6228, expand the existing or_fun_call
lint to detect or_insert calls with a T::new() or T::default()
argument, much like currently done for unwrap_or calls. In that case,
suggest the use of or_default, which is more idiomatic.

Note that even with this change, or_insert_with(T::default) calls
aren't detected as candidates for or_default(), in the same manner
that currently unwrap_or_else(T::default) calls aren't detected as
candidates for unwrap_or_default().

Also, as a nearby cleanup, change KNOW_TYPES from static to const,
since as far as I understand it's preferred (should Clippy have a lint
for that?).

Addresses #3812.

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: [or_fun_call]: Suggest Entry::or_default for Entry::or_insert(Default::default())

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 16, 2022
@bors
Copy link
Contributor

bors commented Aug 19, 2022

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

@bors
Copy link
Contributor

bors commented Aug 19, 2022

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

dswij
dswij approved these changes Aug 24, 2022
View reviewed changes
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and welcome!

The change looks great, it's certainly a nice improvement.

AFAIU, this wouldn't resolve #3812 for insert_with? Would you be interested in taking that in another PR?

@dswij dswij 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 Aug 25, 2022
@dswij
Copy link
Member

dswij commented Sep 4, 2022

Thanks again for this! @bors r+

@bors
Copy link
Contributor

bors commented Sep 4, 2022

📌 Commit e8c8a93 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 4, 2022

⌛ Testing commit e8c8a93 with merge 612cdc5...

bors added a commit that referenced this pull request Sep 4, 2022
@bors
Auto merge of #9342 - relrelb:or_default, r=dswij
612cdc5 
Suggest `Entry::or_default` for `Entry::or_insert(Default::default())`

Unlike past similar work done in #6228, expand the existing `or_fun_call`
lint to detect `or_insert` calls with a `T::new()` or `T::default()`
argument, much like currently done for `unwrap_or` calls. In that case,
suggest the use of `or_default`, which is more idiomatic.

Note that even with this change, `or_insert_with(T::default)` calls
aren't detected as candidates for `or_default()`, in the same manner
that currently `unwrap_or_else(T::default)` calls aren't detected as
candidates for `unwrap_or_default()`.

Also, as a nearby cleanup, change `KNOW_TYPES` from `static` to `const`,
since as far as I understand it's preferred (should Clippy have a lint
for that?).

Addresses #3812.

*Please write a short comment explaining your change (or "none" for internal only changes)*

changelog: [`or_fun_call`]: Suggest `Entry::or_default` for `Entry::or_insert(Default::default())`
@bors
Copy link
Contributor

bors commented Sep 4, 2022

💔 Test failed - checks-action_dev_test

@xFrednet
Copy link
Member

xFrednet commented Sep 4, 2022

The test failure is a result of some changes to cargo dev update_lints on master. To fix it, you will need to rebase. The it should be as simple as running that comment and commiting the changes :)

@relrelb
Suggest Entry::or_default for Entry::or_insert(Default::default())
f0e586c 
Unlike past similar work done in rust-lang#6228, expand the existing `or_fun_call`
lint to detect `or_insert` calls with a `T::new()` or `T::default()`
argument, much like currently done for `unwrap_or` calls. In that case,
suggest the use of `or_default`, which is more idiomatic.

Note that even with this change, `or_insert_with(T::default)` calls
aren't detected as candidates for `or_default()`, in the same manner
that currently `unwrap_or_else(T::default)` calls aren't detected as
candidates for `unwrap_or_default()`.

Also, as a nearby cleanup, change `KNOW_TYPES` from `static` to `const`,
since as far as I understand it's preferred (should Clippy have a lint
for that?).

Fixes rust-lang#3812.
@relrelb
Copy link
Contributor Author

relrelb commented Sep 4, 2022

The test failure is a result of some changes to cargo dev update_lints on master. To fix it, you will need to rebase. The it should be as simple as running that comment and commiting the changes :)

Done.

@llogiq
Copy link
Contributor

llogiq commented Sep 5, 2022

@bors r=dswij

@bors
Copy link
Contributor

bors commented Sep 5, 2022

📌 Commit f0e586c has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 5, 2022

⌛ Testing commit f0e586c with merge b763b14...

@bors
Copy link
Contributor

bors commented Sep 5, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing b763b14 to master...

@bors bors merged commit b763b14 into rust-lang:master Sep 5, 2022
@relrelb relrelb deleted the or_default branch September 5, 2022 05:59
@smoelius
Copy link
Contributor

smoelius commented Dec 14, 2022 •
edited
Loading

Note that even with this change, or_insert_with(T::default) calls
aren't detected as candidates for or_default(), in the same manner
that currently unwrap_or_else(T::default) calls aren't detected as
candidates for unwrap_or_default().

Was this simply unimplemented, or is there an impediment that I am not seeing? Would I be wasting everyone's time by trying to do this?

@smoelius smoelius mentioned this pull request Dec 15, 2022
Closed
@smoelius smoelius mentioned this pull request Dec 26, 2022
Merged
bors added a commit that referenced this pull request Jul 23, 2023
@bors
Auto merge of #10120 - smoelius:or_insert_with, r=blyxyas
e4923c2 
`unwrap_or_else_default` -> `unwrap_or_default` and improve resulting lint

Resolves #10080 (though it doesn't implement exactly what's described there)

This PR does the following:
1. Merges `unwrap_or_else_default.rs`'s code into `or_fun_call.rs`
2. Extracts the code to handle `unwrap_or(/* default value */)` and similar, and moves it into `unwrap_or_else_default`
3. Implements the missing functionality from #9342, e.g.,, to handle `or_insert_with(Default::default)`
4. Renames `unwrap_or_else_default` to `unwrap_or_default` (since the "new" lint handles both `unwrap_or` and `unwrap_or_else`, it seemed sensible to use the shortened name)

This PR is currently two commits. The first implements 1-3, the second implements 4.

A word about 2: the `or_fun_call` lint currently produces warnings like the following:
```
error: use of `unwrap_or` followed by a call to `new`
  --> $DIR/or_fun_call.rs:56:14
   |
LL |     with_new.unwrap_or(Vec::new());
   |              ^^^^^^^^^^^^^^^^^^^^^ help: try: `unwrap_or_default()`
```
To me, such warnings look like they should come from `unwrap_or_else_default`, not `or_fun_call`, especially since `or_fun_call` is [in the nursery](#9829).

---

changelog: Move: Renamed `unwrap_or_else_default` to [`unwrap_or_default`]
[#10120](#10120)
changelog: Enhancement: [`unwrap_or_default`]: Now handles more functions, like `or_insert_with`
[#10120](#10120)
<!-- changelog_checked-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij approved these changes

Assignees

@dswij dswij

Labels
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.
7 participants
@relrelb @rust-highfive @bors @dswij @xFrednet @llogiq @smoelius