Skip to content

Improve map_entry suggestion #6937

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
Merged
merged 4 commits into from
Apr 16, 2021
Merged

Improve map_entry suggestion #6937

merged 4 commits into from
Apr 16, 2021

Conversation

Jarcho
Copy link
Contributor

@Jarcho Jarcho commented Mar 19, 2021

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
Copy link

r? @Manishearth

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 19, 2021
@Jarcho Jarcho force-pushed the map_entry_suggestion branch 3 times, most recently from 08d4f34 to c6feb9b Compare March 21, 2021 13:40
@Manishearth
Copy link
Member

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

@giraffate
Copy link
Contributor

@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
Copy link
Member

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

r? @giraffate
@bors delegate=giraffate

@bors
Copy link
Contributor

bors commented Mar 23, 2021

✌️ @giraffate can now approve this pull request

giraffate
giraffate reviewed Mar 23, 2021
View reviewed changes
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

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

@giraffate 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 Jarcho force-pushed the map_entry_suggestion branch 2 times, most recently from f36a1fd to e3cd56a Compare March 25, 2021 13:49
@bors
Copy link
Contributor

bors commented Mar 25, 2021

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

giraffate
giraffate suggested changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

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

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

bors commented Apr 6, 2021

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

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

bors commented Apr 6, 2021

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

@Jarcho Jarcho force-pushed the map_entry_suggestion branch from 9ac6111 to de18142 Compare April 6, 2021 20:35
@giraffate 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
@bors
Copy link
Contributor

bors commented Apr 12, 2021

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

@Y-Nak
Copy link
Contributor

Y-Nak commented Apr 12, 2021

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

Y-Nak
Y-Nak reviewed Apr 13, 2021
View reviewed changes
Copy link
Contributor

@Y-Nak Y-Nak 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.

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:

@giraffate
Copy link
Contributor

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

@Jarcho Jarcho force-pushed the map_entry_suggestion branch 2 times, most recently from 762651b to a0c4eca Compare April 14, 2021 14:36
Y-Nak
Y-Nak approved these changes Apr 15, 2021
View reviewed changes
Copy link
Contributor

@Y-Nak Y-Nak 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.

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

@Jarcho 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
@Jarcho
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
@Jarcho
map_entry improvements
b63a5b5 
Lint `if _.[!]contains_key(&_) { .. } else { .. }` so long as one of the branches contains an insertion.
@Jarcho
map_entry improvements
3323ff7 
Suggest using `or_insert_with` when possible
@Jarcho
Minor cleanup of map_entry and a few additional tests.
bcf3488
@Jarcho Jarcho force-pushed the map_entry_suggestion branch from 12e96e8 to bcf3488 Compare April 15, 2021 12:25
@giraffate
Copy link
Contributor

@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
Copy link
Contributor Author

Jarcho commented Apr 15, 2021

No worries. It's a big change.

giraffate
giraffate approved these changes Apr 16, 2021
View reviewed changes
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

It looks good to me, thanks for your work!

@giraffate
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Apr 16, 2021

📌 Commit bcf3488 has been approved by giraffate

@bors
Copy link
Contributor

bors commented Apr 16, 2021

⌛ Testing commit bcf3488 with merge 1e0a3ff...

@bors
Copy link
Contributor

bors commented Apr 16, 2021

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

@bors bors merged commit 1e0a3ff into rust-lang:master Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 left review comments

+2 more reviewers

@Y-Nak Y-Nak Y-Nak approved these changes

@giraffate giraffate giraffate approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@giraffate giraffate

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
7 participants
@Jarcho @rust-highfive @Manishearth @giraffate @bors @Y-Nak @flip1995