Auto merge of #12873 - lochetti:issue_11103, r=llogiq

bors · bors · commit d2400a49a436 · 2024-07-04T05:39:28.000Z
Add new lint `hashset_insert_after_contains` This PR closes #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]
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5798,6 +5798,7 @@ Released 2018-09-13
 [`semicolon_outside_block`]: https://rust-lang.github.io/rust-clippy/master/index.html#semicolon_outside_block
 [`separated_literal_suffix`]: https://rust-lang.github.io/rust-clippy/master/index.html#separated_literal_suffix
 [`serde_api_misuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#serde_api_misuse
+[`set_contains_or_insert`]: https://rust-lang.github.io/rust-clippy/master/index.html#set_contains_or_insert
 [`shadow_reuse`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_reuse
 [`shadow_same`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_same
 [`shadow_unrelated`]: https://rust-lang.github.io/rust-clippy/master/index.html#shadow_unrelated
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -645,6 +645,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::semicolon_block::SEMICOLON_OUTSIDE_BLOCK_INFO,
     crate::semicolon_if_nothing_returned::SEMICOLON_IF_NOTHING_RETURNED_INFO,
     crate::serde_api::SERDE_API_MISUSE_INFO,
+    crate::set_contains_or_insert::SET_CONTAINS_OR_INSERT_INFO,
     crate::shadow::SHADOW_REUSE_INFO,
     crate::shadow::SHADOW_SAME_INFO,
     crate::shadow::SHADOW_UNRELATED_INFO,
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -318,6 +318,7 @@ mod self_named_constructors;
 mod semicolon_block;
 mod semicolon_if_nothing_returned;
 mod serde_api;
+mod set_contains_or_insert;
 mod shadow;
 mod significant_drop_tightening;
 mod single_call_fn;
@@ -1172,6 +1173,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     });
     store.register_late_pass(move |_| Box::new(string_patterns::StringPatterns::new(msrv())));
     store.register_early_pass(|| Box::new(field_scoped_visibility_modifiers::FieldScopedVisibilityModifiers));
+    store.register_late_pass(|_| Box::new(set_contains_or_insert::HashsetInsertAfterContains));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 
diff --git a/clippy_lints/src/set_contains_or_insert.rs b/clippy_lints/src/set_contains_or_insert.rs
@@ -0,0 +1,125 @@
+use std::ops::ControlFlow;
+
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::ty::is_type_diagnostic_item;
+use clippy_utils::visitors::for_each_expr;
+use clippy_utils::{higher, peel_hir_expr_while, SpanlessEq};
+use rustc_hir::{Expr, ExprKind, UnOp};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::declare_lint_pass;
+use rustc_span::symbol::Symbol;
+use rustc_span::{sym, Span};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `contains` to see if a value is not
+    /// present on `HashSet` followed by a `insert`.
+    ///
+    /// ### Why is this bad?
+    /// Using just `insert` and checking the returned `bool` is more efficient.
+    ///
+    /// ### Known problems
+    /// In case the value that wants to be inserted is borrowed and also expensive or impossible
+    /// to clone. In such a scenario, the developer might want to check with `contains` before inserting,
+    /// to avoid the clone. In this case, it will report a false positive.
+    ///
+    /// ### Example
+    /// ```rust
+    /// use std::collections::HashSet;
+    /// let mut set = HashSet::new();
+    /// let value = 5;
+    /// if !set.contains(&value) {
+    ///     set.insert(value);
+    ///     println!("inserted {value:?}");
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::collections::HashSet;
+    /// let mut set = HashSet::new();
+    /// let value = 5;
+    /// if set.insert(&value) {
+    ///     println!("inserted {value:?}");
+    /// }
+    /// ```
+    #[clippy::version = "1.80.0"]
+    pub SET_CONTAINS_OR_INSERT,
+    nursery,
+    "call to `HashSet::contains` followed by `HashSet::insert`"
+}
+
+declare_lint_pass!(HashsetInsertAfterContains => [SET_CONTAINS_OR_INSERT]);
+
+impl<'tcx> LateLintPass<'tcx> for HashsetInsertAfterContains {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if !expr.span.from_expansion()
+            && let Some(higher::If {
+                cond: cond_expr,
+                then: then_expr,
+                ..
+            }) = higher::If::hir(expr)
+            && let Some(contains_expr) = try_parse_op_call(cx, cond_expr, sym!(contains))//try_parse_contains(cx, cond_expr)
+            && let Some(insert_expr) = find_insert_calls(cx, &contains_expr, then_expr)
+        {
+            span_lint(
+                cx,
+                SET_CONTAINS_OR_INSERT,
+                vec![contains_expr.span, insert_expr.span],
+                "usage of `HashSet::insert` after `HashSet::contains`",
+            );
+        }
+    }
+}
+
+struct OpExpr<'tcx> {
+    receiver: &'tcx Expr<'tcx>,
+    value: &'tcx Expr<'tcx>,
+    span: Span,
+}
+
+fn try_parse_op_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, symbol: Symbol) -> Option<OpExpr<'tcx>> {
+    let expr = peel_hir_expr_while(expr, |e| {
+        if let ExprKind::Unary(UnOp::Not, e) = e.kind {
+            Some(e)
+        } else {
+            None
+        }
+    });
+
+    if let ExprKind::MethodCall(path, receiver, [value], span) = expr.kind {
+        let value = value.peel_borrows();
+        let value = peel_hir_expr_while(value, |e| {
+            if let ExprKind::Unary(UnOp::Deref, e) = e.kind {
+                Some(e)
+            } else {
+                None
+            }
+        });
+        let receiver = receiver.peel_borrows();
+        let receiver_ty = cx.typeck_results().expr_ty(receiver).peel_refs();
+        if value.span.eq_ctxt(expr.span)
+            && is_type_diagnostic_item(cx, receiver_ty, sym::HashSet)
+            && path.ident.name == symbol
+        {
+            return Some(OpExpr { receiver, value, span });
+        }
+    }
+    None
+}
+
+fn find_insert_calls<'tcx>(
+    cx: &LateContext<'tcx>,
+    contains_expr: &OpExpr<'tcx>,
+    expr: &'tcx Expr<'_>,
+) -> Option<OpExpr<'tcx>> {
+    for_each_expr(cx, expr, |e| {
+        if let Some(insert_expr) = try_parse_op_call(cx, e, sym!(insert))
+            && SpanlessEq::new(cx).eq_expr(contains_expr.receiver, insert_expr.receiver)
+            && SpanlessEq::new(cx).eq_expr(contains_expr.value, insert_expr.value)
+        {
+            ControlFlow::Break(insert_expr)
+        } else {
+            ControlFlow::Continue(())
+        }
+    })
+}
diff --git a/tests/ui/set_contains_or_insert.rs b/tests/ui/set_contains_or_insert.rs
@@ -0,0 +1,83 @@
+#![allow(unused)]
+#![allow(clippy::nonminimal_bool)]
+#![allow(clippy::needless_borrow)]
+#![warn(clippy::set_contains_or_insert)]
+
+use std::collections::HashSet;
+
+fn main() {
+    should_warn_cases();
+
+    should_not_warn_cases();
+}
+
+fn should_warn_cases() {
+    let mut set = HashSet::new();
+    let value = 5;
+
+    if !set.contains(&value) {
+        set.insert(value);
+        println!("Just a comment");
+    }
+
+    if set.contains(&value) {
+        set.insert(value);
+        println!("Just a comment");
+    }
+
+    if !set.contains(&value) {
+        set.insert(value);
+    }
+
+    if !!set.contains(&value) {
+        set.insert(value);
+        println!("Just a comment");
+    }
+
+    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);
+    }
+}
+
+fn should_not_warn_cases() {
+    let mut set = HashSet::new();
+    let value = 5;
+    let another_value = 6;
+
+    if !set.contains(&value) {
+        set.insert(another_value);
+    }
+
+    if !set.contains(&value) {
+        println!("Just a comment");
+    }
+
+    if simply_true() {
+        set.insert(value);
+    }
+
+    if !set.contains(&value) {
+        set.replace(value); //it is not insert
+        println!("Just a comment");
+    }
+
+    if set.contains(&value) {
+        println!("value is already in set");
+    } else {
+        set.insert(value);
+    }
+}
+
+fn simply_true() -> bool {
+    true
+}
diff --git a/tests/ui/set_contains_or_insert.stderr b/tests/ui/set_contains_or_insert.stderr
@@ -0,0 +1,61 @@
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/set_contains_or_insert.rs:18:13
+   |
+LL |     if !set.contains(&value) {
+   |             ^^^^^^^^^^^^^^^^
+LL |         set.insert(value);
+   |             ^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::set-contains-or-insert` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::set_contains_or_insert)]`
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/set_contains_or_insert.rs:23:12
+   |
+LL |     if set.contains(&value) {
+   |            ^^^^^^^^^^^^^^^^
+LL |         set.insert(value);
+   |             ^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/set_contains_or_insert.rs:28:13
+   |
+LL |     if !set.contains(&value) {
+   |             ^^^^^^^^^^^^^^^^
+LL |         set.insert(value);
+   |             ^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/set_contains_or_insert.rs:32:14
+   |
+LL |     if !!set.contains(&value) {
+   |              ^^^^^^^^^^^^^^^^
+LL |         set.insert(value);
+   |             ^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/set_contains_or_insert.rs:37:15
+   |
+LL |     if (&set).contains(&value) {
+   |               ^^^^^^^^^^^^^^^^
+LL |         set.insert(value);
+   |             ^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/set_contains_or_insert.rs:42:13
+   |
+LL |     if !set.contains(borrow_value) {
+   |             ^^^^^^^^^^^^^^^^^^^^^^
+LL |         set.insert(*borrow_value);
+   |             ^^^^^^^^^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/set_contains_or_insert.rs:47:20
+   |
+LL |     if !borrow_set.contains(&value) {
+   |                    ^^^^^^^^^^^^^^^^
+LL |         borrow_set.insert(value);
+   |                    ^^^^^^^^^^^^^
+
+error: aborting due to 7 previous errors
+
