Rename lint, generalize function, add known issues, use multispan

lochetti · lochetti · commit e48d9e57b75e · 2024-06-03T19:04:07.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5735,6 +5735,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
@@ -212,7 +212,6 @@ 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,
@@ -643,6 +642,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
@@ -149,7 +149,6 @@ 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;
@@ -316,6 +315,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;
@@ -1166,7 +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));
+    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
@@ -1,12 +1,13 @@
 use std::ops::ControlFlow;
 
-use clippy_utils::diagnostics::span_lint_and_then;
+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! {
@@ -17,6 +18,11 @@ declare_clippy_lint! {
     /// ### 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 scenario, the developer might want to check with `contain` before inserting,
+    /// to avoid the clone. In this case, it will report a false positive.
+    ///
     /// ### Example
     /// ```rust
     /// use std::collections::HashSet;
@@ -37,12 +43,12 @@ declare_clippy_lint! {
     /// }
     /// ```
     #[clippy::version = "1.80.0"]
-    pub HASHSET_INSERT_AFTER_CONTAINS,
+    pub SET_CONTAINS_OR_INSERT,
     nursery,
-    "unnecessary call to `HashSet::contains` followed by `HashSet::insert`"
+    "call to `HashSet::contains` followed by `HashSet::insert`"
 }
 
-declare_lint_pass!(HashsetInsertAfterContains => [HASHSET_INSERT_AFTER_CONTAINS]);
+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<'_>) {
@@ -52,56 +58,35 @@ impl<'tcx> LateLintPass<'tcx> for HashsetInsertAfterContains {
                 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)
+            && 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_and_then(
+            span_lint(
                 cx,
-                HASHSET_INSERT_AFTER_CONTAINS,
-                expr.span,
+                SET_CONTAINS_OR_INSERT,
+                vec![contains_expr.span, insert_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> {
+struct OpExpr<'tcx> {
     receiver: &'tcx Expr<'tcx>,
     value: &'tcx Expr<'tcx>,
     span: Span,
 }
-fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Option<ContainsExpr<'tcx>> {
+
+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 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 {
+    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 {
@@ -110,28 +95,31 @@ fn try_parse_insert<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Optio
                 None
             }
         });
-
+        let receiver = receiver.peel_borrows();
         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
+        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 });
         }
-    } else {
-        None
     }
+    None
 }
 
-fn find_insert_calls<'tcx>(cx: &LateContext<'tcx>, contains_expr: &ContainsExpr<'tcx>, expr: &'tcx Expr<'_>) -> bool {
+fn find_insert_calls<'tcx>(
+    cx: &LateContext<'tcx>,
+    contains_expr: &OpExpr<'tcx>,
+    expr: &'tcx Expr<'_>,
+) -> Option<OpExpr<'tcx>> {
     for_each_expr(expr, |e| {
-        if let Some(insert_expr) = try_parse_insert(cx, 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(())
+            ControlFlow::Break(insert_expr)
         } else {
             ControlFlow::Continue(())
         }
     })
-    .is_some()
 }
diff --git a/tests/ui/hashset_insert_after_contains.stderr b/tests/ui/hashset_insert_after_contains.stderr
diff --git a/tests/ui/set_contains_or_insert.rs b/tests/ui/set_contains_or_insert.rs
@@ -1,7 +1,7 @@
 #![allow(unused)]
 #![allow(clippy::nonminimal_bool)]
 #![allow(clippy::needless_borrow)]
-#![warn(clippy::hashset_insert_after_contains)]
+#![warn(clippy::set_contains_or_insert)]
 
 use std::collections::HashSet;
 
@@ -70,6 +70,12 @@ fn should_not_warn_cases() {
         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 {
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
+
