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

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]

6 files changed, 273 insertions, 0 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0974631ac5d..23b3ea9f221 100644
--- 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
index 53f346c04e7..ef5dfd3ee32 100644
--- 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
index fa4c7084f4d..1747d508559 100644
--- 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
new file mode 100644
index 00000000000..5e65b9fa517
--- /dev/null
+++ 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
new file mode 100644
index 00000000000..8465007402a
--- /dev/null
+++ 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
new file mode 100644
index 00000000000..507e20964fc
--- /dev/null
+++ 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
+