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

Improve accuracy of mut_key #7640

Merged
merged 2 commits into from
Sep 14, 2021
Merged

Improve accuracy of mut_key #7640

merged 2 commits into from
Sep 14, 2021

Conversation

kneasle
Copy link
Contributor

@kneasle kneasle commented Sep 5, 2021

Fixes #6745.

Whilst writing the tests for this, I noticed what I believe is a false negative (the code in @xFrednet's comment doesn't trigger the lint). Currently the tests contain a case for this (which is blatantly ignored), but I'm not at all sure how to implement this (since the lint currently behaves completely differently for ADTs). I'm not sure what should be done - on the one hand the extra test cases are misleading, but on the other hand they don't cause much harm and would save effort for anyone fixing that false negative.


changelog: Improve accuracy of clippy::mutable_key_type.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @camsteffen (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 5, 2021
clippy_lints/src/mut_key.rs Outdated Show resolved Hide resolved
tests/ui/mut_key.rs Outdated Show resolved Hide resolved
@camsteffen
Copy link
Contributor

Whilst writing the tests for this, I noticed what I believe is a false negative (the code in @xFrednet's comment doesn't trigger the lint). Currently the tests contain a case for this (which is blatantly ignored), but I'm not at all sure how to implement this (since the lint currently behaves completely differently for ADTs). I'm not sure what should be done - on the one hand the extra test cases are misleading, but on the other hand they don't cause much harm and would save effort for anyone fixing that false negative.

My personal opinion is that this case is out of scope for the lint. I think if a user defines a type with a pointer and a custom impl Hash, they are on their own.

clippy_lints/src/mut_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/mut_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/mut_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/mut_key.rs Show resolved Hide resolved
tests/ui/mut_key.rs Outdated Show resolved Hide resolved
tests/ui/mut_key.rs Outdated Show resolved Hide resolved
@kneasle kneasle changed the title Fix FP when using raw pointers as hashed keys Improve accuracy of mut_key Sep 9, 2021
Copy link
Contributor

@camsteffen camsteffen left a comment

Choose a reason for hiding this comment

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

A few nits and this is nearly ready. Please update the changelog in your post to reflect added changes.

clippy_lints/src/mut_key.rs Outdated Show resolved Hide resolved
tests/ui/mut_key.rs Outdated Show resolved Hide resolved
clippy_lints/src/mut_key.rs Show resolved Hide resolved
@kneasle kneasle requested a review from camsteffen September 10, 2021 21:37
@kneasle
Copy link
Contributor Author

kneasle commented Sep 12, 2021

Wait hold on what's going on with the unit tests? It seems to be that my local build of clippy is producing different test results to the build in the PR checker. So when I cargo dev bless my local copy it causes the PR check to fail. @camsteffen what d'ya reckon I should do? I can update my PR branch to be what the test runner wants (which will let this PR be merged), but I feel like I can't be alone in having this discrepancy.

@camsteffen
Copy link
Contributor

Your test output is not including changes from recent PRs. Try rebasing, don't run with TESTNAME, and maybe try cargo clean. Or just don't check in unrelated changes.

@kneasle
Copy link
Contributor Author

kneasle commented Sep 13, 2021

Aha I've figured out what's going - I appear to not have target-dir not set in <clippy>/.cargo/config. So it was compiling my new changes into ~/.build/rust but the test runner was still reading the old version from <clippy>/test.

@kneasle
Copy link
Contributor Author

kneasle commented Sep 13, 2021

Yes finally! The tests are passing at last.

@camsteffen
Copy link
Contributor

Please squash some commits and I'll merge.

@kneasle
Copy link
Contributor Author

kneasle commented Sep 14, 2021

Please squash some commits and I'll merge.

Rebased down to two commits :).

@camsteffen
Copy link
Contributor

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Sep 14, 2021

📌 Commit b2ffb28 has been approved by camsteffen

@bors
Copy link
Contributor

bors commented Sep 14, 2021

⌛ Testing commit b2ffb28 with merge 746a005...

@bors
Copy link
Contributor

bors commented Sep 14, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 746a005 to master...

@bors bors merged commit 746a005 into rust-lang:master Sep 14, 2021
@kneasle
Copy link
Contributor Author

kneasle commented Sep 14, 2021

🎉🎉🎉🎉

@kneasle kneasle deleted the mut-key-false-positive branch January 6, 2022 23:55
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.

mutable_key_type false positive for raw pointers
4 participants