From 3601464919f57ec13cdedd1cf839066bb473d861 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 8 Feb 2023 21:09:46 -0500 Subject: [PATCH] Support callable decorators in classmethod_decorators et al --- crates/ruff/src/ast/function_type.rs | 18 ++++---- crates/ruff/src/ast/helpers.rs | 10 +++++ .../flake8_pytest_style/rules/helpers.rs | 22 +++------- crates/ruff/src/rules/pep8_naming/mod.rs | 20 +++++++++ ...naming__tests__classmethod_decorators.snap | 35 ++++++++++++++++ crates/ruff/src/visibility.rs | 42 +++++++++++-------- 6 files changed, 105 insertions(+), 42 deletions(-) create mode 100644 crates/ruff/src/rules/pep8_naming/snapshots/ruff__rules__pep8_naming__tests__classmethod_decorators.snap diff --git a/crates/ruff/src/ast/function_type.rs b/crates/ruff/src/ast/function_type.rs index 59e988abb9dd13..2c79610c7429ab 100644 --- a/crates/ruff/src/ast/function_type.rs +++ b/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,18 +29,20 @@ 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]) @@ -48,7 +50,7 @@ pub fn classify( }) || 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)) diff --git a/crates/ruff/src/ast/helpers.rs b/crates/ruff/src/ast/helpers.rs index 9d64269c258bd0..24027438fb6fb7 100644 --- a/crates/ruff/src/ast/helpers.rs +++ b/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(located: &Located, locator: &Locator) -> bool { let start = if match_leading_content(located, locator) { diff --git a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs b/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs index 127e8ead33df9c..c629c97415dcc4 100644 --- a/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs +++ b/crates/ruff/src/rules/flake8_pytest_style/rules/helpers.rs @@ -1,21 +1,11 @@ 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 { decorators .iter() @@ -23,9 +13,7 @@ pub fn get_mark_decorators(decorators: &[Expr]) -> impl Iterator { } 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"] }) diff --git a/crates/ruff/src/rules/pep8_naming/mod.rs b/crates/ruff/src/rules/pep8_naming/mod.rs index ed976ad072e0eb..74143ab716143b 100644 --- a/crates/ruff/src/rules/pep8_naming/mod.rs +++ b/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(()) + } } diff --git a/crates/ruff/src/rules/pep8_naming/snapshots/ruff__rules__pep8_naming__tests__classmethod_decorators.snap b/crates/ruff/src/rules/pep8_naming/snapshots/ruff__rules__pep8_naming__tests__classmethod_decorators.snap new file mode 100644 index 00000000000000..85f1aa926a00a7 --- /dev/null +++ b/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: ~ + diff --git a/crates/ruff/src/visibility.rs b/crates/ruff/src/visibility.rs index 70b24a4f509717..d0f2d11ef4f384 100644 --- a/crates/ruff/src/visibility.rs +++ b/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,18 +31,22 @@ 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"] + }) }) } @@ -50,33 +54,37 @@ pub fn is_classmethod(checker: &Checker, decorator_list: &[Expr]) -> bool { 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".