From f09dc8b67c0e826f16eec4ba6205faa8f2f85bf3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 3 Nov 2024 14:16:34 -0500 Subject: [PATCH] Detect items that hash to same value in duplicate dictionaries (#14065) ## Summary Closes https://github.com/astral-sh/ruff/issues/12772. --- .../resources/test/fixtures/pyflakes/F601.py | 10 + .../src/rules/pyflakes/rules/repeated_keys.rs | 184 ++++++++++-------- ..._rules__pyflakes__tests__F601_F601.py.snap | 60 +++++- 3 files changed, 170 insertions(+), 84 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py b/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py index 3ef8f1e55d..3fdf218b68 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py +++ b/crates/ruff_linter/resources/test/fixtures/pyflakes/F601.py @@ -58,3 +58,13 @@ x = { t={"x":"test123", "x":("test123")} t={"x":("test123"), "x":"test123"} + +# Regression test for: https://github.com/astral-sh/ruff/issues/12772 +x = { + 1: "abc", + 1: "def", + True: "ghi", + 0: "foo", + 0: "bar", + False: "baz", +} diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs b/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs index 06ef03d97f..8aa757ae83 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/repeated_keys.rs @@ -1,8 +1,9 @@ use rustc_hash::{FxBuildHasher, FxHashMap, FxHashSet}; +use std::collections::hash_map::Entry; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::comparable::ComparableExpr; +use ruff_python_ast::comparable::{ComparableExpr, HashableExpr}; use ruff_python_ast::parenthesize::parenthesized_range; use ruff_python_ast::{self as ast, Expr}; use ruff_text_size::Ranged; @@ -44,6 +45,7 @@ use crate::registry::Rule; #[violation] pub struct MultiValueRepeatedKeyLiteral { name: SourceCodeSnippet, + existing: SourceCodeSnippet, } impl Violation for MultiValueRepeatedKeyLiteral { @@ -51,16 +53,26 @@ impl Violation for MultiValueRepeatedKeyLiteral { #[derive_message_formats] fn message(&self) -> String { - let MultiValueRepeatedKeyLiteral { name } = self; - if let Some(name) = name.full_display() { - format!("Dictionary key literal `{name}` repeated") - } else { - format!("Dictionary key literal repeated") + let MultiValueRepeatedKeyLiteral { name, existing } = self; + match (name.full_display(), existing.full_display()) { + (Some(name), None) => { + format!("Dictionary key literal `{name}` repeated") + } + (Some(name), Some(existing)) => { + if name == existing { + format!("Dictionary key literal `{name}` repeated") + } else { + format!( + "Dictionary key literal `{name}` repeated (`{name}` hashes to the same value as `{existing}`)" + ) + } + } + _ => format!("Dictionary key literal repeated"), } } fn fix_title(&self) -> Option { - let MultiValueRepeatedKeyLiteral { name } = self; + let MultiValueRepeatedKeyLiteral { name, .. } = self; if let Some(name) = name.full_display() { Some(format!("Remove repeated key literal `{name}`")) } else { @@ -129,7 +141,7 @@ impl Violation for MultiValueRepeatedKeyVariable { /// F601, F602 pub(crate) fn repeated_keys(checker: &mut Checker, dict: &ast::ExprDict) { // Generate a map from key to (index, value). - let mut seen: FxHashMap> = + let mut seen: FxHashMap)> = FxHashMap::with_capacity_and_hasher(dict.len(), FxBuildHasher); // Detect duplicate keys. @@ -138,86 +150,92 @@ pub(crate) fn repeated_keys(checker: &mut Checker, dict: &ast::ExprDict) { continue; }; - let comparable_key = ComparableExpr::from(key); + let comparable_key = HashableExpr::from(key); let comparable_value = ComparableExpr::from(value); - let Some(seen_values) = seen.get_mut(&comparable_key) else { - seen.insert(comparable_key, FxHashSet::from_iter([comparable_value])); - continue; - }; - - match key { - Expr::StringLiteral(_) - | Expr::BytesLiteral(_) - | Expr::NumberLiteral(_) - | Expr::BooleanLiteral(_) - | Expr::NoneLiteral(_) - | Expr::EllipsisLiteral(_) - | Expr::Tuple(_) - | Expr::FString(_) => { - if checker.enabled(Rule::MultiValueRepeatedKeyLiteral) { - let mut diagnostic = Diagnostic::new( - MultiValueRepeatedKeyLiteral { - name: SourceCodeSnippet::from_str(checker.locator().slice(key)), - }, - key.range(), - ); - if !seen_values.insert(comparable_value) { - diagnostic.set_fix(Fix::unsafe_edit(Edit::deletion( - parenthesized_range( - dict.value(i - 1).into(), - dict.into(), - checker.comment_ranges(), - checker.locator().contents(), - ) - .unwrap_or_else(|| dict.value(i - 1).range()) - .end(), - parenthesized_range( - dict.value(i).into(), - dict.into(), - checker.comment_ranges(), - checker.locator().contents(), - ) - .unwrap_or_else(|| dict.value(i).range()) - .end(), - ))); + match seen.entry(comparable_key) { + Entry::Vacant(entry) => { + entry.insert((key, FxHashSet::from_iter([comparable_value]))); + } + Entry::Occupied(mut entry) => { + let (seen_key, seen_values) = entry.get_mut(); + match key { + Expr::StringLiteral(_) + | Expr::BytesLiteral(_) + | Expr::NumberLiteral(_) + | Expr::BooleanLiteral(_) + | Expr::NoneLiteral(_) + | Expr::EllipsisLiteral(_) + | Expr::Tuple(_) + | Expr::FString(_) => { + if checker.enabled(Rule::MultiValueRepeatedKeyLiteral) { + let mut diagnostic = Diagnostic::new( + MultiValueRepeatedKeyLiteral { + name: SourceCodeSnippet::from_str(checker.locator().slice(key)), + existing: SourceCodeSnippet::from_str( + checker.locator().slice(*seen_key), + ), + }, + key.range(), + ); + if !seen_values.insert(comparable_value) { + diagnostic.set_fix(Fix::unsafe_edit(Edit::deletion( + parenthesized_range( + dict.value(i - 1).into(), + dict.into(), + checker.comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or_else(|| dict.value(i - 1).range()) + .end(), + parenthesized_range( + dict.value(i).into(), + dict.into(), + checker.comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or_else(|| dict.value(i).range()) + .end(), + ))); + } + checker.diagnostics.push(diagnostic); + } } - checker.diagnostics.push(diagnostic); + Expr::Name(_) => { + if checker.enabled(Rule::MultiValueRepeatedKeyVariable) { + let mut diagnostic = Diagnostic::new( + MultiValueRepeatedKeyVariable { + name: SourceCodeSnippet::from_str(checker.locator().slice(key)), + }, + key.range(), + ); + let comparable_value: ComparableExpr = dict.value(i).into(); + if !seen_values.insert(comparable_value) { + diagnostic.set_fix(Fix::unsafe_edit(Edit::deletion( + parenthesized_range( + dict.value(i - 1).into(), + dict.into(), + checker.comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or_else(|| dict.value(i - 1).range()) + .end(), + parenthesized_range( + dict.value(i).into(), + dict.into(), + checker.comment_ranges(), + checker.locator().contents(), + ) + .unwrap_or_else(|| dict.value(i).range()) + .end(), + ))); + } + checker.diagnostics.push(diagnostic); + } + } + _ => {} } } - Expr::Name(_) => { - if checker.enabled(Rule::MultiValueRepeatedKeyVariable) { - let mut diagnostic = Diagnostic::new( - MultiValueRepeatedKeyVariable { - name: SourceCodeSnippet::from_str(checker.locator().slice(key)), - }, - key.range(), - ); - let comparable_value: ComparableExpr = dict.value(i).into(); - if !seen_values.insert(comparable_value) { - diagnostic.set_fix(Fix::unsafe_edit(Edit::deletion( - parenthesized_range( - dict.value(i - 1).into(), - dict.into(), - checker.comment_ranges(), - checker.locator().contents(), - ) - .unwrap_or_else(|| dict.value(i - 1).range()) - .end(), - parenthesized_range( - dict.value(i).into(), - dict.into(), - checker.comment_ranges(), - checker.locator().contents(), - ) - .unwrap_or_else(|| dict.value(i).range()) - .end(), - ))); - } - checker.diagnostics.push(diagnostic); - } - } - _ => {} } } } diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap index ace4761b43..3d786c91f5 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F601_F601.py.snap @@ -23,7 +23,18 @@ F601.py:6:5: F601 Dictionary key literal `("a", "b")` repeated | = help: Remove repeated key literal `("a", "b")` -F601.py:9:5: F601 Dictionary key literal `1` repeated +F601.py:8:5: F601 Dictionary key literal `1` repeated (`1` hashes to the same value as `1.0`) + | + 6 | ("a", "b"): 4, + 7 | 1.0: 2, + 8 | 1: 0, + | ^ F601 + 9 | 1: 3, +10 | b"123": 1, + | + = help: Remove repeated key literal `1` + +F601.py:9:5: F601 Dictionary key literal `1` repeated (`1` hashes to the same value as `1.0`) | 7 | 1.0: 2, 8 | 1: 0, @@ -299,6 +310,7 @@ F601.py:58:19: F601 [*] Dictionary key literal `"x"` repeated 58 |+t={"x":"test123"} 59 59 | 60 60 | t={"x":("test123"), "x":"test123"} +61 61 | F601.py:60:21: F601 [*] Dictionary key literal `"x"` repeated | @@ -306,6 +318,8 @@ F601.py:60:21: F601 [*] Dictionary key literal `"x"` repeated 59 | 60 | t={"x":("test123"), "x":"test123"} | ^^^ F601 +61 | +62 | # Regression test for: https://github.com/astral-sh/ruff/issues/12772 | = help: Remove repeated key literal `"x"` @@ -315,5 +329,49 @@ F601.py:60:21: F601 [*] Dictionary key literal `"x"` repeated 59 59 | 60 |-t={"x":("test123"), "x":"test123"} 60 |+t={"x":("test123")} +61 61 | +62 62 | # Regression test for: https://github.com/astral-sh/ruff/issues/12772 +63 63 | x = { +F601.py:65:5: F601 Dictionary key literal `1` repeated + | +63 | x = { +64 | 1: "abc", +65 | 1: "def", + | ^ F601 +66 | True: "ghi", +67 | 0: "foo", + | + = help: Remove repeated key literal `1` +F601.py:66:5: F601 Dictionary key literal `True` repeated (`True` hashes to the same value as `1`) + | +64 | 1: "abc", +65 | 1: "def", +66 | True: "ghi", + | ^^^^ F601 +67 | 0: "foo", +68 | 0: "bar", + | + = help: Remove repeated key literal `True` + +F601.py:68:5: F601 Dictionary key literal `0` repeated + | +66 | True: "ghi", +67 | 0: "foo", +68 | 0: "bar", + | ^ F601 +69 | False: "baz", +70 | } + | + = help: Remove repeated key literal `0` + +F601.py:69:5: F601 Dictionary key literal `False` repeated (`False` hashes to the same value as `0`) + | +67 | 0: "foo", +68 | 0: "bar", +69 | False: "baz", + | ^^^^^ F601 +70 | } + | + = help: Remove repeated key literal `False`