From 12b5c3a54c0a34bae9dfcfeaf27961f4432d5623 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 6 May 2024 14:19:22 -0400 Subject: [PATCH] [`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. --- .../test/fixtures/flake8_bugbear/B019.py | 12 +++ .../src/checkers/ast/analyze/statement.rs | 2 +- .../rules/cached_instance_method.rs | 74 +++++++++++-------- 3 files changed, 56 insertions(+), 32 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 55d32aa1bad6e..bbca28a563da2 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B019.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B019.py @@ -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}" diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index a046261725621..fcdc1af2c3828 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -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); diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/cached_instance_method.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/cached_instance_method.rs index 016bc6bac5412..0ebdec98d4a37 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/cached_instance_method.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/cached_instance_method.rs @@ -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; @@ -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 @@ -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"] + ) + }) +}