Support callable decorators in classmethod_decorators et al

crates/ruff/src/ast/function_type.rs

@@ -1,6 +1,6 @@
 use rustpython_parser::ast::Expr;
 
-use crate::ast::helpers::to_call_path;
+use crate::ast::helpers::{map_callable, to_call_path};
 use crate::ast::types::{Scope, ScopeKind};
 use crate::checkers::ast::Checker;
 
@@ -29,26 +29,28 @@ pub fn classify(
     if decorator_list.iter().any(|expr| {
         // The method is decorated with a static method decorator (like
         // `@staticmethod`).
-        checker.resolve_call_path(expr).map_or(false, |call_path| {
-            staticmethod_decorators
-                .iter()
-                .any(|decorator| call_path == to_call_path(decorator))
-        })
+        checker
+            .resolve_call_path(map_callable(expr))
+            .map_or(false, |call_path| {
+                staticmethod_decorators
+                    .iter()
+                    .any(|decorator| call_path == to_call_path(decorator))
+            })
     }) {
         FunctionType::StaticMethod
     } else if CLASS_METHODS.contains(&name)
         // Special-case class method, like `__new__`.
         || scope.bases.iter().any(|expr| {
             // The class itself extends a known metaclass, so all methods are class methods.
-            checker.resolve_call_path(expr).map_or(false, |call_path| {
+            checker.resolve_call_path(map_callable(expr)).map_or(false, |call_path| {
                 METACLASS_BASES
                     .iter()
                     .any(|(module, member)| call_path.as_slice() == [*module, *member])
             })
         })
         || decorator_list.iter().any(|expr| {
             // The method is decorated with a class method decorator (like `@classmethod`).
-            checker.resolve_call_path(expr).map_or(false, |call_path| {
+            checker.resolve_call_path(map_callable(expr)).map_or(false, |call_path| {
                 classmethod_decorators
                     .iter()
                     .any(|decorator| call_path == to_call_path(decorator))

crates/ruff/src/ast/helpers.rs

@@ -568,6 +568,16 @@ pub fn collect_arg_names<'a>(arguments: &'a Arguments) -> FxHashSet<&'a str> {
     arg_names
 }
 
+/// Given an [`Expr`] that can be callable or not (like a decorator, which could be used with or
+/// without explicit call syntax), return the underlying callable.
+pub fn map_callable(decorator: &Expr) -> &Expr {
+    if let ExprKind::Call { func, .. } = &decorator.node {
+        func
+    } else {
+        decorator
+    }
+}
+
 /// Returns `true` if a statement or expression includes at least one comment.
 pub fn has_comments<T>(located: &Located<T>, locator: &Locator) -> bool {
     let start = if match_leading_content(located, locator) {

crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs

@@ -1,31 +1,19 @@
 use num_traits::identities::Zero;
 use rustpython_parser::ast::{Constant, Expr, ExprKind, Keyword};
 
-use crate::ast::helpers::collect_call_path;
+use crate::ast::helpers::{collect_call_path, map_callable};
 use crate::checkers::ast::Checker;
 
 const ITERABLE_INITIALIZERS: &[&str] = &["dict", "frozenset", "list", "tuple", "set"];
 
-/// Given a decorators that can be used with or without explicit call syntax, return
-/// the underlying callable.
-fn callable_decorator(decorator: &Expr) -> &Expr {
-    if let ExprKind::Call { func, .. } = &decorator.node {
-        func
-    } else {
-        decorator
-    }
-}
-
 pub fn get_mark_decorators(decorators: &[Expr]) -> impl Iterator<Item = &Expr> {
     decorators
         .iter()
         .filter(|decorator| is_pytest_mark(decorator))
 }
 
 pub fn get_mark_name(decorator: &Expr) -> &str {
-    collect_call_path(callable_decorator(decorator))
-        .last()
-        .unwrap()
+    collect_call_path(map_callable(decorator)).last().unwrap()
 }
 
 pub fn is_pytest_fail(call: &Expr, checker: &Checker) -> bool {
@@ -47,7 +35,7 @@ pub fn is_pytest_fixture(decorator: &Expr, checker: &Checker) -> bool {
 }
 
 pub fn is_pytest_mark(decorator: &Expr) -> bool {
-    let segments = collect_call_path(callable_decorator(decorator));
+    let segments = collect_call_path(map_callable(decorator));
     if segments.len() > 2 {
         segments[0] == "pytest" && segments[1] == "mark"
     } else {
@@ -57,7 +45,7 @@ pub fn is_pytest_mark(decorator: &Expr) -> bool {
 
 pub fn is_pytest_yield_fixture(decorator: &Expr, checker: &Checker) -> bool {
     checker
-        .resolve_call_path(callable_decorator(decorator))
+        .resolve_call_path(map_callable(decorator))
         .map_or(false, |call_path| {
             call_path.as_slice() == ["pytest", "yield_fixture"]
         })
@@ -115,7 +103,7 @@ pub fn is_falsy_constant(expr: &Expr) -> bool {
 
 pub fn is_pytest_parametrize(decorator: &Expr, checker: &Checker) -> bool {
     checker
-        .resolve_call_path(callable_decorator(decorator))
+        .resolve_call_path(map_callable(decorator))
         .map_or(false, |call_path| {
             call_path.as_slice() == ["pytest", "mark", "parametrize"]
         })

crates/ruff/src/rules/pep8_naming/mod.rs

@@ -11,6 +11,7 @@ mod tests {
     use test_case::test_case;
 
     use crate::registry::Rule;
+    use crate::rules::pep8_naming;
     use crate::test::test_path;
     use crate::{assert_yaml_snapshot, settings};
 
@@ -38,4 +39,23 @@ mod tests {
         assert_yaml_snapshot!(snapshot, diagnostics);
         Ok(())
     }
+
+    #[test]
+    fn classmethod_decorators() -> Result<()> {
+        let diagnostics = test_path(
+            Path::new("pep8_naming").join("N805.py").as_path(),
+            &settings::Settings {
+                pep8_naming: pep8_naming::settings::Settings {
+                    classmethod_decorators: vec![
+                        "classmethod".to_string(),
+                        "pydantic.validator".to_string(),
+                    ],
+                    ..Default::default()
+                },
+                ..settings::Settings::for_rule(Rule::InvalidFirstArgumentNameForMethod)
+            },
+        )?;
+        assert_yaml_snapshot!(diagnostics);
+        Ok(())
+    }
 }

crates/ruff/src/rules/pep8_naming/snapshots/ruff__rules__pep8_naming__tests__classmethod_decorators.snap

@@ -0,0 +1,35 @@
+---
+source: crates/ruff/src/rules/pep8_naming/mod.rs
+expression: diagnostics
+---
+- kind:
+    InvalidFirstArgumentNameForMethod: ~
+  location:
+    row: 7
+    column: 19
+  end_location:
+    row: 7
+    column: 23
+  fix: ~
+  parent: ~
+- kind:
+    InvalidFirstArgumentNameForMethod: ~
+  location:
+    row: 12
+    column: 29
+  end_location:
+    row: 12
+    column: 33
+  fix: ~
+  parent: ~
+- kind:
+    InvalidFirstArgumentNameForMethod: ~
+  location:
+    row: 60
+    column: 28
+  end_location:
+    row: 60
+    column: 32
+  fix: ~
+  parent: ~
+

crates/ruff/src/visibility.rs

@@ -5,7 +5,7 @@ use std::path::Path;
 
 use rustpython_parser::ast::{Expr, Stmt, StmtKind};
 
-use crate::ast::helpers::collect_call_path;
+use crate::ast::helpers::{collect_call_path, map_callable};
 use crate::checkers::ast::Checker;
 use crate::docstrings::definition::Documentable;
 
@@ -31,52 +31,60 @@ pub struct VisibleScope {
 /// Returns `true` if a function is a "static method".
 pub fn is_staticmethod(checker: &Checker, decorator_list: &[Expr]) -> bool {
     decorator_list.iter().any(|expr| {
-        checker.resolve_call_path(expr).map_or(false, |call_path| {
-            call_path.as_slice() == ["", "staticmethod"]
-        })
+        checker
+            .resolve_call_path(map_callable(expr))
+            .map_or(false, |call_path| {
+                call_path.as_slice() == ["", "staticmethod"]
+            })
     })
 }
 
 /// Returns `true` if a function is a "class method".
 pub fn is_classmethod(checker: &Checker, decorator_list: &[Expr]) -> bool {
     decorator_list.iter().any(|expr| {
-        checker.resolve_call_path(expr).map_or(false, |call_path| {
-            call_path.as_slice() == ["", "classmethod"]
-        })
+        checker
+            .resolve_call_path(map_callable(expr))
+            .map_or(false, |call_path| {
+                call_path.as_slice() == ["", "classmethod"]
+            })
     })
 }
 
 /// Returns `true` if a function definition is an `@overload`.
 pub fn is_overload(checker: &Checker, decorator_list: &[Expr]) -> bool {
     decorator_list
         .iter()
-        .any(|expr| checker.match_typing_expr(expr, "overload"))
+        .any(|expr| checker.match_typing_expr(map_callable(expr), "overload"))
 }
 
 /// Returns `true` if a function definition is an `@override` (PEP 698).
 pub fn is_override(checker: &Checker, decorator_list: &[Expr]) -> bool {
     decorator_list
         .iter()
-        .any(|expr| checker.match_typing_expr(expr, "override"))
+        .any(|expr| checker.match_typing_expr(map_callable(expr), "override"))
 }
 
 /// Returns `true` if a function definition is an `@abstractmethod`.
 pub fn is_abstract(checker: &Checker, decorator_list: &[Expr]) -> bool {
     decorator_list.iter().any(|expr| {
-        checker.resolve_call_path(expr).map_or(false, |call_path| {
-            call_path.as_slice() == ["abc", "abstractmethod"]
-                || call_path.as_slice() == ["abc", "abstractproperty"]
-        })
+        checker
+            .resolve_call_path(map_callable(expr))
+            .map_or(false, |call_path| {
+                call_path.as_slice() == ["abc", "abstractmethod"]
+                    || call_path.as_slice() == ["abc", "abstractproperty"]
+            })
     })
 }
 
 /// Returns `true` if a function definition is a `@property`.
 pub fn is_property(checker: &Checker, decorator_list: &[Expr]) -> bool {
     decorator_list.iter().any(|expr| {
-        checker.resolve_call_path(expr).map_or(false, |call_path| {
-            call_path.as_slice() == ["", "property"]
-                || call_path.as_slice() == ["functools", "cached_property"]
-        })
+        checker
+            .resolve_call_path(map_callable(expr))
+            .map_or(false, |call_path| {
+                call_path.as_slice() == ["", "property"]
+                    || call_path.as_slice() == ["functools", "cached_property"]
+            })
     })
 }
 /// Returns `true` if a function is a "magic method".