Skip to content

Commit

Permalink
[flake8-bugbear] Ignore enum classes in cached-instance-method (`…
Browse files Browse the repository at this point in the history
…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 #9912.
  • Loading branch information
charliermarsh committed May 6, 2024
1 parent a73b8c8 commit 12b5c3a
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 32 deletions.
12 changes: 12 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B019.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,3 +106,15 @@ def called_lru_cached_instance_method(self, y):
@lru_cache()
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}"
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
}
}
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) {
flake8_bugbear::rules::mutable_argument_default(checker, function_def);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use ruff_python_ast::{self as ast, Decorator, Expr};

use ruff_diagnostics::{Diagnostic, 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 crate::checkers::ast::Checker;
Expand All @@ -20,6 +21,9 @@ use crate::checkers::ast::Checker;
/// instance of the class, or use the `@lru_cache` decorator on a function
/// outside of the class.
///
/// This rule ignores instance methods on enumeration classes, as enum members
/// are singletons.
///
/// ## Example
/// ```python
/// from functools import lru_cache
Expand Down Expand Up @@ -70,42 +74,50 @@ impl Violation for CachedInstanceMethod {
}
}

fn is_cache_func(expr: &Expr, semantic: &SemanticModel) -> bool {
semantic
.resolve_qualified_name(expr)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["functools", "lru_cache" | "cache"]
)
})
}

/// B019
pub(crate) fn cached_instance_method(checker: &mut Checker, decorator_list: &[Decorator]) {
if !checker.semantic().current_scope().kind.is_class() {
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 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" {

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;
}
}
}
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()));
}
}
}

/// 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 {
semantic
.resolve_qualified_name(expr)
.is_some_and(|qualified_name| {
matches!(
qualified_name.segments(),
["functools", "lru_cache" | "cache"]
)
})
}

0 comments on commit 12b5c3a

Please sign in to comment.