From 7a4419a2a52b7f9e4eda4afb4bd1fe770d59c0ae Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Tue, 30 Jul 2024 14:48:36 +0100 Subject: [PATCH] Improve handling of metaclasses in various linter rules (#12579) --- .../test/fixtures/flake8_bugbear/B019.py | 6 ++++++ ...__flake8_bugbear__tests__B019_B019.py.snap | 9 +++++++- .../flake8_pyi/rules/non_self_return_type.rs | 12 +---------- .../rules/invalid_first_argument_name.rs | 21 +++++++++++++++---- .../ruff_python_semantic/src/analyze/class.rs | 10 +++++++++ .../src/analyze/function_type.rs | 17 ++++----------- 6 files changed, 46 insertions(+), 29 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B019.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B019.py index bbca28a563da2..e8a5a50f3305d 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B019.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B019.py @@ -118,3 +118,9 @@ class Foo(enum.Enum): @functools.cache def bar(self, arg: str) -> str: return f"{self} - {arg}" + + +class Metaclass(type): + @functools.lru_cache + def lru_cached_instance_method_on_metaclass(cls, x: int): + ... diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B019_B019.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B019_B019.py.snap index 76541860c05f5..907178352a1ad 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B019_B019.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B019_B019.py.snap @@ -80,4 +80,11 @@ B019.py:106:5: B019 Use of `functools.lru_cache` or `functools.cache` on methods 108 | ... | - +B019.py:124:5: B019 Use of `functools.lru_cache` or `functools.cache` on methods can lead to memory leaks + | +123 | class Metaclass(type): +124 | @functools.lru_cache + | ^^^^^^^^^^^^^^^^^^^^ B019 +125 | def lru_cached_instance_method_on_metaclass(cls, x: int): +126 | ... + | diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs index d2a28ae2e8d1c..b02cb555b5cc7 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs @@ -135,7 +135,7 @@ pub(crate) fn non_self_return_type( }; // PEP 673 forbids the use of `typing(_extensions).Self` in metaclasses. - if is_metaclass(class_def, semantic) { + if analyze::class::is_metaclass(class_def, semantic) { return; } @@ -219,16 +219,6 @@ pub(crate) fn non_self_return_type( } } -/// Returns `true` if the given class is a metaclass. -fn is_metaclass(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { - analyze::class::any_qualified_name(class_def, semantic, &|qualified_name| { - matches!( - qualified_name.segments(), - ["" | "builtins", "type"] | ["abc", "ABCMeta"] | ["enum", "EnumMeta" | "EnumType"] - ) - }) -} - /// Returns `true` if the method is an in-place binary operator. fn is_inplace_bin_op(name: &str) -> bool { matches!( diff --git a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs index 999bb151a172b..c43f2fafe59f3 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs +++ b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs @@ -5,6 +5,7 @@ use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::ParameterWithDefault; use ruff_python_codegen::Stylist; +use ruff_python_semantic::analyze::class::is_metaclass; use ruff_python_semantic::analyze::function_type; use ruff_python_semantic::{Scope, ScopeKind, SemanticModel}; use ruff_text_size::Ranged; @@ -190,22 +191,34 @@ pub(crate) fn invalid_first_argument_name( panic!("Expected ScopeKind::Function") }; - let Some(parent) = checker.semantic().first_non_type_parent_scope(scope) else { + let semantic = checker.semantic(); + + let Some(parent_scope) = semantic.first_non_type_parent_scope(scope) else { + return; + }; + + let ScopeKind::Class(parent) = parent_scope.kind else { return; }; let function_type = match function_type::classify( name, decorator_list, - parent, - checker.semantic(), + parent_scope, + semantic, &checker.settings.pep8_naming.classmethod_decorators, &checker.settings.pep8_naming.staticmethod_decorators, ) { function_type::FunctionType::Function | function_type::FunctionType::StaticMethod => { return; } - function_type::FunctionType::Method => FunctionType::Method, + function_type::FunctionType::Method => { + if is_metaclass(parent, semantic) { + FunctionType::ClassMethod + } else { + FunctionType::Method + } + } function_type::FunctionType::ClassMethod => FunctionType::ClassMethod, }; if !checker.enabled(function_type.rule()) { diff --git a/crates/ruff_python_semantic/src/analyze/class.rs b/crates/ruff_python_semantic/src/analyze/class.rs index 44aa216d07da8..4ea0d3cb08094 100644 --- a/crates/ruff_python_semantic/src/analyze/class.rs +++ b/crates/ruff_python_semantic/src/analyze/class.rs @@ -110,3 +110,13 @@ pub fn is_enumeration(class_def: &ast::StmtClassDef, semantic: &SemanticModel) - ) }) } + +/// Returns `true` if the given class is a metaclass. +pub fn is_metaclass(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { + any_qualified_name(class_def, semantic, &|qualified_name| { + matches!( + qualified_name.segments(), + ["" | "builtins", "type"] | ["abc", "ABCMeta"] | ["enum", "EnumMeta" | "EnumType"] + ) + }) +} diff --git a/crates/ruff_python_semantic/src/analyze/function_type.rs b/crates/ruff_python_semantic/src/analyze/function_type.rs index a13881df15108..a9ba29c0e5128 100644 --- a/crates/ruff_python_semantic/src/analyze/function_type.rs +++ b/crates/ruff_python_semantic/src/analyze/function_type.rs @@ -3,7 +3,7 @@ use ruff_python_ast::name::{QualifiedName, UnqualifiedName}; use ruff_python_ast::{Decorator, Expr, Stmt, StmtExpr, StmtFunctionDef, StmtRaise}; use crate::model::SemanticModel; -use crate::scope::{Scope, ScopeKind}; +use crate::scope::Scope; #[derive(Debug, Copy, Clone)] pub enum FunctionType { @@ -17,12 +17,12 @@ pub enum FunctionType { pub fn classify( name: &str, decorator_list: &[Decorator], - scope: &Scope, + parent_scope: &Scope, semantic: &SemanticModel, classmethod_decorators: &[String], staticmethod_decorators: &[String], ) -> FunctionType { - let ScopeKind::Class(class_def) = &scope.kind else { + if !parent_scope.kind.is_class() { return FunctionType::Function; }; if decorator_list @@ -30,16 +30,7 @@ pub fn classify( .any(|decorator| is_static_method(decorator, semantic, staticmethod_decorators)) { FunctionType::StaticMethod - } else if matches!(name, "__new__" | "__init_subclass__" | "__class_getitem__") - // Special-case class method, like `__new__`. - || class_def.bases().iter().any(|expr| { - // The class itself extends a known metaclass, so all methods are class methods. - semantic - .resolve_qualified_name(map_callable(expr)) - .is_some_and( |qualified_name| { - matches!(qualified_name.segments(), ["" | "builtins", "type"] | ["abc", "ABCMeta"]) - }) - }) + } else if matches!(name, "__new__" | "__init_subclass__" | "__class_getitem__") // Special-case class method, like `__new__`. || decorator_list.iter().any(|decorator| is_class_method(decorator, semantic, classmethod_decorators)) { FunctionType::ClassMethod