From 7566ca8ff7caa28ea44f1053fd1f2963fafcefcb Mon Sep 17 00:00:00 2001 From: qdegraaf <34540841+qdegraaf@users.noreply.github.com> Date: Wed, 12 Jul 2023 05:46:53 +0200 Subject: [PATCH] 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 --- .../resources/test/fixtures/pyflakes/F601.py | 5 + .../src/rules/pyflakes/rules/repeated_keys.rs | 145 +++++++----------- ..._rules__pyflakes__tests__F601_F601.py.snap | 41 +++++ ..._rules__pyflakes__tests__F602_F602.py.snap | 12 ++ 4 files changed, 112 insertions(+), 91 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pyflakes/F601.py b/crates/ruff/resources/test/fixtures/pyflakes/F601.py index 3a42484852334..e8e9e6a89f2da 100644 --- a/crates/ruff/resources/test/fixtures/pyflakes/F601.py +++ b/crates/ruff/resources/test/fixtures/pyflakes/F601.py @@ -48,3 +48,8 @@ x = {"a": 1, "a": 1} x = {"a": 1, "b": 2, "a": 1} + +x = { + ('a', 'b'): 'asdf', + ('a', 'b'): 'qwer', +} diff --git a/crates/ruff/src/rules/pyflakes/rules/repeated_keys.rs b/crates/ruff/src/rules/pyflakes/rules/repeated_keys.rs index 13c4ceaf9a7a3..ec4412042cfb2 100644 --- a/crates/ruff/src/rules/pyflakes/rules/repeated_keys.rs +++ b/crates/ruff/src/rules/pyflakes/rules/repeated_keys.rs @@ -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 { - 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 { - 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 { - 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], values: &[Expr]) { // Generate a map from key to (index, value). - let mut seen: FxHashMap> = + let mut seen: FxHashMap> = 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], 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); + + 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(), + ))); } } - 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); + checker.diagnostics.push(diagnostic); + } + } + 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); } - } else { - seen.insert(dict_key, FxHashSet::from_iter([(&values[i]).into()])); } + _ => {} } } } diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F601_F601.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F601_F601.py.snap index f6fa5fdd65e8d..1a5b94a092caf 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F601_F601.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F601_F601.py.snap @@ -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')` diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F602_F602.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F602_F602.py.snap index cdbc8c32907a9..c29e85b114409 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F602_F602.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F602_F602.py.snap @@ -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 |