New lint: [`duplicate_map_keys`] #12575

SeseMueller · 2024-03-27T21:33:24Z

Fixes #11978

changelog: new [duplicate_map_keys] lint

…s to follow

rustbot · 2024-03-27T21:33:29Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

SeseMueller · 2024-03-27T22:06:48Z

As outlined in #11978, the main goal now is to also detect if multiple inserts into a HashMap are done on the same key, which is considerably more difficult. I'll start work on it in the near future.

llogiq · 2024-03-28T00:32:14Z

clippy_lints/src/hash_collision.rs

+    /// 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.


Suggested change

/// 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.

/// Warn when a `Hashmap` is set up with the

/// same key appearing twice.

///

/// ### Why is this bad?

/// When two items are inserted into a `HashMap` with the same key,

/// the second item will overwrite the first item.

/// This can lead to data loss.

llogiq · 2024-03-28T00:34:47Z

clippy_lints/src/hash_collision.rs

+    ///
+    /// ### Why is this bad?
+    /// This can lead to data loss.
+    ///


Suggested change

///

///

/// # Known Problems

/// False negatives: The lint only looks into

/// `HashMap::from([..])` calls.

llogiq · 2024-03-28T00:38:37Z

clippy_lints/src/hash_collision.rs

+    // Then check the keys
+    {
+        // Put all keys in a vector
+        let mut literals = Vec::new();


You could use SpanlessEq to correctly lint on arbitrary key expressions.

Also, why not return a (possibly empty) set of duplicate key spans instead of a plain bool? That way we could better pinpoint the error.

Thanks for point out SpanlessEq. The function now returns Option<Vec<(Expr, Expr)>>, so all pairs of found duplicates. I should probably someday change it to return all Expressions with their duplicates instead of split over multiple pairs.

llogiq · 2024-03-28T00:44:38Z

tests/ui/hash_collision.rs

+use std::collections::HashMap;
+
+fn main() {
+    let example = HashMap::from([(5, 1), (5, 2)]);


I'd like to see more tests. For example one with a () key, one with a key expr containing a return statement, and one with a custom key type containing a bool where eq always returns true and hash is an empty function, then HashMap::from([(Bad(true), 1), (Bad(false)), 2)]). That would constitute an (acceptable) false negative.

I've now written and tested all tests you recommended, but I didn't fully understand what you meant with "key expr containg a return statement"

let _ = HashMap::from([(return Ok(()), 1), (return Err(()), 2)]); // expect no lint

is what I did, but everything should already be caught by clippy::diverging_sub_expression, unless I missed or misunderstood something.

y21 · 2024-03-28T08:24:36Z

clippy_lints/src/hash_collision.rs

+    /// let example = HashMap::from([(5, 1), (5, 2)]);
+    /// ```
+    #[clippy::version = "1.79.0"]
+    pub HASH_COLLISION,


"Hash collision" to me means different keys with the same hash, but this lint specifically looks for same keys.
(A hash collision also isn't bad in and of itself and won't lead to overwriting entries, a hashmap can deal with that)

Should this be renamed from hash_collision to something else? What about duplicate_map_keys?

Thanks, I should have caught that. duplicate_map_keys is a good name, I've changed it in the latest commit.

…y, so that needs to change

… now)

llogiq · 2024-04-04T09:35:44Z

clippy_lints/src/duplicate_map_keys.rs

+    expr: &'a rustc_hir::Expr<'_>,
+) -> Option<Vec<(rustc_hir::Expr<'a>, 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


Suggested change

if let ExprKind::Call(func, args) = &expr.kind

if let ExprKind::Call(func, [arg]) = &expr.kind

Matching a 1-element slice would let you avoid the args.len() == 1 check and index later.

llogiq · 2024-04-04T09:42:25Z

clippy_lints/src/duplicate_map_keys.rs

+        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]);
+            }
+        }


Suggested change

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]);

}

}

let keys = args.iter().filter_map(|arg| {

if let ExprKind::Tup([key, ..]) = arg.kind {

Some(key)

} else {

None

}

}).collect::<Vec<_>>();

llogiq · 2024-04-04T10:31:14Z

clippy_lints/src/duplicate_map_keys.rs

+        for i in 0..keys.len() {
+            for j in i + 1..keys.len() {


This is a quadratic algorithm that will slow down considerably with larger sets (try a thousand elements). A better way would be to use a SpanlessHash per item and add them to a HashSet, which is linear.

bors · 2024-05-12T16:46:30Z

☔ The latest upstream changes (presumably #12107) made this pull request unmergeable. Please resolve the merge conflicts.

llogiq · 2024-05-30T13:44:41Z

Are you still interested in following through with this PR? Do you need anything from our side?

SeseMueller · 2024-05-30T16:27:09Z

Are you still interested in following through with this PR? Do you need anything from our side?

Yes, I am interested, but I currently have little time. Uni takes more time than I thought. I hope to continue work within a month as then the hard part of the semester should be over... I don't currently need any help, thank you.

llogiq · 2024-05-31T16:39:13Z

In that case I'd prefer to close this for now so it doesn't clutter my queue. Please feel invited to reopen as soon as you find the time to continue working on it.

Sese Mueller added 2 commits March 27, 2024 20:42

Implemented first lint configuration. More work for more complex case…

7a903bd

…s to follow

Fixed cargo test not passing; minor changes

cff98f2

rustbot assigned llogiq Mar 27, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 27, 2024

Sese Mueller added 2 commits March 27, 2024 22:43

Update lint description

434a92e

removed trailing whitespace

efb8e4d

llogiq reviewed Mar 28, 2024

View reviewed changes

y21 reviewed Mar 28, 2024

View reviewed changes

Changed lint name to duplicate_map_keys

6c13a90

SeseMueller changed the title ~~New lint: [hash_collision]~~ New lint: [duplicate_map_keys] Mar 28, 2024

Sese Mueller added 3 commits March 29, 2024 19:01

Improved lint text and added tests. It only matches literals currentl…

ca6d9fb

…y, so that needs to change

made test for duplicate keys more precise (also works on non-literals…

cdd8618

… now)

Changed tests around, false negative now works as expected

27af9c0

llogiq reviewed Apr 4, 2024

View reviewed changes

llogiq closed this May 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: [`duplicate_map_keys`] #12575

New lint: [`duplicate_map_keys`] #12575

SeseMueller commented Mar 27, 2024 •

edited by llogiq

Loading

rustbot commented Mar 27, 2024

SeseMueller commented Mar 27, 2024

llogiq Mar 28, 2024

llogiq Mar 28, 2024

llogiq Mar 28, 2024

SeseMueller Mar 29, 2024

llogiq Mar 28, 2024

SeseMueller Mar 29, 2024 •

edited

Loading

y21 Mar 28, 2024

SeseMueller Mar 28, 2024

llogiq Apr 4, 2024

llogiq Apr 4, 2024

llogiq Apr 4, 2024

bors commented May 12, 2024

llogiq commented May 30, 2024

SeseMueller commented May 30, 2024

llogiq commented May 31, 2024

-    ///
+    ///
+    /// # Known Problems
+    /// False negatives: The lint only looks into
+    /// `HashMap::from([..])` calls.

	if let ExprKind::Call(func, args) = &expr.kind
	if let ExprKind::Call(func, [arg]) = &expr.kind

New lint: [duplicate_map_keys] #12575

New lint: [duplicate_map_keys] #12575

Conversation

SeseMueller commented Mar 27, 2024 • edited by llogiq Loading

rustbot commented Mar 27, 2024

SeseMueller commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeseMueller Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 12, 2024

llogiq commented May 30, 2024

SeseMueller commented May 30, 2024

llogiq commented May 31, 2024

New lint: [`duplicate_map_keys`] #12575

New lint: [`duplicate_map_keys`] #12575

SeseMueller commented Mar 27, 2024 •

edited by llogiq

Loading

SeseMueller Mar 29, 2024 •

edited

Loading