Skip to content

Commit

Permalink
[pylint] - implement R0202 and R0203 with autofixes (#8335)
Browse files Browse the repository at this point in the history
## Summary

Implements
[`no-classmethod-decorator`/`R0202`](https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/no-classmethod-decorator.html)
and
[`no-staticmethod-decorator`/`R0203`](https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/no-staticmethod-decorator.html)
with autofixes.

They're similar enough that all code is reusable for both.

See: #970 

## Test Plan

`cargo test`
  • Loading branch information
diceroll123 authored Nov 30, 2023
1 parent bbad4b4 commit 4212b41
Show file tree
Hide file tree
Showing 9 changed files with 293 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -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)
6 changes: 6 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
202 changes: 202 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/no_method_decorator.rs
Original file line number Diff line number Diff line change
@@ -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<String, TextRange> = 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;
}
};
};
}
}
Original file line number Diff line number Diff line change
@@ -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)


Original file line number Diff line number Diff line change
@@ -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 |+


2 changes: 2 additions & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 4212b41

Please sign in to comment.