[flake8-bugbear] Ignore enum classes in cached-instance-method (B019) (#11312)

## Summary

While I was here, I also updated the rule to use
`function_type::classify` rather than hard-coding `staticmethod` and
friends.

Per Carl:

> Enum instances are already referred to by the class, forming a cycle
that won't get collected until the class itself does. At which point the
`lru_cache` itself would be collected, too.

Closes https://github.com/astral-sh/ruff/issues/9912.
This commit is contained in:
Charlie Marsh 2024-05-06 14:19:22 -04:00 committed by GitHub
parent a73b8c82a8
commit 12b5c3a54c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 57 additions and 33 deletions

View file

@ -106,3 +106,15 @@ class Foo:
@lru_cache() @lru_cache()
def another_called_lru_cached_instance_method(self, y): def another_called_lru_cached_instance_method(self, y):
... ...
import enum
class Foo(enum.Enum):
ONE = enum.auto()
TWO = enum.auto()
@functools.cache
def bar(self, arg: str) -> str:
return f"{self} - {arg}"

View file

@ -203,7 +203,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
} }
} }
if checker.enabled(Rule::CachedInstanceMethod) { if checker.enabled(Rule::CachedInstanceMethod) {
flake8_bugbear::rules::cached_instance_method(checker, decorator_list); flake8_bugbear::rules::cached_instance_method(checker, function_def);
} }
if checker.enabled(Rule::MutableArgumentDefault) { if checker.enabled(Rule::MutableArgumentDefault) {
flake8_bugbear::rules::mutable_argument_default(checker, function_def); flake8_bugbear::rules::mutable_argument_default(checker, function_def);

View file

@ -1,8 +1,9 @@
use ruff_python_ast::{self as ast, Decorator, Expr};
use ruff_diagnostics::{Diagnostic, Violation}; use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation}; use ruff_macros::{derive_message_formats, violation};
use ruff_python_semantic::SemanticModel; use ruff_python_ast::helpers::map_callable;
use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::analyze::{class, function_type};
use ruff_python_semantic::{ScopeKind, SemanticModel};
use ruff_text_size::Ranged; use ruff_text_size::Ranged;
use crate::checkers::ast::Checker; use crate::checkers::ast::Checker;
@ -20,6 +21,9 @@ use crate::checkers::ast::Checker;
/// instance of the class, or use the `@lru_cache` decorator on a function /// instance of the class, or use the `@lru_cache` decorator on a function
/// outside of the class. /// outside of the class.
/// ///
/// This rule ignores instance methods on enumeration classes, as enum members
/// are singletons.
///
/// ## Example /// ## Example
/// ```python /// ```python
/// from functools import lru_cache /// from functools import lru_cache
@ -70,6 +74,43 @@ impl Violation for CachedInstanceMethod {
} }
} }
/// B019
pub(crate) fn cached_instance_method(checker: &mut Checker, function_def: &ast::StmtFunctionDef) {
let scope = checker.semantic().current_scope();
// Parent scope _must_ be a class.
let ScopeKind::Class(class_def) = scope.kind else {
return;
};
// The function must be an _instance_ method.
let type_ = function_type::classify(
&function_def.name,
&function_def.decorator_list,
scope,
checker.semantic(),
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
);
if !matches!(type_, function_type::FunctionType::Method) {
return;
}
for decorator in &function_def.decorator_list {
if is_cache_func(map_callable(&decorator.expression), checker.semantic()) {
// If we found a cached instance method, validate (lazily) that the class is not an enum.
if class::is_enumeration(class_def, checker.semantic()) {
return;
}
checker
.diagnostics
.push(Diagnostic::new(CachedInstanceMethod, decorator.range()));
}
}
}
/// Returns `true` if the given expression is a call to `functools.lru_cache` or `functools.cache`.
fn is_cache_func(expr: &Expr, semantic: &SemanticModel) -> bool { fn is_cache_func(expr: &Expr, semantic: &SemanticModel) -> bool {
semantic semantic
.resolve_qualified_name(expr) .resolve_qualified_name(expr)
@ -80,32 +121,3 @@ fn is_cache_func(expr: &Expr, semantic: &SemanticModel) -> bool {
) )
}) })
} }
/// B019
pub(crate) fn cached_instance_method(checker: &mut Checker, decorator_list: &[Decorator]) {
if !checker.semantic().current_scope().kind.is_class() {
return;
}
for decorator in decorator_list {
// TODO(charlie): This should take into account `classmethod-decorators` and
// `staticmethod-decorators`.
if let Expr::Name(ast::ExprName { id, .. }) = &decorator.expression {
if id == "classmethod" || id == "staticmethod" {
return;
}
}
}
for decorator in decorator_list {
if is_cache_func(
match &decorator.expression {
Expr::Call(ast::ExprCall { func, .. }) => func,
_ => &decorator.expression,
},
checker.semantic(),
) {
checker
.diagnostics
.push(Diagnostic::new(CachedInstanceMethod, decorator.range()));
}
}
}