diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/no_method_decorator.py b/crates/ruff_linter/resources/test/fixtures/pylint/no_method_decorator.py new file mode 100644 index 0000000000..e8351cb80a --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/no_method_decorator.py @@ -0,0 +1,19 @@ +from random import choice + +class Fruit: + COLORS = [] + + def __init__(self, color): + self.color = color + + def pick_colors(cls, *args): # [no-classmethod-decorator] + """classmethod to pick fruit colors""" + cls.COLORS = args + + pick_colors = classmethod(pick_colors) + + def pick_one_color(): # [no-staticmethod-decorator] + """staticmethod to pick one fruit color""" + return choice(Fruit.COLORS) + + pick_one_color = staticmethod(pick_one_color) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index 448ce1d2ba..addf24b810 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -384,6 +384,12 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { range: _, }, ) => { + if checker.enabled(Rule::NoClassmethodDecorator) { + pylint::rules::no_classmethod_decorator(checker, class_def); + } + if checker.enabled(Rule::NoStaticmethodDecorator) { + pylint::rules::no_staticmethod_decorator(checker, class_def); + } if checker.enabled(Rule::DjangoNullableModelStringField) { flake8_django::rules::nullable_model_string_field(checker, body); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 4dc1f13c55..e127c26c68 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -245,6 +245,8 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "E2515") => (RuleGroup::Stable, rules::pylint::rules::InvalidCharacterZeroWidthSpace), (Pylint, "R0124") => (RuleGroup::Stable, rules::pylint::rules::ComparisonWithItself), (Pylint, "R0133") => (RuleGroup::Stable, rules::pylint::rules::ComparisonOfConstant), + (Pylint, "R0202") => (RuleGroup::Preview, rules::pylint::rules::NoClassmethodDecorator), + (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, "R0911") => (RuleGroup::Stable, rules::pylint::rules::TooManyReturnStatements), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index e2f272619b..b2ebed57d6 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -154,6 +154,8 @@ mod tests { Rule::RepeatedKeywordArgument, Path::new("repeated_keyword_argument.py") )] + #[test_case(Rule::NoClassmethodDecorator, Path::new("no_method_decorator.py"))] + #[test_case(Rule::NoStaticmethodDecorator, Path::new("no_method_decorator.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/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index b130d36260..c332cdd243 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -35,6 +35,7 @@ pub(crate) use manual_import_from::*; pub(crate) use misplaced_bare_raise::*; pub(crate) use named_expr_without_context::*; pub(crate) use nested_min_max::*; +pub(crate) use no_method_decorator::*; pub(crate) use no_self_use::*; pub(crate) use non_ascii_module_import::*; pub(crate) use non_ascii_name::*; @@ -108,6 +109,7 @@ mod manual_import_from; mod misplaced_bare_raise; mod named_expr_without_context; mod nested_min_max; +mod no_method_decorator; mod no_self_use; mod non_ascii_module_import; mod non_ascii_name; diff --git a/crates/ruff_linter/src/rules/pylint/rules/no_method_decorator.rs b/crates/ruff_linter/src/rules/pylint/rules/no_method_decorator.rs new file mode 100644 index 0000000000..d2dc1ea329 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/no_method_decorator.rs @@ -0,0 +1,202 @@ +use std::collections::HashMap; + +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, DiagnosticKind, Edit, Fix}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr, Stmt}; +use ruff_python_trivia::indentation_at_offset; +use ruff_text_size::{Ranged, TextRange, TextSize}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for the use of a classmethod being made without the decorator. +/// +/// ## Why is this bad? +/// When it comes to consistency and readability, it's preferred to use the decorator. +/// +/// ## Example +/// ```python +/// class Foo: +/// def bar(cls): +/// ... +/// +/// bar = classmethod(bar) +/// ``` +/// +/// Use instead: +/// ```python +/// class Foo: +/// @classmethod +/// def bar(cls): +/// ... +/// ``` +#[violation] +pub struct NoClassmethodDecorator; + +impl AlwaysFixableViolation for NoClassmethodDecorator { + #[derive_message_formats] + fn message(&self) -> String { + format!("Class method defined without decorator") + } + + fn fix_title(&self) -> String { + format!("Add @classmethod decorator") + } +} + +/// ## What it does +/// Checks for the use of a staticmethod being made without the decorator. +/// +/// ## Why is this bad? +/// When it comes to consistency and readability, it's preferred to use the decorator. +/// +/// ## Example +/// ```python +/// class Foo: +/// def bar(cls): +/// ... +/// +/// bar = staticmethod(bar) +/// ``` +/// +/// Use instead: +/// ```python +/// class Foo: +/// @staticmethod +/// def bar(cls): +/// ... +/// ``` +#[violation] +pub struct NoStaticmethodDecorator; + +impl AlwaysFixableViolation for NoStaticmethodDecorator { + #[derive_message_formats] + fn message(&self) -> String { + format!("Static method defined without decorator") + } + + fn fix_title(&self) -> String { + format!("Add @staticmethod decorator") + } +} + +enum MethodType { + Classmethod, + Staticmethod, +} + +/// PLR0202 +pub(crate) fn no_classmethod_decorator(checker: &mut Checker, class_def: &ast::StmtClassDef) { + get_undecorated_methods(checker, class_def, &MethodType::Classmethod); +} + +/// PLR0203 +pub(crate) fn no_staticmethod_decorator(checker: &mut Checker, class_def: &ast::StmtClassDef) { + get_undecorated_methods(checker, class_def, &MethodType::Staticmethod); +} + +fn get_undecorated_methods( + checker: &mut Checker, + class_def: &ast::StmtClassDef, + method_type: &MethodType, +) { + let mut explicit_decorator_calls: HashMap = HashMap::default(); + + let (method_name, diagnostic_type): (&str, DiagnosticKind) = match method_type { + MethodType::Classmethod => ("classmethod", NoClassmethodDecorator.into()), + MethodType::Staticmethod => ("staticmethod", NoStaticmethodDecorator.into()), + }; + + // gather all explicit *method calls + for stmt in &class_def.body { + if let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = stmt { + if let Expr::Call(ast::ExprCall { + func, arguments, .. + }) = value.as_ref() + { + if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() { + if id == method_name && checker.semantic().is_builtin(method_name) { + if arguments.args.len() != 1 { + continue; + } + + if targets.len() != 1 { + continue; + } + + let target_name = match targets.first() { + Some(Expr::Name(ast::ExprName { id, .. })) => id.to_string(), + _ => continue, + }; + + if let Expr::Name(ast::ExprName { id, .. }) = &arguments.args[0] { + if target_name == *id { + explicit_decorator_calls.insert(id.clone(), stmt.range()); + } + }; + } + } + } + }; + } + + if explicit_decorator_calls.is_empty() { + return; + }; + + for stmt in &class_def.body { + if let Stmt::FunctionDef(ast::StmtFunctionDef { + name, + decorator_list, + .. + }) = stmt + { + if !explicit_decorator_calls.contains_key(name.as_str()) { + continue; + }; + + // if we find the decorator we're looking for, skip + if decorator_list.iter().any(|decorator| { + if let Expr::Name(ast::ExprName { id, .. }) = &decorator.expression { + if id == method_name && checker.semantic().is_builtin(method_name) { + return true; + } + } + + false + }) { + continue; + } + + let mut diagnostic = Diagnostic::new( + diagnostic_type.clone(), + TextRange::new(stmt.range().start(), stmt.range().start()), + ); + + let indentation = indentation_at_offset(stmt.range().start(), checker.locator()); + + match indentation { + Some(indentation) => { + let range = &explicit_decorator_calls[name.as_str()]; + + // SAFETY: Ruff only supports formatting files <= 4GB + #[allow(clippy::cast_possible_truncation)] + diagnostic.set_fix(Fix::safe_edits( + Edit::insertion( + format!("@{method_name}\n{indentation}"), + stmt.range().start(), + ), + [Edit::deletion( + range.start() - TextSize::from(indentation.len() as u32), + range.end(), + )], + )); + checker.diagnostics.push(diagnostic); + } + None => { + continue; + } + }; + }; + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0202_no_method_decorator.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0202_no_method_decorator.py.snap new file mode 100644 index 0000000000..d6fb470659 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0202_no_method_decorator.py.snap @@ -0,0 +1,31 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +no_method_decorator.py:9:5: PLR0202 [*] Class method defined without decorator + | + 7 | self.color = color + 8 | + 9 | def pick_colors(cls, *args): # [no-classmethod-decorator] + | PLR0202 +10 | """classmethod to pick fruit colors""" +11 | cls.COLORS = args + | + = help: Add @classmethod decorator + +ℹ Safe fix +6 6 | def __init__(self, color): +7 7 | self.color = color +8 8 | + 9 |+ @classmethod +9 10 | def pick_colors(cls, *args): # [no-classmethod-decorator] +10 11 | """classmethod to pick fruit colors""" +11 12 | cls.COLORS = args +12 13 | +13 |- pick_colors = classmethod(pick_colors) +14 14 | + 15 |+ +15 16 | def pick_one_color(): # [no-staticmethod-decorator] +16 17 | """staticmethod to pick one fruit color""" +17 18 | return choice(Fruit.COLORS) + + diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0203_no_method_decorator.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0203_no_method_decorator.py.snap new file mode 100644 index 0000000000..a1fe06afaf --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR0203_no_method_decorator.py.snap @@ -0,0 +1,27 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +no_method_decorator.py:15:5: PLR0203 [*] Static method defined without decorator + | +13 | pick_colors = classmethod(pick_colors) +14 | +15 | def pick_one_color(): # [no-staticmethod-decorator] + | PLR0203 +16 | """staticmethod to pick one fruit color""" +17 | return choice(Fruit.COLORS) + | + = help: Add @staticmethod decorator + +ℹ Safe fix +12 12 | +13 13 | pick_colors = classmethod(pick_colors) +14 14 | + 15 |+ @staticmethod +15 16 | def pick_one_color(): # [no-staticmethod-decorator] +16 17 | """staticmethod to pick one fruit color""" +17 18 | return choice(Fruit.COLORS) +18 19 | +19 |- pick_one_color = staticmethod(pick_one_color) + 20 |+ + + diff --git a/ruff.schema.json b/ruff.schema.json index cb37f44161..b8cac040eb 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3102,6 +3102,8 @@ "PLR0133", "PLR02", "PLR020", + "PLR0202", + "PLR0203", "PLR0206", "PLR04", "PLR040",