Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pylint] Implement self-cls-assignment (W0642) #9267

Merged
merged 7 commits into from
Apr 15, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class Fruit:
@classmethod
def list_fruits(cls) -> None:
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
cls = "apple" # PLW0642
cls: Fruit = "apple" # PLW0642
cls += "orange" # PLW0642
cls, blah = "apple", "orange" # PLW0642
blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

def print_color(self) -> None:
self = "red" # PLW0642
self: Self = "red" # PLW0642
self += "blue" # PLW0642
self, blah = "red", "blue" # PLW0642
blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642

def ok(self) -> None:
cls = None # OK because the rule looks for the name in the signature
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 11 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
}
}
Stmt::AugAssign(aug_assign @ ast::StmtAugAssign { target, .. }) => {
if checker.enabled(Rule::SelfClsAssignment) {
pylint::rules::self_cls_assignment(checker, target);
}
if checker.enabled(Rule::GlobalStatement) {
if let Expr::Name(ast::ExprName { id, .. }) = target.as_ref() {
pylint::rules::global_statement(checker, id);
Expand Down Expand Up @@ -1406,6 +1409,11 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
}
}
Stmt::Assign(assign @ ast::StmtAssign { targets, value, .. }) => {
if checker.enabled(Rule::SelfClsAssignment) {
for target in targets {
pylint::rules::self_cls_assignment(checker, target);
}
}
if checker.enabled(Rule::RedeclaredAssignedName) {
pylint::rules::redeclared_assigned_name(checker, targets);
}
Expand Down Expand Up @@ -1540,6 +1548,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
);
}
}
if checker.enabled(Rule::SelfClsAssignment) {
pylint::rules::self_cls_assignment(checker, target);
}
if checker.enabled(Rule::SelfAssigningVariable) {
pylint::rules::self_annotated_assignment(checker, assign_stmt);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pylint, "W0602") => (RuleGroup::Stable, rules::pylint::rules::GlobalVariableNotAssigned),
(Pylint, "W0603") => (RuleGroup::Stable, rules::pylint::rules::GlobalStatement),
(Pylint, "W0604") => (RuleGroup::Preview, rules::pylint::rules::GlobalAtModuleLevel),
(Pylint, "W0642") => (RuleGroup::Preview, rules::pylint::rules::SelfClsAssignment),
(Pylint, "W0711") => (RuleGroup::Stable, rules::pylint::rules::BinaryOpException),
(Pylint, "W1501") => (RuleGroup::Preview, rules::pylint::rules::BadOpenMode),
(Pylint, "W1508") => (RuleGroup::Stable, rules::pylint::rules::InvalidEnvvarDefault),
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ mod tests {
#[test_case(Rule::NoStaticmethodDecorator, Path::new("no_method_decorator.py"))]
#[test_case(Rule::PotentialIndexError, Path::new("potential_index_error.py"))]
#[test_case(Rule::SuperWithoutBrackets, Path::new("super_without_brackets.py"))]
#[test_case(Rule::SelfClsAssignment, Path::new("self_cls_assignment.py"))]
#[test_case(Rule::TooManyNestedBlocks, Path::new("too_many_nested_blocks.py"))]
#[test_case(Rule::DictIterMissingItems, Path::new("dict_iter_missing_items.py"))]
#[test_case(
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 @@ -60,6 +60,7 @@ pub(crate) use repeated_isinstance_calls::*;
pub(crate) use repeated_keyword_argument::*;
pub(crate) use return_in_init::*;
pub(crate) use self_assigning_variable::*;
pub(crate) use self_cls_assignment::*;
pub(crate) use single_string_slots::*;
pub(crate) use singledispatch_method::*;
pub(crate) use singledispatchmethod_function::*;
Expand Down Expand Up @@ -156,6 +157,7 @@ mod repeated_isinstance_calls;
mod repeated_keyword_argument;
mod return_in_init;
mod self_assigning_variable;
mod self_cls_assignment;
mod single_string_slots;
mod singledispatch_method;
mod singledispatchmethod_function;
Expand Down
104 changes: 104 additions & 0 deletions crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
use ruff_python_ast::{self as ast, Expr};

use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::{analyze::function_type, ScopeKind};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for assignment of `self` and `cls` in methods.
///
/// ## Why is this bad?
/// The identifiers `self` and `cls` are conventional in Python for
/// the first argument of instance methods and class methods, respectively.
///
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
#[violation]
pub struct SelfClsAssignment {
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
keyword: String,
}

impl Violation for SelfClsAssignment {
#[derive_message_formats]
fn message(&self) -> String {
let SelfClsAssignment { keyword } = self;
format!("Assignment of variable `{keyword}`")
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
}

/// PLW0127
pub(crate) fn self_cls_assignment(checker: &mut Checker, target: &Expr) {
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
let ScopeKind::Function(ast::StmtFunctionDef {
name,
decorator_list,
parameters,
..
}) = checker.semantic().current_scope().kind
else {
return;
};

let Some(parent) = &checker
.semantic()
.first_non_type_parent_scope(checker.semantic().current_scope())
else {
return;
};

let keyword = match function_type::classify(
name,
decorator_list,
parent,
checker.semantic(),
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
) {
function_type::FunctionType::ClassMethod { .. } => {
let Some(first_arg) = parameters.args.first() else {
return;
};
if first_arg.parameter.name.as_str() != "cls" {
return;
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

"cls"
}
function_type::FunctionType::Method { .. } => {
let Some(first_arg) = parameters.args.first() else {
return;
};
if first_arg.parameter.name.as_str() != "self" {
return;
}
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved

"self"
}
_ => return,
};

check_expr(checker, target, keyword);
}

fn check_expr(checker: &mut Checker, target: &Expr, keyword: &str) {
match target {
Expr::Name(_) => {
if let Expr::Name(ast::ExprName { id, .. }) = target {
if id.as_str() == keyword {
checker.diagnostics.push(Diagnostic::new(
SelfClsAssignment {
keyword: keyword.to_string(),
},
target.range(),
));
}
}
}
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
dhruvmanila marked this conversation as resolved.
Show resolved Hide resolved
for element in elts {
check_expr(checker, element, keyword);
}
}
_ => {}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
self_cls_assignment.py:4:9: PLW0642 Assignment of variable `cls`
|
2 | @classmethod
3 | def list_fruits(cls) -> None:
4 | cls = "apple" # PLW0642
| ^^^ PLW0642
5 | cls: Fruit = "apple" # PLW0642
6 | cls += "orange" # PLW0642
|

self_cls_assignment.py:5:9: PLW0642 Assignment of variable `cls`
|
3 | def list_fruits(cls) -> None:
4 | cls = "apple" # PLW0642
5 | cls: Fruit = "apple" # PLW0642
| ^^^ PLW0642
6 | cls += "orange" # PLW0642
7 | cls, blah = "apple", "orange" # PLW0642
|

self_cls_assignment.py:6:9: PLW0642 Assignment of variable `cls`
|
4 | cls = "apple" # PLW0642
5 | cls: Fruit = "apple" # PLW0642
6 | cls += "orange" # PLW0642
| ^^^ PLW0642
7 | cls, blah = "apple", "orange" # PLW0642
8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642
|

self_cls_assignment.py:7:9: PLW0642 Assignment of variable `cls`
|
5 | cls: Fruit = "apple" # PLW0642
6 | cls += "orange" # PLW0642
7 | cls, blah = "apple", "orange" # PLW0642
| ^^^ PLW0642
8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642
|

self_cls_assignment.py:8:16: PLW0642 Assignment of variable `cls`
|
6 | cls += "orange" # PLW0642
7 | cls, blah = "apple", "orange" # PLW0642
8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642
| ^^^ PLW0642
9 |
10 | def print_color(self) -> None:
|

self_cls_assignment.py:11:9: PLW0642 Assignment of variable `self`
|
10 | def print_color(self) -> None:
11 | self = "red" # PLW0642
| ^^^^ PLW0642
12 | self: Self = "red" # PLW0642
13 | self += "blue" # PLW0642
|

self_cls_assignment.py:12:9: PLW0642 Assignment of variable `self`
|
10 | def print_color(self) -> None:
11 | self = "red" # PLW0642
12 | self: Self = "red" # PLW0642
| ^^^^ PLW0642
13 | self += "blue" # PLW0642
14 | self, blah = "red", "blue" # PLW0642
|

self_cls_assignment.py:13:9: PLW0642 Assignment of variable `self`
|
11 | self = "red" # PLW0642
12 | self: Self = "red" # PLW0642
13 | self += "blue" # PLW0642
| ^^^^ PLW0642
14 | self, blah = "red", "blue" # PLW0642
15 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642
|

self_cls_assignment.py:14:9: PLW0642 Assignment of variable `self`
|
12 | self: Self = "red" # PLW0642
13 | self += "blue" # PLW0642
14 | self, blah = "red", "blue" # PLW0642
| ^^^^ PLW0642
15 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642
|

self_cls_assignment.py:15:16: PLW0642 Assignment of variable `self`
|
13 | self += "blue" # PLW0642
14 | self, blah = "red", "blue" # PLW0642
15 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642
| ^^^^ PLW0642
16 |
17 | def ok(self) -> None:
|


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.