From 006c0f3674df7e886eee1423822f483cdc48f90d Mon Sep 17 00:00:00 2001 From: Steve C Date: Sun, 24 Dec 2023 17:03:05 -0500 Subject: [PATCH 1/5] [pylint] - implement W0642/self-cls-assignment --- .../fixtures/pylint/self_cls_assignment.py | 16 +++ .../src/checkers/ast/analyze/statement.rs | 11 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/pylint/mod.rs | 1 + .../ruff_linter/src/rules/pylint/rules/mod.rs | 2 + .../rules/pylint/rules/self_cls_assignment.rs | 102 ++++++++++++++++++ ...tests__PLW0642_self_cls_assignment.py.snap | 81 ++++++++++++++ ruff.schema.json | 2 + 8 files changed, 216 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py create mode 100644 crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py b/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py new file mode 100644 index 00000000000000..a7ec0e90719893 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py @@ -0,0 +1,16 @@ +class Fruit: + @classmethod + def list_fruits(cls) -> None: + cls = "apple" + cls: Fruit = "apple" + cls += "orange" + cls, blah = "apple", "orange" + + def print_color(self) -> None: + self = "red" + self: Self = "red" + self += "blue" + self, blah = "red", "blue" + + def ok(self) -> None: + cls = None # OK because the rule looks for the name in the signature diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index c31f1479976635..0489c8f77fd35a 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1060,6 +1060,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); @@ -1389,6 +1392,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::LambdaAssignment) { if let [target] = &targets[..] { pycodestyle::rules::lambda_assignment(checker, target, value, None, stmt); @@ -1514,6 +1522,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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index d073f0223f0890..39f9cd9f6dce15 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -282,6 +282,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (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, "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), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index 9182956716f34e..1665e92677611d 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -168,6 +168,7 @@ mod tests { #[test_case(Rule::UnnecessaryDunderCall, Path::new("unnecessary_dunder_call.py"))] #[test_case(Rule::NoStaticmethodDecorator, Path::new("no_method_decorator.py"))] #[test_case(Rule::SuperWithoutBrackets, Path::new("super_without_brackets.py"))] + #[test_case(Rule::SelfClsAssignment, Path::new("self_cls_assignment.py"))] #[test_case( Rule::UnnecessaryDictIndexLookup, Path::new("unnecessary_dict_index_lookup.py") diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index ba5f2916dca13e..386cce9a59cc25 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -50,6 +50,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 subprocess_popen_preexec_fn::*; pub(crate) use subprocess_run_without_check::*; @@ -132,6 +133,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 subprocess_popen_preexec_fn; mod subprocess_run_without_check; diff --git a/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs new file mode 100644 index 00000000000000..647e4f364f5aea --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs @@ -0,0 +1,102 @@ +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. +/// +#[violation] +pub struct SelfClsAssignment { + keyword: String, +} + +impl Violation for SelfClsAssignment { + #[derive_message_formats] + fn message(&self) -> String { + let SelfClsAssignment { keyword } = self; + format!("Assignment of variable `{keyword}`") + } +} + +/// PLW0127 +pub(crate) fn self_cls_assignment(checker: &mut Checker, target: &Expr) { + 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; + } + + "cls" + } + function_type::FunctionType::Method { .. } => { + let Some(first_arg) = parameters.args.first() else { + return; + }; + if first_arg.parameter.name.as_str() != "self" { + return; + } + + "self" + } + _ => return, + }; + + match target { + Expr::Name(_) => check_name(checker, target, keyword), + Expr::Tuple(ast::ExprTuple { elts, .. }) => { + for element in elts { + check_name(checker, element, keyword); + } + } + _ => {} + } +} + +fn check_name(checker: &mut Checker, target: &Expr, keyword: &str) { + 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(), + )); + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap new file mode 100644 index 00000000000000..db8ac1d400e119 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap @@ -0,0 +1,81 @@ +--- +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 +5 | cls: Fruit = "apple" +6 | cls += "orange" + | + +self_cls_assignment.py:5:9: PLW0642 Assignment of variable `cls` + | +3 | def list_fruits(cls) -> None: +4 | cls = "apple" +5 | cls: Fruit = "apple" + | ^^^ PLW0642 +6 | cls += "orange" +7 | cls, blah = "apple", "orange" + | + +self_cls_assignment.py:6:9: PLW0642 Assignment of variable `cls` + | +4 | cls = "apple" +5 | cls: Fruit = "apple" +6 | cls += "orange" + | ^^^ PLW0642 +7 | cls, blah = "apple", "orange" + | + +self_cls_assignment.py:7:9: PLW0642 Assignment of variable `cls` + | +5 | cls: Fruit = "apple" +6 | cls += "orange" +7 | cls, blah = "apple", "orange" + | ^^^ PLW0642 +8 | +9 | def print_color(self) -> None: + | + +self_cls_assignment.py:10:9: PLW0642 Assignment of variable `self` + | + 9 | def print_color(self) -> None: +10 | self = "red" + | ^^^^ PLW0642 +11 | self: Self = "red" +12 | self += "blue" + | + +self_cls_assignment.py:11:9: PLW0642 Assignment of variable `self` + | + 9 | def print_color(self) -> None: +10 | self = "red" +11 | self: Self = "red" + | ^^^^ PLW0642 +12 | self += "blue" +13 | self, blah = "red", "blue" + | + +self_cls_assignment.py:12:9: PLW0642 Assignment of variable `self` + | +10 | self = "red" +11 | self: Self = "red" +12 | self += "blue" + | ^^^^ PLW0642 +13 | self, blah = "red", "blue" + | + +self_cls_assignment.py:13:9: PLW0642 Assignment of variable `self` + | +11 | self: Self = "red" +12 | self += "blue" +13 | self, blah = "red", "blue" + | ^^^^ PLW0642 +14 | +15 | def ok(self) -> None: + | + + diff --git a/ruff.schema.json b/ruff.schema.json index ef61102357472a..165d7fed5c4dc6 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3243,6 +3243,8 @@ "PLW0602", "PLW0603", "PLW0604", + "PLW064", + "PLW0642", "PLW07", "PLW071", "PLW0711", From aae45126ed122960138000a2b4f53d9c9bf6a563 Mon Sep 17 00:00:00 2001 From: Steve C Date: Tue, 26 Dec 2023 15:32:23 -0500 Subject: [PATCH 2/5] recursively check tuple exprs --- .../fixtures/pylint/self_cls_assignment.py | 18 ++-- .../rules/pylint/rules/self_cls_assignment.rs | 32 ++++--- ...tests__PLW0642_self_cls_assignment.py.snap | 92 +++++++++++-------- 3 files changed, 83 insertions(+), 59 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py b/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py index a7ec0e90719893..8a39360620cdef 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py @@ -1,16 +1,18 @@ class Fruit: @classmethod def list_fruits(cls) -> None: - cls = "apple" - cls: Fruit = "apple" - cls += "orange" - cls, blah = "apple", "orange" + cls = "apple" # PLW0642 + cls: Fruit = "apple" # PLW0642 + cls += "orange" # PLW0642 + cls, blah = "apple", "orange" # PLW0642 + blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 def print_color(self) -> None: - self = "red" - self: Self = "red" - self += "blue" - self, blah = "red", "blue" + 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 diff --git a/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs index 647e4f364f5aea..ec93b317c7c6ef 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs @@ -77,26 +77,28 @@ pub(crate) fn self_cls_assignment(checker: &mut Checker, target: &Expr) { _ => return, }; + check_expr(checker, target, keyword); +} + +fn check_expr(checker: &mut Checker, target: &Expr, keyword: &str) { match target { - Expr::Name(_) => check_name(checker, target, keyword), + 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, .. }) => { for element in elts { - check_name(checker, element, keyword); + check_expr(checker, element, keyword); } } _ => {} } } - -fn check_name(checker: &mut Checker, target: &Expr, keyword: &str) { - 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(), - )); - } - } -} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap index db8ac1d400e119..81392066063af3 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap @@ -5,77 +5,97 @@ self_cls_assignment.py:4:9: PLW0642 Assignment of variable `cls` | 2 | @classmethod 3 | def list_fruits(cls) -> None: -4 | cls = "apple" +4 | cls = "apple" # PLW0642 | ^^^ PLW0642 -5 | cls: Fruit = "apple" -6 | cls += "orange" +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" -5 | cls: Fruit = "apple" +4 | cls = "apple" # PLW0642 +5 | cls: Fruit = "apple" # PLW0642 | ^^^ PLW0642 -6 | cls += "orange" -7 | cls, blah = "apple", "orange" +6 | cls += "orange" # PLW0642 +7 | cls, blah = "apple", "orange" # PLW0642 | self_cls_assignment.py:6:9: PLW0642 Assignment of variable `cls` | -4 | cls = "apple" -5 | cls: Fruit = "apple" -6 | cls += "orange" +4 | cls = "apple" # PLW0642 +5 | cls: Fruit = "apple" # PLW0642 +6 | cls += "orange" # PLW0642 | ^^^ PLW0642 -7 | cls, blah = "apple", "orange" +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" -6 | cls += "orange" -7 | cls, blah = "apple", "orange" +5 | cls: Fruit = "apple" # PLW0642 +6 | cls += "orange" # PLW0642 +7 | cls, blah = "apple", "orange" # PLW0642 | ^^^ PLW0642 -8 | -9 | def print_color(self) -> None: +8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 | -self_cls_assignment.py:10:9: PLW0642 Assignment of variable `self` +self_cls_assignment.py:8:16: PLW0642 Assignment of variable `cls` | - 9 | def print_color(self) -> None: -10 | self = "red" - | ^^^^ PLW0642 -11 | self: Self = "red" -12 | self += "blue" + 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` | - 9 | def print_color(self) -> None: -10 | self = "red" -11 | self: Self = "red" +10 | def print_color(self) -> None: +11 | self = "red" # PLW0642 | ^^^^ PLW0642 -12 | self += "blue" -13 | self, blah = "red", "blue" +12 | self: Self = "red" # PLW0642 +13 | self += "blue" # PLW0642 | self_cls_assignment.py:12:9: PLW0642 Assignment of variable `self` | -10 | self = "red" -11 | self: Self = "red" -12 | self += "blue" +10 | def print_color(self) -> None: +11 | self = "red" # PLW0642 +12 | self: Self = "red" # PLW0642 | ^^^^ PLW0642 -13 | self, blah = "red", "blue" +13 | self += "blue" # PLW0642 +14 | self, blah = "red", "blue" # PLW0642 | self_cls_assignment.py:13:9: PLW0642 Assignment of variable `self` | -11 | self: Self = "red" -12 | self += "blue" -13 | self, blah = "red", "blue" +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 -14 | -15 | def ok(self) -> None: +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: | From 1c668d8d6923533c978f352a584ed41ccce977e8 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 13 Apr 2024 13:43:13 -0400 Subject: [PATCH 3/5] address requested changes --- ...ssignment.py => self_or_cls_assignment.py} | 17 ++- .../src/checkers/ast/analyze/statement.rs | 12 +- crates/ruff_linter/src/codes.rs | 2 +- crates/ruff_linter/src/rules/pylint/mod.rs | 2 +- .../ruff_linter/src/rules/pylint/rules/mod.rs | 4 +- .../rules/pylint/rules/self_cls_assignment.rs | 104 -------------- .../pylint/rules/self_or_cls_assignment.rs | 134 ++++++++++++++++++ ...tests__PLW0642_self_cls_assignment.py.snap | 101 ------------- ...ts__PLW0642_self_or_cls_assignment.py.snap | 119 ++++++++++++++++ 9 files changed, 279 insertions(+), 216 deletions(-) rename crates/ruff_linter/resources/test/fixtures/pylint/{self_cls_assignment.py => self_or_cls_assignment.py} (61%) delete mode 100644 crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs create mode 100644 crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs delete mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap create mode 100644 crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py b/crates/ruff_linter/resources/test/fixtures/pylint/self_or_cls_assignment.py similarity index 61% rename from crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py rename to crates/ruff_linter/resources/test/fixtures/pylint/self_or_cls_assignment.py index 8a39360620cdef..5932586a52f8b6 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/self_cls_assignment.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/self_or_cls_assignment.py @@ -6,6 +6,7 @@ def list_fruits(cls) -> None: cls += "orange" # PLW0642 cls, blah = "apple", "orange" # PLW0642 blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 + blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 def print_color(self) -> None: self = "red" # PLW0642 @@ -13,6 +14,20 @@ def print_color(self) -> None: self += "blue" # PLW0642 self, blah = "red", "blue" # PLW0642 blah, (self, blah2) = "apple", ("orange", "banana") # 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 + + @classmethod + def ok(cls) -> None: + self = None + + @staticmethod + def list_fruits_static(self, cls) -> None: + self = "apple" # Ok + cls = "banana" # Ok + +def list_fruits(self, cls) -> None: + self = "apple" # Ok + cls = "banana" # Ok diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index ae55fac108c329..524dddf0fa184b 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1055,8 +1055,8 @@ 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::SelfOrClsAssignment) { + pylint::rules::self_or_cls_assignment(checker, target); } if checker.enabled(Rule::GlobalStatement) { if let Expr::Name(ast::ExprName { id, .. }) = target.as_ref() { @@ -1409,9 +1409,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { } } Stmt::Assign(assign @ ast::StmtAssign { targets, value, .. }) => { - if checker.enabled(Rule::SelfClsAssignment) { + if checker.enabled(Rule::SelfOrClsAssignment) { for target in targets { - pylint::rules::self_cls_assignment(checker, target); + pylint::rules::self_or_cls_assignment(checker, target); } } if checker.enabled(Rule::RedeclaredAssignedName) { @@ -1548,8 +1548,8 @@ 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::SelfOrClsAssignment) { + pylint::rules::self_or_cls_assignment(checker, target); } if checker.enabled(Rule::SelfAssigningVariable) { pylint::rules::self_annotated_assignment(checker, assign_stmt); diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 984ab526c12edb..3a30d8288616bb 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -310,7 +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, "W0642") => (RuleGroup::Preview, rules::pylint::rules::SelfOrClsAssignment), (Pylint, "W0711") => (RuleGroup::Stable, rules::pylint::rules::BinaryOpException), (Pylint, "W1501") => (RuleGroup::Preview, rules::pylint::rules::BadOpenMode), (Pylint, "W1508") => (RuleGroup::Stable, rules::pylint::rules::InvalidEnvvarDefault), diff --git a/crates/ruff_linter/src/rules/pylint/mod.rs b/crates/ruff_linter/src/rules/pylint/mod.rs index effbefd97422da..5165f28bd13433 100644 --- a/crates/ruff_linter/src/rules/pylint/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/mod.rs @@ -180,7 +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::SelfOrClsAssignment, Path::new("self_or_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( diff --git a/crates/ruff_linter/src/rules/pylint/rules/mod.rs b/crates/ruff_linter/src/rules/pylint/rules/mod.rs index 1c66f2748cfe63..450aac0de855ea 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/mod.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/mod.rs @@ -60,7 +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 self_or_cls_assignment::*; pub(crate) use single_string_slots::*; pub(crate) use singledispatch_method::*; pub(crate) use singledispatchmethod_function::*; @@ -157,7 +157,7 @@ mod repeated_isinstance_calls; mod repeated_keyword_argument; mod return_in_init; mod self_assigning_variable; -mod self_cls_assignment; +mod self_or_cls_assignment; mod single_string_slots; mod singledispatch_method; mod singledispatchmethod_function; diff --git a/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs deleted file mode 100644 index ec93b317c7c6ef..00000000000000 --- a/crates/ruff_linter/src/rules/pylint/rules/self_cls_assignment.rs +++ /dev/null @@ -1,104 +0,0 @@ -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. -/// -#[violation] -pub struct SelfClsAssignment { - keyword: String, -} - -impl Violation for SelfClsAssignment { - #[derive_message_formats] - fn message(&self) -> String { - let SelfClsAssignment { keyword } = self; - format!("Assignment of variable `{keyword}`") - } -} - -/// PLW0127 -pub(crate) fn self_cls_assignment(checker: &mut Checker, target: &Expr) { - 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; - } - - "cls" - } - function_type::FunctionType::Method { .. } => { - let Some(first_arg) = parameters.args.first() else { - return; - }; - if first_arg.parameter.name.as_str() != "self" { - return; - } - - "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, .. }) => { - for element in elts { - check_expr(checker, element, keyword); - } - } - _ => {} - } -} diff --git a/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs new file mode 100644 index 00000000000000..95caf2a3be21f1 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs @@ -0,0 +1,134 @@ +use ast::ParameterWithDefault; +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. +/// +/// ## Example +/// ```python +/// class A: +/// def method(self): +/// self = 1 +/// def class_method(cls): +/// cls = 1 +/// ``` +/// +#[violation] +pub struct SelfOrClsAssignment { + method_type: MethodType, +} + +impl Violation for SelfOrClsAssignment { + #[derive_message_formats] + fn message(&self) -> String { + let SelfOrClsAssignment { method_type } = self; + + format!( + "Invalid assignment to `{}` argument in {} method", + method_type.arg_name(), + method_type.function_type() + ) + } +} + +/// PLW0127 +pub(crate) fn self_or_cls_assignment(checker: &mut Checker, target: &Expr) { + 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 Some(ParameterWithDefault { + parameter: self_or_cls, + .. + }) = parameters + .posonlyargs + .first() + .or_else(|| parameters.args.first()) + else { + return; + }; + + let function_type = function_type::classify( + name, + decorator_list, + parent, + checker.semantic(), + &checker.settings.pep8_naming.classmethod_decorators, + &checker.settings.pep8_naming.staticmethod_decorators, + ); + + let method_type = match (function_type, self_or_cls.name.as_str()) { + (function_type::FunctionType::Method { .. }, "self") => MethodType::Instance, + (function_type::FunctionType::ClassMethod { .. }, "cls") => MethodType::Class, + _ => return, + }; + + check_expr(checker, target, method_type); +} + +fn check_expr(checker: &mut Checker, target: &Expr, method_type: MethodType) { + match target { + Expr::Name(_) => { + if let Expr::Name(ast::ExprName { id, .. }) = target { + if id.as_str() == method_type.arg_name() { + checker.diagnostics.push(Diagnostic::new( + SelfOrClsAssignment { method_type }, + target.range(), + )); + } + } + } + Expr::Tuple(ast::ExprTuple { elts, .. }) | Expr::List(ast::ExprList { elts, .. }) => { + for element in elts { + check_expr(checker, element, method_type); + } + } + _ => {} + } +} + +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +enum MethodType { + Instance, + Class, +} + +impl MethodType { + fn arg_name(self) -> &'static str { + match self { + MethodType::Instance => "self", + MethodType::Class => "cls", + } + } + + fn function_type(self) -> &'static str { + match self { + MethodType::Instance => "instance", + MethodType::Class => "class", + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap deleted file mode 100644 index 81392066063af3..00000000000000 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_cls_assignment.py.snap +++ /dev/null @@ -1,101 +0,0 @@ ---- -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: - | - - diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap new file mode 100644 index 00000000000000..8886cf66f35811 --- /dev/null +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap @@ -0,0 +1,119 @@ +--- +source: crates/ruff_linter/src/rules/pylint/mod.rs +--- +self_or_cls_assignment.py:4:9: PLW0642 Invalid assignment to `cls` argument in class method + | +2 | @classmethod +3 | def list_fruits(cls) -> None: +4 | cls = "apple" # PLW0642 + | ^^^ PLW0642 +5 | cls: Fruit = "apple" # PLW0642 +6 | cls += "orange" # PLW0642 + | + +self_or_cls_assignment.py:5:9: PLW0642 Invalid assignment to `cls` argument in class method + | +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_or_cls_assignment.py:6:9: PLW0642 Invalid assignment to `cls` argument in class method + | +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_or_cls_assignment.py:7:9: PLW0642 Invalid assignment to `cls` argument in class method + | +5 | cls: Fruit = "apple" # PLW0642 +6 | cls += "orange" # PLW0642 +7 | cls, blah = "apple", "orange" # PLW0642 + | ^^^ PLW0642 +8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 +9 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 + | + +self_or_cls_assignment.py:8:16: PLW0642 Invalid assignment to `cls` argument in class method + | +6 | cls += "orange" # PLW0642 +7 | cls, blah = "apple", "orange" # PLW0642 +8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 + | ^^^ PLW0642 +9 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 + | + +self_or_cls_assignment.py:9:16: PLW0642 Invalid assignment to `cls` argument in class method + | + 7 | cls, blah = "apple", "orange" # PLW0642 + 8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 + 9 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 + | ^^^ PLW0642 +10 | +11 | def print_color(self) -> None: + | + +self_or_cls_assignment.py:12:9: PLW0642 Invalid assignment to `self` argument in instance method + | +11 | def print_color(self) -> None: +12 | self = "red" # PLW0642 + | ^^^^ PLW0642 +13 | self: Self = "red" # PLW0642 +14 | self += "blue" # PLW0642 + | + +self_or_cls_assignment.py:13:9: PLW0642 Invalid assignment to `self` argument in instance method + | +11 | def print_color(self) -> None: +12 | self = "red" # PLW0642 +13 | self: Self = "red" # PLW0642 + | ^^^^ PLW0642 +14 | self += "blue" # PLW0642 +15 | self, blah = "red", "blue" # PLW0642 + | + +self_or_cls_assignment.py:14:9: PLW0642 Invalid assignment to `self` argument in instance method + | +12 | self = "red" # PLW0642 +13 | self: Self = "red" # PLW0642 +14 | self += "blue" # PLW0642 + | ^^^^ PLW0642 +15 | self, blah = "red", "blue" # PLW0642 +16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 + | + +self_or_cls_assignment.py:15:9: PLW0642 Invalid assignment to `self` argument in instance method + | +13 | self: Self = "red" # PLW0642 +14 | self += "blue" # PLW0642 +15 | self, blah = "red", "blue" # PLW0642 + | ^^^^ PLW0642 +16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 +17 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 + | + +self_or_cls_assignment.py:16:16: PLW0642 Invalid assignment to `self` argument in instance method + | +14 | self += "blue" # PLW0642 +15 | self, blah = "red", "blue" # PLW0642 +16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 + | ^^^^ PLW0642 +17 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 + | + +self_or_cls_assignment.py:17:16: PLW0642 Invalid assignment to `self` argument in instance method + | +15 | self, blah = "red", "blue" # PLW0642 +16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 +17 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 + | ^^^^ PLW0642 +18 | +19 | def ok(self) -> None: + | From 512085a26da4c14c650b7179be14dfbf844b612e Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 13 Apr 2024 13:47:01 -0400 Subject: [PATCH 4/5] fix example --- .../src/rules/pylint/rules/self_or_cls_assignment.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs index 95caf2a3be21f1..97d33e8f7f8a45 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs @@ -16,10 +16,12 @@ use crate::checkers::ast::Checker; /// the first argument of instance methods and class methods, respectively. /// /// ## Example +/// /// ```python /// class A: /// def method(self): /// self = 1 +/// /// def class_method(cls): /// cls = 1 /// ``` From 623d980f29b8681918fccc2ae25f0472bd99223c Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Mon, 15 Apr 2024 14:29:25 +0530 Subject: [PATCH 5/5] Check starred expr, update docs and tests --- .../fixtures/pylint/self_or_cls_assignment.py | 10 ++ .../pylint/rules/self_or_cls_assignment.rs | 50 +++--- ...ts__PLW0642_self_or_cls_assignment.py.snap | 149 +++++++++++------- 3 files changed, 135 insertions(+), 74 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/self_or_cls_assignment.py b/crates/ruff_linter/resources/test/fixtures/pylint/self_or_cls_assignment.py index 5932586a52f8b6..999b930ffe7bdf 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/self_or_cls_assignment.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/self_or_cls_assignment.py @@ -4,18 +4,27 @@ def list_fruits(cls) -> None: cls = "apple" # PLW0642 cls: Fruit = "apple" # PLW0642 cls += "orange" # PLW0642 + *cls = "banana" # PLW0642 cls, blah = "apple", "orange" # PLW0642 blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 + @classmethod + def add_fruits(cls, fruits, /) -> None: + cls = fruits # PLW0642 + def print_color(self) -> None: self = "red" # PLW0642 self: Self = "red" # PLW0642 self += "blue" # PLW0642 + *self = "blue" # PLW0642 self, blah = "red", "blue" # PLW0642 blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 + def print_color(self, color, /) -> None: + self = color + def ok(self) -> None: cls = None # OK because the rule looks for the name in the signature @@ -28,6 +37,7 @@ def list_fruits_static(self, cls) -> None: self = "apple" # Ok cls = "banana" # Ok + def list_fruits(self, cls) -> None: self = "apple" # Ok cls = "banana" # Ok diff --git a/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs b/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs index 97d33e8f7f8a45..622c1aaee9eceb 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/self_or_cls_assignment.rs @@ -1,31 +1,41 @@ -use ast::ParameterWithDefault; -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_python_ast::{self as ast, Expr, ParameterWithDefault}; +use ruff_python_semantic::analyze::function_type::{self as function_type, FunctionType}; +use ruff_python_semantic::ScopeKind; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; /// ## What it does -/// Checks for assignment of `self` and `cls` in methods. +/// Checks for assignment of `self` and `cls` in instance and class methods respectively. /// /// ## Why is this bad? -/// The identifiers `self` and `cls` are conventional in Python for -/// the first argument of instance methods and class methods, respectively. +/// The identifiers `self` and `cls` are conventional in Python for the first argument of instance +/// methods and class methods, respectively. /// /// ## Example /// /// ```python -/// class A: -/// def method(self): -/// self = 1 +/// class Versions: +/// def add(self, version): +/// self = version /// -/// def class_method(cls): -/// cls = 1 +/// @classmethod +/// def from_list(cls, versions): +/// cls = versions /// ``` /// +/// Use instead: +/// ```python +/// class Versions: +/// def add(self, version): +/// self.versions.append(version) +/// +/// @classmethod +/// def from_list(cls, versions): +/// return cls(versions) +/// ``` #[violation] pub struct SelfOrClsAssignment { method_type: MethodType, @@ -37,9 +47,8 @@ impl Violation for SelfOrClsAssignment { let SelfOrClsAssignment { method_type } = self; format!( - "Invalid assignment to `{}` argument in {} method", + "Invalid assignment to `{}` argument in {method_type} method", method_type.arg_name(), - method_type.function_type() ) } } @@ -84,8 +93,8 @@ pub(crate) fn self_or_cls_assignment(checker: &mut Checker, target: &Expr) { ); let method_type = match (function_type, self_or_cls.name.as_str()) { - (function_type::FunctionType::Method { .. }, "self") => MethodType::Instance, - (function_type::FunctionType::ClassMethod { .. }, "cls") => MethodType::Class, + (FunctionType::Method { .. }, "self") => MethodType::Instance, + (FunctionType::ClassMethod { .. }, "cls") => MethodType::Class, _ => return, }; @@ -109,6 +118,7 @@ fn check_expr(checker: &mut Checker, target: &Expr, method_type: MethodType) { check_expr(checker, element, method_type); } } + Expr::Starred(ast::ExprStarred { value, .. }) => check_expr(checker, value, method_type), _ => {} } } @@ -126,11 +136,13 @@ impl MethodType { MethodType::Class => "cls", } } +} - fn function_type(self) -> &'static str { +impl std::fmt::Display for MethodType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - MethodType::Instance => "instance", - MethodType::Class => "class", + MethodType::Instance => f.write_str("instance"), + MethodType::Class => f.write_str("class"), } } } diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap index 8886cf66f35811..8cf7aa94b1c6f5 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLW0642_self_or_cls_assignment.py.snap @@ -18,7 +18,7 @@ self_or_cls_assignment.py:5:9: PLW0642 Invalid assignment to `cls` argument in c 5 | cls: Fruit = "apple" # PLW0642 | ^^^ PLW0642 6 | cls += "orange" # PLW0642 -7 | cls, blah = "apple", "orange" # PLW0642 +7 | *cls = "banana" # PLW0642 | self_or_cls_assignment.py:6:9: PLW0642 Invalid assignment to `cls` argument in class method @@ -27,93 +27,132 @@ self_or_cls_assignment.py:6:9: PLW0642 Invalid assignment to `cls` argument in c 5 | cls: Fruit = "apple" # PLW0642 6 | cls += "orange" # PLW0642 | ^^^ PLW0642 -7 | cls, blah = "apple", "orange" # PLW0642 -8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 +7 | *cls = "banana" # PLW0642 +8 | cls, blah = "apple", "orange" # PLW0642 | -self_or_cls_assignment.py:7:9: PLW0642 Invalid assignment to `cls` argument in class method +self_or_cls_assignment.py:7:10: PLW0642 Invalid assignment to `cls` argument in class method | 5 | cls: Fruit = "apple" # PLW0642 6 | cls += "orange" # PLW0642 -7 | cls, blah = "apple", "orange" # PLW0642 - | ^^^ PLW0642 -8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 -9 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 +7 | *cls = "banana" # PLW0642 + | ^^^ PLW0642 +8 | cls, blah = "apple", "orange" # PLW0642 +9 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 | -self_or_cls_assignment.py:8:16: PLW0642 Invalid assignment to `cls` argument in class method - | -6 | cls += "orange" # PLW0642 -7 | cls, blah = "apple", "orange" # PLW0642 -8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 - | ^^^ PLW0642 -9 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 - | +self_or_cls_assignment.py:8:9: PLW0642 Invalid assignment to `cls` argument in class method + | + 6 | cls += "orange" # PLW0642 + 7 | *cls = "banana" # PLW0642 + 8 | cls, blah = "apple", "orange" # PLW0642 + | ^^^ PLW0642 + 9 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 +10 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 + | self_or_cls_assignment.py:9:16: PLW0642 Invalid assignment to `cls` argument in class method | - 7 | cls, blah = "apple", "orange" # PLW0642 - 8 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 - 9 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 + 7 | *cls = "banana" # PLW0642 + 8 | cls, blah = "apple", "orange" # PLW0642 + 9 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 + | ^^^ PLW0642 +10 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 + | + +self_or_cls_assignment.py:10:16: PLW0642 Invalid assignment to `cls` argument in class method + | + 8 | cls, blah = "apple", "orange" # PLW0642 + 9 | blah, (cls, blah2) = "apple", ("orange", "banana") # PLW0642 +10 | blah, [cls, blah2] = "apple", ("orange", "banana") # PLW0642 | ^^^ PLW0642 -10 | -11 | def print_color(self) -> None: +11 | +12 | @classmethod | -self_or_cls_assignment.py:12:9: PLW0642 Invalid assignment to `self` argument in instance method +self_or_cls_assignment.py:14:9: PLW0642 Invalid assignment to `cls` argument in class method | -11 | def print_color(self) -> None: -12 | self = "red" # PLW0642 +12 | @classmethod +13 | def add_fruits(cls, fruits, /) -> None: +14 | cls = fruits # PLW0642 + | ^^^ PLW0642 +15 | +16 | def print_color(self) -> None: + | + +self_or_cls_assignment.py:17:9: PLW0642 Invalid assignment to `self` argument in instance method + | +16 | def print_color(self) -> None: +17 | self = "red" # PLW0642 | ^^^^ PLW0642 -13 | self: Self = "red" # PLW0642 -14 | self += "blue" # PLW0642 +18 | self: Self = "red" # PLW0642 +19 | self += "blue" # PLW0642 | -self_or_cls_assignment.py:13:9: PLW0642 Invalid assignment to `self` argument in instance method +self_or_cls_assignment.py:18:9: PLW0642 Invalid assignment to `self` argument in instance method | -11 | def print_color(self) -> None: -12 | self = "red" # PLW0642 -13 | self: Self = "red" # PLW0642 +16 | def print_color(self) -> None: +17 | self = "red" # PLW0642 +18 | self: Self = "red" # PLW0642 | ^^^^ PLW0642 -14 | self += "blue" # PLW0642 -15 | self, blah = "red", "blue" # PLW0642 +19 | self += "blue" # PLW0642 +20 | *self = "blue" # PLW0642 | -self_or_cls_assignment.py:14:9: PLW0642 Invalid assignment to `self` argument in instance method +self_or_cls_assignment.py:19:9: PLW0642 Invalid assignment to `self` argument in instance method | -12 | self = "red" # PLW0642 -13 | self: Self = "red" # PLW0642 -14 | self += "blue" # PLW0642 +17 | self = "red" # PLW0642 +18 | self: Self = "red" # PLW0642 +19 | self += "blue" # PLW0642 | ^^^^ PLW0642 -15 | self, blah = "red", "blue" # PLW0642 -16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 +20 | *self = "blue" # PLW0642 +21 | self, blah = "red", "blue" # PLW0642 + | + +self_or_cls_assignment.py:20:10: PLW0642 Invalid assignment to `self` argument in instance method + | +18 | self: Self = "red" # PLW0642 +19 | self += "blue" # PLW0642 +20 | *self = "blue" # PLW0642 + | ^^^^ PLW0642 +21 | self, blah = "red", "blue" # PLW0642 +22 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 | -self_or_cls_assignment.py:15:9: PLW0642 Invalid assignment to `self` argument in instance method +self_or_cls_assignment.py:21:9: PLW0642 Invalid assignment to `self` argument in instance method | -13 | self: Self = "red" # PLW0642 -14 | self += "blue" # PLW0642 -15 | self, blah = "red", "blue" # PLW0642 +19 | self += "blue" # PLW0642 +20 | *self = "blue" # PLW0642 +21 | self, blah = "red", "blue" # PLW0642 | ^^^^ PLW0642 -16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 -17 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 +22 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 +23 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 | -self_or_cls_assignment.py:16:16: PLW0642 Invalid assignment to `self` argument in instance method +self_or_cls_assignment.py:22:16: PLW0642 Invalid assignment to `self` argument in instance method | -14 | self += "blue" # PLW0642 -15 | self, blah = "red", "blue" # PLW0642 -16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 +20 | *self = "blue" # PLW0642 +21 | self, blah = "red", "blue" # PLW0642 +22 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 | ^^^^ PLW0642 -17 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 +23 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 | -self_or_cls_assignment.py:17:16: PLW0642 Invalid assignment to `self` argument in instance method +self_or_cls_assignment.py:23:16: PLW0642 Invalid assignment to `self` argument in instance method | -15 | self, blah = "red", "blue" # PLW0642 -16 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 -17 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 +21 | self, blah = "red", "blue" # PLW0642 +22 | blah, (self, blah2) = "apple", ("orange", "banana") # PLW0642 +23 | blah, [self, blah2] = "apple", ("orange", "banana") # PLW0642 | ^^^^ PLW0642 -18 | -19 | def ok(self) -> None: +24 | +25 | def print_color(self, color, /) -> None: + | + +self_or_cls_assignment.py:26:9: PLW0642 Invalid assignment to `self` argument in instance method + | +25 | def print_color(self, color, /) -> None: +26 | self = color + | ^^^^ PLW0642 +27 | +28 | def ok(self) -> None: |