Consider __new__ methods as special function type for enforcing class method or static method rules (#13305)

## Summary

`__new__` methods are technically static methods, with `cls` as their
first argument. However, Ruff currently classifies them as classmethod,
which causes two issues:

- It conveys incorrect information, leading to confusion. For example,
in cases like ARG003, `__new__` is explicitly treated as a classmethod.
- Future rules that should apply to staticmethod may not be applied
correctly due to this misclassification.

Motivated by this, the current PR makes the following adjustments:

1. Introduces `FunctionType::NewMethod` as an enum variant, since, for
the purposes of lint rules, `__new__` sometimes behaves like a static
method and other times like a class method. This is an internal change.

2. The following rule behaviors and messages are totally unchanged:
- [too-many-arguments
(PLR0913)](https://docs.astral.sh/ruff/rules/too-many-arguments/#too-many-arguments-plr0913)
- [too-many-positional-arguments
(PLR0917)](https://docs.astral.sh/ruff/rules/too-many-positional-arguments/#too-many-positional-arguments-plr0917)
3. The following rule behaviors are unchanged, but the messages have
been changed for correctness to use "`__new__` method" instead of "class
method":
- [self-or-cls-assignment
(PLW0642)](https://docs.astral.sh/ruff/rules/self-or-cls-assignment/#self-or-cls-assignment-plw0642)
4. The following rules are changed _unconditionally_ (not gated behind
preview) because their current behavior is an honest bug: it just isn't
true that `__new__` is a class method, and it _is_ true that `__new__`
is a static method:
- [unused-class-method-argument
(ARG003)](https://docs.astral.sh/ruff/rules/unused-class-method-argument/#unused-class-method-argument-arg003)
no longer applies to `__new__`
- [unused-static-method-argument
(ARG004)](https://docs.astral.sh/ruff/rules/unused-static-method-argument/#unused-static-method-argument-arg004)
now applies to `__new__`
5. The only changes which differ based on `preview` are the following:
- [invalid-first-argument-name-for-class-method
(N804)](https://docs.astral.sh/ruff/rules/invalid-first-argument-name-for-class-method/#invalid-first-argument-name-for-class-method-n804):
This is _skipped_ when `preview` is _enabled_. When `preview` is
_disabled_, the rule is the same but the _message_ has been modified to
say "`__new__` method" instead of "class method".
- [bad-staticmethod-argument
(PLW0211)](https://docs.astral.sh/ruff/rules/bad-staticmethod-argument/#bad-staticmethod-argument-plw0211):
When `preview` is enabled, this now applies to `__new__`.

Closes #13154

---------

Co-authored-by: dylwil3 <dylwil3@gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
This commit is contained in:
cake-monotone 2025-02-17 05:12:25 +09:00 committed by GitHub
parent f29c7b03ec
commit 96dd1b1587
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
22 changed files with 184 additions and 24 deletions

View file

@ -210,6 +210,9 @@ def f(a, b):
# Unused arguments on magic methods.
###
class C:
def __new__(cls, x):
print("Hello, world!")
def __init__(self, x) -> None:
print("Hello, world!")
@ -219,6 +222,12 @@ class C:
def __exit__(self, exc_type, exc_value, traceback) -> None:
print("Hello, world!")
def __init_subclass__(cls, x) -> None:
print("Hello, world!")
def __class_getitem__(cls, x):
print("Hello, world!")
###
# Used arguments on chained cast.

View file

@ -48,3 +48,9 @@ class Foo:
@staticmethod
def __new__(cls, x, y, z): # OK, see https://docs.python.org/3/reference/datamodel.html#basic-customization
pass
# `__new__` is an implicit staticmethod, so this should still trigger (with
# `self` but not with `cls` as first argument - see above).
class Foo:
def __new__(self, x, y, z): # [bad-staticmethod-argument]
pass

View file

@ -41,3 +41,10 @@ class Fruit:
def list_fruits(self, cls) -> None:
self = "apple" # Ok
cls = "banana" # Ok
# `__new__` is implicitly a static method
# but for the purposes of this check we treat
# it as a class method.
class Foo:
def __new__(cls):
cls = "apple" # PLW0642

View file

@ -74,3 +74,8 @@ class C:
def f(y, z, a, b, c): # OK
pass
class Foo:
# `__new__` counts args like a classmethod
# even though it is an implicit staticmethod
def __new__(cls,a,b,c,d,e): # Ok
...

View file

@ -59,3 +59,10 @@ class C:
def f(): # OK
pass
class Foo:
# `__new__` counts args like a classmethod
# even though it is an implicit staticmethod
def __new__(cls,a,b,c,d,e): # Ok
...

View file

@ -159,7 +159,7 @@ pub(crate) fn custom_type_var_instead_of_self(
// to a type variable, and we emit the diagnostic on some methods that do not have return
// annotations.
let (method, diagnostic_range) = match function_kind {
FunctionType::ClassMethod => {
FunctionType::ClassMethod | FunctionType::NewMethod => {
if checker.settings.preview.is_enabled() {
(
Method::PreviewClass(PreviewClassMethod {

View file

@ -422,7 +422,6 @@ pub(crate) fn unused_arguments(checker: &Checker, scope: &Scope) {
&& !is_not_implemented_stub_with_variable(function_def, checker.semantic())
&& (!visibility::is_magic(name)
|| visibility::is_init(name)
|| visibility::is_new(name)
|| visibility::is_call(name))
&& !visibility::is_abstract(decorator_list, checker.semantic())
&& !visibility::is_override(decorator_list, checker.semantic())
@ -437,7 +436,6 @@ pub(crate) fn unused_arguments(checker: &Checker, scope: &Scope) {
&& !is_not_implemented_stub_with_variable(function_def, checker.semantic())
&& (!visibility::is_magic(name)
|| visibility::is_init(name)
|| visibility::is_new(name)
|| visibility::is_call(name))
&& !visibility::is_abstract(decorator_list, checker.semantic())
&& !visibility::is_override(decorator_list, checker.semantic())
@ -452,7 +450,6 @@ pub(crate) fn unused_arguments(checker: &Checker, scope: &Scope) {
&& !is_not_implemented_stub_with_variable(function_def, checker.semantic())
&& (!visibility::is_magic(name)
|| visibility::is_init(name)
|| visibility::is_new(name)
|| visibility::is_call(name))
&& !visibility::is_abstract(decorator_list, checker.semantic())
&& !visibility::is_override(decorator_list, checker.semantic())
@ -461,6 +458,19 @@ pub(crate) fn unused_arguments(checker: &Checker, scope: &Scope) {
function(Argumentable::StaticMethod, parameters, scope, checker);
}
}
function_type::FunctionType::NewMethod => {
if checker.enabled(Argumentable::StaticMethod.rule_code())
&& !function_type::is_stub(function_def, checker.semantic())
&& !is_not_implemented_stub_with_variable(function_def, checker.semantic())
&& !visibility::is_abstract(decorator_list, checker.semantic())
&& !visibility::is_override(decorator_list, checker.semantic())
&& !visibility::is_overload(decorator_list, checker.semantic())
{
// we use `method()` here rather than `function()`, as although `__new__` is
// an implicit staticmethod, `__new__` methods must always have >= parameter
method(Argumentable::StaticMethod, parameters, scope, checker);
}
}
}
}
ScopeKind::Lambda(ast::ExprLambda { parameters, .. }) => {

View file

@ -58,11 +58,11 @@ ARG.py:66:17: ARG002 Unused method argument: `x`
68 | raise NotImplementedError("must use msg")
|
ARG.py:213:24: ARG002 Unused method argument: `x`
ARG.py:216:24: ARG002 Unused method argument: `x`
|
211 | ###
212 | class C:
213 | def __init__(self, x) -> None:
| ^ ARG002
214 | print("Hello, world!")
215 |
216 | def __init__(self, x) -> None:
| ^ ARG002
217 | print("Hello, world!")
|

View file

@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/flake8_unused_arguments/mod.rs
snapshot_kind: text
---
ARG.py:47:16: ARG003 Unused class method argument: `x`
|

View file

@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/flake8_unused_arguments/mod.rs
snapshot_kind: text
---
ARG.py:51:11: ARG004 Unused static method argument: `cls`
|
@ -25,3 +24,12 @@ ARG.py:55:11: ARG004 Unused static method argument: `x`
| ^ ARG004
56 | print("Hello, world!")
|
ARG.py:213:22: ARG004 Unused static method argument: `x`
|
211 | ###
212 | class C:
213 | def __new__(cls, x):
| ^ ARG004
214 | print("Hello, world!")
|

View file

@ -90,6 +90,7 @@ mod tests {
}
#[test_case(Rule::InvalidArgumentName, Path::new("N803.py"))]
#[test_case(Rule::InvalidArgumentName, Path::new("N804.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",

View file

@ -127,6 +127,8 @@ impl Violation for InvalidFirstArgumentNameForMethod {
#[derive(ViolationMetadata)]
pub(crate) struct InvalidFirstArgumentNameForClassMethod {
argument_name: String,
// Whether the method is `__new__`
is_new: bool,
}
impl Violation for InvalidFirstArgumentNameForClassMethod {
@ -134,12 +136,19 @@ impl Violation for InvalidFirstArgumentNameForClassMethod {
ruff_diagnostics::FixAvailability::Sometimes;
#[derive_message_formats]
// The first string below is what shows up in the documentation
// in the rule table, and it is the more common case.
#[allow(clippy::if_not_else)]
fn message(&self) -> String {
"First argument of a class method should be named `cls`".to_string()
if !self.is_new {
"First argument of a class method should be named `cls`".to_string()
} else {
"First argument of `__new__` method should be named `cls`".to_string()
}
}
fn fix_title(&self) -> Option<String> {
let Self { argument_name } = self;
let Self { argument_name, .. } = self;
Some(format!("Rename `{argument_name}` to `cls`"))
}
}
@ -150,13 +159,24 @@ enum FunctionType {
Method,
/// The function is a class method.
ClassMethod,
/// The function is the method `__new__`
NewMethod,
}
impl FunctionType {
fn diagnostic_kind(self, argument_name: String) -> DiagnosticKind {
match self {
Self::Method => InvalidFirstArgumentNameForMethod { argument_name }.into(),
Self::ClassMethod => InvalidFirstArgumentNameForClassMethod { argument_name }.into(),
Self::ClassMethod => InvalidFirstArgumentNameForClassMethod {
argument_name,
is_new: false,
}
.into(),
Self::NewMethod => InvalidFirstArgumentNameForClassMethod {
argument_name,
is_new: true,
}
.into(),
}
}
@ -164,6 +184,7 @@ impl FunctionType {
match self {
Self::Method => "self",
Self::ClassMethod => "cls",
Self::NewMethod => "cls",
}
}
@ -171,6 +192,7 @@ impl FunctionType {
match self {
Self::Method => Rule::InvalidFirstArgumentNameForMethod,
Self::ClassMethod => Rule::InvalidFirstArgumentNameForClassMethod,
Self::NewMethod => Rule::InvalidFirstArgumentNameForClassMethod,
}
}
}
@ -214,6 +236,11 @@ pub(crate) fn invalid_first_argument_name(checker: &Checker, scope: &Scope) {
IsMetaclass::Maybe => return,
},
function_type::FunctionType::ClassMethod => FunctionType::ClassMethod,
// In preview, this violation is caught by `PLW0211` instead
function_type::FunctionType::NewMethod if checker.settings.preview.is_enabled() => {
return;
}
function_type::FunctionType::NewMethod => FunctionType::NewMethod,
};
if !checker.enabled(function_type.rule()) {
return;

View file

@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/pep8_naming/mod.rs
---

View file

@ -442,6 +442,10 @@ mod tests {
)]
#[test_case(Rule::InvalidEnvvarDefault, Path::new("invalid_envvar_default.py"))]
#[test_case(Rule::BadStrStripCall, Path::new("bad_str_strip_call.py"))]
#[test_case(
Rule::BadStaticmethodArgument,
Path::new("bad_staticmethod_argument.py")
)]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",

View file

@ -11,6 +11,9 @@ use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for static methods that use `self` or `cls` as their first argument.
///
/// If [`preview`] mode is enabled, this rule also applies to
/// `__new__` methods, which are implicitly static.
///
/// ## Why is this bad?
/// [PEP 8] recommends the use of `self` and `cls` as the first arguments for
/// instance methods and class methods, respectively. Naming the first argument
@ -72,9 +75,14 @@ pub(crate) fn bad_staticmethod_argument(checker: &Checker, scope: &Scope) {
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
);
if !matches!(type_, function_type::FunctionType::StaticMethod) {
return;
}
match type_ {
function_type::FunctionType::StaticMethod => {}
function_type::FunctionType::NewMethod if checker.settings.preview.is_enabled() => {}
_ => {
return;
}
};
let Some(ParameterWithDefault {
parameter: self_or_cls,

View file

@ -10,6 +10,9 @@ use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for assignment of `self` and `cls` in instance and class methods respectively.
///
/// This check also applies to `__new__` even though this is technically
/// a static method.
///
/// ## Why is this bad?
/// The identifiers `self` and `cls` are conventional in Python for the first parameter of instance
/// methods and class methods, respectively. Assigning new values to these variables can be
@ -102,6 +105,7 @@ pub(crate) fn self_or_cls_assignment(checker: &Checker, target: &Expr) {
let method_type = match (function_type, self_or_cls.name().as_str()) {
(FunctionType::Method { .. }, "self") => MethodType::Instance,
(FunctionType::ClassMethod { .. }, "cls") => MethodType::Class,
(FunctionType::NewMethod, "cls") => MethodType::New,
_ => return,
};
@ -134,6 +138,7 @@ fn check_expr(checker: &Checker, target: &Expr, method_type: MethodType) {
enum MethodType {
Instance,
Class,
New,
}
impl MethodType {
@ -141,6 +146,7 @@ impl MethodType {
match self {
MethodType::Instance => "self",
MethodType::Class => "cls",
MethodType::New => "cls",
}
}
}
@ -150,6 +156,7 @@ impl std::fmt::Display for MethodType {
match self {
MethodType::Instance => f.write_str("instance"),
MethodType::Class => f.write_str("class"),
MethodType::New => f.write_str("`__new__`"),
}
}
}

View file

@ -93,7 +93,9 @@ pub(crate) fn too_many_arguments(checker: &Checker, function_def: &ast::StmtFunc
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
),
function_type::FunctionType::Method | function_type::FunctionType::ClassMethod
function_type::FunctionType::Method
| function_type::FunctionType::ClassMethod
| function_type::FunctionType::NewMethod
) {
// If so, we need to subtract one from the number of positional arguments, since the first
// argument is always `self` or `cls`.

View file

@ -97,7 +97,9 @@ pub(crate) fn too_many_positional_arguments(
&checker.settings.pep8_naming.classmethod_decorators,
&checker.settings.pep8_naming.staticmethod_decorators,
),
function_type::FunctionType::Method | function_type::FunctionType::ClassMethod
function_type::FunctionType::Method
| function_type::FunctionType::ClassMethod
| function_type::FunctionType::NewMethod
) {
// If so, we need to subtract one from the number of positional arguments, since the first
// argument is always `self` or `cls`.

View file

@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
snapshot_kind: text
---
bad_staticmethod_argument.py:3:13: PLW0211 First argument of a static method should not be named `self`
|

View file

@ -150,3 +150,12 @@ self_or_cls_assignment.py:26:9: PLW0642 Reassigned `self` variable in instance m
28 | def ok(self) -> None:
|
= help: Consider using a different variable name
self_or_cls_assignment.py:50:9: PLW0642 Reassigned `cls` variable in `__new__` method
|
48 | class Foo:
49 | def __new__(cls):
50 | cls = "apple" # PLW0642
| ^^^ PLW0642
|
= help: Consider using a different variable name

View file

@ -0,0 +1,37 @@
---
source: crates/ruff_linter/src/rules/pylint/mod.rs
---
bad_staticmethod_argument.py:3:13: PLW0211 First argument of a static method should not be named `self`
|
1 | class Wolf:
2 | @staticmethod
3 | def eat(self): # [bad-staticmethod-argument]
| ^^^^ PLW0211
4 | pass
|
bad_staticmethod_argument.py:15:13: PLW0211 First argument of a static method should not be named `cls`
|
13 | class Sheep:
14 | @staticmethod
15 | def eat(cls, x, y, z): # [bad-staticmethod-argument]
| ^^^ PLW0211
16 | pass
|
bad_staticmethod_argument.py:19:15: PLW0211 First argument of a static method should not be named `self`
|
18 | @staticmethod
19 | def sleep(self, x, y, z): # [bad-staticmethod-argument]
| ^^^^ PLW0211
20 | pass
|
bad_staticmethod_argument.py:55:17: PLW0211 First argument of a static method should not be named `self`
|
53 | # `self` but not with `cls` as first argument - see above).
54 | class Foo:
55 | def __new__(self, x, y, z): # [bad-staticmethod-argument]
| ^^^^ PLW0211
56 | pass
|

View file

@ -11,6 +11,9 @@ pub enum FunctionType {
Method,
ClassMethod,
StaticMethod,
/// `__new__` is an implicit static method but
/// is treated similarly to class methods for several lint rules
NewMethod,
}
/// Classify a function based on its scope, name, and decorators.
@ -30,17 +33,22 @@ pub fn classify(
.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__`.
|| decorator_list.iter().any(|decorator| is_class_method(decorator, semantic, classmethod_decorators))
} else if decorator_list
.iter()
.any(|decorator| is_class_method(decorator, semantic, classmethod_decorators))
{
FunctionType::ClassMethod
} else {
// It's an instance method.
FunctionType::Method
match name {
"__new__" => FunctionType::NewMethod, // Implicit static method.
"__init_subclass__" | "__class_getitem__" => FunctionType::ClassMethod, // Implicit class methods.
_ => FunctionType::Method, // Default to instance method.
}
}
}
/// Return `true` if a [`Decorator`] is indicative of a static method.
/// Note: Implicit static methods like `__new__` are not considered.
fn is_static_method(
decorator: &Decorator,
semantic: &SemanticModel,
@ -81,6 +89,7 @@ fn is_static_method(
}
/// Return `true` if a [`Decorator`] is indicative of a class method.
/// Note: Implicit class methods like `__init_subclass__` and `__class_getitem__` are not considered.
fn is_class_method(
decorator: &Decorator,
semantic: &SemanticModel,