diff --git a/CHANGELOG.md b/CHANGELOG.md index 87bffe7f74d6..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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index eba048ad90be..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, diff --git a/clippy_lints/src/duplicate_map_keys.rs b/clippy_lints/src/duplicate_map_keys.rs new file mode 100644 index 000000000000..0282c35aa90b --- /dev/null +++ b/clippy_lints/src/duplicate_map_keys.rs @@ -0,0 +1,90 @@ +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; +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 DUPLICATE_MAP_KEYS, + suspicious, + "`HashMap` with duplicate keys loses data" +} + +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()) { + span_lint_and_note( + cx, + DUPLICATE_MAP_KEYS, + expr.span, + "this `HashMap` uses one key for multiple items 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<'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 + && 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 keys = Vec::new(); + + for arg in *args { + // + if let ExprKind::Tup(args) = &arg.kind + && !args.is_empty() + // && let ExprKind::Lit(lit) = args[0].kind + { + keys.push(args[0]); + } + } + + // Check if there are any duplicate keys + 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])); + } + } + } + return Some(duplicate); + } + None +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 169e51e2cb93..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; @@ -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(duplicate_map_keys::DuplicateMapKeys)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/duplicate_map_keys.rs b/tests/ui/duplicate_map_keys.rs new file mode 100644 index 000000000000..881f8e3e7065 --- /dev/null +++ b/tests/ui/duplicate_map_keys.rs @@ -0,0 +1,38 @@ +#![warn(clippy::duplicate_map_keys)] +#![allow(clippy::diverging_sub_expression)] +use std::collections::HashMap; +use std::hash::{Hash, Hasher}; + +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 + + 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 + + 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 +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 new file mode 100644 index 000000000000..b49f0007a931 --- /dev/null +++ b/tests/ui/duplicate_map_keys.stderr @@ -0,0 +1,28 @@ +error: this `HashMap` uses one key for multiple items and will lose data + --> tests/ui/duplicate_map_keys.rs:7:13 + | +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: 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 +