Add new lint hashset_insert_after_contains

CHANGELOG.md

@@ -5342,6 +5342,7 @@ Released 2018-09-13
 [`get_first`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_first
 [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len
 [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap
+[`hashset_insert_after_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#hashset_insert_after_contains
 [`host_endian_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#host_endian_bytes
 [`identity_conversion`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_conversion
 [`identity_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#identity_op

clippy_lints/src/declared_lints.rs

@@ -212,6 +212,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::functions::TOO_MANY_ARGUMENTS_INFO,
     crate::functions::TOO_MANY_LINES_INFO,
     crate::future_not_send::FUTURE_NOT_SEND_INFO,
+    crate::hashset_insert_after_contains::HASHSET_INSERT_AFTER_CONTAINS_INFO,
     crate::if_let_mutex::IF_LET_MUTEX_INFO,
     crate::if_not_else::IF_NOT_ELSE_INFO,
     crate::if_then_some_else_none::IF_THEN_SOME_ELSE_NONE_INFO,

clippy_lints/src/hashset_insert_after_contains.rs

@@ -0,0 +1,137 @@
+use std::ops::ControlFlow;
+
+use clippy_utils::diagnostics::span_lint_and_then;
+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::{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.
+    ///
+    /// ### 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 HASHSET_INSERT_AFTER_CONTAINS,
+    nursery,
+    "unnecessary call to `HashSet::contains` followed by `HashSet::insert`"
+}
+
+declare_lint_pass!(HashsetInsertAfterContains => [HASHSET_INSERT_AFTER_CONTAINS]);
+
+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_contains(cx, cond_expr)
+            && find_insert_calls(cx, &contains_expr, then_expr)
+        {
+            span_lint_and_then(
+                cx,
+                HASHSET_INSERT_AFTER_CONTAINS,
+                expr.span,
+                "usage of `HashSet::insert` after `HashSet::contains`",
+                |diag| {
+                    diag.note("`HashSet::insert` returns whether it was inserted")
+                        .span_help(contains_expr.span, "remove the `HashSet::contains` call");
+                },
+            );
+        }
+    }
+}
+
+struct ContainsExpr<'tcx> {
+    receiver: &'tcx Expr<'tcx>,
+    value: &'tcx Expr<'tcx>,
+    span: Span,
+}
+fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<ContainsExpr<'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 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 == sym!(contains)
+        {
+            return Some(ContainsExpr { receiver, value, span });
+        }
+    }
+    None
+}
+
+struct InsertExpr<'tcx> {
+    receiver: &'tcx Expr<'tcx>,
+    value: &'tcx Expr<'tcx>,
+}
+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| {
+            if let ExprKind::Unary(UnOp::Deref, e) = e.kind {
+                Some(e)
+            } else {
+                None
+            }
+        });
+
+        let receiver_ty = cx.typeck_results().expr_ty(receiver).peel_refs();
+        if is_type_diagnostic_item(cx, receiver_ty, sym::HashSet) && path.ident.name == sym!(insert) {
+            Some(InsertExpr { receiver, value })
+        } else {
+            None
+        }
+    } else {
+        None
+    }
+}
+
+fn find_insert_calls<'tcx>(cx: &LateContext<'tcx>, contains_expr: &ContainsExpr<'tcx>, expr: &'tcx Expr<'_>) -> bool {
+    for_each_expr(expr, |e| {
+        if let Some(insert_expr) = try_parse_insert(cx, e)
+            && SpanlessEq::new(cx).eq_expr(contains_expr.receiver, insert_expr.receiver)
+            && SpanlessEq::new(cx).eq_expr(contains_expr.value, insert_expr.value)
+        {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
+        }
+    })
+    .is_some()
+}

clippy_lints/src/lib.rs

@@ -149,6 +149,7 @@ mod from_raw_with_void_ptr;
 mod from_str_radix_10;
 mod functions;
 mod future_not_send;
+mod hashset_insert_after_contains;
 mod if_let_mutex;
 mod if_not_else;
 mod if_then_some_else_none;
@@ -1165,6 +1166,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
             ..Default::default()
         })
     });
+    store.register_late_pass(|_| Box::new(hashset_insert_after_contains::HashsetInsertAfterContains));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

tests/ui/hashset_insert_after_contains.rs

@@ -0,0 +1,77 @@
+#![allow(unused)]
+#![allow(clippy::nonminimal_bool)]
+#![allow(clippy::needless_borrow)]
+#![warn(clippy::hashset_insert_after_contains)]
+
+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");
+    }
+}
+
+fn simply_true() -> bool {
+    true
+}

tests/ui/hashset_insert_after_contains.stderr

@@ -0,0 +1,112 @@
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/hashset_insert_after_contains.rs:18:5
+   |
+LL | /     if !set.contains(&value) {
+LL | |         set.insert(value);
+LL | |         println!("Just a comment");
+LL | |     }
+   | |_____^
+   |
+   = note: `HashSet::insert` returns whether it was inserted
+help: remove the `HashSet::contains` call
+  --> tests/ui/hashset_insert_after_contains.rs:18:13
+   |
+LL |     if !set.contains(&value) {
+   |             ^^^^^^^^^^^^^^^^
+   = note: `-D clippy::hashset-insert-after-contains` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::hashset_insert_after_contains)]`
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/hashset_insert_after_contains.rs:23:5
+   |
+LL | /     if set.contains(&value) {
+LL | |         set.insert(value);
+LL | |         println!("Just a comment");
+LL | |     }
+   | |_____^
+   |
+   = note: `HashSet::insert` returns whether it was inserted
+help: remove the `HashSet::contains` call
+  --> tests/ui/hashset_insert_after_contains.rs:23:12
+   |
+LL |     if set.contains(&value) {
+   |            ^^^^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/hashset_insert_after_contains.rs:28:5
+   |
+LL | /     if !set.contains(&value) {
+LL | |         set.insert(value);
+LL | |     }
+   | |_____^
+   |
+   = note: `HashSet::insert` returns whether it was inserted
+help: remove the `HashSet::contains` call
+  --> tests/ui/hashset_insert_after_contains.rs:28:13
+   |
+LL |     if !set.contains(&value) {
+   |             ^^^^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/hashset_insert_after_contains.rs:32:5
+   |
+LL | /     if !!set.contains(&value) {
+LL | |         set.insert(value);
+LL | |         println!("Just a comment");
+LL | |     }
+   | |_____^
+   |
+   = note: `HashSet::insert` returns whether it was inserted
+help: remove the `HashSet::contains` call
+  --> tests/ui/hashset_insert_after_contains.rs:32:14
+   |
+LL |     if !!set.contains(&value) {
+   |              ^^^^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/hashset_insert_after_contains.rs:37:5
+   |
+LL | /     if (&set).contains(&value) {
+LL | |         set.insert(value);
+LL | |     }
+   | |_____^
+   |
+   = note: `HashSet::insert` returns whether it was inserted
+help: remove the `HashSet::contains` call
+  --> tests/ui/hashset_insert_after_contains.rs:37:15
+   |
+LL |     if (&set).contains(&value) {
+   |               ^^^^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/hashset_insert_after_contains.rs:42:5
+   |
+LL | /     if !set.contains(borrow_value) {
+LL | |         set.insert(*borrow_value);
+LL | |     }
+   | |_____^
+   |
+   = note: `HashSet::insert` returns whether it was inserted
+help: remove the `HashSet::contains` call
+  --> tests/ui/hashset_insert_after_contains.rs:42:13
+   |
+LL |     if !set.contains(borrow_value) {
+   |             ^^^^^^^^^^^^^^^^^^^^^^
+
+error: usage of `HashSet::insert` after `HashSet::contains`
+  --> tests/ui/hashset_insert_after_contains.rs:47:5
+   |
+LL | /     if !borrow_set.contains(&value) {
+LL | |         borrow_set.insert(value);
+LL | |     }
+   | |_____^
+   |
+   = note: `HashSet::insert` returns whether it was inserted
+help: remove the `HashSet::contains` call
+  --> tests/ui/hashset_insert_after_contains.rs:47:20
+   |
+LL |     if !borrow_set.contains(&value) {
+   |                    ^^^^^^^^^^^^^^^^
+
+error: aborting due to 7 previous errors
+