diff options
| -rw-r--r-- | CHANGELOG.md | 2 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 4 | ||||
| -rw-r--r-- | clippy_lints/src/set_contains_or_insert.rs (renamed from clippy_lints/src/hashset_insert_after_contains.rs) | 78 | ||||
| -rw-r--r-- | tests/ui/hashset_insert_after_contains.stderr | 112 | ||||
| -rw-r--r-- | tests/ui/set_contains_or_insert.rs (renamed from tests/ui/hashset_insert_after_contains.rs) | 8 | ||||
| -rw-r--r-- | tests/ui/set_contains_or_insert.stderr | 61 |
7 files changed, 105 insertions, 162 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c4a24f2b0b..23b3ea9f221 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 ff6ce7b5090..ef5dfd3ee32 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 fd23659d9c5..1747d508559 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 index a793d6e2eb1..d6b6f655ac3 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<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) @@ -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<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 deleted file mode 100644 index d547dfa5c1a..00000000000 --- 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 index 11e4cde84d6..8465007402a 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 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 + |