Add `entry_or_insert_with_default` lint #6228

cauebs · 2020-10-26T07:40:01Z

Checks for calls to .or_insert_with on Entry (for BTreeMap or HashMap) with Default::default or equivalent as the argument, and suggests .or_default.

Feedback is welcome!

changelog: none

rust-highfive · 2020-10-26T07:40:04Z

r? @Manishearth

(rust_highfive has picked a reviewer for you, use r? to override)

cauebs · 2020-10-26T14:30:08Z

Had some trouble with the documentation test. Still not sure how to run them locally... All good now.

bors · 2020-10-29T15:56:57Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

camsteffen · 2020-10-31T13:48:05Z

clippy_lints/src/methods/mod.rs

+        }
+    }
+}
+


I wonder if this function should be merged with check_unwrap_or_default a few lines below

Not sure, but I would need some help figuring out how, to be honest.

Yes, this should be merged. You'll want to hoist that function out and make the unwrap_or part a parameter

cauebs · 2020-10-31T19:40:34Z

@Manishearth can you just add the hacktoberfest-accepted label to this PR before monday? We can iron things out later if necessary.

Manishearth

We don't use that label, but marking this as approved should be enough

Manishearth · 2020-11-01T03:41:21Z

clippy_lints/src/methods/mod.rs

@@ -1515,6 +1556,9 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
            ["unwrap_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "unwrap_or"),
            ["get_or_insert_with", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "get_or_insert"),
            ["ok_or_else", ..] => unnecessary_lazy_eval::lint(cx, expr, arg_lists[0], "ok_or"),
+            ["or_insert_with", ..] => {
+                lint_entry_or_insert_with_default(cx, method_spans[0].with_hi(expr.span.hi()), arg_lists[0])


I don't think you should need to do span splicing

Can you elaborate? I confess I didn't quite understand this part and just tried to adapt what was being done for the others.

Actually, never mind, some of the others do span splicing too. I just don't think the splicing should be here, but if you're merging this with the check_unwrap_or_default thing then it's going to go away from here anyway.

Manishearth · 2020-11-01T03:43:25Z

clippy_lints/src/methods/mod.rs

+        }
+    }
+}
+


Yes, this should be merged. You'll want to hoist that function out and make the unwrap_or part a parameter

giraffate · 2020-11-23T23:47:04Z

ping from triage @cauebs. There seems to be a small fix left to be done. Do you have any questions on how to proceed here?

cauebs · 2020-11-24T02:22:19Z

I believe it needs a small rewrite, and I haven't had the time to do it. Next week I'll probably be able to get this done. Sorry for the delay.

giraffate · 2020-12-09T02:40:06Z

ping from triage @cauebs. Sorry for pinging again, but do you have any update on this?

giraffate · 2020-12-22T23:33:20Z

ping from triage @cauebs. Thanks for contributing Clippy! Sadly this PR didn't have any update in the last 2 weeks. If you have more time to continue working on this, feel free to reopen.

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.

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.

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())`

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.

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())`

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.

jonboh · 2023-10-10T18:29:23Z

@rustbot label -S-inactive-closed

rust-highfive assigned Manishearth Oct 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 26, 2020

cauebs force-pushed the entry-or-insert-with-default branch from aee2279 to 8a98057 Compare October 26, 2020 14:02

Add entry_or_insert_with_default lint

286cb69

cauebs force-pushed the entry-or-insert-with-default branch from 8a98057 to 286cb69 Compare October 26, 2020 14:15

camsteffen reviewed Oct 31, 2020

View reviewed changes

Manishearth approved these changes Nov 1, 2020

View reviewed changes

cauebs marked this pull request as draft November 1, 2020 21:52

giraffate closed this Dec 22, 2020

giraffate added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 22, 2020

relrelb mentioned this pull request Aug 16, 2022

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

Merged

rustbot removed the S-inactive-closed Status: Closed due to inactivity label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `entry_or_insert_with_default` lint #6228

Add `entry_or_insert_with_default` lint #6228

cauebs commented Oct 26, 2020

rust-highfive commented Oct 26, 2020

cauebs commented Oct 26, 2020

bors commented Oct 29, 2020

camsteffen Oct 31, 2020

cauebs Oct 31, 2020

Manishearth Nov 1, 2020

cauebs commented Oct 31, 2020

Manishearth left a comment

Manishearth Nov 1, 2020

cauebs Nov 1, 2020

Manishearth Nov 1, 2020

Manishearth Nov 1, 2020

giraffate commented Nov 23, 2020

cauebs commented Nov 24, 2020

giraffate commented Dec 9, 2020

giraffate commented Dec 22, 2020

jonboh commented Oct 10, 2023

Add entry_or_insert_with_default lint #6228

Add entry_or_insert_with_default lint #6228

Conversation

cauebs commented Oct 26, 2020

rust-highfive commented Oct 26, 2020

cauebs commented Oct 26, 2020

bors commented Oct 29, 2020

camsteffen Oct 31, 2020

Choose a reason for hiding this comment

cauebs Oct 31, 2020

Choose a reason for hiding this comment

Manishearth Nov 1, 2020

Choose a reason for hiding this comment

cauebs commented Oct 31, 2020

Manishearth left a comment

Choose a reason for hiding this comment

Manishearth Nov 1, 2020

Choose a reason for hiding this comment

cauebs Nov 1, 2020

Choose a reason for hiding this comment

Manishearth Nov 1, 2020

Choose a reason for hiding this comment

Manishearth Nov 1, 2020

Choose a reason for hiding this comment

giraffate commented Nov 23, 2020

cauebs commented Nov 24, 2020

giraffate commented Dec 9, 2020

giraffate commented Dec 22, 2020

jonboh commented Oct 10, 2023

Add `entry_or_insert_with_default` lint #6228

Add `entry_or_insert_with_default` lint #6228