From 5deb63a05f2163d7e1a7fdc1e66ff31d6d8cfaf9 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 7 Sep 2022 12:57:50 -0400 Subject: [PATCH] Implement F601 and F602 (#122) --- README.md | 2 + examples/generate_rules_table.rs | 2 + resources/test/fixtures/F601.py | 12 ++++++ resources/test/fixtures/F602.py | 7 +++ resources/test/fixtures/pyproject.toml | 2 + src/check_ast.rs | 56 ++++++++++++++++++++++++ src/checks.rs | 22 ++++++++++ src/linter.rs | 60 ++++++++++++++++++++++++++ src/pyproject.rs | 2 + src/settings.rs | 2 + 10 files changed, 167 insertions(+) create mode 100644 resources/test/fixtures/F601.py create mode 100644 resources/test/fixtures/F602.py diff --git a/README.md b/README.md index 4fd08fd7ea..44ae5a3c5d 100644 --- a/README.md +++ b/README.md @@ -156,6 +156,8 @@ Beyond rule-set parity, ruff suffers from the following limitations vis-à-vis F | F401 | UnusedImport | `...` imported but unused | | F403 | ImportStarUsage | Unable to detect undefined names | | F541 | FStringMissingPlaceholders | f-string without any placeholders | +| F601 | MultiValueRepeatedKeyLiteral | Dictionary key literal repeated | +| F602 | MultiValueRepeatedKeyVariable | Dictionary key `...` repeated | | F631 | AssertTuple | Assert test is a non-empty tuple, which is always `True` | | F634 | IfTuple | If test is a tuple, which is always `True` | | F704 | YieldOutsideFunction | a `yield` or `yield from` statement outside of a function/method | diff --git a/examples/generate_rules_table.rs b/examples/generate_rules_table.rs index 6cd4a54bb1..19422a8789 100644 --- a/examples/generate_rules_table.rs +++ b/examples/generate_rules_table.rs @@ -13,6 +13,8 @@ fn main() { CheckKind::ImportStarUsage, CheckKind::LineTooLong, CheckKind::ModuleImportNotAtTopOfFile, + CheckKind::MultiValueRepeatedKeyLiteral, + CheckKind::MultiValueRepeatedKeyVariable("...".to_string()), CheckKind::NoAssertEquals, CheckKind::NoneComparison(RejectedCmpop::Eq), CheckKind::NotInTest, diff --git a/resources/test/fixtures/F601.py b/resources/test/fixtures/F601.py new file mode 100644 index 0000000000..fa43186d27 --- /dev/null +++ b/resources/test/fixtures/F601.py @@ -0,0 +1,12 @@ +x = { + "a": 1, + "a": 2, + "b": 3, + ("a", "b"): 3, + ("a", "b"): 4, + 1.0: 2, + 1: 0, + 1: 3, + b"123": 1, + b"123": 4, +} diff --git a/resources/test/fixtures/F602.py b/resources/test/fixtures/F602.py new file mode 100644 index 0000000000..f904411da3 --- /dev/null +++ b/resources/test/fixtures/F602.py @@ -0,0 +1,7 @@ +a = 1 +b = 2 +x = { + a: 1, + a: 2, + b: 3, +} diff --git a/resources/test/fixtures/pyproject.toml b/resources/test/fixtures/pyproject.toml index b2ab76ee56..c5f97c784d 100644 --- a/resources/test/fixtures/pyproject.toml +++ b/resources/test/fixtures/pyproject.toml @@ -13,6 +13,8 @@ select = [ "F401", "F403", "F541", + "F601", + "F602", "F631", "F634", "F704", diff --git a/src/check_ast.rs b/src/check_ast.rs index b97e295c45..2eb5e28550 100644 --- a/src/check_ast.rs +++ b/src/check_ast.rs @@ -56,6 +56,20 @@ impl Checker<'_> { } } +#[derive(Debug, PartialEq)] +enum DictionaryKey<'a> { + Constant(&'a Constant), + Variable(&'a String), +} + +fn convert_to_value(expr: &Expr) -> Option { + match &expr.node { + ExprKind::Constant { value, .. } => Some(DictionaryKey::Constant(value)), + ExprKind::Name { id, .. } => Some(DictionaryKey::Variable(id)), + _ => None, + } +} + impl Visitor for Checker<'_> { fn visit_stmt(&mut self, stmt: &Stmt) { match &stmt.node { @@ -503,6 +517,48 @@ impl Visitor for Checker<'_> { } } } + + ExprKind::Dict { keys, .. } => { + if self.settings.select.contains(&CheckCode::F601) + || self.settings.select.contains(&CheckCode::F602) + { + let num_keys = keys.len(); + for i in 0..num_keys { + let k1 = &keys[i]; + let v1 = convert_to_value(k1); + for k2 in keys.iter().take(num_keys).skip(i + 1) { + let v2 = convert_to_value(k2); + match (&v1, &v2) { + ( + Some(DictionaryKey::Constant(v1)), + Some(DictionaryKey::Constant(v2)), + ) => { + if self.settings.select.contains(&CheckCode::F601) && v1 == v2 { + self.checks.push(Check::new( + CheckKind::MultiValueRepeatedKeyLiteral, + k2.location, + )) + } + } + ( + Some(DictionaryKey::Variable(v1)), + Some(DictionaryKey::Variable(v2)), + ) => { + if self.settings.select.contains(&CheckCode::F602) && v1 == v2 { + self.checks.push(Check::new( + CheckKind::MultiValueRepeatedKeyVariable( + v2.to_string(), + ), + k2.location, + )) + } + } + _ => {} + } + } + } + } + } ExprKind::GeneratorExp { .. } | ExprKind::ListComp { .. } | ExprKind::DictComp { .. } diff --git a/src/checks.rs b/src/checks.rs index a6a2e36fa9..85d9c1ca01 100644 --- a/src/checks.rs +++ b/src/checks.rs @@ -19,6 +19,8 @@ pub enum CheckCode { F401, F403, F541, + F601, + F602, F631, F634, F704, @@ -50,6 +52,8 @@ impl FromStr for CheckCode { "F401" => Ok(CheckCode::F401), "F403" => Ok(CheckCode::F403), "F541" => Ok(CheckCode::F541), + "F601" => Ok(CheckCode::F601), + "F602" => Ok(CheckCode::F602), "F631" => Ok(CheckCode::F631), "F634" => Ok(CheckCode::F634), "F704" => Ok(CheckCode::F704), @@ -82,6 +86,8 @@ impl CheckCode { CheckCode::F401 => "F401", CheckCode::F403 => "F403", CheckCode::F541 => "F541", + CheckCode::F601 => "F601", + CheckCode::F602 => "F602", CheckCode::F631 => "F631", CheckCode::F634 => "F634", CheckCode::F704 => "F704", @@ -112,6 +118,8 @@ impl CheckCode { CheckCode::F401 => &LintSource::AST, CheckCode::F403 => &LintSource::AST, CheckCode::F541 => &LintSource::AST, + CheckCode::F601 => &LintSource::AST, + CheckCode::F602 => &LintSource::AST, CheckCode::F631 => &LintSource::AST, CheckCode::F634 => &LintSource::AST, CheckCode::F704 => &LintSource::AST, @@ -154,6 +162,8 @@ pub enum CheckKind { ImportStarUsage, LineTooLong, ModuleImportNotAtTopOfFile, + MultiValueRepeatedKeyLiteral, + MultiValueRepeatedKeyVariable(String), NoAssertEquals, NoneComparison(RejectedCmpop), NotInTest, @@ -184,6 +194,8 @@ impl CheckKind { CheckKind::LineTooLong => "LineTooLong", CheckKind::DoNotAssignLambda => "DoNotAssignLambda", CheckKind::ModuleImportNotAtTopOfFile => "ModuleImportNotAtTopOfFile", + CheckKind::MultiValueRepeatedKeyLiteral => "MultiValueRepeatedKeyLiteral", + CheckKind::MultiValueRepeatedKeyVariable(_) => "MultiValueRepeatedKeyVariable", CheckKind::NoAssertEquals => "NoAssertEquals", CheckKind::NoneComparison(_) => "NoneComparison", CheckKind::NotInTest => "NotInTest", @@ -214,6 +226,8 @@ impl CheckKind { CheckKind::LineTooLong => &CheckCode::E501, CheckKind::DoNotAssignLambda => &CheckCode::E731, CheckKind::ModuleImportNotAtTopOfFile => &CheckCode::E402, + CheckKind::MultiValueRepeatedKeyLiteral => &CheckCode::F601, + CheckKind::MultiValueRepeatedKeyVariable(_) => &CheckCode::F602, CheckKind::NoAssertEquals => &CheckCode::R002, CheckKind::NoneComparison(_) => &CheckCode::E711, CheckKind::NotInTest => &CheckCode::E713, @@ -258,6 +272,12 @@ impl CheckKind { CheckKind::ModuleImportNotAtTopOfFile => { "Module level import not at top of file".to_string() } + CheckKind::MultiValueRepeatedKeyLiteral => { + "Dictionary key literal repeated".to_string() + } + CheckKind::MultiValueRepeatedKeyVariable(name) => { + format!("Dictionary key `{name}` repeated") + } CheckKind::NoAssertEquals => { "`assertEquals` is deprecated, use `assertEqual` instead".to_string() } @@ -328,6 +348,8 @@ impl CheckKind { CheckKind::DoNotAssignLambda => false, CheckKind::LineTooLong => false, CheckKind::ModuleImportNotAtTopOfFile => false, + CheckKind::MultiValueRepeatedKeyLiteral => false, + CheckKind::MultiValueRepeatedKeyVariable(_) => false, CheckKind::NoAssertEquals => true, CheckKind::NotInTest => false, CheckKind::NotIsTest => false, diff --git a/src/linter.rs b/src/linter.rs index 1ef914ffca..723617c3ff 100644 --- a/src/linter.rs +++ b/src/linter.rs @@ -391,6 +391,66 @@ mod tests { Ok(()) } + #[test] + fn f601() -> Result<()> { + let actual = check_path( + Path::new("./resources/test/fixtures/F601.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + select: BTreeSet::from([CheckCode::F601]), + }, + &autofix::Mode::Generate, + )?; + let expected = vec![ + Check { + kind: CheckKind::MultiValueRepeatedKeyLiteral, + location: Location::new(3, 6), + fix: None, + }, + Check { + kind: CheckKind::MultiValueRepeatedKeyLiteral, + location: Location::new(9, 5), + fix: None, + }, + Check { + kind: CheckKind::MultiValueRepeatedKeyLiteral, + location: Location::new(11, 7), + fix: None, + }, + ]; + assert_eq!(actual.len(), expected.len()); + for i in 0..actual.len() { + assert_eq!(actual[i], expected[i]); + } + + Ok(()) + } + + #[test] + fn f602() -> Result<()> { + let actual = check_path( + Path::new("./resources/test/fixtures/F602.py"), + &settings::Settings { + line_length: 88, + exclude: vec![], + select: BTreeSet::from([CheckCode::F602]), + }, + &autofix::Mode::Generate, + )?; + let expected = vec![Check { + kind: CheckKind::MultiValueRepeatedKeyVariable("a".to_string()), + location: Location::new(5, 5), + fix: None, + }]; + assert_eq!(actual.len(), expected.len()); + for i in 0..actual.len() { + assert_eq!(actual[i], expected[i]); + } + + Ok(()) + } + #[test] fn f631() -> Result<()> { let actual = check_path( diff --git a/src/pyproject.rs b/src/pyproject.rs index 4af4d0b77b..66fd1dab90 100644 --- a/src/pyproject.rs +++ b/src/pyproject.rs @@ -248,6 +248,8 @@ other-attribute = 1 CheckCode::F401, CheckCode::F403, CheckCode::F541, + CheckCode::F601, + CheckCode::F602, CheckCode::F631, CheckCode::F634, CheckCode::F704, diff --git a/src/settings.rs b/src/settings.rs index 6597178128..ed0352f8f1 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -55,6 +55,8 @@ impl Settings { CheckCode::F401, CheckCode::F403, CheckCode::F541, + CheckCode::F601, + CheckCode::F602, CheckCode::F631, CheckCode::F634, CheckCode::F704,