From 7eab24dd9613dfc840afa2d326c722d5c17442c5 Mon Sep 17 00:00:00 2001 From: Leonardo Esparis Date: Mon, 16 Jan 2023 21:29:12 +0100 Subject: [PATCH 1/3] add prefer_unique_enums rule --- README.md | 1 + resources/test/fixtures/flake8_pie/PIE796.py | 54 +++++++++++++ ruff.schema.json | 1 + src/checkers/ast.rs | 4 + src/registry.rs | 1 + src/rules/flake8_pie/mod.rs | 1 + src/rules/flake8_pie/rules.rs | 47 ++++++++++++ ...__flake8_pie__tests__PIE796_PIE796.py.snap | 75 +++++++++++++++++++ src/violations.rs | 13 ++++ 9 files changed, 197 insertions(+) create mode 100644 resources/test/fixtures/flake8_pie/PIE796.py create mode 100644 src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap diff --git a/README.md b/README.md index 1d1d8c4ec256a..2a76db2b7bad8 100644 --- a/README.md +++ b/README.md @@ -1141,6 +1141,7 @@ For more, see [flake8-pie](https://pypi.org/project/flake8-pie/0.16.0/) on PyPI. | ---- | ---- | ------- | --- | | PIE790 | NoUnnecessaryPass | Unnecessary `pass` statement | 🛠 | | PIE794 | DupeClassFieldDefinitions | Class field `...` is defined multiple times | 🛠 | +| PIE796 | PreferUniqueEnums | Consider using removing dupe values. | | | PIE807 | PreferListBuiltin | Prefer `list()` over useless lambda | 🛠 | ### flake8-commas (COM) diff --git a/resources/test/fixtures/flake8_pie/PIE796.py b/resources/test/fixtures/flake8_pie/PIE796.py new file mode 100644 index 0000000000000..e6c8cb0399e32 --- /dev/null +++ b/resources/test/fixtures/flake8_pie/PIE796.py @@ -0,0 +1,54 @@ +class FakeEnum(enum.Enum): + A = "A" + B = "B" + C = "B" # PIE796 + + +class FakeEnum2(Enum): + A = 1 + B = 2 + C = 2 # PIE796 + + +class FakeEnum3(str, Enum): + A = "1" + B = "2" + C = "2" # PIE796 + +class FakeEnum4(Enum): + A = 1.0 + B = 2.5 + C = 2.5 # PIE796 + + +class FakeEnum5(Enum): + A = 1.0 + B = True + C = False + D = False # PIE796 + + +class FakeEnum6(Enum): + A = 1 + B = 2 + C = None + D = None # PIE796 + + +@enum.unique +class FakeEnum7(enum.Enum): + A = "A" + B = "B" + C = "C" + +@unique +class FakeEnum8(Enum): + A = 1 + B = 2 + C = 2 # PIE796 + +class FakeEnum9(enum.Enum): + A = "A" + B = "B" + C = "C" + diff --git a/ruff.schema.json b/ruff.schema.json index 65425735ac964..821e604156efb 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1431,6 +1431,7 @@ "PIE79", "PIE790", "PIE794", + "PIE796", "PIE8", "PIE80", "PIE807", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index a76d8912c9efb..21c7d898c33c4 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -701,6 +701,10 @@ where flake8_pie::rules::dupe_class_field_definitions(self, stmt, body); } + if self.settings.enabled.contains(&RuleCode::PIE796) { + flake8_pie::rules::prefer_unique_enums(self, stmt, body); + } + self.check_builtin_shadowing(name, stmt, false); for expr in bases { diff --git a/src/registry.rs b/src/registry.rs index 8c781dad40f16..96d9793fb2ab5 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -410,6 +410,7 @@ ruff_macros::define_rule_mapping!( // flake8-pie PIE790 => violations::NoUnnecessaryPass, PIE794 => violations::DupeClassFieldDefinitions, + PIE796 => violations::PreferUniqueEnums, PIE807 => violations::PreferListBuiltin, // flake8-commas COM812 => violations::TrailingCommaMissing, diff --git a/src/rules/flake8_pie/mod.rs b/src/rules/flake8_pie/mod.rs index 754e274a88d3e..ef4cf551c04a3 100644 --- a/src/rules/flake8_pie/mod.rs +++ b/src/rules/flake8_pie/mod.rs @@ -14,6 +14,7 @@ mod tests { #[test_case(RuleCode::PIE790, Path::new("PIE790.py"); "PIE790")] #[test_case(RuleCode::PIE794, Path::new("PIE794.py"); "PIE794")] + #[test_case(RuleCode::PIE796, Path::new("PIE796.py"); "PIE796")] #[test_case(RuleCode::PIE807, Path::new("PIE807.py"); "PIE807")] fn rules(rule_code: RuleCode, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); diff --git a/src/rules/flake8_pie/rules.rs b/src/rules/flake8_pie/rules.rs index 072b8353c0894..2fe5cfaefde1d 100644 --- a/src/rules/flake8_pie/rules.rs +++ b/src/rules/flake8_pie/rules.rs @@ -106,6 +106,53 @@ pub fn dupe_class_field_definitions<'a, 'b>( } } +// PIE796 +pub fn prefer_unique_enums<'a, 'b>(checker: &mut Checker<'a>, parent: &'b Stmt, body: &'b [Stmt]) +where + 'b: 'a, +{ + let StmtKind::ClassDef { bases, .. } = &parent.node else { + return; + }; + + let is_enum = bases.iter().any(|stmt| match &stmt.node { + ExprKind::Name { id, .. } => id == "Enum", + ExprKind::Attribute { value, attr, .. } => { + attr.ends_with("Enum") + && match &value.node { + ExprKind::Name { id, .. } => id == "enum", + _ => false, + } + } + _ => false, + }); + + if !is_enum { + return; + } + + let mut seen_targets: FxHashSet = FxHashSet::default(); + for stmt in body { + let target = match &stmt.node { + StmtKind::Assign { value, .. } => match &value.node { + ExprKind::Constant { value, .. } => value.to_string(), + _ => { + continue; + } + }, + _ => { + continue; + } + }; + + if !seen_targets.insert(target) { + let diagnostic = + Diagnostic::new(violations::PreferUniqueEnums, Range::from_located(stmt)); + checker.diagnostics.push(diagnostic); + } + } +} + /// PIE807 pub fn prefer_list_builtin(checker: &mut Checker, expr: &Expr) { let ExprKind::Lambda { args, body } = &expr.node else { diff --git a/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap b/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap new file mode 100644 index 0000000000000..d67b67dfbc3fb --- /dev/null +++ b/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap @@ -0,0 +1,75 @@ +--- +source: src/rules/flake8_pie/mod.rs +expression: diagnostics +--- +- kind: + PreferUniqueEnums: ~ + location: + row: 4 + column: 4 + end_location: + row: 4 + column: 11 + fix: ~ + parent: ~ +- kind: + PreferUniqueEnums: ~ + location: + row: 10 + column: 4 + end_location: + row: 10 + column: 9 + fix: ~ + parent: ~ +- kind: + PreferUniqueEnums: ~ + location: + row: 16 + column: 4 + end_location: + row: 16 + column: 11 + fix: ~ + parent: ~ +- kind: + PreferUniqueEnums: ~ + location: + row: 21 + column: 4 + end_location: + row: 21 + column: 11 + fix: ~ + parent: ~ +- kind: + PreferUniqueEnums: ~ + location: + row: 28 + column: 4 + end_location: + row: 28 + column: 13 + fix: ~ + parent: ~ +- kind: + PreferUniqueEnums: ~ + location: + row: 35 + column: 4 + end_location: + row: 35 + column: 12 + fix: ~ + parent: ~ +- kind: + PreferUniqueEnums: ~ + location: + row: 48 + column: 4 + end_location: + row: 48 + column: 9 + fix: ~ + parent: ~ + diff --git a/src/violations.rs b/src/violations.rs index 6b9b609905b79..e067b9245f739 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -5970,6 +5970,19 @@ impl AlwaysAutofixableViolation for DupeClassFieldDefinitions { } } +define_violation!( + pub struct PreferUniqueEnums; +); +impl Violation for PreferUniqueEnums { + fn message(&self) -> String { + "Consider using removing dupe values.".to_string() + } + + fn placeholder() -> Self { + PreferUniqueEnums + } +} + define_violation!( pub struct PreferListBuiltin; ); From 9e5b663e084177bcb8e6778501c17f33a189d57c Mon Sep 17 00:00:00 2001 From: Leonardo Esparis Date: Tue, 17 Jan 2023 00:22:48 +0100 Subject: [PATCH 2/3] apply changes --- README.md | 2 +- src/rules/flake8_pie/rules.rs | 24 ++++++++----------- ...__flake8_pie__tests__PIE796_PIE796.py.snap | 14 +++++------ src/violations.rs | 7 +++--- 4 files changed, 22 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index 2a76db2b7bad8..fa16b9a8b9532 100644 --- a/README.md +++ b/README.md @@ -1141,7 +1141,7 @@ For more, see [flake8-pie](https://pypi.org/project/flake8-pie/0.16.0/) on PyPI. | ---- | ---- | ------- | --- | | PIE790 | NoUnnecessaryPass | Unnecessary `pass` statement | 🛠 | | PIE794 | DupeClassFieldDefinitions | Class field `...` is defined multiple times | 🛠 | -| PIE796 | PreferUniqueEnums | Consider using removing dupe values. | | +| PIE796 | PreferUniqueEnums | Enum contains duplicate value: ... | | | PIE807 | PreferListBuiltin | Prefer `list()` over useless lambda | 🛠 | ### flake8-commas (COM) diff --git a/src/rules/flake8_pie/rules.rs b/src/rules/flake8_pie/rules.rs index 2fe5cfaefde1d..7f48f21a67707 100644 --- a/src/rules/flake8_pie/rules.rs +++ b/src/rules/flake8_pie/rules.rs @@ -2,6 +2,8 @@ use log::error; use rustc_hash::FxHashSet; use rustpython_ast::{Constant, Expr, ExprKind, Stmt, StmtKind}; +use crate::ast::comparable::ComparableExpr; +use crate::ast::helpers::unparse_expr; use crate::ast::types::{Range, RefEquality}; use crate::autofix::helpers::delete_stmt; use crate::checkers::ast::Checker; @@ -131,23 +133,17 @@ where return; } - let mut seen_targets: FxHashSet = FxHashSet::default(); + let mut seen_targets: FxHashSet = FxHashSet::default(); for stmt in body { - let target = match &stmt.node { - StmtKind::Assign { value, .. } => match &value.node { - ExprKind::Constant { value, .. } => value.to_string(), - _ => { - continue; - } - }, - _ => { - continue; - } + let StmtKind::Assign { value, .. } = &stmt.node else { + continue; }; - if !seen_targets.insert(target) { - let diagnostic = - Diagnostic::new(violations::PreferUniqueEnums, Range::from_located(stmt)); + if !seen_targets.insert(ComparableExpr::from(value)) { + let diagnostic = Diagnostic::new( + violations::PreferUniqueEnums(unparse_expr(value, checker.stylist)), + Range::from_located(stmt), + ); checker.diagnostics.push(diagnostic); } } diff --git a/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap b/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap index d67b67dfbc3fb..0079555160e94 100644 --- a/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap +++ b/src/rules/flake8_pie/snapshots/ruff__rules__flake8_pie__tests__PIE796_PIE796.py.snap @@ -3,7 +3,7 @@ source: src/rules/flake8_pie/mod.rs expression: diagnostics --- - kind: - PreferUniqueEnums: ~ + PreferUniqueEnums: "\"B\"" location: row: 4 column: 4 @@ -13,7 +13,7 @@ expression: diagnostics fix: ~ parent: ~ - kind: - PreferUniqueEnums: ~ + PreferUniqueEnums: "2" location: row: 10 column: 4 @@ -23,7 +23,7 @@ expression: diagnostics fix: ~ parent: ~ - kind: - PreferUniqueEnums: ~ + PreferUniqueEnums: "\"2\"" location: row: 16 column: 4 @@ -33,7 +33,7 @@ expression: diagnostics fix: ~ parent: ~ - kind: - PreferUniqueEnums: ~ + PreferUniqueEnums: "2.5" location: row: 21 column: 4 @@ -43,7 +43,7 @@ expression: diagnostics fix: ~ parent: ~ - kind: - PreferUniqueEnums: ~ + PreferUniqueEnums: "False" location: row: 28 column: 4 @@ -53,7 +53,7 @@ expression: diagnostics fix: ~ parent: ~ - kind: - PreferUniqueEnums: ~ + PreferUniqueEnums: None location: row: 35 column: 4 @@ -63,7 +63,7 @@ expression: diagnostics fix: ~ parent: ~ - kind: - PreferUniqueEnums: ~ + PreferUniqueEnums: "2" location: row: 48 column: 4 diff --git a/src/violations.rs b/src/violations.rs index e067b9245f739..bc009a2777dbf 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -5971,15 +5971,16 @@ impl AlwaysAutofixableViolation for DupeClassFieldDefinitions { } define_violation!( - pub struct PreferUniqueEnums; + pub struct PreferUniqueEnums(pub String); ); impl Violation for PreferUniqueEnums { fn message(&self) -> String { - "Consider using removing dupe values.".to_string() + let PreferUniqueEnums(value) = self; + format!("Enum contains duplicate value: {value}") } fn placeholder() -> Self { - PreferUniqueEnums + PreferUniqueEnums("...".to_string()) } } From afcf170ed7467bf92acbc0a8a0643611f72e13cf Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 16 Jan 2023 19:27:20 -0500 Subject: [PATCH 3/3] Minor tweaks --- README.md | 2 +- src/rules/flake8_pie/rules.rs | 24 +++++++++--------------- src/violations.rs | 12 ++++++++---- 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index d161d58ac1dc6..fc9aa1bd78099 100644 --- a/README.md +++ b/README.md @@ -1141,7 +1141,7 @@ For more, see [flake8-pie](https://pypi.org/project/flake8-pie/0.16.0/) on PyPI. | ---- | ---- | ------- | --- | | PIE790 | NoUnnecessaryPass | Unnecessary `pass` statement | 🛠 | | PIE794 | DupeClassFieldDefinitions | Class field `...` is defined multiple times | 🛠 | -| PIE796 | PreferUniqueEnums | Enum contains duplicate value: ... | | +| PIE796 | PreferUniqueEnums | Enum contains duplicate value: `...` | | | PIE807 | PreferListBuiltin | Prefer `list()` over useless lambda | 🛠 | ### flake8-commas (COM) diff --git a/src/rules/flake8_pie/rules.rs b/src/rules/flake8_pie/rules.rs index 7f48f21a67707..cbe2a5f3681d4 100644 --- a/src/rules/flake8_pie/rules.rs +++ b/src/rules/flake8_pie/rules.rs @@ -108,7 +108,7 @@ pub fn dupe_class_field_definitions<'a, 'b>( } } -// PIE796 +/// PIE796 pub fn prefer_unique_enums<'a, 'b>(checker: &mut Checker<'a>, parent: &'b Stmt, body: &'b [Stmt]) where 'b: 'a, @@ -117,19 +117,11 @@ where return; }; - let is_enum = bases.iter().any(|stmt| match &stmt.node { - ExprKind::Name { id, .. } => id == "Enum", - ExprKind::Attribute { value, attr, .. } => { - attr.ends_with("Enum") - && match &value.node { - ExprKind::Name { id, .. } => id == "enum", - _ => false, - } - } - _ => false, - }); - - if !is_enum { + if !bases.iter().any(|expr| { + checker + .resolve_call_path(expr) + .map_or(false, |call_path| call_path == ["enum", "Enum"]) + }) { return; } @@ -141,7 +133,9 @@ where if !seen_targets.insert(ComparableExpr::from(value)) { let diagnostic = Diagnostic::new( - violations::PreferUniqueEnums(unparse_expr(value, checker.stylist)), + violations::PreferUniqueEnums { + value: unparse_expr(value, checker.stylist), + }, Range::from_located(stmt), ); checker.diagnostics.push(diagnostic); diff --git a/src/violations.rs b/src/violations.rs index bc009a2777dbf..ca01eaa7f377a 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -5971,16 +5971,20 @@ impl AlwaysAutofixableViolation for DupeClassFieldDefinitions { } define_violation!( - pub struct PreferUniqueEnums(pub String); + pub struct PreferUniqueEnums { + pub value: String, + } ); impl Violation for PreferUniqueEnums { fn message(&self) -> String { - let PreferUniqueEnums(value) = self; - format!("Enum contains duplicate value: {value}") + let PreferUniqueEnums { value } = self; + format!("Enum contains duplicate value: `{value}`") } fn placeholder() -> Self { - PreferUniqueEnums("...".to_string()) + PreferUniqueEnums { + value: "...".to_string(), + } } }