From eac452d52fe4a895a8261cda878cca26b2c612b0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 7 Jan 2023 11:38:52 -0500 Subject: [PATCH] Use single rule --- resources/test/fixtures/pyflakes/F601.py | 3 +- resources/test/fixtures/pyflakes/F602.py | 3 +- src/checkers/ast.rs | 9 +- src/pyflakes/plugins/mod.rs | 2 +- src/pyflakes/plugins/repeated_keys.rs | 131 +++++---- .../ruff__pyflakes__tests__F601_F601.py.snap | 221 ++-------------- .../ruff__pyflakes__tests__F602_F602.py.snap | 249 ++---------------- 7 files changed, 126 insertions(+), 492 deletions(-) diff --git a/resources/test/fixtures/pyflakes/F601.py b/resources/test/fixtures/pyflakes/F601.py index 241c6c2d0f3a8c..3a424848523341 100644 --- a/resources/test/fixtures/pyflakes/F601.py +++ b/resources/test/fixtures/pyflakes/F601.py @@ -46,4 +46,5 @@ a: 4, } -x = {"b": 2, "a": 1, "b": 2} +x = {"a": 1, "a": 1} +x = {"a": 1, "b": 2, "a": 1} diff --git a/resources/test/fixtures/pyflakes/F602.py b/resources/test/fixtures/pyflakes/F602.py index c4d5f2cac5648c..56bf68f6a276cd 100644 --- a/resources/test/fixtures/pyflakes/F602.py +++ b/resources/test/fixtures/pyflakes/F602.py @@ -41,4 +41,5 @@ a: 4, } -x = {b: 2, a: 1, b: 2} +x = {a: 1, a: 1} +x = {a: 1, b: 2, a: 1} diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index 1d116a1fe6fc18..a34355bab96ce7 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -2379,11 +2379,10 @@ where } } ExprKind::Dict { keys, values } => { - if self.settings.enabled.contains(&CheckCode::F601) { - pyflakes::plugins::repeated_literal_keys(self, keys, values); - } - if self.settings.enabled.contains(&CheckCode::F602) { - pyflakes::plugins::repeated_variable_keys(self, keys, values); + if self.settings.enabled.contains(&CheckCode::F601) + || self.settings.enabled.contains(&CheckCode::F602) + { + pyflakes::plugins::repeated_keys(self, keys, values); } } ExprKind::Yield { .. } => { diff --git a/src/pyflakes/plugins/mod.rs b/src/pyflakes/plugins/mod.rs index 82ea2550c25776..993765daefd8f9 100644 --- a/src/pyflakes/plugins/mod.rs +++ b/src/pyflakes/plugins/mod.rs @@ -4,7 +4,7 @@ pub use if_tuple::if_tuple; pub use invalid_literal_comparisons::invalid_literal_comparison; pub use invalid_print_syntax::invalid_print_syntax; pub use raise_not_implemented::raise_not_implemented; -pub use repeated_keys::{repeated_literal_keys, repeated_variable_keys}; +pub use repeated_keys::repeated_keys; pub(crate) use strings::{ percent_format_expected_mapping, percent_format_expected_sequence, percent_format_extra_named_arguments, percent_format_missing_arguments, diff --git a/src/pyflakes/plugins/repeated_keys.rs b/src/pyflakes/plugins/repeated_keys.rs index 39a52c97fcbcf7..922339ab945ad4 100644 --- a/src/pyflakes/plugins/repeated_keys.rs +++ b/src/pyflakes/plugins/repeated_keys.rs @@ -1,4 +1,7 @@ -use rustpython_ast::{Constant, Expr, ExprKind}; +use std::hash::{BuildHasherDefault, Hash}; + +use rustc_hash::FxHashMap; +use rustpython_ast::{Expr, ExprKind}; use crate::ast::cmp; use crate::ast::helpers::unparse_expr; @@ -6,91 +9,77 @@ use crate::ast::types::Range; use crate::autofix::Fix; use crate::checkers::ast::Checker; use crate::registry::{Check, CheckCode, CheckKind}; +use crate::source_code_style::SourceCodeStyleDetector; -#[derive(Debug, PartialEq)] +#[derive(Debug, Eq, PartialEq, Hash)] enum DictionaryKey<'a> { - Constant(&'a Constant), + Constant(String), Variable(&'a str), } -impl<'a> TryFrom<&'a Expr> for DictionaryKey<'a> { - type Error = (); - - fn try_from(expr: &'a Expr) -> Result { - match &expr.node { - ExprKind::Constant { value, .. } => Ok(DictionaryKey::Constant(value)), - ExprKind::Name { id, .. } => Ok(DictionaryKey::Variable(id)), - _ => Err(()), - } +fn into_dictionary_key<'a>( + expr: &'a Expr, + stylist: &SourceCodeStyleDetector, +) -> Option> { + match &expr.node { + ExprKind::Constant { .. } => Some(DictionaryKey::Constant(unparse_expr(expr, stylist))), + ExprKind::Name { id, .. } => Some(DictionaryKey::Variable(id)), + _ => None, } } -/// F601 -pub fn repeated_literal_keys(checker: &mut Checker, keys: &[Expr], values: &[Expr]) { - let num_keys = keys.len(); - for i in 0..num_keys { - let k1: Option = (&keys[i]).try_into().ok(); - for j in i + 1..num_keys { - let k2: Option = (&keys[j]).try_into().ok(); - if let (Some(DictionaryKey::Constant(v1)), Some(DictionaryKey::Constant(v2))) = - (&k1, &k2) - { - if v1 == v2 { - let mut check = Check::new( - CheckKind::MultiValueRepeatedKeyLiteral(unparse_expr( - &keys[j], - checker.style, - )), - Range::from_located(&keys[j]), - ); - if checker.patch(&CheckCode::F601) { - if cmp::expr(&values[i], &values[j]) { - check.amend(if j < num_keys - 1 { - Fix::deletion(keys[j].location, keys[j + 1].location) +/// F601, F602 +pub fn repeated_keys(checker: &mut Checker, keys: &[Expr], values: &[Expr]) { + // Generate a map from key to (index, value). + let mut seen: FxHashMap> = + FxHashMap::with_capacity_and_hasher(keys.len(), BuildHasherDefault::default()); + + // Detect duplicate keys. + for (i, key) in keys.iter().enumerate() { + if let Some(key) = into_dictionary_key(key, checker.style) { + if let Some(seen_values) = seen.get_mut(&key) { + match key { + DictionaryKey::Constant(key) => { + if checker.settings.enabled.contains(&CheckCode::F601) { + let mut check = Check::new( + CheckKind::MultiValueRepeatedKeyLiteral(key), + Range::from_located(&keys[i]), + ); + if seen_values.iter().any(|value| cmp::expr(value, &values[i])) { + if checker.patch(&CheckCode::F601) { + check.amend(Fix::deletion( + values[i - 1].end_location.unwrap(), + values[i].end_location.unwrap(), + )); + } } else { - Fix::deletion( - values[j - 1].end_location.unwrap(), - values[j].end_location.unwrap(), - ) - }); + seen_values.push(&values[i]); + } + checker.checks.push(check); } } - checker.checks.push(check); - } - } - } - } -} - -/// F602 -pub fn repeated_variable_keys(checker: &mut Checker, keys: &[Expr], values: &[Expr]) { - let num_keys = keys.len(); - for i in 0..num_keys { - let k1: Option = (&keys[i]).try_into().ok(); - for j in i + 1..num_keys { - let k2: Option = (&keys[j]).try_into().ok(); - if let (Some(DictionaryKey::Variable(v1)), Some(DictionaryKey::Variable(v2))) = - (&k1, &k2) - { - if v1 == v2 { - let mut check = Check::new( - CheckKind::MultiValueRepeatedKeyVariable((*v2).to_string()), - Range::from_located(&keys[j]), - ); - if checker.patch(&CheckCode::F602) { - if cmp::expr(&values[i], &values[j]) { - check.amend(if j < num_keys - 1 { - Fix::deletion(keys[j].location, keys[j + 1].location) + DictionaryKey::Variable(key) => { + if checker.settings.enabled.contains(&CheckCode::F602) { + let mut check = Check::new( + CheckKind::MultiValueRepeatedKeyVariable(key.to_string()), + Range::from_located(&keys[i]), + ); + if seen_values.iter().any(|value| cmp::expr(value, &values[i])) { + if checker.patch(&CheckCode::F602) { + check.amend(Fix::deletion( + values[i - 1].end_location.unwrap(), + values[i].end_location.unwrap(), + )); + } } else { - Fix::deletion( - values[j - 1].end_location.unwrap(), - values[j].end_location.unwrap(), - ) - }); + seen_values.push(&values[i]); + } + checker.checks.push(check); } } - checker.checks.push(check); } + } else { + seen.insert(key, vec![&values[i]]); } } } diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F601_F601.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F601_F601.py.snap index 5594514f141446..a0cef90719e477 100644 --- a/src/pyflakes/snapshots/ruff__pyflakes__tests__F601_F601.py.snap +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F601_F601.py.snap @@ -52,36 +52,6 @@ expression: checks column: 7 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 17 - column: 4 - end_location: - row: 17 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 18 - column: 4 - end_location: - row: 18 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 18 - column: 4 - end_location: - row: 18 - column: 7 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyLiteral: "\"a\"" location: @@ -119,36 +89,6 @@ expression: checks column: 7 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 24 - column: 4 - end_location: - row: 24 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 25 - column: 4 - end_location: - row: 25 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 25 - column: 4 - end_location: - row: 25 - column: 7 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyLiteral: "\"a\"" location: @@ -160,41 +100,11 @@ expression: checks fix: content: "" location: - row: 25 - column: 4 + row: 24 + column: 10 end_location: - row: 26 - column: 4 - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 26 - column: 4 - end_location: - row: 26 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 26 - column: 4 - end_location: - row: 26 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 26 - column: 4 - end_location: - row: 26 - column: 7 - fix: ~ + row: 25 + column: 10 parent: ~ - kind: MultiValueRepeatedKeyLiteral: "\"a\"" @@ -217,21 +127,11 @@ expression: checks fix: content: "" location: - row: 31 - column: 4 + row: 30 + column: 10 end_location: - row: 32 - column: 4 - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 32 - column: 4 - end_location: - row: 32 - column: 7 - fix: ~ + row: 31 + column: 10 parent: ~ - kind: MultiValueRepeatedKeyLiteral: "\"a\"" @@ -253,56 +153,6 @@ expression: checks column: 7 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 33 - column: 4 - end_location: - row: 33 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 33 - column: 4 - end_location: - row: 33 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 34 - column: 4 - end_location: - row: 34 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 34 - column: 4 - end_location: - row: 34 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 34 - column: 4 - end_location: - row: 34 - column: 7 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyLiteral: "\"a\"" location: @@ -333,26 +183,6 @@ expression: checks column: 7 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 43 - column: 4 - end_location: - row: 43 - column: 7 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyLiteral: "\"a\"" - location: - row: 45 - column: 4 - end_location: - row: 45 - column: 7 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyLiteral: "\"a\"" location: @@ -361,40 +191,47 @@ expression: checks end_location: row: 45 column: 7 - fix: ~ + fix: + content: "" + location: + row: 44 + column: 8 + end_location: + row: 45 + column: 10 parent: ~ - kind: MultiValueRepeatedKeyLiteral: "\"a\"" location: - row: 45 - column: 4 + row: 49 + column: 13 end_location: - row: 45 - column: 7 + row: 49 + column: 16 fix: content: "" location: - row: 45 - column: 4 + row: 49 + column: 11 end_location: - row: 46 - column: 4 + row: 49 + column: 19 parent: ~ - kind: - MultiValueRepeatedKeyLiteral: "\"b\"" + MultiValueRepeatedKeyLiteral: "\"a\"" location: - row: 49 + row: 50 column: 21 end_location: - row: 49 + row: 50 column: 24 fix: content: "" location: - row: 49 + row: 50 column: 19 end_location: - row: 49 + row: 50 column: 27 parent: ~ diff --git a/src/pyflakes/snapshots/ruff__pyflakes__tests__F602_F602.py.snap b/src/pyflakes/snapshots/ruff__pyflakes__tests__F602_F602.py.snap index 51316d75728d56..bdb7ea4400e3f3 100644 --- a/src/pyflakes/snapshots/ruff__pyflakes__tests__F602_F602.py.snap +++ b/src/pyflakes/snapshots/ruff__pyflakes__tests__F602_F602.py.snap @@ -32,36 +32,6 @@ expression: checks column: 5 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 12 - column: 4 - end_location: - row: 12 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 13 - column: 4 - end_location: - row: 13 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 13 - column: 4 - end_location: - row: 13 - column: 5 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyVariable: a location: @@ -99,36 +69,6 @@ expression: checks column: 5 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 19 - column: 4 - end_location: - row: 19 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 20 - column: 4 - end_location: - row: 20 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 20 - column: 4 - end_location: - row: 20 - column: 5 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyVariable: a location: @@ -140,41 +80,11 @@ expression: checks fix: content: "" location: - row: 20 - column: 4 + row: 19 + column: 8 end_location: - row: 21 - column: 4 - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 21 - column: 4 - end_location: - row: 21 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 21 - column: 4 - end_location: - row: 21 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 21 - column: 4 - end_location: - row: 21 - column: 5 - fix: ~ + row: 20 + column: 8 parent: ~ - kind: MultiValueRepeatedKeyVariable: a @@ -197,21 +107,11 @@ expression: checks fix: content: "" location: - row: 26 - column: 4 + row: 25 + column: 8 end_location: - row: 27 - column: 4 - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 27 - column: 4 - end_location: - row: 27 - column: 5 - fix: ~ + row: 26 + column: 8 parent: ~ - kind: MultiValueRepeatedKeyVariable: a @@ -233,56 +133,6 @@ expression: checks column: 5 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 28 - column: 4 - end_location: - row: 28 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 28 - column: 4 - end_location: - row: 28 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 29 - column: 4 - end_location: - row: 29 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 29 - column: 4 - end_location: - row: 29 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 29 - column: 4 - end_location: - row: 29 - column: 5 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyVariable: a location: @@ -304,21 +154,11 @@ expression: checks fix: content: "" location: - row: 35 - column: 4 + row: 34 + column: 10 end_location: - row: 36 - column: 4 - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 37 - column: 4 - end_location: - row: 37 - column: 5 - fix: ~ + row: 35 + column: 8 parent: ~ - kind: MultiValueRepeatedKeyVariable: a @@ -340,26 +180,6 @@ expression: checks column: 5 fix: ~ parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 39 - column: 4 - end_location: - row: 39 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 39 - column: 4 - end_location: - row: 39 - column: 5 - fix: ~ - parent: ~ - kind: MultiValueRepeatedKeyVariable: a location: @@ -373,48 +193,35 @@ expression: checks - kind: MultiValueRepeatedKeyVariable: a location: - row: 41 - column: 4 - end_location: - row: 41 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: a - location: - row: 41 - column: 4 + row: 44 + column: 11 end_location: - row: 41 - column: 5 - fix: ~ + row: 44 + column: 12 + fix: + content: "" + location: + row: 44 + column: 9 + end_location: + row: 44 + column: 15 parent: ~ - kind: MultiValueRepeatedKeyVariable: a location: - row: 41 - column: 4 - end_location: - row: 41 - column: 5 - fix: ~ - parent: ~ -- kind: - MultiValueRepeatedKeyVariable: b - location: - row: 44 + row: 45 column: 17 end_location: - row: 44 + row: 45 column: 18 fix: content: "" location: - row: 44 + row: 45 column: 15 end_location: - row: 44 + row: 45 column: 21 parent: ~