Skip to content

Commit

Permalink
Support local and dynamic class- and static-method decorators (#8592)
Browse files Browse the repository at this point in the history
## Summary

This brings ruff's behavior in line with what `pep8-naming` already does
and thus closes #8397.

I had initially implemented this to look at the last segment of a dotted
path only when the entry in the `*-decorators` setting started with a
`.`, but in the end I thought it's better to remain consistent w/
`pep8-naming` and doing a match against the last segment of the
decorator name in any case.

If you prefer to diverge from this in favor of less ambiguity in the
configuration let me know and I'll change it so you would need to put
e.g. `.expression` in the `classmethod-decorators` list.

## Test Plan

Tested against the file in the issue linked below, plus the new testcase
added in this PR.
  • Loading branch information
ThiefMaster authored Nov 10, 2023
1 parent 565ddeb commit 4ebd0bd
Show file tree
Hide file tree
Showing 8 changed files with 279 additions and 33 deletions.
30 changes: 30 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pep8_naming/N805.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
20 changes: 20 additions & 0 deletions crates/ruff_linter/src/rules/pep8_naming/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
|


Original file line number Diff line number Diff line change
Expand Up @@ -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
|


Original file line number Diff line number Diff line change
@@ -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
|


113 changes: 86 additions & 27 deletions crates/ruff_python_semantic/src/analyze/function_type.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -25,20 +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`).
semantic
.resolve_call_path(map_callable(&decorator.expression))
.is_some_and(|call_path| {
matches!(
call_path.as_slice(),
["", "staticmethod"] | ["abc", "abstractstaticmethod"]
) || staticmethod_decorators
.iter()
.any(|decorator| call_path == from_qualified_name(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__`.
Expand All @@ -50,23 +41,91 @@ 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`).
semantic
.resolve_call_path(map_callable(&decorator.expression))
.is_some_and( |call_path| {
matches!(
call_path.as_slice(),
["", "classmethod"] | ["abc", "abstractclassmethod"]
) || classmethod_decorators
.iter()
.any(|decorator| call_path == from_qualified_name(decorator))
})
})
|| decorator_list.iter().any(|decorator| is_class_method(decorator, semantic, classmethod_decorators))
{
FunctionType::ClassMethod
} else {
// It's an instance method.
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
}
16 changes: 12 additions & 4 deletions crates/ruff_workspace/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2242,13 +2242,20 @@ 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]",
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<Vec<String>>,
Expand All @@ -2261,7 +2268,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]",
Expand Down
4 changes: 2 additions & 2 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 4ebd0bd

Please sign in to comment.