Use single rule

astral-sh · Jan 7, 2023 · eac452d · eac452d
1 parent 753b7b8
commit eac452d
Show file tree

Hide file tree

Showing 7 changed files with 126 additions and 492 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -1,96 +1,85 @@
-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;
 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<Self, Self::Error> {
-        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<DictionaryKey<'a>> {
+    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<DictionaryKey> = (&keys[i]).try_into().ok();
-        for j in i + 1..num_keys {
-            let k2: Option<DictionaryKey> = (&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<DictionaryKey, Vec<&Expr>> =
+        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<DictionaryKey> = (&keys[i]).try_into().ok();
-        for j in i + 1..num_keys {
-            let k2: Option<DictionaryKey> = (&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]]);
             }
         }
     }