rust-lang
Add new lint `hashset_insert_after_contains`
This PR closes https://github.com/rust-lang/rust-clippy/issues/11103.
This is my first PR creating a new lint (and the second attempt of creating this PR, the first one I was not able to continue because of personal reasons). Thanks for the patience :)
The idea of the lint is to find insert in hashmanps inside if staments that are checking if the hashmap contains the same value that is being inserted. This is not necessary since you could simply call the insert and check for the bool returned if you still need the if statement.
changelog: new lint: [hashset_insert_after_contains]
r? @llogiq
rustbot has assigned @llogiq. They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
:umbrella: The latest upstream changes (presumably #12849) made this pull request unmergeable. Please resolve the merge conflicts.
Sorry this is taking so long, I am unfortunately quite busy at the moment. I'll ping you once I've run the check so you don't need to rebase more than once.
I finally came around to do a lintcheck run, and it looked OK. So r=me after a rebase and the docs fix @bitfield suggested.
I finally came around to do a lintcheck run, and it looked OK. So r=me after a rebase and the docs fix @bitfield suggested.
Oops. It looks like I can't r=you... :)
:pushpin: Commit 4e71fc4302898e28d7e4c570edf2f3d1feffe116 has been approved by llogiq
It is now in the queue for this repository.
:hourglass: Testing commit 4e71fc4302898e28d7e4c570edf2f3d1feffe116 with merge d2400a49a43669025c37031791c1d200852ef160...
:sunny: Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test Approved by: llogiq Pushing d2400a49a43669025c37031791c1d200852ef160 to master...
Awesome work! One question -- would it make sense to generalize the name of this lint to insert_after_contains ? This way it can also cover HashMap and possibly other dictionary-like patterns?
@nyurik: Currently the lint only catches HashMaps, but I'd welcome a PR to change that along with the name.
I could do a quick PR to rename the lint, but implementing it for other types might take a bit longer, and might not even make the cutoff for the next release (at which point the lint name would become permanent)
Oh, my apologies, this was already renamed in the code to set_contains_or_insert. Some options of a generalized name:
-
contains_or_insert(short and to the point?) -
collection_contains_or_insert - ???