Skip to content

Commit

Permalink
Refactor repeated_keys() to use ComparableExpr (#5696)
Browse files Browse the repository at this point in the history 
## 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: #5691
  • Loading branch information
@qdegraaf
qdegraaf committed Jul 12, 2023
1 parent 5dd9e56 commit 7566ca8
Show file tree
Hide file tree
Showing 4 changed files with 112 additions and 91 deletions.
5 changes: 5 additions & 0 deletions crates/ruff/resources/test/fixtures/pyflakes/F601.py
View file
Expand Up @@ -48,3 +48,8 @@

x = {"a": 1, "a": 1}
x = {"a": 1, "b": 2, "a": 1}

x = {
('a', 'b'): 'asdf',
('a', 'b'): 'qwer',
}
145 changes: 54 additions & 91 deletions crates/ruff/src/rules/pyflakes/rules/repeated_keys.rs
View file
@@ -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};
Expand Down Expand Up @@ -43,28 +43,20 @@ use crate::registry::{AsRule, Rule};
#[violation]
pub struct MultiValueRepeatedKeyLiteral {
name: String,
repeated_value: bool,
}

impl Violation for MultiValueRepeatedKeyLiteral {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[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}`"))
}
}

Expand Down Expand Up @@ -100,113 +92,84 @@ impl Violation for MultiValueRepeatedKeyLiteral {
#[violation]
pub struct MultiValueRepeatedKeyVariable {
name: String,
repeated_value: bool,
}

impl Violation for MultiValueRepeatedKeyVariable {
const AUTOFIX: AutofixKind = AutofixKind::Sometimes;

#[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.
for (i, key) in keys.iter().enumerate() {
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()]));
}
_ => {}
}
}
}
41 changes: 41 additions & 0 deletions crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F601_F601.py.snap
View file
Expand Up @@ -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
|
Expand All @@ -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
|
Expand All @@ -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
|
Expand All @@ -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
|
Expand All @@ -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
|
Expand Down Expand Up @@ -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
|
Expand All @@ -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
|
Expand Down Expand Up @@ -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
|
Expand Down Expand Up @@ -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
|
Expand All @@ -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
|
Expand All @@ -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
|
Expand All @@ -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
|
Expand All @@ -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
|
Expand Down Expand Up @@ -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"`

Expand All @@ -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')`


0 comments on commit 7566ca8

Please sign in to comment.