From d361a836b7e6acb2bcad110e784fedc01b6db6ca Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Thu, 12 Oct 2023 23:14:40 -0300 Subject: [PATCH 1/2] Fix false positive in `PLR6301` Don't report a diagnostic if the method contains a `super()` call. --- .../test/fixtures/pylint/no_self_use.py | 23 +++++++++++ .../src/rules/pylint/rules/no_self_use.rs | 30 ++++++++++++++ ...pylint__tests__PLR6301_no_self_use.py.snap | 39 +++++++------------ 3 files changed, 68 insertions(+), 24 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pylint/no_self_use.py b/crates/ruff_linter/resources/test/fixtures/pylint/no_self_use.py index e1cb0c3bda711..db74e179c71c1 100644 --- a/crates/ruff_linter/resources/test/fixtures/pylint/no_self_use.py +++ b/crates/ruff_linter/resources/test/fixtures/pylint/no_self_use.py @@ -1,5 +1,7 @@ import abc +from typing_extensions import override + class Person: def developer_greeting(self, name): # [no-self-use] @@ -60,3 +62,24 @@ class Prop: @property def count(self): return 24 + + +class A: + def foo(self): + ... + + +class B(A): + @override + def foo(self): + ... + + def foobar(self): + super() + + def bar(self): + super().foo() + + def baz(self): + if super().foo(): + ... diff --git a/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs b/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs index 1b55f3a8349d4..9cb1b4d9277a4 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs @@ -1,3 +1,5 @@ +use ast::visitor::{self, Visitor}; +use ast::Expr; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::call_path::{from_qualified_name, CallPath}; @@ -105,6 +107,14 @@ pub(crate) fn no_self_use(checker: &Checker, scope: &Scope, diagnostics: &mut Ve return; }; + // Traverse the method's body looking for `super()` calls + let mut call_visitor = CallVisitor { is_super: false }; + call_visitor.visit_body(body); + + if call_visitor.is_super { + return; + } + if parameter.name.as_str() == "self" && scope .get("self") @@ -119,3 +129,23 @@ pub(crate) fn no_self_use(checker: &Checker, scope: &Scope, diagnostics: &mut Ve )); } } + +struct CallVisitor { + is_super: bool, +} + +impl<'a> Visitor<'a> for CallVisitor { + fn visit_expr(&mut self, expr: &'a Expr) { + match expr { + Expr::Call(ast::ExprCall { func, .. }) => match func.as_ref() { + Expr::Name(expr_name) => { + self.is_super = expr_name.id == "super"; + } + _ => { + visitor::walk_expr(self, expr); + } + }, + _ => visitor::walk_expr(self, expr), + } + } +} diff --git a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR6301_no_self_use.py.snap b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR6301_no_self_use.py.snap index 405976321a990..3b45a59ad686a 100644 --- a/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR6301_no_self_use.py.snap +++ b/crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__PLR6301_no_self_use.py.snap @@ -1,39 +1,30 @@ --- source: crates/ruff_linter/src/rules/pylint/mod.rs --- -no_self_use.py:5:28: PLR6301 Method `developer_greeting` could be a function or static method +no_self_use.py:7:28: PLR6301 Method `developer_greeting` could be a function or static method | -4 | class Person: -5 | def developer_greeting(self, name): # [no-self-use] +6 | class Person: +7 | def developer_greeting(self, name): # [no-self-use] | ^^^^ PLR6301 -6 | print(f"Greetings {name}!") +8 | 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 +no_self_use.py:10:20: PLR6301 Method `greeting_1` could be a function or static method | - 9 | print("Hello!") -10 | -11 | def greeting_2(self): # [no-self-use] + 8 | print(f"Greetings {name}!") + 9 | +10 | def greeting_1(self): # [no-self-use] | ^^^^ PLR6301 -12 | print("Hi!") +11 | print("Hello!") | -no_self_use.py:55:25: PLR6301 Method `abstract_method` could be a function or static method +no_self_use.py:13:20: PLR6301 Method `greeting_2` could be a function or static method | -53 | class Sub(Base): -54 | @override -55 | def abstract_method(self): - | ^^^^ PLR6301 -56 | print("concrete method") +11 | print("Hello!") +12 | +13 | def greeting_2(self): # [no-self-use] + | ^^^^ PLR6301 +14 | print("Hi!") | From fe5f699a72c62f3c03aa57a045a72c2185e5ee30 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 13 Oct 2023 15:48:01 -0400 Subject: [PATCH 2/2] Use bindings --- .../checkers/ast/analyze/deferred_scopes.rs | 2 +- .../src/rules/pylint/rules/no_self_use.rs | 60 ++++++++----------- 2 files changed, 27 insertions(+), 35 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index 493186ec444ec..e3b18d0685a27 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -306,7 +306,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { if scope.kind.is_function() { if checker.enabled(Rule::NoSelfUse) { - pylint::rules::no_self_use(checker, scope, &mut diagnostics); + pylint::rules::no_self_use(checker, scope_id, scope, &mut diagnostics); } } } diff --git a/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs b/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs index 9cb1b4d9277a4..1be5e7d6372f0 100644 --- a/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs +++ b/crates/ruff_linter/src/rules/pylint/rules/no_self_use.rs @@ -1,12 +1,10 @@ -use ast::visitor::{self, Visitor}; -use ast::Expr; 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}; use ruff_python_semantic::{ analyze::{function_type, visibility}, - Scope, ScopeKind, + Scope, ScopeId, ScopeKind, }; use ruff_text_size::Ranged; @@ -47,7 +45,12 @@ impl Violation for NoSelfUse { } /// PLR6301 -pub(crate) fn no_self_use(checker: &Checker, scope: &Scope, diagnostics: &mut Vec) { +pub(crate) fn no_self_use( + checker: &Checker, + scope_id: ScopeId, + scope: &Scope, + diagnostics: &mut Vec, +) { let Some(parent) = &checker.semantic().first_non_type_parent_scope(scope) else { return; }; @@ -107,19 +110,28 @@ pub(crate) fn no_self_use(checker: &Checker, scope: &Scope, diagnostics: &mut Ve return; }; - // Traverse the method's body looking for `super()` calls - let mut call_visitor = CallVisitor { is_super: false }; - call_visitor.visit_body(body); - - if call_visitor.is_super { + if parameter.name.as_str() != "self" { 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()) + // If the method contains a `super` reference, then it should be considered to use self + // implicitly. + if let Some(binding_id) = checker.semantic().global_scope().get("super") { + let binding = checker.semantic().binding(binding_id); + if binding.kind.is_builtin() { + if binding + .references() + .any(|id| checker.semantic().reference(id).scope_id() == scope_id) + { + return; + } + } + } + + if 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 { @@ -129,23 +141,3 @@ pub(crate) fn no_self_use(checker: &Checker, scope: &Scope, diagnostics: &mut Ve )); } } - -struct CallVisitor { - is_super: bool, -} - -impl<'a> Visitor<'a> for CallVisitor { - fn visit_expr(&mut self, expr: &'a Expr) { - match expr { - Expr::Call(ast::ExprCall { func, .. }) => match func.as_ref() { - Expr::Name(expr_name) => { - self.is_super = expr_name.id == "super"; - } - _ => { - visitor::walk_expr(self, expr); - } - }, - _ => visitor::walk_expr(self, expr), - } - } -}