diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/set_contains_or_insert.rs | 125 | ||||
| -rw-r--r-- | tests/ui/set_contains_or_insert.rs | 83 | ||||
| -rw-r--r-- | tests/ui/set_contains_or_insert.stderr | 61 |
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 + |