From c80e0b2dc1d10d25f338cf54887f0cdd6942c460 Mon Sep 17 00:00:00 2001 From: Adrian Moennich Date: Fri, 10 Nov 2023 00:13:21 +0100 Subject: [PATCH] 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"