From 93ff635e07090302eec4ee35ea8f5b858e30acf0 Mon Sep 17 00:00:00 2001 From: augustelalande Date: Thu, 4 Apr 2024 18:36:14 -0400 Subject: [PATCH 1/2] implement bad-staticmethod-argument --- .../pylint/bad_staticmethod_argument.py | 44 ++++++++ .../checkers/ast/analyze/deferred_scopes.rs | 15 ++- crates/ruff_linter/src/codes.rs | 13 +-- crates/ruff_linter/src/rules/pylint/mod.rs | 4 + .../pylint/rules/bad_staticmethod_argument.rs | 102 ++++++++++++++++++ .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + ..._PLW0211_bad_staticmethod_argument.py.snap | 28 +++++ ruff.schema.json | 2 + 8 files changed, 199 insertions(+), 11 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/bad_staticmethod_argument.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0211_bad_staticmethod_argument.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/bad_staticmethod_argument.py b/crates/ruff_linter/resources/test/fixtures/pylint/bad_staticmethod_argument.py new file mode 100644 index 00000000000000..6de4d74ee485f4 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/bad_staticmethod_argument.py @@ -0,0 +1,44 @@ +class Wolf: + @staticmethod + def eat(self): # [bad-staticmethod-argument] + pass + + +class Wolf: + @staticmethod + def eat(sheep): + pass + + +class Sheep: + @staticmethod + def eat(cls, x, y, z): # [bad-staticmethod-argument] + pass + + @staticmethod + def sleep(self, x, y, z): # [bad-staticmethod-argument] + pass + + def grow(self, x, y, z): + pass + + @classmethod + def graze(cls, x, y, z): + pass + + +class Foo: + @staticmethod + def eat(x, self, z): + pass + + @staticmethod + def sleep(x, cls, z): + pass + + def grow(self, x, y, z): + pass + + @classmethod + def graze(cls, x, y, z): + pass diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index 0a85c041b0bdf5..c4d0ee7944b785 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -15,15 +15,19 @@ use crate::rules::{ pub(crate) fn deferred_scopes(checker: &mut Checker) { if !checker.any_enabled(&[ Rule::AsyncioDanglingTask, + Rule::BadStaticmethodArgument, + Rule::BuiltinAttributeShadowing, Rule::GlobalVariableNotAssigned, Rule::ImportPrivateName, Rule::ImportShadowedByLoopVar, - Rule::InvalidFirstArgumentNameForMethod, Rule::InvalidFirstArgumentNameForClassMethod, + Rule::InvalidFirstArgumentNameForMethod, Rule::NoSelfUse, Rule::RedefinedArgumentFromLocal, Rule::RedefinedWhileUnused, Rule::RuntimeImportInTypeCheckingBlock, + Rule::SingledispatchMethod, + Rule::SingledispatchmethodFunction, Rule::TooManyLocals, Rule::TypingOnlyFirstPartyImport, Rule::TypingOnlyStandardLibraryImport, @@ -31,19 +35,16 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { Rule::UndefinedLocal, Rule::UnusedAnnotation, Rule::UnusedClassMethodArgument, - Rule::BuiltinAttributeShadowing, Rule::UnusedFunctionArgument, Rule::UnusedImport, Rule::UnusedLambdaArgument, Rule::UnusedMethodArgument, Rule::UnusedPrivateProtocol, Rule::UnusedPrivateTypeAlias, - Rule::UnusedPrivateTypeVar, Rule::UnusedPrivateTypedDict, + Rule::UnusedPrivateTypeVar, Rule::UnusedStaticMethodArgument, Rule::UnusedVariable, - Rule::SingledispatchMethod, - Rule::SingledispatchmethodFunction, ]) { return; } @@ -424,6 +425,10 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { pylint::rules::singledispatchmethod_function(checker, scope, &mut diagnostics); } + if checker.enabled(Rule::BadStaticmethodArgument) { + pylint::rules::bad_staticmethod_argument(checker, scope, &mut diagnostics); + } + if checker.any_enabled(&[ Rule::InvalidFirstArgumentNameForClassMethod, Rule::InvalidFirstArgumentNameForMethod, diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 75cb2966bc30f6..10a7fb752592f9 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -225,12 +225,12 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C0208") => (RuleGroup::Stable, rules::pylint::rules::IterationOverSet), (Pylint, "C0414") => (RuleGroup::Stable, rules::pylint::rules::UselessImportAlias), (Pylint, "C0415") => (RuleGroup::Preview, rules::pylint::rules::ImportOutsideTopLevel), - (Pylint, "C2401") => (RuleGroup::Preview, rules::pylint::rules::NonAsciiName), - (Pylint, "C2403") => (RuleGroup::Preview, rules::pylint::rules::NonAsciiImportName), - (Pylint, "C2801") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDunderCall), #[allow(deprecated)] (Pylint, "C1901") => (RuleGroup::Nursery, rules::pylint::rules::CompareToEmptyString), + (Pylint, "C2401") => (RuleGroup::Preview, rules::pylint::rules::NonAsciiName), + (Pylint, "C2403") => (RuleGroup::Preview, rules::pylint::rules::NonAsciiImportName), (Pylint, "C2701") => (RuleGroup::Preview, rules::pylint::rules::ImportPrivateName), + (Pylint, "C2801") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDunderCall), (Pylint, "C3002") => (RuleGroup::Stable, rules::pylint::rules::UnnecessaryDirectLambdaCall), (Pylint, "E0100") => (RuleGroup::Stable, rules::pylint::rules::YieldInInit), (Pylint, "E0101") => (RuleGroup::Stable, rules::pylint::rules::ReturnInInit), @@ -272,6 +272,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R0203") => (RuleGroup::Preview, rules::pylint::rules::NoStaticmethodDecorator), (Pylint, "R0206") => (RuleGroup::Stable, rules::pylint::rules::PropertyWithParameters), (Pylint, "R0402") => (RuleGroup::Stable, rules::pylint::rules::ManualFromImport), + (Pylint, "R0904") => (RuleGroup::Preview, rules::pylint::rules::TooManyPublicMethods), (Pylint, "R0911") => (RuleGroup::Stable, rules::pylint::rules::TooManyReturnStatements), (Pylint, "R0912") => (RuleGroup::Stable, rules::pylint::rules::TooManyBranches), (Pylint, "R0913") => (RuleGroup::Stable, rules::pylint::rules::TooManyArguments), @@ -282,9 +283,9 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1701") => (RuleGroup::Stable, rules::pylint::rules::RepeatedIsinstanceCalls), (Pylint, "R1702") => (RuleGroup::Preview, rules::pylint::rules::TooManyNestedBlocks), (Pylint, "R1704") => (RuleGroup::Preview, rules::pylint::rules::RedefinedArgumentFromLocal), + (Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary), (Pylint, "R1711") => (RuleGroup::Stable, rules::pylint::rules::UselessReturn), (Pylint, "R1714") => (RuleGroup::Stable, rules::pylint::rules::RepeatedEqualityComparison), - (Pylint, "R1706") => (RuleGroup::Removed, rules::pylint::rules::AndOrTernary), (Pylint, "R1722") => (RuleGroup::Stable, rules::pylint::rules::SysExitAlias), (Pylint, "R1733") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryDictIndexLookup), (Pylint, "R1736") => (RuleGroup::Preview, rules::pylint::rules::UnnecessaryListIndexLookup), @@ -302,11 +303,12 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W0129") => (RuleGroup::Stable, rules::pylint::rules::AssertOnStringLiteral), (Pylint, "W0131") => (RuleGroup::Stable, rules::pylint::rules::NamedExprWithoutContext), (Pylint, "W0133") => (RuleGroup::Preview, rules::pylint::rules::UselessExceptionStatement), + (Pylint, "W0211") => (RuleGroup::Preview, rules::pylint::rules::BadStaticmethodArgument), (Pylint, "W0245") => (RuleGroup::Preview, rules::pylint::rules::SuperWithoutBrackets), (Pylint, "W0406") => (RuleGroup::Stable, rules::pylint::rules::ImportSelf), (Pylint, "W0602") => (RuleGroup::Stable, rules::pylint::rules::GlobalVariableNotAssigned), - (Pylint, "W0604") => (RuleGroup::Preview, rules::pylint::rules::GlobalAtModuleLevel), (Pylint, "W0603") => (RuleGroup::Stable, rules::pylint::rules::GlobalStatement), + (Pylint, "W0604") => (RuleGroup::Preview, rules::pylint::rules::GlobalAtModuleLevel), (Pylint, "W0711") => (RuleGroup::Stable, rules::pylint::rules::BinaryOpException), (Pylint, "W1501") => (RuleGroup::Preview, rules::pylint::rules::BadOpenMode), (Pylint, "W1508") => (RuleGroup::Stable, rules::pylint::rules::InvalidEnvvarDefault), @@ -316,7 +318,6 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { #[allow(deprecated)] (Pylint, "W1641") => (RuleGroup::Nursery, rules::pylint::rules::EqWithoutHash), (Pylint, "W2101") => (RuleGroup::Preview, rules::pylint::rules::UselessWithLock), - (Pylint, "R0904") => (RuleGroup::Preview, rules::pylint::rules::TooManyPublicMethods), (Pylint, "W2901") => (RuleGroup::Stable, rules::pylint::rules::RedefinedLoopName), #[allow(deprecated)] (Pylint, "W3201") => (RuleGroup::Nursery, rules::pylint::rules::BadDunderMethodName), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 86d3d2e897e371..867e6dfc9596b5 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -190,6 +190,10 @@ mod tests { Path::new("useless_exception_statement.py") )] #[test_case(Rule::NanComparison, Path::new("nan_comparison.py"))] + #[test_case( + Rule::BadStaticmethodArgument, + Path::new("bad_staticmethod_argument.py") + )] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs b/crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs new file mode 100644 index 00000000000000..e2100af551ebe4 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs @@ -0,0 +1,102 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_python_ast::ParameterWithDefault; +use ruff_python_semantic::analyze::function_type; +use ruff_python_semantic::{Scope, ScopeKind}; +use ruff_text_size::Ranged; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for static methods that use `self` or `cls` as their first argument. +/// +/// ## Why is this bad? +/// [PEP 8] recommends the use of `self` and `cls` as the first arguments of +/// instance methods and class methods respectively. Naming the first argument +/// of a static method as self or cls can be misleading, as static methods do +/// not receive an instance or class reference as their first argument. +/// +/// ## Example +/// ```python +/// class Wolf: +/// @staticmethod +/// def eat(self): +/// pass +/// ``` +/// +/// Use instead: +/// ```python +/// class Wolf: +/// @staticmethod +/// def eat(sheep): +/// pass +/// ``` +/// +/// [PEP 8]: https://peps.python.org/pep-0008/#function-and-method-arguments +#[violation] +pub struct BadStaticmethodArgument { + argument_name: String, +} + +impl Violation for BadStaticmethodArgument { + #[derive_message_formats] + fn message(&self) -> String { + let Self { argument_name } = self; + format!("First argument of a static method should not be named `{argument_name}`") + } +} + +/// PLW0211 +pub(crate) fn bad_staticmethod_argument( + checker: &Checker, + scope: &Scope, + diagnostics: &mut Vec, +) { + let ScopeKind::Function(ast::StmtFunctionDef { + name, + parameters, + decorator_list, + .. + }) = &scope.kind + else { + panic!("Expected ScopeKind::Function") + }; + + let Some(parent) = &checker.semantic().first_non_type_parent_scope(scope) else { + return; + }; + + let type_ = function_type::classify( + name, + decorator_list, + parent, + checker.semantic(), + &checker.settings.pep8_naming.classmethod_decorators, + &checker.settings.pep8_naming.staticmethod_decorators, + ); + if !matches!(type_, function_type::FunctionType::StaticMethod) { + return; + } + + let Some(ParameterWithDefault { + parameter: self_or_cls, + .. + }) = parameters + .posonlyargs + .first() + .or_else(|| parameters.args.first()) + else { + return; + }; + + if matches!(self_or_cls.name.as_str(), "self" | "cls") { + let diagnostic = Diagnostic::new( + BadStaticmethodArgument { + argument_name: self_or_cls.name.to_string(), + }, + self_or_cls.range(), + ); + diagnostics.push(diagnostic); + } +} diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 99bbb5063efdb6..93f89e1f6cec04 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -3,6 +3,7 @@ pub(crate) use assert_on_string_literal::*; pub(crate) use await_outside_async::*; pub(crate) use bad_dunder_method_name::*; pub(crate) use bad_open_mode::*; +pub(crate) use bad_staticmethod_argument::*; pub(crate) use bad_str_strip_call::*; pub(crate) use bad_string_format_character::BadStringFormatCharacter; pub(crate) use bad_string_format_type::*; @@ -97,6 +98,7 @@ mod assert_on_string_literal; mod await_outside_async; mod bad_dunder_method_name; mod bad_open_mode; +mod bad_staticmethod_argument; mod bad_str_strip_call; pub(crate) mod bad_string_format_character; mod bad_string_format_type; diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0211_bad_staticmethod_argument.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0211_bad_staticmethod_argument.py.snap new file mode 100644 index 00000000000000..add63e311b0946 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0211_bad_staticmethod_argument.py.snap @@ -0,0 +1,28 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +bad_staticmethod_argument.py:3:13: PLW0211 First argument of a static method should not be named `self` + | +1 | class Wolf: +2 | @staticmethod +3 | def eat(self): # [bad-staticmethod-argument] + | ^^^^ PLW0211 +4 | pass + | + +bad_staticmethod_argument.py:15:13: PLW0211 First argument of a static method should not be named `cls` + | +13 | class Sheep: +14 | @staticmethod +15 | def eat(cls, x, y, z): # [bad-staticmethod-argument] + | ^^^ PLW0211 +16 | pass + | + +bad_staticmethod_argument.py:19:15: PLW0211 First argument of a static method should not be named `self` + | +18 | @staticmethod +19 | def sleep(self, x, y, z): # [bad-staticmethod-argument] + | ^^^^ PLW0211 +20 | pass + | diff --git a/ruff.schema.json b/ruff.schema.json index f6d36e1cfc51a2..4aa04519ad8054 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3389,6 +3389,8 @@ "PLW0131", "PLW0133", "PLW02", + "PLW021", + "PLW0211", "PLW024", "PLW0245", "PLW04", From e3f0670d23835afa5917f7eaadde70c5e10bdb8f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 5 Apr 2024 17:25:02 -0400 Subject: [PATCH 2/2] Tweak docs --- .../pylint/rules/bad_staticmethod_argument.rs | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs b/crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs index e2100af551ebe4..c99cd52c8cc1bf 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/bad_staticmethod_argument.rs @@ -3,7 +3,7 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::ParameterWithDefault; use ruff_python_semantic::analyze::function_type; -use ruff_python_semantic::{Scope, ScopeKind}; +use ruff_python_semantic::Scope; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -12,10 +12,10 @@ use crate::checkers::ast::Checker; /// Checks for static methods that use `self` or `cls` as their first argument. /// /// ## Why is this bad? -/// [PEP 8] recommends the use of `self` and `cls` as the first arguments of -/// instance methods and class methods respectively. Naming the first argument -/// of a static method as self or cls can be misleading, as static methods do -/// not receive an instance or class reference as their first argument. +/// [PEP 8] recommends the use of `self` and `cls` as the first arguments for +/// instance methods and class methods, respectively. Naming the first argument +/// of a static method as `self` or `cls` can be misleading, as static methods +/// do not receive an instance or class reference as their first argument. /// /// ## Example /// ```python @@ -53,15 +53,16 @@ pub(crate) fn bad_staticmethod_argument( scope: &Scope, diagnostics: &mut Vec, ) { - let ScopeKind::Function(ast::StmtFunctionDef { + let Some(func) = scope.kind.as_function() else { + return; + }; + + let ast::StmtFunctionDef { name, - parameters, decorator_list, + parameters, .. - }) = &scope.kind - else { - panic!("Expected ScopeKind::Function") - }; + } = func; let Some(parent) = &checker.semantic().first_non_type_parent_scope(scope) else { return;