Improve `map_entry` suggestion by Jarcho · Pull Request #6937 · rust-lang/rust-clippy

Jarcho · 2021-03-19T18:13:06Z

fixes: #5176
fixes: #4674
fixes: #4664
fixes: #1450

Still need to handle the value returned by insert correctly.

changelog: Improve map_entry suggestion. Will now suggest or_insert, insert_with or match _.entry(_) as appopriate.
changelog: Fix map_entry false positives where the entry api can't be used. e.g. when the map is used for multiple things.

rust-highfive · 2021-03-19T18:13:09Z

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

Manishearth · 2021-03-23T00:20:42Z

@giraffate Woudl you like to review this PR? No worries if you can't.

giraffate · 2021-03-23T00:46:59Z

@Manishearth Yes, I'll do it. ( I think it will take a couple of days until first review, since the diff is so large. )

Manishearth · 2021-03-23T02:02:31Z

Totally fair, and feel free to pass it back to me if you need

r? @giraffate
@bors delegate=giraffate

bors · 2021-03-23T02:02:33Z

✌️ @giraffate can now approve this pull request

giraffate

@Jarcho Sorry, I'm just starting to read the code and still in the middle of review, but I made some comments.

clippy_lints/src/entry.rs

bors · 2021-03-25T18:08:00Z

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

giraffate

Sorry, I'm in the middle of review, but I made some review on this.

clippy_lints/src/entry.rs

tests/ui/entry_unfixable.rs

bors · 2021-04-06T15:44:02Z

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

bors · 2021-04-06T17:06:49Z

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

bors · 2021-04-12T09:05:47Z

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

Y-Nak · 2021-04-12T14:25:28Z

@Jarcho I'm still reviewing this and will finish in the next 24 hours.

Y-Nak

I looked through the changes and left some comments, but it takes more time to finish the review.

Btw, I ran lintcheck, this lint was triggered only 1 time and that seemed like true-positive:+1:

clippy_lints/src/entry.rs

clippy_utils/src/lib.rs

clippy_lints/src/entry.rs

giraffate · 2021-04-14T13:23:24Z

@Y-Nak Thanks for your review! Nice catches!

Y-Nak

I finished the review and added some comments.
@giraffate Could you have a final look?

clippy_lints/src/entry.rs

Fix false positives where the map is used before inserting into the map. Fix false positives where two insertions happen. Suggest using `if let Entry::Vacant(e) = _.entry(_)` when `or_insert` might be a semantic change

Lint `if _.[!]contains_key(&_) { .. } else { .. }` so long as one of the branches contains an insertion.

Suggest using `or_insert_with` when possible

giraffate · 2021-04-15T14:40:48Z

@Y-Nak Yes, thanks!

@Jarcho I think overall looks good. Just to be sure I will look through the changes again in the next 24 hour. Thanks for your patience!

Jarcho · 2021-04-15T20:20:26Z

No worries. It's a big change.

giraffate

It looks good to me, thanks for your work!

giraffate · 2021-04-16T13:23:18Z

@bors r+

bors · 2021-04-16T13:23:20Z

📌 Commit bcf3488 has been approved by giraffate

bors · 2021-04-16T13:23:27Z

⌛ Testing commit bcf3488 with merge 1e0a3ff...

bors · 2021-04-16T13:36:27Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 1e0a3ff to master...

rust-highfive assigned Manishearth Mar 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 19, 2021

Jarcho force-pushed the map_entry_suggestion branch 3 times, most recently from 08d4f34 to c6feb9b Compare March 21, 2021 13:40

rust-highfive unassigned Manishearth Mar 23, 2021

rust-highfive assigned giraffate Mar 23, 2021

giraffate reviewed Mar 23, 2021

View reviewed changes

clippy_lints/src/entry.rs Show resolved Hide resolved

clippy_lints/src/entry.rs Outdated Show resolved Hide resolved

giraffate 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 Mar 24, 2021

Jarcho force-pushed the map_entry_suggestion branch 2 times, most recently from f36a1fd to e3cd56a Compare March 25, 2021 13:49

giraffate suggested changes Mar 26, 2021

View reviewed changes

Jarcho force-pushed the map_entry_suggestion branch 3 times, most recently from 2458362 to 0bfc9c9 Compare April 3, 2021 12:29

Jarcho force-pushed the map_entry_suggestion branch from 0bfc9c9 to 9ac6111 Compare April 6, 2021 16:16

Jarcho force-pushed the map_entry_suggestion branch from 9ac6111 to de18142 Compare April 6, 2021 20:35

giraffate 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 Apr 12, 2021

Y-Nak reviewed Apr 13, 2021

View reviewed changes

Jarcho force-pushed the map_entry_suggestion branch 2 times, most recently from 762651b to a0c4eca Compare April 14, 2021 14:36

Y-Nak approved these changes Apr 15, 2021

View reviewed changes

Jarcho force-pushed the map_entry_suggestion branch 2 times, most recently from ff78dd4 to 12e96e8 Compare April 15, 2021 12:09

Jarcho added 4 commits April 15, 2021 08:19

Improve map_entry lint

ce5e927

Fix false positives where the map is used before inserting into the map. Fix false positives where two insertions happen. Suggest using `if let Entry::Vacant(e) = _.entry(_)` when `or_insert` might be a semantic change

map_entry improvements

b63a5b5

Lint `if _.[!]contains_key(&_) { .. } else { .. }` so long as one of the branches contains an insertion.

map_entry improvements

3323ff7

Suggest using `or_insert_with` when possible

Minor cleanup of map_entry and a few additional tests.

bcf3488

Jarcho force-pushed the map_entry_suggestion branch from 12e96e8 to bcf3488 Compare April 15, 2021 12:25

giraffate approved these changes Apr 16, 2021

View reviewed changes

bors merged commit 1e0a3ff into rust-lang:master Apr 16, 2021

Conversation

Jarcho commented Mar 19, 2021

Uh oh!

rust-highfive commented Mar 19, 2021

Uh oh!

Manishearth commented Mar 23, 2021

Uh oh!

giraffate commented Mar 23, 2021

Uh oh!

Manishearth commented Mar 23, 2021

Uh oh!

bors commented Mar 23, 2021

Uh oh!

giraffate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bors commented Mar 25, 2021

Uh oh!

giraffate left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bors commented Apr 6, 2021

Uh oh!

bors commented Apr 6, 2021

Uh oh!

bors commented Apr 12, 2021

Uh oh!

Y-Nak commented Apr 12, 2021

Uh oh!

Y-Nak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giraffate commented Apr 14, 2021

Uh oh!

Y-Nak left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giraffate commented Apr 15, 2021

Uh oh!

Jarcho commented Apr 15, 2021

Uh oh!

giraffate left a comment

Choose a reason for hiding this comment

Uh oh!

giraffate commented Apr 16, 2021

Uh oh!

bors commented Apr 16, 2021

Uh oh!

bors commented Apr 16, 2021

Uh oh!

bors commented Apr 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Y-Nak left a comment •

edited

Loading

Y-Nak left a comment •

edited

Loading