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_with(Default::default) #3812

Closed
Arnavion opened this issue Feb 24, 2019 · 8 comments
Closed

Suggest Entry::or_default for Entry::or_insert_with(Default::default) #3812

Arnavion opened this issue Feb 24, 2019 · 8 comments
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-complexity Lint: Belongs in the complexity lint group

Comments

@Arnavion
Copy link
Contributor

For code that calls .or_insert_with(Default::default) on a std::collections::btree_map::Entry or std::collections::hash_map::Entry, clippy should suggest std::collections::btree_map::Entry::or_default and std::collections::hash_map::Entry::or_default
@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy L-complexity Lint: Belongs in the complexity lint group A-lint Area: New lints labels Feb 24, 2019
@splashofcrimson
Copy link

Hey, I'm a new contributor here - is anyone working on this? If not, may I gave it a shot?

@Manishearth
Copy link
Member

Go ahead! Let us know if you need help!

@HarrisonMc555
Copy link
Contributor

HarrisonMc555 commented Apr 19, 2019 •
edited
Loading

@splashofcrimson I just wanted to let you know that if you're wondering how to get started, I think that the lint that i just wrote is really similar. You can find the code here. Hopefully you find it helpful :-)

If you don't want to do it anymore, I could probably take a shot at it.

@wookietreiber
Copy link

Without knowing clippy internals, is it possible to issue a warning for any or_insert_with(MyStruct::default), i.e. not just for Default::default?

@llogiq
Copy link
Contributor

llogiq commented Aug 27, 2019

I think the right way is to resolve the path and see if it is a Default implementation. Otherwise MyStruct could have its own default function without implementing Default (or worse, while implementing Default another way).

@cauebs
Copy link
Contributor

cauebs commented Oct 25, 2020

Seems to me no one is working on this, so I'll give it a go today!

@cauebs
Copy link
Contributor

cauebs commented Oct 25, 2020 •
edited
Loading

I understand we don't have to suggest unwrap_or_default for {Option | Result}.unwrap_or(Default::default()) because that is already covered by or_fun_call, correct?

@ebroto
Copy link
Member

ebroto commented Oct 25, 2020

I understand we don't have to suggest unwrap_or_default for {Option | Result}.unwrap_or(Default::default()) because that is already covered by or_fun_call, correct?

Yes. FYI you can test this quickly in the playground, Clippy can be executed there.

This would not be linted though:

Some(42).unwrap_or_else(Default::default);

This could be improved to suggest unwrap_or_default, but may be the subject of another issue.

And about the maps

map.entry("42").or_insert(Default::default());

Is linted, but recommends or_insert_with(Default::default), which is the subject of this issue. I think it's OK if the suggestion of one lint triggers another lint (as long as it's not circular xD).

This lint should probably be implemented in the methods module.

@cauebs cauebs mentioned this issue Oct 26, 2020
Closed
@camsteffen camsteffen mentioned this issue Jan 7, 2021
Closed
@camsteffen camsteffen mentioned this issue Feb 16, 2021
Closed
relrelb added a commit to relrelb/rust-clippy that referenced this issue Aug 16, 2022
@relrelb
Suggest Entry::or_default for Entry::or_insert(Default::default())
747ee06 
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?).

The new lint detected 1 instance in the dogfood tests, so fix it along
the way.

Fixes rust-lang#3812.
@relrelb relrelb mentioned this issue Aug 16, 2022
Merged
relrelb added a commit to relrelb/rust-clippy that referenced this issue Aug 19, 2022
@relrelb
Suggest Entry::or_default for Entry::or_insert(Default::default())
49a230b 
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?).

The new lint detected 1 instance in the dogfood tests, so fix it along
the way.

Fixes rust-lang#3812.
relrelb added a commit to relrelb/rust-clippy that referenced this issue Aug 19, 2022
@relrelb
Suggest Entry::or_default for Entry::or_insert(Default::default())
e8c8a93 
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.
bors added a commit that referenced this issue 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 added a commit that referenced this issue Sep 5, 2022
@bors
Auto merge of #9342 - relrelb:or_default, r=dswij
b763b14 
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 bors closed this as completed in f0e586c Sep 5, 2022
kraktus pushed a commit to kraktus/rust-clippy that referenced this issue Sep 5, 2022
@relrelb @kraktus
Suggest Entry::or_default for Entry::or_insert(Default::default())
72eaa07 
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.
kartva pushed a commit to kartva/rust-clippy that referenced this issue Sep 6, 2022
@relrelb @kartva
Suggest Entry::or_default for Entry::or_insert(Default::default())
d1ac8dc 
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.
kartva pushed a commit to kartva/rust-clippy that referenced this issue Sep 7, 2022
@relrelb @kartva
Suggest Entry::or_default for Entry::or_insert(Default::default())
f2b9fd2 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-complexity Lint: Belongs in the complexity lint group
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add entry_or_insert_with_default lint cauebs/rust-clippy
9 participants
@ebroto @wookietreiber @Arnavion @Manishearth @llogiq @cauebs @flip1995 @HarrisonMc555 @splashofcrimson