From 7a903bd53f330fe5b00642235b80e8a7155ed7ae Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Wed, 27 Mar 2024 20:42:29 +0100 Subject: [PATCH 1/8] Implemented first lint configuration. More work for more complex cases to follow --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/hash_collision.rs | 87 ++++++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 2 + tests/ui/hash_collision.rs | 6 +++ tests/ui/hash_collision.stderr | 12 +++++ 6 files changed, 109 insertions(+) create mode 100644 clippy_lints/src/hash_collision.rs create mode 100644 tests/ui/hash_collision.rs create mode 100644 tests/ui/hash_collision.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 87bffe7f74d6..8e1a888dd3c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5290,6 +5290,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 +[`hash_collision`]: https://rust-lang.github.io/rust-clippy/master/index.html#hash_collision [`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 eba048ad90be..8f2167734598 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -210,6 +210,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::hash_collision::HASH_COLLISION_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/hash_collision.rs b/clippy_lints/src/hash_collision.rs new file mode 100644 index 000000000000..30874c376062 --- /dev/null +++ b/clippy_lints/src/hash_collision.rs @@ -0,0 +1,87 @@ +use clippy_utils::{diagnostics::span_lint_and_note, last_path_segment, ty::is_type_diagnostic_item}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::declare_lint_pass; +use rustc_hir::ExprKind; +use rustc_span::symbol::sym; + +declare_clippy_lint! { + /// ### What it does + /// When two items are inserted into a `HashMap` with the same key, + /// the second item will overwrite the first item. + /// ### Why is this bad? + /// This can lead to data loss. + /// ### Example + /// ```no_run + /// # use std::collections::HashMap; + /// let example = HashMap::from([(5, 1), (5, 2)]); + /// ``` + #[clippy::version = "1.79.0"] + pub HASH_COLLISION, + suspicious, + "default lint description" +} + +declare_lint_pass!(HashCollision => [HASH_COLLISION]); + +impl<'tcx> LateLintPass<'tcx> for HashCollision { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) { + if has_hash_collision(cx, expr) { + span_lint_and_note( + cx, + HASH_COLLISION, + expr.span, + "This `HashMap` has a hash collision and will lose data", + None, + "Consider using a different keys for all items", + ) + } + } +} + +// TODO: Also check for different sources of hash collisions +// TODO: Maybe other types of hash maps should be checked as well? +fn has_hash_collision(cx: &LateContext<'_>, expr: &rustc_hir::Expr<'_>) -> bool { + // If the expression is a call to `HashMap::from`, check if the keys are the same + if let ExprKind::Call(func, args) = &expr.kind + // First check for HashMap::from + && let ty = cx.typeck_results().expr_ty(expr) + && is_type_diagnostic_item(cx, ty, sym::HashMap) + && let ExprKind::Path(func_path) = func.kind + && last_path_segment(&func_path).ident.name == sym::from + // There should be exactly one argument to HashMap::from + && args.len() == 1 + // which should be an array + && let ExprKind::Array(args) = &args[0].kind + // Then check the keys + { + // Put all keys in a vector + let mut literals = Vec::new(); + + for arg in args.iter() { // + if let ExprKind::Tup(args) = &arg.kind + && args.len() > 0 + && let ExprKind::Lit(lit) = args[0].kind + { + literals.push(lit.node.clone()); + } + + } + // Debug: it gets here, but doesn't trigger the lint + + // Check if there are any duplicate keys + let mut duplicate = false; + for i in 0..literals.len() { + for j in i + 1..literals.len() { + if literals[i] == literals[j] { + duplicate = true; + break; + } + } + if duplicate { + break; + } + } + return duplicate; + } + false +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 169e51e2cb93..472922bb310b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -153,6 +153,7 @@ mod from_raw_with_void_ptr; mod from_str_radix_10; mod functions; mod future_not_send; +mod hash_collision; mod if_let_mutex; mod if_not_else; mod if_then_some_else_none; @@ -1129,6 +1130,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects)); store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault)); store.register_late_pass(|_| Box::new(integer_division_remainder_used::IntegerDivisionRemainderUsed)); + store.register_late_pass(|_| Box::new(hash_collision::HashCollision)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/hash_collision.rs b/tests/ui/hash_collision.rs new file mode 100644 index 000000000000..d85cbb71c3f6 --- /dev/null +++ b/tests/ui/hash_collision.rs @@ -0,0 +1,6 @@ +#![warn(clippy::hash_collision)] +use std::collections::HashMap; + +fn main() { + let example = HashMap::from([(5, 1), (5, 2)]); +} diff --git a/tests/ui/hash_collision.stderr b/tests/ui/hash_collision.stderr new file mode 100644 index 000000000000..9fce063ad958 --- /dev/null +++ b/tests/ui/hash_collision.stderr @@ -0,0 +1,12 @@ +error: This `HashMap` has a hash collision and will lose data + --> tests/ui/hash_collision.rs:5:19 + | +LL | let example = HashMap::from([(5, 1), (5, 2)]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: Consider using a different keys for all items + = note: `-D clippy::hash-collision` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::hash_collision)]` + +error: aborting due to 1 previous error + From cff98f2974e68535be99f9785665ef96b94d26af Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Wed, 27 Mar 2024 22:29:42 +0100 Subject: [PATCH 2/8] Fixed cargo test not passing; minor changes --- clippy_lints/src/hash_collision.rs | 38 ++++++++++++++++-------------- tests/ui/hash_collision.stderr | 4 ++-- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/hash_collision.rs b/clippy_lints/src/hash_collision.rs index 30874c376062..7201f4407c6c 100644 --- a/clippy_lints/src/hash_collision.rs +++ b/clippy_lints/src/hash_collision.rs @@ -1,7 +1,9 @@ -use clippy_utils::{diagnostics::span_lint_and_note, last_path_segment, ty::is_type_diagnostic_item}; +use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::last_path_segment; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_hir::ExprKind; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; -use rustc_hir::ExprKind; use rustc_span::symbol::sym; declare_clippy_lint! { @@ -30,10 +32,10 @@ impl<'tcx> LateLintPass<'tcx> for HashCollision { cx, HASH_COLLISION, expr.span, - "This `HashMap` has a hash collision and will lose data", + "this `HashMap` has a hash collision and will lose data", None, - "Consider using a different keys for all items", - ) + "consider using a different keys for all items", + ); } } } @@ -42,7 +44,7 @@ impl<'tcx> LateLintPass<'tcx> for HashCollision { // TODO: Maybe other types of hash maps should be checked as well? fn has_hash_collision(cx: &LateContext<'_>, expr: &rustc_hir::Expr<'_>) -> bool { // If the expression is a call to `HashMap::from`, check if the keys are the same - if let ExprKind::Call(func, args) = &expr.kind + if let ExprKind::Call(func, args) = &expr.kind // First check for HashMap::from && let ty = cx.typeck_results().expr_ty(expr) && is_type_diagnostic_item(cx, ty, sym::HashMap) @@ -52,19 +54,19 @@ fn has_hash_collision(cx: &LateContext<'_>, expr: &rustc_hir::Expr<'_>) -> bool && args.len() == 1 // which should be an array && let ExprKind::Array(args) = &args[0].kind - // Then check the keys - { - // Put all keys in a vector - let mut literals = Vec::new(); - - for arg in args.iter() { // - if let ExprKind::Tup(args) = &arg.kind - && args.len() > 0 + // Then check the keys + { + // Put all keys in a vector + let mut literals = Vec::new(); + + for arg in *args { + // + if let ExprKind::Tup(args) = &arg.kind + && !args.is_empty() && let ExprKind::Lit(lit) = args[0].kind - { - literals.push(lit.node.clone()); - } - + { + literals.push(lit.node.clone()); + } } // Debug: it gets here, but doesn't trigger the lint diff --git a/tests/ui/hash_collision.stderr b/tests/ui/hash_collision.stderr index 9fce063ad958..b1ee7bfe91bb 100644 --- a/tests/ui/hash_collision.stderr +++ b/tests/ui/hash_collision.stderr @@ -1,10 +1,10 @@ -error: This `HashMap` has a hash collision and will lose data +error: this `HashMap` has a hash collision and will lose data --> tests/ui/hash_collision.rs:5:19 | LL | let example = HashMap::from([(5, 1), (5, 2)]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | - = note: Consider using a different keys for all items + = note: consider using a different keys for all items = note: `-D clippy::hash-collision` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::hash_collision)]` From 434a92e85dd7c6940ddd90d256f43988ad7937bb Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Wed, 27 Mar 2024 22:43:35 +0100 Subject: [PATCH 3/8] Update lint description --- clippy_lints/src/hash_collision.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/hash_collision.rs b/clippy_lints/src/hash_collision.rs index 7201f4407c6c..4da2ca8d3e49 100644 --- a/clippy_lints/src/hash_collision.rs +++ b/clippy_lints/src/hash_collision.rs @@ -10,8 +10,10 @@ declare_clippy_lint! { /// ### What it does /// When two items are inserted into a `HashMap` with the same key, /// the second item will overwrite the first item. + /// /// ### Why is this bad? /// This can lead to data loss. + /// /// ### Example /// ```no_run /// # use std::collections::HashMap; @@ -20,7 +22,7 @@ declare_clippy_lint! { #[clippy::version = "1.79.0"] pub HASH_COLLISION, suspicious, - "default lint description" + "`HashMap` with two identical keys loses data" } declare_lint_pass!(HashCollision => [HASH_COLLISION]); From efb8e4d79c4aa1859d3bb3edfc8e5e935b66006c Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Wed, 27 Mar 2024 22:45:35 +0100 Subject: [PATCH 4/8] removed trailing whitespace --- clippy_lints/src/hash_collision.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/hash_collision.rs b/clippy_lints/src/hash_collision.rs index 4da2ca8d3e49..73370242320f 100644 --- a/clippy_lints/src/hash_collision.rs +++ b/clippy_lints/src/hash_collision.rs @@ -10,10 +10,10 @@ declare_clippy_lint! { /// ### What it does /// When two items are inserted into a `HashMap` with the same key, /// the second item will overwrite the first item. - /// + /// /// ### Why is this bad? /// This can lead to data loss. - /// + /// /// ### Example /// ```no_run /// # use std::collections::HashMap; From 6c13a9044ab7ff409c60e34f1ad3be4d8f52760a Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Thu, 28 Mar 2024 22:02:28 +0100 Subject: [PATCH 5/8] Changed lint name to duplicate_map_keys --- CHANGELOG.md | 2 +- clippy_lints/src/declared_lints.rs | 2 +- .../src/{hash_collision.rs => duplicate_map_keys.rs} | 8 ++++---- clippy_lints/src/lib.rs | 4 ++-- tests/ui/{hash_collision.rs => duplicate_map_keys.rs} | 2 +- .../{hash_collision.stderr => duplicate_map_keys.stderr} | 6 +++--- 6 files changed, 12 insertions(+), 12 deletions(-) rename clippy_lints/src/{hash_collision.rs => duplicate_map_keys.rs} (94%) rename tests/ui/{hash_collision.rs => duplicate_map_keys.rs} (72%) rename tests/ui/{hash_collision.stderr => duplicate_map_keys.stderr} (59%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e1a888dd3c3..c30607e30d90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5209,6 +5209,7 @@ Released 2018-09-13 [`drop_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_copy [`drop_non_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_non_drop [`drop_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#drop_ref +[`duplicate_map_keys`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_map_keys [`duplicate_mod`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_mod [`duplicate_underscore_argument`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicate_underscore_argument [`duplicated_attributes`]: https://rust-lang.github.io/rust-clippy/master/index.html#duplicated_attributes @@ -5290,7 +5291,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 -[`hash_collision`]: https://rust-lang.github.io/rust-clippy/master/index.html#hash_collision [`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 8f2167734598..441b1269d20d 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -154,6 +154,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::drop_forget_ref::DROP_NON_DROP_INFO, crate::drop_forget_ref::FORGET_NON_DROP_INFO, crate::drop_forget_ref::MEM_FORGET_INFO, + crate::duplicate_map_keys::DUPLICATE_MAP_KEYS_INFO, crate::duplicate_mod::DUPLICATE_MOD_INFO, crate::else_if_without_else::ELSE_IF_WITHOUT_ELSE_INFO, crate::empty_drop::EMPTY_DROP_INFO, @@ -210,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::hash_collision::HASH_COLLISION_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/hash_collision.rs b/clippy_lints/src/duplicate_map_keys.rs similarity index 94% rename from clippy_lints/src/hash_collision.rs rename to clippy_lints/src/duplicate_map_keys.rs index 73370242320f..c4550902f6d9 100644 --- a/clippy_lints/src/hash_collision.rs +++ b/clippy_lints/src/duplicate_map_keys.rs @@ -20,19 +20,19 @@ declare_clippy_lint! { /// let example = HashMap::from([(5, 1), (5, 2)]); /// ``` #[clippy::version = "1.79.0"] - pub HASH_COLLISION, + pub DUPLICATE_MAP_KEYS, suspicious, "`HashMap` with two identical keys loses data" } -declare_lint_pass!(HashCollision => [HASH_COLLISION]); +declare_lint_pass!(DuplicateMapKeys => [DUPLICATE_MAP_KEYS]); -impl<'tcx> LateLintPass<'tcx> for HashCollision { +impl<'tcx> LateLintPass<'tcx> for DuplicateMapKeys { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) { if has_hash_collision(cx, expr) { span_lint_and_note( cx, - HASH_COLLISION, + DUPLICATE_MAP_KEYS, expr.span, "this `HashMap` has a hash collision and will lose data", None, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 472922bb310b..b54df51fcc87 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -121,6 +121,7 @@ mod disallowed_types; mod doc; mod double_parens; mod drop_forget_ref; +mod duplicate_map_keys; mod duplicate_mod; mod else_if_without_else; mod empty_drop; @@ -153,7 +154,6 @@ mod from_raw_with_void_ptr; mod from_str_radix_10; mod functions; mod future_not_send; -mod hash_collision; mod if_let_mutex; mod if_not_else; mod if_then_some_else_none; @@ -1130,7 +1130,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects)); store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault)); store.register_late_pass(|_| Box::new(integer_division_remainder_used::IntegerDivisionRemainderUsed)); - store.register_late_pass(|_| Box::new(hash_collision::HashCollision)); + store.register_late_pass(|_| Box::new(duplicate_map_keys::DuplicateMapKeys)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/hash_collision.rs b/tests/ui/duplicate_map_keys.rs similarity index 72% rename from tests/ui/hash_collision.rs rename to tests/ui/duplicate_map_keys.rs index d85cbb71c3f6..924d98b00c9a 100644 --- a/tests/ui/hash_collision.rs +++ b/tests/ui/duplicate_map_keys.rs @@ -1,4 +1,4 @@ -#![warn(clippy::hash_collision)] +#![warn(clippy::duplicate_map_keys)] use std::collections::HashMap; fn main() { diff --git a/tests/ui/hash_collision.stderr b/tests/ui/duplicate_map_keys.stderr similarity index 59% rename from tests/ui/hash_collision.stderr rename to tests/ui/duplicate_map_keys.stderr index b1ee7bfe91bb..5df87cd89903 100644 --- a/tests/ui/hash_collision.stderr +++ b/tests/ui/duplicate_map_keys.stderr @@ -1,12 +1,12 @@ error: this `HashMap` has a hash collision and will lose data - --> tests/ui/hash_collision.rs:5:19 + --> tests/ui/duplicate_map_keys.rs:5:19 | LL | let example = HashMap::from([(5, 1), (5, 2)]); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: consider using a different keys for all items - = note: `-D clippy::hash-collision` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(clippy::hash_collision)]` + = note: `-D clippy::duplicate-map-keys` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::duplicate_map_keys)]` error: aborting due to 1 previous error From ca6d9fb3d33794405b0ca35ba27819762d53cf26 Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Fri, 29 Mar 2024 19:01:32 +0100 Subject: [PATCH 6/8] Improved lint text and added tests. It only matches literals currently, so that needs to change --- clippy_lints/src/duplicate_map_keys.rs | 4 ++-- tests/ui/duplicate_map_keys.rs | 26 +++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/duplicate_map_keys.rs b/clippy_lints/src/duplicate_map_keys.rs index c4550902f6d9..7272a689710a 100644 --- a/clippy_lints/src/duplicate_map_keys.rs +++ b/clippy_lints/src/duplicate_map_keys.rs @@ -22,7 +22,7 @@ declare_clippy_lint! { #[clippy::version = "1.79.0"] pub DUPLICATE_MAP_KEYS, suspicious, - "`HashMap` with two identical keys loses data" + "`HashMap` with duplicate keys loses data" } declare_lint_pass!(DuplicateMapKeys => [DUPLICATE_MAP_KEYS]); @@ -34,7 +34,7 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMapKeys { cx, DUPLICATE_MAP_KEYS, expr.span, - "this `HashMap` has a hash collision and will lose data", + "this `HashMap` uses one key for multiple items and will lose data", None, "consider using a different keys for all items", ); diff --git a/tests/ui/duplicate_map_keys.rs b/tests/ui/duplicate_map_keys.rs index 924d98b00c9a..9c921854c993 100644 --- a/tests/ui/duplicate_map_keys.rs +++ b/tests/ui/duplicate_map_keys.rs @@ -1,6 +1,30 @@ #![warn(clippy::duplicate_map_keys)] use std::collections::HashMap; +use std::hash::{Hash, Hasher}; fn main() { - let example = HashMap::from([(5, 1), (5, 2)]); + // true positive + let _ = HashMap::from([(5, 1), (5, 2)]); + let _ = HashMap::from([((), 1), ((), 2)]); + let _ = HashMap::from([("a", 1), ("a", 2)]); + + // true negative + let _ = HashMap::from([(5, 1), (6, 2)]); + let _ = HashMap::from([("a", 1), ("b", 2)]); + + // false negative + let _ = HashMap::from([(Bad(true), 1), (Bad(false), 2)]); +} + +// Construction of false negative +#[derive(PartialEq)] +struct Bad(bool); + +// eq is used in the lint, but hash in the HashMap +impl Eq for Bad {} + +impl Hash for Bad { + fn hash(&self, state: &mut H) { + // empty + } } From cdd861802c8a5621d4f780a9cff6e454dd86a1bc Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Fri, 29 Mar 2024 19:21:52 +0100 Subject: [PATCH 7/8] made test for duplicate keys more precise (also works on non-literals now) --- clippy_lints/src/duplicate_map_keys.rs | 32 +++++++++++--------------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/duplicate_map_keys.rs b/clippy_lints/src/duplicate_map_keys.rs index 7272a689710a..8533cb695eaa 100644 --- a/clippy_lints/src/duplicate_map_keys.rs +++ b/clippy_lints/src/duplicate_map_keys.rs @@ -1,4 +1,4 @@ -use clippy_utils::diagnostics::span_lint_and_note; +use clippy_utils::{diagnostics::span_lint_and_note, SpanlessEq}; use clippy_utils::last_path_segment; use clippy_utils::ty::is_type_diagnostic_item; use rustc_hir::ExprKind; @@ -29,7 +29,7 @@ declare_lint_pass!(DuplicateMapKeys => [DUPLICATE_MAP_KEYS]); impl<'tcx> LateLintPass<'tcx> for DuplicateMapKeys { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) { - if has_hash_collision(cx, expr) { + if has_hash_collision(cx, expr).is_some_and( |v| !v.is_empty()) { span_lint_and_note( cx, DUPLICATE_MAP_KEYS, @@ -44,7 +44,7 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMapKeys { // TODO: Also check for different sources of hash collisions // TODO: Maybe other types of hash maps should be checked as well? -fn has_hash_collision(cx: &LateContext<'_>, expr: &rustc_hir::Expr<'_>) -> bool { +fn has_hash_collision<'a>(cx: &LateContext<'_>, expr: &'a rustc_hir::Expr<'_>) -> Option, rustc_hir::Expr<'a>)>> { // If the expression is a call to `HashMap::from`, check if the keys are the same if let ExprKind::Call(func, args) = &expr.kind // First check for HashMap::from @@ -59,33 +59,29 @@ fn has_hash_collision(cx: &LateContext<'_>, expr: &rustc_hir::Expr<'_>) -> bool // Then check the keys { // Put all keys in a vector - let mut literals = Vec::new(); + let mut keys = Vec::new(); for arg in *args { // if let ExprKind::Tup(args) = &arg.kind && !args.is_empty() - && let ExprKind::Lit(lit) = args[0].kind + // && let ExprKind::Lit(lit) = args[0].kind { - literals.push(lit.node.clone()); + keys.push(args[0]); } } - // Debug: it gets here, but doesn't trigger the lint // Check if there are any duplicate keys - let mut duplicate = false; - for i in 0..literals.len() { - for j in i + 1..literals.len() { - if literals[i] == literals[j] { - duplicate = true; - break; + let mut duplicate = Vec::new(); + let mut spannless_eq = SpanlessEq::new(cx); + for i in 0..keys.len() { + for j in i + 1..keys.len() { + if spannless_eq.eq_expr(&keys[i], &keys[j]) { + duplicate.push((keys[i], keys[j])); } } - if duplicate { - break; - } } - return duplicate; + return Some(duplicate); } - false + None } From 27af9c001425576e2e2cb86d980643249140ad6e Mon Sep 17 00:00:00 2001 From: Sese Mueller Date: Fri, 29 Mar 2024 21:26:02 +0100 Subject: [PATCH 8/8] Changed tests around, false negative now works as expected --- clippy_lints/src/duplicate_map_keys.rs | 13 ++++++----- tests/ui/duplicate_map_keys.rs | 30 ++++++++++++++++---------- tests/ui/duplicate_map_keys.stderr | 26 +++++++++++++++++----- 3 files changed, 48 insertions(+), 21 deletions(-) diff --git a/clippy_lints/src/duplicate_map_keys.rs b/clippy_lints/src/duplicate_map_keys.rs index 8533cb695eaa..0282c35aa90b 100644 --- a/clippy_lints/src/duplicate_map_keys.rs +++ b/clippy_lints/src/duplicate_map_keys.rs @@ -1,6 +1,6 @@ -use clippy_utils::{diagnostics::span_lint_and_note, SpanlessEq}; -use clippy_utils::last_path_segment; +use clippy_utils::diagnostics::span_lint_and_note; use clippy_utils::ty::is_type_diagnostic_item; +use clippy_utils::{last_path_segment, SpanlessEq}; use rustc_hir::ExprKind; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -29,7 +29,7 @@ declare_lint_pass!(DuplicateMapKeys => [DUPLICATE_MAP_KEYS]); impl<'tcx> LateLintPass<'tcx> for DuplicateMapKeys { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx rustc_hir::Expr<'tcx>) { - if has_hash_collision(cx, expr).is_some_and( |v| !v.is_empty()) { + if has_hash_collision(cx, expr).is_some_and(|v| !v.is_empty()) { span_lint_and_note( cx, DUPLICATE_MAP_KEYS, @@ -44,7 +44,10 @@ impl<'tcx> LateLintPass<'tcx> for DuplicateMapKeys { // TODO: Also check for different sources of hash collisions // TODO: Maybe other types of hash maps should be checked as well? -fn has_hash_collision<'a>(cx: &LateContext<'_>, expr: &'a rustc_hir::Expr<'_>) -> Option, rustc_hir::Expr<'a>)>> { +fn has_hash_collision<'a>( + cx: &LateContext<'_>, + expr: &'a rustc_hir::Expr<'_>, +) -> Option, rustc_hir::Expr<'a>)>> { // If the expression is a call to `HashMap::from`, check if the keys are the same if let ExprKind::Call(func, args) = &expr.kind // First check for HashMap::from @@ -65,7 +68,7 @@ fn has_hash_collision<'a>(cx: &LateContext<'_>, expr: &'a rustc_hir::Expr<'_>) - // if let ExprKind::Tup(args) = &arg.kind && !args.is_empty() - // && let ExprKind::Lit(lit) = args[0].kind + // && let ExprKind::Lit(lit) = args[0].kind { keys.push(args[0]); } diff --git a/tests/ui/duplicate_map_keys.rs b/tests/ui/duplicate_map_keys.rs index 9c921854c993..881f8e3e7065 100644 --- a/tests/ui/duplicate_map_keys.rs +++ b/tests/ui/duplicate_map_keys.rs @@ -1,28 +1,36 @@ #![warn(clippy::duplicate_map_keys)] +#![allow(clippy::diverging_sub_expression)] use std::collections::HashMap; use std::hash::{Hash, Hasher}; -fn main() { - // true positive - let _ = HashMap::from([(5, 1), (5, 2)]); - let _ = HashMap::from([((), 1), ((), 2)]); - let _ = HashMap::from([("a", 1), ("a", 2)]); +fn main() -> Result<(), ()> { + let _ = HashMap::from([(5, 1), (5, 2)]); // expect lint + let _ = HashMap::from([((), 1), ((), 2)]); // expect lint + let _ = HashMap::from([("a", 1), ("a", 2)]); // expect lint - // true negative - let _ = HashMap::from([(5, 1), (6, 2)]); - let _ = HashMap::from([("a", 1), ("b", 2)]); + let _ = HashMap::from([(return Ok(()), 1), (return Err(()), 2)]); // expect no lint + // is this what you meant? I'm not sure how to put return into there - // false negative - let _ = HashMap::from([(Bad(true), 1), (Bad(false), 2)]); + let _ = HashMap::from([(5, 1), (6, 2)]); // expect no lint + let _ = HashMap::from([("a", 1), ("b", 2)]); // expect no lint + + let _ = HashMap::from([(Bad(true), 1), (Bad(false), 2)]); // expect no lint (false negative) + + Ok(()) } // Construction of false negative -#[derive(PartialEq)] struct Bad(bool); // eq is used in the lint, but hash in the HashMap impl Eq for Bad {} +impl PartialEq for Bad { + fn eq(&self, other: &Self) -> bool { + true + } +} + impl Hash for Bad { fn hash(&self, state: &mut H) { // empty diff --git a/tests/ui/duplicate_map_keys.stderr b/tests/ui/duplicate_map_keys.stderr index 5df87cd89903..b49f0007a931 100644 --- a/tests/ui/duplicate_map_keys.stderr +++ b/tests/ui/duplicate_map_keys.stderr @@ -1,12 +1,28 @@ -error: this `HashMap` has a hash collision and will lose data - --> tests/ui/duplicate_map_keys.rs:5:19 +error: this `HashMap` uses one key for multiple items and will lose data + --> tests/ui/duplicate_map_keys.rs:7:13 | -LL | let example = HashMap::from([(5, 1), (5, 2)]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +LL | let _ = HashMap::from([(5, 1), (5, 2)]); // expect lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: consider using a different keys for all items = note: `-D clippy::duplicate-map-keys` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(clippy::duplicate_map_keys)]` -error: aborting due to 1 previous error +error: this `HashMap` uses one key for multiple items and will lose data + --> tests/ui/duplicate_map_keys.rs:8:13 + | +LL | let _ = HashMap::from([((), 1), ((), 2)]); // expect lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: consider using a different keys for all items + +error: this `HashMap` uses one key for multiple items and will lose data + --> tests/ui/duplicate_map_keys.rs:9:13 + | +LL | let _ = HashMap::from([("a", 1), ("a", 2)]); // expect lint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: consider using a different keys for all items + +error: aborting due to 3 previous errors