From c80e0b2dc1d10d25f338cf54887f0cdd6942c460 Mon Sep 17 00:00:00 2001 From: Adrian Moennich Date: Fri, 10 Nov 2023 00:13:21 +0100 Subject: [PATCH 1/2] Fix {class,static}method-decorators for dotted names --- .../test/fixtures/pep8_naming/N805.py | 30 ++++++++ .../ruff_linter/src/rules/pep8_naming/mod.rs | 20 ++++++ ...les__pep8_naming__tests__N805_N805.py.snap | 33 +++++++++ ...naming__tests__classmethod_decorators.snap | 25 +++++++ ...aming__tests__staticmethod_decorators.snap | 71 +++++++++++++++++++ .../src/analyze/function_type.rs | 44 +++++++++--- crates/ruff_workspace/src/options.rs | 6 +- ruff.schema.json | 4 +- 8 files changed, 221 insertions(+), 12 deletions(-) create mode 100644 crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__staticmethod_decorators.snap diff --git a/crates/ruff_linter/resources/test/fixtures/pep8_naming/N805.py b/crates/ruff_linter/resources/test/fixtures/pep8_naming/N805.py index 99fcf2ed0c9a8..395da9c81a8c0 100644 --- a/crates/ruff_linter/resources/test/fixtures/pep8_naming/N805.py +++ b/crates/ruff_linter/resources/test/fixtures/pep8_naming/N805.py @@ -63,3 +63,33 @@ def good_method_pos_only(self, blah, /, something: str): def bad_method_pos_only(this, blah, /, self, something: str): pass + + +class ModelClass: + @hybrid_property + def bad(cls): + pass + + @bad.expression + def bad(self): + pass + + @bad.wtf + def bad(cls): + pass + + @hybrid_property + def good(self): + pass + + @good.expression + def good(cls): + pass + + @good.wtf + def good(self): + pass + + @foobar.thisisstatic + def badstatic(foo): + pass diff --git a/crates/ruff_linter/src/rules/pep8_naming/mod.rs b/crates/ruff_linter/src/rules/pep8_naming/mod.rs index 49ab25abfc2c9..ff73c6d1ebbcf 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/mod.rs +++ b/crates/ruff_linter/src/rules/pep8_naming/mod.rs @@ -96,6 +96,26 @@ mod tests { classmethod_decorators: vec![ "classmethod".to_string(), "pydantic.validator".to_string(), + "expression".to_string(), + ], + ..Default::default() + }, + ..settings::LinterSettings::for_rule(Rule::InvalidFirstArgumentNameForMethod) + }, + )?; + assert_messages!(diagnostics); + Ok(()) + } + + #[test] + fn staticmethod_decorators() -> Result<()> { + let diagnostics = test_path( + Path::new("pep8_naming").join("N805.py").as_path(), + &settings::LinterSettings { + pep8_naming: pep8_naming::settings::Settings { + staticmethod_decorators: vec![ + "staticmethod".to_string(), + "thisisstatic".to_string(), ], ..Default::default() }, diff --git a/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N805_N805.py.snap b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N805_N805.py.snap index 0038513dbc99a..3a50cb65fb51d 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N805_N805.py.snap +++ b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__N805_N805.py.snap @@ -43,4 +43,37 @@ N805.py:64:29: N805 First argument of a method should be named `self` 65 | pass | +N805.py:70:13: N805 First argument of a method should be named `self` + | +68 | class ModelClass: +69 | @hybrid_property +70 | def bad(cls): + | ^^^ N805 +71 | pass + | + +N805.py:78:13: N805 First argument of a method should be named `self` + | +77 | @bad.wtf +78 | def bad(cls): + | ^^^ N805 +79 | pass + | + +N805.py:86:14: N805 First argument of a method should be named `self` + | +85 | @good.expression +86 | def good(cls): + | ^^^ N805 +87 | pass + | + +N805.py:94:19: N805 First argument of a method should be named `self` + | +93 | @foobar.thisisstatic +94 | def badstatic(foo): + | ^^^ N805 +95 | pass + | + diff --git a/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__classmethod_decorators.snap b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__classmethod_decorators.snap index 2cdbc4438e4e1..59e0688efa8ff 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__classmethod_decorators.snap +++ b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__classmethod_decorators.snap @@ -27,4 +27,29 @@ N805.py:64:29: N805 First argument of a method should be named `self` 65 | pass | +N805.py:70:13: N805 First argument of a method should be named `self` + | +68 | class ModelClass: +69 | @hybrid_property +70 | def bad(cls): + | ^^^ N805 +71 | pass + | + +N805.py:78:13: N805 First argument of a method should be named `self` + | +77 | @bad.wtf +78 | def bad(cls): + | ^^^ N805 +79 | pass + | + +N805.py:94:19: N805 First argument of a method should be named `self` + | +93 | @foobar.thisisstatic +94 | def badstatic(foo): + | ^^^ N805 +95 | pass + | + diff --git a/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__staticmethod_decorators.snap b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__staticmethod_decorators.snap new file mode 100644 index 0000000000000..8e72fe683ac0b --- /dev/null +++ b/crates/ruff_linter/src/rules/pep8_naming/snapshots/ruff_linter__rules__pep8_naming__tests__staticmethod_decorators.snap @@ -0,0 +1,71 @@ +--- +source: crates/ruff_linter/src/rules/pep8_naming/mod.rs +--- +N805.py:7:20: N805 First argument of a method should be named `self` + | +6 | class Class: +7 | def bad_method(this): + | ^^^^ N805 +8 | pass + | + +N805.py:12:30: N805 First argument of a method should be named `self` + | +10 | if False: +11 | +12 | def extra_bad_method(this): + | ^^^^ N805 +13 | pass + | + +N805.py:31:15: N805 First argument of a method should be named `self` + | +30 | @pydantic.validator +31 | def lower(cls, my_field: str) -> str: + | ^^^ N805 +32 | pass + | + +N805.py:35:15: N805 First argument of a method should be named `self` + | +34 | @pydantic.validator("my_field") +35 | def lower(cls, my_field: str) -> str: + | ^^^ N805 +36 | pass + | + +N805.py:64:29: N805 First argument of a method should be named `self` + | +62 | pass +63 | +64 | def bad_method_pos_only(this, blah, /, self, something: str): + | ^^^^ N805 +65 | pass + | + +N805.py:70:13: N805 First argument of a method should be named `self` + | +68 | class ModelClass: +69 | @hybrid_property +70 | def bad(cls): + | ^^^ N805 +71 | pass + | + +N805.py:78:13: N805 First argument of a method should be named `self` + | +77 | @bad.wtf +78 | def bad(cls): + | ^^^ N805 +79 | pass + | + +N805.py:86:14: N805 First argument of a method should be named `self` + | +85 | @good.expression +86 | def good(cls): + | ^^^ N805 +87 | pass + | + + diff --git a/crates/ruff_python_semantic/src/analyze/function_type.rs b/crates/ruff_python_semantic/src/analyze/function_type.rs index 8289eb26fd68c..b6f9e259c8019 100644 --- a/crates/ruff_python_semantic/src/analyze/function_type.rs +++ b/crates/ruff_python_semantic/src/analyze/function_type.rs @@ -1,3 +1,4 @@ +use ruff_python_ast::call_path::collect_call_path; use ruff_python_ast::call_path::from_qualified_name; use ruff_python_ast::helpers::map_callable; use ruff_python_ast::Decorator; @@ -28,16 +29,29 @@ pub fn classify( if decorator_list.iter().any(|decorator| { // The method is decorated with a static method decorator (like // `@staticmethod`). - semantic - .resolve_call_path(map_callable(&decorator.expression)) - .is_some_and(|call_path| { + let real_decorator = map_callable(&decorator.expression); + match semantic.resolve_call_path(real_decorator) { + Some(call_path) => { matches!( call_path.as_slice(), ["", "staticmethod"] | ["abc", "abstractstaticmethod"] ) || staticmethod_decorators .iter() .any(|decorator| call_path == from_qualified_name(decorator)) - }) + } + // We do not have a resolvable call path, most likely from a decorator similar + // to `@someproperty.setter` - for those we match the last element. + // It would be nicer to use e.g. `.whatever` in the `staticmethod-decorators` config + // option but `pep8-naming` doesn't do this either. + // XXX: It's quite unlikely that this is ever used for staticmethods, but we need it + // for classmethods below and like this the behavior is consistent. + None => collect_call_path(real_decorator).is_some_and(|call_path| { + let tail = call_path.as_slice().last().unwrap(); + staticmethod_decorators + .iter() + .any(|decorator| tail == decorator) + }), + } }) { FunctionType::StaticMethod } else if matches!(name, "__new__" | "__init_subclass__" | "__class_getitem__") @@ -52,16 +66,30 @@ pub fn classify( }) || decorator_list.iter().any(|decorator| { // The method is decorated with a class method decorator (like `@classmethod`). - semantic - .resolve_call_path(map_callable(&decorator.expression)) - .is_some_and( |call_path| { + let real_decorator = map_callable(&decorator.expression); + match semantic.resolve_call_path(real_decorator) { + Some(call_path) => { matches!( call_path.as_slice(), ["", "classmethod"] | ["abc", "abstractclassmethod"] ) || classmethod_decorators .iter() .any(|decorator| call_path == from_qualified_name(decorator)) - }) + }, + // We do not have a resolvable call path, most likely from a decorator similar + // to `@someproperty.setter` - for those we match the last element. + // It would be nicer to use e.g. `.whatever` in the `classmethod-decorators` config + // option but `pep8-naming` doesn't do this either. + None => { + collect_call_path(real_decorator) + .is_some_and(|call_path| { + let tail = call_path.as_slice().last().unwrap(); + classmethod_decorators + .iter() + .any(|decorator| tail == decorator) + }) + } + } }) { FunctionType::ClassMethod diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 031d0503112d6..140f7e136c933 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2242,7 +2242,8 @@ pub struct Pep8NamingOptions { /// in this list takes a `cls` argument as its first argument. /// /// Expects to receive a list of fully-qualified names (e.g., `pydantic.validator`, - /// rather than `validator`). + /// rather than `validator`) or alternatively a plain name which is then matched against + /// the last segment in case the decorator itself consists of a dotted name. #[option( default = r#"[]"#, value_type = "list[str]", @@ -2261,7 +2262,8 @@ pub struct Pep8NamingOptions { /// in this list has no `self` or `cls` argument. /// /// Expects to receive a list of fully-qualified names (e.g., `belay.Device.teardown`, - /// rather than `teardown`). + /// rather than `teardown`) or alternatively a plain name which is then matched against + /// the last segment in case the decorator itself consists of a dotted name. #[option( default = r#"[]"#, value_type = "list[str]", diff --git a/ruff.schema.json b/ruff.schema.json index 36f7f31eaab77..56aaa3c59cdb6 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2147,7 +2147,7 @@ "type": "object", "properties": { "classmethod-decorators": { - "description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a class method (in addition to the builtin `@classmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list takes a `cls` argument as its first argument.\n\nExpects to receive a list of fully-qualified names (e.g., `pydantic.validator`, rather than `validator`).", + "description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a class method (in addition to the builtin `@classmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list takes a `cls` argument as its first argument.\n\nExpects to receive a list of fully-qualified names (e.g., `pydantic.validator`, rather than `validator`) or alternatively a plain name which is then matched against the last segment in case the decorator itself consists of a dotted name.", "type": [ "array", "null" @@ -2177,7 +2177,7 @@ } }, "staticmethod-decorators": { - "description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a static method (in addition to the builtin `@staticmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list has no `self` or `cls` argument.\n\nExpects to receive a list of fully-qualified names (e.g., `belay.Device.teardown`, rather than `teardown`).", + "description": "A list of decorators that, when applied to a method, indicate that the method should be treated as a static method (in addition to the builtin `@staticmethod`).\n\nFor example, Ruff will expect that any method decorated by a decorator in this list has no `self` or `cls` argument.\n\nExpects to receive a list of fully-qualified names (e.g., `belay.Device.teardown`, rather than `teardown`) or alternatively a plain name which is then matched against the last segment in case the decorator itself consists of a dotted name.", "type": [ "array", "null" From 2cad28dd07cb4ea3febb80877e88e09c2b25bdb1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 9 Nov 2023 21:00:07 -0500 Subject: [PATCH 2/2] Break out into standalone methods --- .../src/analyze/function_type.rs | 139 +++++++++++------- crates/ruff_workspace/src/options.rs | 10 +- 2 files changed, 93 insertions(+), 56 deletions(-) diff --git a/crates/ruff_python_semantic/src/analyze/function_type.rs b/crates/ruff_python_semantic/src/analyze/function_type.rs index b6f9e259c8019..4dca8221e0ab6 100644 --- a/crates/ruff_python_semantic/src/analyze/function_type.rs +++ b/crates/ruff_python_semantic/src/analyze/function_type.rs @@ -26,33 +26,10 @@ pub fn classify( let ScopeKind::Class(class_def) = &scope.kind else { return FunctionType::Function; }; - if decorator_list.iter().any(|decorator| { - // The method is decorated with a static method decorator (like - // `@staticmethod`). - let real_decorator = map_callable(&decorator.expression); - match semantic.resolve_call_path(real_decorator) { - Some(call_path) => { - matches!( - call_path.as_slice(), - ["", "staticmethod"] | ["abc", "abstractstaticmethod"] - ) || staticmethod_decorators - .iter() - .any(|decorator| call_path == from_qualified_name(decorator)) - } - // We do not have a resolvable call path, most likely from a decorator similar - // to `@someproperty.setter` - for those we match the last element. - // It would be nicer to use e.g. `.whatever` in the `staticmethod-decorators` config - // option but `pep8-naming` doesn't do this either. - // XXX: It's quite unlikely that this is ever used for staticmethods, but we need it - // for classmethods below and like this the behavior is consistent. - None => collect_call_path(real_decorator).is_some_and(|call_path| { - let tail = call_path.as_slice().last().unwrap(); - staticmethod_decorators - .iter() - .any(|decorator| tail == decorator) - }), - } - }) { + if decorator_list + .iter() + .any(|decorator| is_static_method(decorator, semantic, staticmethod_decorators)) + { FunctionType::StaticMethod } else if matches!(name, "__new__" | "__init_subclass__" | "__class_getitem__") // Special-case class method, like `__new__`. @@ -64,33 +41,7 @@ pub fn classify( matches!(call_path.as_slice(), ["", "type"] | ["abc", "ABCMeta"]) }) }) - || decorator_list.iter().any(|decorator| { - // The method is decorated with a class method decorator (like `@classmethod`). - let real_decorator = map_callable(&decorator.expression); - match semantic.resolve_call_path(real_decorator) { - Some(call_path) => { - matches!( - call_path.as_slice(), - ["", "classmethod"] | ["abc", "abstractclassmethod"] - ) || classmethod_decorators - .iter() - .any(|decorator| call_path == from_qualified_name(decorator)) - }, - // We do not have a resolvable call path, most likely from a decorator similar - // to `@someproperty.setter` - for those we match the last element. - // It would be nicer to use e.g. `.whatever` in the `classmethod-decorators` config - // option but `pep8-naming` doesn't do this either. - None => { - collect_call_path(real_decorator) - .is_some_and(|call_path| { - let tail = call_path.as_slice().last().unwrap(); - classmethod_decorators - .iter() - .any(|decorator| tail == decorator) - }) - } - } - }) + || decorator_list.iter().any(|decorator| is_class_method(decorator, semantic, classmethod_decorators)) { FunctionType::ClassMethod } else { @@ -98,3 +49,83 @@ pub fn classify( FunctionType::Method } } + +/// Return `true` if a [`Decorator`] is indicative of a static method. +fn is_static_method( + decorator: &Decorator, + semantic: &SemanticModel, + staticmethod_decorators: &[String], +) -> bool { + let decorator = map_callable(&decorator.expression); + + // The decorator is an import, so should match against a qualified path. + if semantic + .resolve_call_path(decorator) + .is_some_and(|call_path| { + matches!( + call_path.as_slice(), + ["", "staticmethod"] | ["abc", "abstractstaticmethod"] + ) || staticmethod_decorators + .iter() + .any(|decorator| call_path == from_qualified_name(decorator)) + }) + { + return true; + } + + // We do not have a resolvable call path, most likely from a decorator like + // `@someproperty.setter`. Instead, match on the last element. + if !staticmethod_decorators.is_empty() { + if collect_call_path(decorator).is_some_and(|call_path| { + call_path.last().is_some_and(|tail| { + staticmethod_decorators + .iter() + .any(|decorator| tail == decorator) + }) + }) { + return true; + } + } + + false +} + +/// Return `true` if a [`Decorator`] is indicative of a class method. +fn is_class_method( + decorator: &Decorator, + semantic: &SemanticModel, + classmethod_decorators: &[String], +) -> bool { + let decorator = map_callable(&decorator.expression); + + // The decorator is an import, so should match against a qualified path. + if semantic + .resolve_call_path(decorator) + .is_some_and(|call_path| { + matches!( + call_path.as_slice(), + ["", "classmethod"] | ["abc", "abstractclassmethod"] + ) || classmethod_decorators + .iter() + .any(|decorator| call_path == from_qualified_name(decorator)) + }) + { + return true; + } + + // We do not have a resolvable call path, most likely from a decorator like + // `@someproperty.setter`. Instead, match on the last element. + if !classmethod_decorators.is_empty() { + if collect_call_path(decorator).is_some_and(|call_path| { + call_path.last().is_some_and(|tail| { + classmethod_decorators + .iter() + .any(|decorator| tail == decorator) + }) + }) { + return true; + } + } + + false +} diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 140f7e136c933..2b66a4653a461 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -2248,8 +2248,14 @@ pub struct Pep8NamingOptions { default = r#"[]"#, value_type = "list[str]", example = r#" - # Allow Pydantic's `@validator` decorator to trigger class method treatment. - classmethod-decorators = ["pydantic.validator"] + classmethod-decorators = [ + # Allow Pydantic's `@validator` decorator to trigger class method treatment. + "pydantic.validator", + # Allow SQLAlchemy's dynamic decorators, like `@field.expression`, to trigger class method treatment. + "declared_attr", + "expression", + "comparator", + ] "# )] pub classmethod_decorators: Option>,