From 5edcfa9a48d661970bea92d2be38c7b2412b389e Mon Sep 17 00:00:00 2001 From: AlonMenczer Date: Sat, 13 May 2023 15:50:40 +0300 Subject: [PATCH 1/4] Add pylint duplicate-bases rule --- .../test/fixtures/pylint/duplicate_bases.py | 23 +++++++++++ crates/ruff/src/checkers/ast/mod.rs | 4 ++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/pylint/mod.rs | 1 + .../src/rules/pylint/rules/duplicate_bases.rs | 40 +++++++++++++++++++ crates/ruff/src/rules/pylint/rules/mod.rs | 2 + ...nt__tests__PLE0241_duplicate_bases.py.snap | 11 +++++ ruff.schema.json | 3 ++ 9 files changed, 86 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py create mode 100644 crates/ruff/src/rules/pylint/rules/duplicate_bases.rs create mode 100644 crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap diff --git a/crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py b/crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py new file mode 100644 index 0000000000000..cde2a63777481 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py @@ -0,0 +1,23 @@ +### +# Errors. +### +class A: + ... + + +class B(A, A): + ... + +### +# Non-errors. +### +class C: + ... + + +class D(C): + ... + + +class E(A, C): + ... \ No newline at end of file diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index d77b7685649cd..8951c2c6658fa 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -785,6 +785,10 @@ where if self.settings.rules.enabled(Rule::BuiltinVariableShadowing) { flake8_builtins::rules::builtin_variable_shadowing(self, name, stmt); } + + if self.settings.rules.enabled(Rule::DuplicateBases) { + pylint::rules::duplicate_bases(self, name, bases); + } } StmtKind::Import(ast::StmtImport { names }) => { if self.settings.rules.enabled(Rule::MultipleImportsOnOneLine) { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 9fea0d295c107..c14b72b76f1f4 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -213,6 +213,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Pylint, "W2901") => Rule::RedefinedLoopName, (Pylint, "E0302") => Rule::UnexpectedSpecialMethodSignature, (Pylint, "W3301") => Rule::NestedMinMax, + (Pylint, "E0241") => Rule::DuplicateBases, // flake8-builtins (Flake8Builtins, "001") => Rule::BuiltinVariableShadowing, diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 5464f1b210eb3..23e77127b0f95 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -190,6 +190,7 @@ ruff_macros::register_rules!( rules::pylint::rules::LoggingTooManyArgs, rules::pylint::rules::UnexpectedSpecialMethodSignature, rules::pylint::rules::NestedMinMax, + rules::pylint::rules::DuplicateBases, // flake8-builtins rules::flake8_builtins::rules::BuiltinVariableShadowing, rules::flake8_builtins::rules::BuiltinArgumentShadowing, diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index be74e2b0680b0..eb703e349703c 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -46,6 +46,7 @@ mod tests { #[test_case(Rule::ImportSelf, Path::new("import_self/module.py"); "PLW0406")] #[test_case(Rule::InvalidAllFormat, Path::new("invalid_all_format.py"); "PLE0605")] #[test_case(Rule::InvalidAllObject, Path::new("invalid_all_object.py"); "PLE0604")] + #[test_case(Rule::DuplicateBases, Path::new("duplicate_bases.py"); "PLE0241")] #[test_case(Rule::InvalidCharacterBackspace, Path::new("invalid_characters.py"); "PLE2510")] #[test_case(Rule::InvalidCharacterEsc, Path::new("invalid_characters.py"); "PLE2513")] #[test_case(Rule::InvalidCharacterNul, Path::new("invalid_characters.py"); "PLE2514")] diff --git a/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs b/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs new file mode 100644 index 0000000000000..fe7bf99a18faa --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs @@ -0,0 +1,40 @@ +use std::collections::HashSet; + +use rustpython_parser::ast::{self, Expr, ExprKind, Identifier}; + +use crate::checkers::ast::Checker; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; + +#[violation] +pub struct DuplicateBases { + name: String, +} + +impl Violation for DuplicateBases { + #[derive_message_formats] + fn message(&self) -> String { + let DuplicateBases { name } = self; + format!("Duplicate bases for class `{name}`") + } +} + +/// PLE0241 +pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, bases: &[Expr]) { + let mut unique_bases: HashSet<&Identifier> = HashSet::new(); + + for base in bases { + if let ExprKind::Name(ast::ExprName { id, .. }) = &base.node { + if unique_bases.contains(id) { + checker.diagnostics.push(Diagnostic::new( + DuplicateBases { + name: name.to_string(), + }, + base.range(), + )) + } + unique_bases.insert(id); + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 19ac6fb52f0f6..78917e49216c4 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -26,6 +26,7 @@ pub(crate) use logging::{logging_call, LoggingTooFewArgs, LoggingTooManyArgs}; pub(crate) use magic_value_comparison::{magic_value_comparison, MagicValueComparison}; pub(crate) use manual_import_from::{manual_from_import, ManualFromImport}; pub(crate) use nested_min_max::{nested_min_max, NestedMinMax}; +pub(crate) use duplicate_bases::{duplicate_bases, DuplicateBases}; pub(crate) use nonlocal_without_binding::NonlocalWithoutBinding; pub(crate) use property_with_parameters::{property_with_parameters, PropertyWithParameters}; pub(crate) use redefined_loop_name::{redefined_loop_name, RedefinedLoopName}; @@ -57,6 +58,7 @@ mod collapsible_else_if; mod compare_to_empty_string; mod comparison_of_constant; mod continue_in_finally; +mod duplicate_bases; mod global_statement; mod global_variable_not_assigned; mod import_self; diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap new file mode 100644 index 0000000000000..b09a11f43a22a --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap @@ -0,0 +1,11 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +duplicate_bases.py:8:12: PLE0241 Duplicate bases for class `B` + | +8 | class B(A, A): + | ^ PLE0241 +9 | ... + | + + diff --git a/ruff.schema.json b/ruff.schema.json index fa891b1c6ae57..7a34581deed78 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1959,6 +1959,9 @@ "PLE0116", "PLE0117", "PLE0118", + "PLE02", + "PLE024", + "PLE0241", "PLE03", "PLE030", "PLE0302", From 0b6bb72148117cde5263b2cd8e91ab224e514cfe Mon Sep 17 00:00:00 2001 From: AlonMenczer Date: Sat, 13 May 2023 15:52:56 +0300 Subject: [PATCH 2/4] format --- crates/ruff/src/rules/pylint/rules/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 78917e49216c4..04a444ce209a6 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -8,6 +8,7 @@ pub(crate) use collapsible_else_if::{collapsible_else_if, CollapsibleElseIf}; pub(crate) use compare_to_empty_string::{compare_to_empty_string, CompareToEmptyString}; pub(crate) use comparison_of_constant::{comparison_of_constant, ComparisonOfConstant}; pub(crate) use continue_in_finally::{continue_in_finally, ContinueInFinally}; +pub(crate) use duplicate_bases::{duplicate_bases, DuplicateBases}; pub(crate) use global_statement::{global_statement, GlobalStatement}; pub(crate) use global_variable_not_assigned::GlobalVariableNotAssigned; pub(crate) use import_self::{import_from_self, import_self, ImportSelf}; @@ -26,7 +27,6 @@ pub(crate) use logging::{logging_call, LoggingTooFewArgs, LoggingTooManyArgs}; pub(crate) use magic_value_comparison::{magic_value_comparison, MagicValueComparison}; pub(crate) use manual_import_from::{manual_from_import, ManualFromImport}; pub(crate) use nested_min_max::{nested_min_max, NestedMinMax}; -pub(crate) use duplicate_bases::{duplicate_bases, DuplicateBases}; pub(crate) use nonlocal_without_binding::NonlocalWithoutBinding; pub(crate) use property_with_parameters::{property_with_parameters, PropertyWithParameters}; pub(crate) use redefined_loop_name::{redefined_loop_name, RedefinedLoopName}; From b0b545065bc8ba84ad3e73ad1434034ab4624e7d Mon Sep 17 00:00:00 2001 From: AlonMenczer Date: Sat, 13 May 2023 16:16:04 +0300 Subject: [PATCH 3/4] fix --- crates/ruff/src/rules/pylint/rules/duplicate_bases.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs b/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs index fe7bf99a18faa..8dc81a438b45a 100644 --- a/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs +++ b/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs @@ -32,7 +32,7 @@ pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, bases: &[Expr]) name: name.to_string(), }, base.range(), - )) + )); } unique_bases.insert(id); } From d0ea390685ee21b82624d6283c07789d0815f7ed Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 13 May 2023 10:16:38 -0400 Subject: [PATCH 4/4] Use bool on insert --- .../test/fixtures/pylint/duplicate_bases.py | 3 ++- .../src/rules/pylint/rules/duplicate_bases.rs | 24 ++++++++++--------- ...nt__tests__PLE0241_duplicate_bases.py.snap | 2 +- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py b/crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py index cde2a63777481..491421ccf5eb2 100644 --- a/crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py +++ b/crates/ruff/resources/test/fixtures/pylint/duplicate_bases.py @@ -8,6 +8,7 @@ class A: class B(A, A): ... + ### # Non-errors. ### @@ -20,4 +21,4 @@ class D(C): class E(A, C): - ... \ No newline at end of file + ... diff --git a/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs b/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs index 8dc81a438b45a..5cdd54800d838 100644 --- a/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs +++ b/crates/ruff/src/rules/pylint/rules/duplicate_bases.rs @@ -1,40 +1,42 @@ -use std::collections::HashSet; +use std::hash::BuildHasherDefault; +use rustc_hash::FxHashSet; use rustpython_parser::ast::{self, Expr, ExprKind, Identifier}; -use crate::checkers::ast::Checker; - use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; +use crate::checkers::ast::Checker; + #[violation] pub struct DuplicateBases { - name: String, + base: String, + class: String, } impl Violation for DuplicateBases { #[derive_message_formats] fn message(&self) -> String { - let DuplicateBases { name } = self; - format!("Duplicate bases for class `{name}`") + let DuplicateBases { base, class } = self; + format!("Duplicate base `{base}` for class `{class}`") } } /// PLE0241 pub(crate) fn duplicate_bases(checker: &mut Checker, name: &str, bases: &[Expr]) { - let mut unique_bases: HashSet<&Identifier> = HashSet::new(); - + let mut seen: FxHashSet<&Identifier> = + FxHashSet::with_capacity_and_hasher(bases.len(), BuildHasherDefault::default()); for base in bases { if let ExprKind::Name(ast::ExprName { id, .. }) = &base.node { - if unique_bases.contains(id) { + if !seen.insert(id) { checker.diagnostics.push(Diagnostic::new( DuplicateBases { - name: name.to_string(), + base: id.to_string(), + class: name.to_string(), }, base.range(), )); } - unique_bases.insert(id); } } } diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap index b09a11f43a22a..60b9ce981b7cf 100644 --- a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLE0241_duplicate_bases.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff/src/rules/pylint/mod.rs --- -duplicate_bases.py:8:12: PLE0241 Duplicate bases for class `B` +duplicate_bases.py:8:12: PLE0241 Duplicate base `A` for class `B` | 8 | class B(A, A): | ^ PLE0241