diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ef225559795..12cd6cd8715f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4880,6 +4880,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 ea66c760a2f8..2ba4fef703c1 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -200,6 +200,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..a1c047be0854 --- /dev/null +++ b/clippy_lints/src/hashset_insert_after_contains.rs @@ -0,0 +1,127 @@ +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, declare_tool_lint}; +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.73.0"] + pub HASHSET_INSERT_AFTER_CONTAINS, + perf, + "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| match e.kind { + ExprKind::Unary(UnOp::Not, e) => Some(e), + _ => 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.ctxt() == expr.span.ctxt() + && 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| match e.kind { + ExprKind::Unary(UnOp::Deref, e) => Some(e), + _ => 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 358004cf460b..21411d8c9521 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -144,6 +144,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; @@ -1095,6 +1096,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(|_| Box::new(redundant_locals::RedundantLocals)); store.register_late_pass(|_| Box::new(ignored_unit_patterns::IgnoredUnitPatterns)); + 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..e85691fd4530 --- /dev/null +++ b/tests/ui/hashset_insert_after_contains.stderr @@ -0,0 +1,111 @@ +error: usage of `HashSet::insert` after `HashSet::contains` + --> $DIR/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 + --> $DIR/hashset_insert_after_contains.rs:18:13 + | +LL | if !set.contains(&value) { + | ^^^^^^^^^^^^^^^^ + = note: `-D clippy::hashset-insert-after-contains` implied by `-D warnings` + +error: usage of `HashSet::insert` after `HashSet::contains` + --> $DIR/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 + --> $DIR/hashset_insert_after_contains.rs:23:12 + | +LL | if set.contains(&value) { + | ^^^^^^^^^^^^^^^^ + +error: usage of `HashSet::insert` after `HashSet::contains` + --> $DIR/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 + --> $DIR/hashset_insert_after_contains.rs:28:13 + | +LL | if !set.contains(&value) { + | ^^^^^^^^^^^^^^^^ + +error: usage of `HashSet::insert` after `HashSet::contains` + --> $DIR/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 + --> $DIR/hashset_insert_after_contains.rs:32:14 + | +LL | if !!set.contains(&value) { + | ^^^^^^^^^^^^^^^^ + +error: usage of `HashSet::insert` after `HashSet::contains` + --> $DIR/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 + --> $DIR/hashset_insert_after_contains.rs:37:15 + | +LL | if (&set).contains(&value) { + | ^^^^^^^^^^^^^^^^ + +error: usage of `HashSet::insert` after `HashSet::contains` + --> $DIR/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 + --> $DIR/hashset_insert_after_contains.rs:42:13 + | +LL | if !set.contains(borrow_value) { + | ^^^^^^^^^^^^^^^^^^^^^^ + +error: usage of `HashSet::insert` after `HashSet::contains` + --> $DIR/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 + --> $DIR/hashset_insert_after_contains.rs:47:20 + | +LL | if !borrow_set.contains(&value) { + | ^^^^^^^^^^^^^^^^ + +error: aborting due to 7 previous errors +