diff --git a/crates/ruff/resources/test/fixtures/pylint/no_self_use.py b/crates/ruff/resources/test/fixtures/pylint/no_self_use.py new file mode 100644 index 0000000000..33524c5916 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/no_self_use.py @@ -0,0 +1,62 @@ +import abc + + +class Person: + def developer_greeting(self, name): # [no-self-use] + print(f"Greetings {name}!") + + def greeting_1(self): # [no-self-use] + print("Hello!") + + def greeting_2(self): # [no-self-use] + print("Hi!") + + +# OK +def developer_greeting(): + print("Greetings developer!") + + +# OK +class Person: + name = "Paris" + + def __init__(self): + pass + + def __cmp__(self, other): + print(24) + + def __repr__(self): + return "Person" + + def func(self): + ... + + def greeting_1(self): + print(f"Hello from {self.name} !") + + @staticmethod + def greeting_2(): + print("Hi!") + + +class Base(abc.ABC): + """abstract class""" + + @abstractmethod + def abstract_method(self): + """abstract method could not be a function""" + raise NotImplementedError + + +class Sub(Base): + @override + def abstract_method(self): + print("concret method") + + +class Prop: + @property + def count(self): + return 24 diff --git a/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs index e9f28c852d..32bea594b9 100644 --- a/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff/src/checkers/ast/analyze/deferred_scopes.rs @@ -30,6 +30,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { Rule::UnusedPrivateTypedDict, Rule::UnusedStaticMethodArgument, Rule::UnusedVariable, + Rule::NoSelfUse, ]) { return; } @@ -302,6 +303,12 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { pyflakes::rules::unused_import(checker, scope, &mut diagnostics); } } + + if scope.kind.is_function() { + if checker.enabled(Rule::NoSelfUse) { + pylint::rules::no_self_use(checker, scope, &mut diagnostics); + } + } } checker.diagnostics.extend(diagnostics); } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index a94ccc69fe..db1ab2831c 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -216,6 +216,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "R1722") => (RuleGroup::Unspecified, rules::pylint::rules::SysExitAlias), (Pylint, "R2004") => (RuleGroup::Unspecified, rules::pylint::rules::MagicValueComparison), (Pylint, "R5501") => (RuleGroup::Unspecified, rules::pylint::rules::CollapsibleElseIf), + (Pylint, "R6301") => (RuleGroup::Nursery, rules::pylint::rules::NoSelfUse), (Pylint, "W0120") => (RuleGroup::Unspecified, rules::pylint::rules::UselessElseOnLoop), (Pylint, "W0127") => (RuleGroup::Unspecified, rules::pylint::rules::SelfAssigningVariable), (Pylint, "W0129") => (RuleGroup::Unspecified, rules::pylint::rules::AssertOnStringLiteral), diff --git a/crates/ruff/src/rules/flake8_unused_arguments/mod.rs b/crates/ruff/src/rules/flake8_unused_arguments/mod.rs index 4875916f5a..b1faea7534 100644 --- a/crates/ruff/src/rules/flake8_unused_arguments/mod.rs +++ b/crates/ruff/src/rules/flake8_unused_arguments/mod.rs @@ -1,5 +1,5 @@ //! Rules from [flake8-unused-arguments](https://pypi.org/project/flake8-unused-arguments/). -mod helpers; +pub(crate) mod helpers; pub(crate) mod rules; pub mod settings; diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index fddc48e51e..5d18261946 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -132,6 +132,7 @@ mod tests { Path::new("subprocess_run_without_check.py") )] #[test_case(Rule::BadDunderMethodName, Path::new("bad_dunder_method_name.py"))] + #[test_case(Rule::NoSelfUse, Path::new("no_self_use.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/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index a2a498cb87..532f267ce4 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -29,6 +29,7 @@ pub(crate) use magic_value_comparison::*; pub(crate) use manual_import_from::*; pub(crate) use named_expr_without_context::*; pub(crate) use nested_min_max::*; +pub(crate) use no_self_use::*; pub(crate) use nonlocal_without_binding::*; pub(crate) use property_with_parameters::*; pub(crate) use redefined_loop_name::*; @@ -86,6 +87,7 @@ mod magic_value_comparison; mod manual_import_from; mod named_expr_without_context; mod nested_min_max; +mod no_self_use; mod nonlocal_without_binding; mod property_with_parameters; mod redefined_loop_name; diff --git a/crates/ruff/src/rules/pylint/rules/no_self_use.rs b/crates/ruff/src/rules/pylint/rules/no_self_use.rs new file mode 100644 index 0000000000..3aa6e8d9e0 --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/no_self_use.rs @@ -0,0 +1,120 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::call_path::{from_qualified_name, CallPath}; +use ruff_python_ast::{self as ast, ParameterWithDefault, Ranged}; +use ruff_python_semantic::{ + analyze::{function_type, visibility}, + Scope, ScopeKind, +}; + +use crate::{checkers::ast::Checker, rules::flake8_unused_arguments::helpers}; + +/// ## What it does +/// Checks for the presence of unused `self` parameter in methods definitions. +/// +/// ## Why is this bad? +/// Unused `self` parameters are usually a sign of a method that could be +/// replaced by a function or a static method. +/// +/// ## Example +/// ```python +/// class Person: +/// def greeting(self): +/// print("Greetings friend!") +/// ``` +/// +/// Use instead: +/// ```python +/// class Person: +/// @staticmethod +/// def greeting(): +/// print(f"Greetings friend!") +/// ``` +#[violation] +pub struct NoSelfUse { + method_name: String, +} + +impl Violation for NoSelfUse { + #[derive_message_formats] + fn message(&self) -> String { + let NoSelfUse { method_name } = self; + format!("Method `{method_name}` could be a function or static method") + } +} + +/// PLR6301 +pub(crate) fn no_self_use(checker: &Checker, scope: &Scope, diagnostics: &mut Vec) { + let Some(parent) = &checker.semantic().first_non_type_parent_scope(scope) else { + return; + }; + + let ScopeKind::Function(ast::StmtFunctionDef { + name, + parameters, + body, + decorator_list, + .. + }) = scope.kind + else { + return; + }; + + if !matches!( + function_type::classify( + name, + decorator_list, + parent, + checker.semantic(), + &checker.settings.pep8_naming.classmethod_decorators, + &checker.settings.pep8_naming.staticmethod_decorators, + ), + function_type::FunctionType::Method + ) { + return; + } + + let property_decorators = checker + .settings + .pydocstyle + .property_decorators + .iter() + .map(|decorator| from_qualified_name(decorator)) + .collect::>(); + + if helpers::is_empty(body) + || visibility::is_magic(name) + || visibility::is_abstract(decorator_list, checker.semantic()) + || visibility::is_override(decorator_list, checker.semantic()) + || visibility::is_overload(decorator_list, checker.semantic()) + || visibility::is_property(decorator_list, &property_decorators, checker.semantic()) + { + return; + } + + // Identify the `self` parameter. + let Some(parameter) = parameters + .posonlyargs + .iter() + .chain(¶meters.args) + .chain(¶meters.kwonlyargs) + .next() + .map(ParameterWithDefault::as_parameter) + else { + return; + }; + + if parameter.name.as_str() == "self" + && scope + .get("self") + .map(|binding_id| checker.semantic().binding(binding_id)) + .is_some_and(|binding| binding.kind.is_argument() && !binding.is_used()) + { + diagnostics.push(Diagnostic::new( + NoSelfUse { + method_name: name.to_string(), + }, + parameter.range(), + )); + } +} diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR6301_no_self_use.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR6301_no_self_use.py.snap new file mode 100644 index 0000000000..b37e142863 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR6301_no_self_use.py.snap @@ -0,0 +1,39 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +no_self_use.py:5:28: PLR6301 Method `developer_greeting` could be a function or static method + | +4 | class Person: +5 | def developer_greeting(self, name): # [no-self-use] + | ^^^^ PLR6301 +6 | print(f"Greetings {name}!") + | + +no_self_use.py:8:20: PLR6301 Method `greeting_1` could be a function or static method + | +6 | print(f"Greetings {name}!") +7 | +8 | def greeting_1(self): # [no-self-use] + | ^^^^ PLR6301 +9 | print("Hello!") + | + +no_self_use.py:11:20: PLR6301 Method `greeting_2` could be a function or static method + | + 9 | print("Hello!") +10 | +11 | def greeting_2(self): # [no-self-use] + | ^^^^ PLR6301 +12 | print("Hi!") + | + +no_self_use.py:55:25: PLR6301 Method `abstract_method` could be a function or static method + | +53 | class Sub(Base): +54 | @override +55 | def abstract_method(self): + | ^^^^ PLR6301 +56 | print("concret method") + | + + diff --git a/ruff.schema.json b/ruff.schema.json index faef882ebd..c1591cb938 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2270,6 +2270,7 @@ "PLR55", "PLR550", "PLR5501", + "PLR6301", "PLW", "PLW0", "PLW01",