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.

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

Fix incorrect swap suggestion #4478

Merged
merged 2 commits into from
Sep 11, 2019
Merged

Fix incorrect swap suggestion #4478

merged 2 commits into from
Sep 11, 2019

Conversation

tsurai
Copy link
Contributor

@tsurai tsurai commented Aug 31, 2019

Clippy suggests using swap on fields belonging to the same owner causing two mutable borrows of the owner.

Disclosure: This is my first time working with clippy and rusts HIR. I'm very grateful for assistance and suggestions to improve the PR.

fixes #981
changelog: Fix false positive in manual_swap lint

Clippy suggests using swap on fields belonging to the same owner
causing two mutable borrows of the owner

Fixes rust-lang#981

Signed-off-by: Cristian Kubis <cristian.kubis@tsunix.de>
@flip1995
Copy link
Member

flip1995 commented Sep 2, 2019

Thanks for fixing this issue!

First step would be to add tests for all the cases listed in #981. So slice access, field access and field access through a slice. I think your code currently just handles the field access case. Once you added the tests we can take the next step in fixing this issue.

See also https://github.com/rust-lang/rust-clippy/blob/master/doc/adding_lints.md#Testing on writing tests in Clippy and adding/fixing lints.

@flip1995 flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Sep 2, 2019
These tests make sure that the swap warning will not be triggered
for expressions that will cause multiple mutable references of the
same owner
@tsurai
Copy link
Contributor Author

tsurai commented Sep 2, 2019

Thanks for the feedback!

I've added test cases for the field and field through slice access. Pure slice access alraedy has its own logic and test case.

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Let's wait for travis and then merge this.

@flip1995 flip1995 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 Sep 2, 2019
@flip1995
Copy link
Member

Thanks! (Sorry, forgot about this)

@bors r+

@bors
Copy link
Contributor

bors commented Sep 11, 2019

📌 Commit 9041856 has been approved by flip1995

bors added a commit that referenced this pull request Sep 11, 2019
Fix incorrect swap suggestion

Clippy suggests using swap on fields belonging to the same owner causing two mutable borrows of the owner.

Disclosure: This is my first time working with clippy and rusts HIR. I'm very grateful for assistance and suggestions to improve the PR.

fixes #981
changelog: none
@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Testing commit 9041856 with merge 83dc06c...

@flip1995
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Testing commit 9041856 with merge 4f9e4ea...

bors added a commit that referenced this pull request Sep 11, 2019
Fix incorrect swap suggestion

Clippy suggests using swap on fields belonging to the same owner causing two mutable borrows of the owner.

Disclosure: This is my first time working with clippy and rusts HIR. I'm very grateful for assistance and suggestions to improve the PR.

fixes #981
changelog: Fix false positive in `manual_swap` lint
@flip1995
Copy link
Member

cc #4478 (comment)

added changelog.

@bors
Copy link
Contributor

bors commented Sep 11, 2019

💔 Test failed - checks-travis

@phansch
Copy link
Member

phansch commented Sep 11, 2019

@bors retry (Timeout (30 minutes) reached. Terminating "./ci/base-tests.sh")

bors added a commit that referenced this pull request Sep 11, 2019
Fix incorrect swap suggestion

Clippy suggests using swap on fields belonging to the same owner causing two mutable borrows of the owner.

Disclosure: This is my first time working with clippy and rusts HIR. I'm very grateful for assistance and suggestions to improve the PR.

fixes #981
changelog: Fix false positive in `manual_swap` lint
@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Testing commit 9041856 with merge 6ca5b20...

@bors
Copy link
Contributor

bors commented Sep 11, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 6ca5b20 to master...

@bors bors merged commit 9041856 into rust-lang:master Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use swap or replace as suggested by manual_swap
4 participants