From 0f915f6f30c2fb267e25b0bf14bc4674ba7accca Mon Sep 17 00:00:00 2001 From: Renato Lochetti Date: Fri, 31 May 2024 16:58:19 +0100 Subject: [PATCH 1/4] Add new lint `hashset_insert_after_contains` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + .../src/hashset_insert_after_contains.rs | 137 ++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui/hashset_insert_after_contains.rs | 77 ++++++++++ tests/ui/hashset_insert_after_contains.stderr | 112 ++++++++++++++ 6 files changed, 330 insertions(+) create mode 100644 clippy_lints/src/hashset_insert_after_contains.rs create mode 100644 tests/ui/hashset_insert_after_contains.rs create mode 100644 tests/ui/hashset_insert_after_contains.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 0974631ac5dc..3c4a24f2b0bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5401,6 +5401,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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 53f346c04e78..ff6ce7b50907 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -211,6 +211,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, diff --git a/clippy_lints/src/hashset_insert_after_contains.rs b/clippy_lints/src/hashset_insert_after_contains.rs new file mode 100644 index 000000000000..a793d6e2eb1a --- /dev/null +++ b/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> { + 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> { + 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() +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fa4c7084f4dd..fd23659d9c50 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -150,6 +150,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; @@ -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(hashset_insert_after_contains::HashsetInsertAfterContains)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/hashset_insert_after_contains.rs b/tests/ui/hashset_insert_after_contains.rs new file mode 100644 index 000000000000..11e4cde84d64 --- /dev/null +++ b/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 +} diff --git a/tests/ui/hashset_insert_after_contains.stderr b/tests/ui/hashset_insert_after_contains.stderr new file mode 100644 index 000000000000..d547dfa5c1a3 --- /dev/null +++ b/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 + From 6661e83e7bf2adcc9a68d59aee6e281f7c051b82 Mon Sep 17 00:00:00 2001 From: Renato Lochetti Date: Mon, 3 Jun 2024 19:04:07 +0100 Subject: [PATCH 2/4] Rename lint, generalize function, add known issues, use multispan --- CHANGELOG.md | 2 +- clippy_lints/src/declared_lints.rs | 2 +- clippy_lints/src/lib.rs | 4 +- ..._contains.rs => set_contains_or_insert.rs} | 78 ++++++------ tests/ui/hashset_insert_after_contains.stderr | 112 ------------------ ..._contains.rs => set_contains_or_insert.rs} | 8 +- tests/ui/set_contains_or_insert.stderr | 61 ++++++++++ 7 files changed, 105 insertions(+), 162 deletions(-) rename clippy_lints/src/{hashset_insert_after_contains.rs => set_contains_or_insert.rs} (60%) delete mode 100644 tests/ui/hashset_insert_after_contains.stderr rename tests/ui/{hashset_insert_after_contains.rs => set_contains_or_insert.rs} (89%) create mode 100644 tests/ui/set_contains_or_insert.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c4a24f2b0bb..23b3ea9f2213 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5401,7 +5401,6 @@ 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 @@ -5799,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 ff6ce7b50907..ef5dfd3ee32c 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -211,7 +211,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, @@ -646,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 fd23659d9c50..1747d5085597 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -150,7 +150,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; @@ -319,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; @@ -1173,7 +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(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/hashset_insert_after_contains.rs b/clippy_lints/src/set_contains_or_insert.rs similarity index 60% rename from clippy_lints/src/hashset_insert_after_contains.rs rename to clippy_lints/src/set_contains_or_insert.rs index a793d6e2eb1a..d6b6f655ac36 100644 --- a/clippy_lints/src/hashset_insert_after_contains.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,29 +58,26 @@ 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> { + +fn try_parse_op_call<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, symbol: Symbol) -> Option> { let expr = peel_hir_expr_while(expr, |e| { if let ExprKind::Unary(UnOp::Not, e) = e.kind { Some(e) @@ -82,26 +85,8 @@ fn try_parse_contains<'tcx>(cx: &LateContext<'_>, expr: &'tcx Expr<'_>) -> Optio 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> { - 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> { 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 deleted file mode 100644 index d547dfa5c1a3..000000000000 --- a/tests/ui/hashset_insert_after_contains.stderr +++ /dev/null @@ -1,112 +0,0 @@ -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 - diff --git a/tests/ui/hashset_insert_after_contains.rs b/tests/ui/set_contains_or_insert.rs similarity index 89% rename from tests/ui/hashset_insert_after_contains.rs rename to tests/ui/set_contains_or_insert.rs index 11e4cde84d64..8465007402ab 100644 --- a/tests/ui/hashset_insert_after_contains.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 new file mode 100644 index 000000000000..507e20964fc2 --- /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 + From eff6f68caf08a5c055778150e2154e3061e002a2 Mon Sep 17 00:00:00 2001 From: Renato Lochetti Date: Wed, 3 Jul 2024 19:40:05 +0100 Subject: [PATCH 3/4] Fix typos --- clippy_lints/src/set_contains_or_insert.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/set_contains_or_insert.rs b/clippy_lints/src/set_contains_or_insert.rs index d6b6f655ac36..cf8ad0935e8c 100644 --- a/clippy_lints/src/set_contains_or_insert.rs +++ b/clippy_lints/src/set_contains_or_insert.rs @@ -20,7 +20,7 @@ declare_clippy_lint! { /// /// ### 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 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 From 4e71fc4302898e28d7e4c570edf2f3d1feffe116 Mon Sep 17 00:00:00 2001 From: Renato Lochetti Date: Wed, 3 Jul 2024 19:54:02 +0100 Subject: [PATCH 4/4] Small fix after rebase --- clippy_lints/src/set_contains_or_insert.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/set_contains_or_insert.rs b/clippy_lints/src/set_contains_or_insert.rs index cf8ad0935e8c..5e65b9fa5171 100644 --- a/clippy_lints/src/set_contains_or_insert.rs +++ b/clippy_lints/src/set_contains_or_insert.rs @@ -112,7 +112,7 @@ fn find_insert_calls<'tcx>( contains_expr: &OpExpr<'tcx>, expr: &'tcx Expr<'_>, ) -> Option> { - for_each_expr(expr, |e| { + 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)