New lint `hashset_insert_after_contains` #11296

lochetti · 2023-08-05T07:50:51Z

This PR closes 11103.

This is my first PR creating a new lint, so I am opening as a draft and thanks for the patience :)

I also have used the simpler span_lint() when emitting the lint. If the reviewers think that it is not enough, I can try to evolve the code the use the others span_lint functions.

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]

rustbot · 2023-08-05T07:50:55Z

r? @Manishearth

(rustbot has picked a reviewer for you, use r? to override)

Centri3

Pretty good for a first lint, thanks!

clippy_lints/src/hashset_insert_after_contains.rs

lochetti · 2023-08-07T12:05:26Z

Pretty good for a first lint, thanks!

Thank you for the review, @Centri3 ! The code is looking way simpler with your suggestions :)

Could you please take a look again and let me know what you think? The changes are in the commit

Manishearth · 2023-08-07T16:39:36Z

r? @Centri3 you've already reviewed most of this 😄

Centri3 · 2023-08-08T23:53:34Z

clippy_lints/src/hashset_insert_after_contains.rs

+                kind: ExprKind::AddrOf(_, _, value),
+                span: value_span,
+                ..
+            },


This doesn't account for if it's not a reference. Of course, contains takes a reference but it's free to be x or something if x is &Q. I'd say just call peel_borrows here and also do so for insert_expr.value.

Centri3 · 2023-08-08T23:58:22Z

clippy_lints/src/hashset_insert_after_contains.rs

+    ) = expr.kind
+    {
+        let receiver_ty = cx.typeck_results().expr_ty(receiver);
+        if value_span.ctxt() == expr.span.ctxt()


If the first argument and the call itself are both from the same expansion, this won't catch it. I'd just recommend calling from_expansion on value_span since that'll also check expr.

Let's see if I understood the changes that I need to apply here:

Remove the if value_span.ctxt() == expr.span.ctxt() comparison.

Check if value_span is not from an expansion. If it is from an expasion, we will not lint

Check if value_span is nto from external_macro. If it is from an external macro, we will not lint.

Is it correct?

clippy_lints/src/hashset_insert_after_contains.rs

lochetti · 2023-08-11T08:38:24Z

Hey @Centri3 !

I think I have addressed almost everything from you comments.
I have made the lint work for three more cases that were not being linted before:

if (&set).contains(&value) {
    set.insert(value);
}

let borrow_value = &6;
if !set.contains(borrow_value) {
    set.insert(*borrow_value);
}

let borrow_set = &mut set;
if !borrow_set.contains(&value) {
    borrow_set.insert(value);
}

But I could not understand your comment about from_expansion, I am sorry.
Do you mind going in more details, please? Probably I am missing some knowledge here. If you could also come up with a specific test case -if it is not hard, of course- that is not being addressed, it would make it easier for me to understand :)

Thanks! :)

Centri3 · 2023-08-14T01:29:53Z

The issue with ctxt here is that if they both come from the same context, then I think this will wrongly trigger. This is fine for root (no expansions), this is what you want to check for after all, but if they both come from the same expansion it will wrongly trigger.

However, I think ignoring all expansions isn't the right way to go about this; if it happens to expand to code that references a local, and also inserts into it, then it was likely intentionally done (if the local isn't also from an expansion, of course). So if it's not external, it should still lint.

Centri3 · 2023-08-14T01:30:26Z

clippy_lints/src/hashset_insert_after_contains.rs

+
+impl<'tcx> LateLintPass<'tcx> for HashsetInsertAfterContains {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if !expr.span.from_expansion()


This probably also needs an is_from_proc_macro check right before emitting

Centri3 · 2023-08-14T01:44:43Z

clippy_lints/src/hashset_insert_after_contains.rs

+    span: Span,
+}
+fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<ContainsExpr<'tcx>> {
+    let expr = peel_hir_expr_while(expr, |e| match e.kind {


This could be an if let.

Centri3 · 2023-08-14T01:45:05Z

clippy_lints/src/hashset_insert_after_contains.rs

+        && find_insert_calls(cx, &contains_expr, then_expr) {
+            span_lint_and_then(


Very nitpicky, but:

Suggested change

&& find_insert_calls(cx, &contains_expr, then_expr) {

span_lint_and_then(

&& find_insert_calls(cx, &contains_expr, then_expr)

{

span_lint_and_then(

Centri3 · 2023-08-14T01:45:17Z

clippy_lints/src/hashset_insert_after_contains.rs

+fn try_parse_insert<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<InsertExpr<'tcx>> {
+    if let ExprKind::MethodCall(path, receiver, [value], _) = expr.kind {
+        let value = value.peel_borrows();
+        let value = peel_hir_expr_while(value, |e| match e.kind {


bors · 2023-08-23T12:28:11Z

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

Centri3 · 2024-02-06T22:35:30Z

Hey @lochetti, any progress on this? This doesn't need too many more changes. Do you need help on anything?

@rustbot author

lochetti · 2024-02-07T08:28:27Z

Hey @Centri3 hello! Sorry for that long delay, but I think I will not be able to finish the PR 😬
I am sorry.

Centri3 · 2024-02-07T23:19:12Z

No worries about it! I'll close it then and others can finish it if they wish. Feel free to create a new PR if you wish :3

lochetti · 2024-05-31T16:02:22Z

I have opened a new PR to try to continue the work :)
It is the #12873

rustbot assigned Manishearth Aug 5, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 5, 2023

lochetti force-pushed the issue_11103 branch from c955942 to 61197cc Compare August 5, 2023 08:00

new lint hashset_insert_after_contains

4dff96e

lochetti force-pushed the issue_11103 branch from 61197cc to 4dff96e Compare August 5, 2023 09:55

Centri3 reviewed Aug 6, 2023

View reviewed changes

Addressing reviewer comments

a64e5fe

rustbot assigned Centri3 and unassigned Manishearth Aug 7, 2023

lochetti marked this pull request as ready for review August 8, 2023 07:27

Centri3 reviewed Aug 9, 2023

View reviewed changes

Making it work for cases with borrows and derefs

a6752a7

Centri3 reviewed Aug 14, 2023

View reviewed changes

Centri3 reviewed Aug 20, 2023

View reviewed changes

rustbot 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 Feb 6, 2024

Centri3 closed this Feb 7, 2024

Centri3 added the S-inactive-closed Status: Closed due to inactivity label Feb 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint `hashset_insert_after_contains` #11296

New lint `hashset_insert_after_contains` #11296

lochetti commented Aug 5, 2023

rustbot commented Aug 5, 2023

Centri3 left a comment

lochetti commented Aug 7, 2023

Manishearth commented Aug 7, 2023

Centri3 Aug 8, 2023

Centri3 Aug 8, 2023

lochetti Aug 14, 2023

lochetti commented Aug 11, 2023

Centri3 commented Aug 14, 2023

Centri3 Aug 14, 2023

Centri3 Aug 14, 2023

Centri3 Aug 14, 2023

Centri3 Aug 14, 2023

bors commented Aug 23, 2023

Centri3 commented Feb 6, 2024

lochetti commented Feb 7, 2024

Centri3 commented Feb 7, 2024

lochetti commented May 31, 2024

		&& find_insert_calls(cx, &contains_expr, then_expr) {
		span_lint_and_then(

New lint hashset_insert_after_contains #11296

New lint hashset_insert_after_contains #11296

Conversation

lochetti commented Aug 5, 2023

rustbot commented Aug 5, 2023

Centri3 left a comment

Choose a reason for hiding this comment

lochetti commented Aug 7, 2023

Manishearth commented Aug 7, 2023

Centri3 Aug 8, 2023

Choose a reason for hiding this comment

Centri3 Aug 8, 2023

Choose a reason for hiding this comment

lochetti Aug 14, 2023

Choose a reason for hiding this comment

lochetti commented Aug 11, 2023

Centri3 commented Aug 14, 2023

Centri3 Aug 14, 2023

Choose a reason for hiding this comment

Centri3 Aug 14, 2023

Choose a reason for hiding this comment

Centri3 Aug 14, 2023

Choose a reason for hiding this comment

Centri3 Aug 14, 2023

Choose a reason for hiding this comment

bors commented Aug 23, 2023

Centri3 commented Feb 6, 2024

lochetti commented Feb 7, 2024

Centri3 commented Feb 7, 2024

lochetti commented May 31, 2024

New lint `hashset_insert_after_contains` #11296

New lint `hashset_insert_after_contains` #11296