mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-26 11:59:35 +00:00
Refactor repeated_keys() to use ComparableExpr (#5696)
## Summary Replaces `DictionaryKey` enum with the more general `ComparableExpr` when checking for duplicate keys ## Test Plan Added test fixture from issue. Can potentially be expanded further depending on what exactly we want to flag (e.g. do we also want to check for unhashable types?) and which `ComparableExpr::XYZ` types we consider literals. ## Issue link Closes: https://github.com/astral-sh/ruff/issues/5691
This commit is contained in:
qdegraaf
GitHub
parent
5dd9e56748
commit
7566ca8ff7
4 changed files with 114 additions and 93 deletions
|
|
@ -48,3 +48,8 @@ x = {
|
|||
|
||||
x = {"a": 1, "a": 1}
|
||||
x = {"a": 1, "b": 2, "a": 1}
|
||||
|
||||
x = {
|
||||
('a', 'b'): 'asdf',
|
||||
('a', 'b'): 'qwer',
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,11 +1,11 @@
|
|||
use std::hash::{BuildHasherDefault, Hash};
|
||||
use std::hash::BuildHasherDefault;
|
||||
|
||||
use rustc_hash::{FxHashMap, FxHashSet};
|
||||
use rustpython_parser::ast::{self, Expr, Ranged};
|
||||
use rustpython_parser::ast::{Expr, Ranged};
|
||||
|
||||
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
|
||||
use ruff_macros::{derive_message_formats, violation};
|
||||
use ruff_python_ast::comparable::{ComparableConstant, ComparableExpr};
|
||||
use ruff_python_ast::comparable::ComparableExpr;
|
||||
|
||||
use crate::checkers::ast::Checker;
|
||||
use crate::registry::{AsRule, Rule};
|
||||
|
|
@ -43,7 +43,6 @@ use crate::registry::{AsRule, Rule};
|
|||
#[violation]
|
||||
pub struct MultiValueRepeatedKeyLiteral {
|
||||
name: String,
|
||||
repeated_value: bool,
|
||||
}
|
||||
|
||||
impl Violation for MultiValueRepeatedKeyLiteral {
|
||||
|
|
@ -51,20 +50,13 @@ impl Violation for MultiValueRepeatedKeyLiteral {
|
|||
|
||||
#[derive_message_formats]
|
||||
fn message(&self) -> String {
|
||||
let MultiValueRepeatedKeyLiteral { name, .. } = self;
|
||||
let MultiValueRepeatedKeyLiteral { name } = self;
|
||||
format!("Dictionary key literal `{name}` repeated")
|
||||
}
|
||||
|
||||
fn autofix_title(&self) -> Option<String> {
|
||||
let MultiValueRepeatedKeyLiteral {
|
||||
repeated_value,
|
||||
name,
|
||||
} = self;
|
||||
if *repeated_value {
|
||||
Some(format!("Remove repeated key literal `{name}`"))
|
||||
} else {
|
||||
None
|
||||
}
|
||||
let MultiValueRepeatedKeyLiteral { name } = self;
|
||||
Some(format!("Remove repeated key literal `{name}`"))
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -100,7 +92,6 @@ impl Violation for MultiValueRepeatedKeyLiteral {
|
|||
#[violation]
|
||||
pub struct MultiValueRepeatedKeyVariable {
|
||||
name: String,
|
||||
repeated_value: bool,
|
||||
}
|
||||
|
||||
impl Violation for MultiValueRepeatedKeyVariable {
|
||||
|
|
@ -108,43 +99,20 @@ impl Violation for MultiValueRepeatedKeyVariable {
|
|||
|
||||
#[derive_message_formats]
|
||||
fn message(&self) -> String {
|
||||
let MultiValueRepeatedKeyVariable { name, .. } = self;
|
||||
let MultiValueRepeatedKeyVariable { name } = self;
|
||||
format!("Dictionary key `{name}` repeated")
|
||||
}
|
||||
|
||||
fn autofix_title(&self) -> Option<String> {
|
||||
let MultiValueRepeatedKeyVariable {
|
||||
repeated_value,
|
||||
name,
|
||||
} = self;
|
||||
if *repeated_value {
|
||||
Some(format!("Remove repeated key `{name}`"))
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Eq, PartialEq, Hash)]
|
||||
enum DictionaryKey<'a> {
|
||||
Constant(ComparableConstant<'a>),
|
||||
Variable(&'a str),
|
||||
}
|
||||
|
||||
fn into_dictionary_key(expr: &Expr) -> Option<DictionaryKey> {
|
||||
match expr {
|
||||
Expr::Constant(ast::ExprConstant { value, .. }) => {
|
||||
Some(DictionaryKey::Constant(value.into()))
|
||||
}
|
||||
Expr::Name(ast::ExprName { id, .. }) => Some(DictionaryKey::Variable(id)),
|
||||
_ => None,
|
||||
let MultiValueRepeatedKeyVariable { name } = self;
|
||||
Some(format!("Remove repeated key `{name}`"))
|
||||
}
|
||||
}
|
||||
|
||||
/// F601, F602
|
||||
pub(crate) fn repeated_keys(checker: &mut Checker, keys: &[Option<Expr>], values: &[Expr]) {
|
||||
// Generate a map from key to (index, value).
|
||||
let mut seen: FxHashMap<DictionaryKey, FxHashSet<ComparableExpr>> =
|
||||
let mut seen: FxHashMap<ComparableExpr, FxHashSet<ComparableExpr>> =
|
||||
FxHashMap::with_capacity_and_hasher(keys.len(), BuildHasherDefault::default());
|
||||
|
||||
// Detect duplicate keys.
|
||||
|
|
@ -152,61 +120,56 @@ pub(crate) fn repeated_keys(checker: &mut Checker, keys: &[Option<Expr>], values
|
|||
let Some(key) = key else {
|
||||
continue;
|
||||
};
|
||||
if let Some(dict_key) = into_dictionary_key(key) {
|
||||
if let Some(seen_values) = seen.get_mut(&dict_key) {
|
||||
match dict_key {
|
||||
DictionaryKey::Constant(..) => {
|
||||
if checker.enabled(Rule::MultiValueRepeatedKeyLiteral) {
|
||||
let comparable_value: ComparableExpr = (&values[i]).into();
|
||||
let is_duplicate_value = seen_values.contains(&comparable_value);
|
||||
let mut diagnostic = Diagnostic::new(
|
||||
MultiValueRepeatedKeyLiteral {
|
||||
name: checker.generator().expr(key),
|
||||
repeated_value: is_duplicate_value,
|
||||
},
|
||||
key.range(),
|
||||
);
|
||||
if is_duplicate_value {
|
||||
if checker.patch(diagnostic.kind.rule()) {
|
||||
diagnostic.set_fix(Fix::suggested(Edit::deletion(
|
||||
values[i - 1].end(),
|
||||
values[i].end(),
|
||||
)));
|
||||
}
|
||||
} else {
|
||||
seen_values.insert(comparable_value);
|
||||
}
|
||||
checker.diagnostics.push(diagnostic);
|
||||
}
|
||||
}
|
||||
DictionaryKey::Variable(dict_key) => {
|
||||
if checker.enabled(Rule::MultiValueRepeatedKeyVariable) {
|
||||
let comparable_value: ComparableExpr = (&values[i]).into();
|
||||
let is_duplicate_value = seen_values.contains(&comparable_value);
|
||||
let mut diagnostic = Diagnostic::new(
|
||||
MultiValueRepeatedKeyVariable {
|
||||
name: dict_key.to_string(),
|
||||
repeated_value: is_duplicate_value,
|
||||
},
|
||||
key.range(),
|
||||
);
|
||||
if is_duplicate_value {
|
||||
if checker.patch(diagnostic.kind.rule()) {
|
||||
diagnostic.set_fix(Fix::suggested(Edit::deletion(
|
||||
values[i - 1].end(),
|
||||
values[i].end(),
|
||||
)));
|
||||
}
|
||||
} else {
|
||||
seen_values.insert(comparable_value);
|
||||
}
|
||||
checker.diagnostics.push(diagnostic);
|
||||
|
||||
let comparable_key = ComparableExpr::from(key);
|
||||
let comparable_value = ComparableExpr::from(&values[i]);
|
||||
|
||||
let Some(seen_values) = seen.get_mut(&comparable_key) else {
|
||||
seen.insert(comparable_key, FxHashSet::from_iter([comparable_value]));
|
||||
continue;
|
||||
};
|
||||
|
||||
match key {
|
||||
Expr::Constant(_) | Expr::Tuple(_) | Expr::JoinedStr(_) => {
|
||||
if checker.enabled(Rule::MultiValueRepeatedKeyLiteral) {
|
||||
let mut diagnostic = Diagnostic::new(
|
||||
MultiValueRepeatedKeyLiteral {
|
||||
name: checker.locator.slice(key.range()).to_string(),
|
||||
},
|
||||
key.range(),
|
||||
);
|
||||
if checker.patch(diagnostic.kind.rule()) {
|
||||
if !seen_values.insert(comparable_value) {
|
||||
diagnostic.set_fix(Fix::suggested(Edit::deletion(
|
||||
values[i - 1].end(),
|
||||
values[i].end(),
|
||||
)));
|
||||
}
|
||||
}
|
||||
checker.diagnostics.push(diagnostic);
|
||||
}
|
||||
} else {
|
||||
seen.insert(dict_key, FxHashSet::from_iter([(&values[i]).into()]));
|
||||
}
|
||||
Expr::Name(_) => {
|
||||
if checker.enabled(Rule::MultiValueRepeatedKeyVariable) {
|
||||
let mut diagnostic = Diagnostic::new(
|
||||
MultiValueRepeatedKeyVariable {
|
||||
name: checker.locator.slice(key.range()).to_string(),
|
||||
},
|
||||
key.range(),
|
||||
);
|
||||
if checker.patch(diagnostic.kind.rule()) {
|
||||
let comparable_value: ComparableExpr = (&values[i]).into();
|
||||
if !seen_values.insert(comparable_value) {
|
||||
diagnostic.set_fix(Fix::suggested(Edit::deletion(
|
||||
values[i - 1].end(),
|
||||
values[i].end(),
|
||||
)));
|
||||
}
|
||||
}
|
||||
checker.diagnostics.push(diagnostic);
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -10,6 +10,18 @@ F601.py:3:5: F601 Dictionary key literal `"a"` repeated
|
|||
4 | "b": 3,
|
||||
5 | ("a", "b"): 3,
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:6:5: F601 Dictionary key literal `("a", "b")` repeated
|
||||
|
|
||||
4 | "b": 3,
|
||||
5 | ("a", "b"): 3,
|
||||
6 | ("a", "b"): 4,
|
||||
| ^^^^^^^^^^ F601
|
||||
7 | 1.0: 2,
|
||||
8 | 1: 0,
|
||||
|
|
||||
= help: Remove repeated key literal `("a", "b")`
|
||||
|
||||
F601.py:9:5: F601 Dictionary key literal `1` repeated
|
||||
|
|
||||
|
|
@ -20,6 +32,7 @@ F601.py:9:5: F601 Dictionary key literal `1` repeated
|
|||
10 | b"123": 1,
|
||||
11 | b"123": 4,
|
||||
|
|
||||
= help: Remove repeated key literal `1`
|
||||
|
||||
F601.py:11:5: F601 Dictionary key literal `b"123"` repeated
|
||||
|
|
||||
|
|
@ -29,6 +42,7 @@ F601.py:11:5: F601 Dictionary key literal `b"123"` repeated
|
|||
| ^^^^^^ F601
|
||||
12 | }
|
||||
|
|
||||
= help: Remove repeated key literal `b"123"`
|
||||
|
||||
F601.py:16:5: F601 Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -39,6 +53,7 @@ F601.py:16:5: F601 Dictionary key literal `"a"` repeated
|
|||
17 | "a": 3,
|
||||
18 | "a": 3,
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:17:5: F601 Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -49,6 +64,7 @@ F601.py:17:5: F601 Dictionary key literal `"a"` repeated
|
|||
18 | "a": 3,
|
||||
19 | }
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:18:5: F601 [*] Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -78,6 +94,7 @@ F601.py:23:5: F601 Dictionary key literal `"a"` repeated
|
|||
24 | "a": 3,
|
||||
25 | "a": 3,
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:24:5: F601 Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -88,6 +105,7 @@ F601.py:24:5: F601 Dictionary key literal `"a"` repeated
|
|||
25 | "a": 3,
|
||||
26 | "a": 4,
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:25:5: F601 [*] Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -117,6 +135,7 @@ F601.py:26:5: F601 Dictionary key literal `"a"` repeated
|
|||
| ^^^ F601
|
||||
27 | }
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:31:5: F601 [*] Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -147,6 +166,7 @@ F601.py:32:5: F601 Dictionary key literal `"a"` repeated
|
|||
33 | "a": 3,
|
||||
34 | "a": 4,
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:33:5: F601 Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -157,6 +177,7 @@ F601.py:33:5: F601 Dictionary key literal `"a"` repeated
|
|||
34 | "a": 4,
|
||||
35 | }
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:34:5: F601 Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -166,6 +187,7 @@ F601.py:34:5: F601 Dictionary key literal `"a"` repeated
|
|||
| ^^^ F601
|
||||
35 | }
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:41:5: F601 Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -176,6 +198,7 @@ F601.py:41:5: F601 Dictionary key literal `"a"` repeated
|
|||
42 | a: 2,
|
||||
43 | "a": 3,
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:43:5: F601 Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -186,6 +209,7 @@ F601.py:43:5: F601 Dictionary key literal `"a"` repeated
|
|||
44 | a: 3,
|
||||
45 | "a": 3,
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
F601.py:45:5: F601 [*] Dictionary key literal `"a"` repeated
|
||||
|
|
||||
|
|
@ -224,12 +248,16 @@ F601.py:49:14: F601 [*] Dictionary key literal `"a"` repeated
|
|||
49 |-x = {"a": 1, "a": 1}
|
||||
49 |+x = {"a": 1}
|
||||
50 50 | x = {"a": 1, "b": 2, "a": 1}
|
||||
51 51 |
|
||||
52 52 | x = {
|
||||
|
||||
F601.py:50:22: F601 [*] Dictionary key literal `"a"` repeated
|
||||
|
|
||||
49 | x = {"a": 1, "a": 1}
|
||||
50 | x = {"a": 1, "b": 2, "a": 1}
|
||||
| ^^^ F601
|
||||
51 |
|
||||
52 | x = {
|
||||
|
|
||||
= help: Remove repeated key literal `"a"`
|
||||
|
||||
|
|
@ -239,5 +267,18 @@ F601.py:50:22: F601 [*] Dictionary key literal `"a"` repeated
|
|||
49 49 | x = {"a": 1, "a": 1}
|
||||
50 |-x = {"a": 1, "b": 2, "a": 1}
|
||||
50 |+x = {"a": 1, "b": 2}
|
||||
51 51 |
|
||||
52 52 | x = {
|
||||
53 53 | ('a', 'b'): 'asdf',
|
||||
|
||||
F601.py:54:5: F601 Dictionary key literal `('a', 'b')` repeated
|
||||
|
|
||||
52 | x = {
|
||||
53 | ('a', 'b'): 'asdf',
|
||||
54 | ('a', 'b'): 'qwer',
|
||||
| ^^^^^^^^^^ F601
|
||||
55 | }
|
||||
|
|
||||
= help: Remove repeated key literal `('a', 'b')`
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ F602.py:5:5: F602 Dictionary key `a` repeated
|
|||
6 | b: 3,
|
||||
7 | }
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:11:5: F602 Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -20,6 +21,7 @@ F602.py:11:5: F602 Dictionary key `a` repeated
|
|||
12 | a: 3,
|
||||
13 | a: 3,
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:12:5: F602 Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -30,6 +32,7 @@ F602.py:12:5: F602 Dictionary key `a` repeated
|
|||
13 | a: 3,
|
||||
14 | }
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:13:5: F602 [*] Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -59,6 +62,7 @@ F602.py:18:5: F602 Dictionary key `a` repeated
|
|||
19 | a: 3,
|
||||
20 | a: 3,
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:19:5: F602 Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -69,6 +73,7 @@ F602.py:19:5: F602 Dictionary key `a` repeated
|
|||
20 | a: 3,
|
||||
21 | a: 4,
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:20:5: F602 [*] Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -98,6 +103,7 @@ F602.py:21:5: F602 Dictionary key `a` repeated
|
|||
| ^ F602
|
||||
22 | }
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:26:5: F602 [*] Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -128,6 +134,7 @@ F602.py:27:5: F602 Dictionary key `a` repeated
|
|||
28 | a: 3,
|
||||
29 | a: 4,
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:28:5: F602 Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -138,6 +145,7 @@ F602.py:28:5: F602 Dictionary key `a` repeated
|
|||
29 | a: 4,
|
||||
30 | }
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:29:5: F602 Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -147,6 +155,7 @@ F602.py:29:5: F602 Dictionary key `a` repeated
|
|||
| ^ F602
|
||||
30 | }
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:35:5: F602 [*] Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -177,6 +186,7 @@ F602.py:37:5: F602 Dictionary key `a` repeated
|
|||
38 | "a": 3,
|
||||
39 | a: 3,
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:39:5: F602 Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -187,6 +197,7 @@ F602.py:39:5: F602 Dictionary key `a` repeated
|
|||
40 | "a": 3,
|
||||
41 | a: 4,
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:41:5: F602 Dictionary key `a` repeated
|
||||
|
|
||||
|
|
@ -196,6 +207,7 @@ F602.py:41:5: F602 Dictionary key `a` repeated
|
|||
| ^ F602
|
||||
42 | }
|
||||
|
|
||||
= help: Remove repeated key `a`
|
||||
|
||||
F602.py:44:12: F602 [*] Dictionary key `a` repeated
|
||||
|
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue