Consider `__new__` methods as special function type for enforcing class method or static method rules by cake-monotone · Pull Request #13305 · astral-sh/ruff

cake-monotone · 2024-09-10T11:08:48Z

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:

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.
The following rule behaviors and messages are totally unchanged:
- too-many-arguments (PLR0913)
- too-many-positional-arguments (PLR0917)
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)
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) no longer applies to __new__
- unused-static-method-argument (ARG004) now applies to __new__
The only changes which differ based on preview are the following:
- 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): When preview is enabled, this now applies to __new__.

Closes #13154

github-actions · 2024-09-10T11:22:24Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+2 -2 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

latchbio/latch (+2 -2 violations, +0 -0 fixes)

- src/latch_cli/centromere/utils.py:67:34: ARG003 Unused class method argument: `args`
+ src/latch_cli/centromere/utils.py:67:34: ARG004 Unused static method argument: `args`
- src/latch_cli/centromere/utils.py:67:42: ARG003 Unused class method argument: `kwargs`
+ src/latch_cli/centromere/utils.py:67:42: ARG004 Unused static method argument: `kwargs`

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
ARG004	2	2	0	0	0
ARG003	2	0	2	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -2 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

latchbio/latch (+2 -2 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

- src/latch_cli/centromere/utils.py:67:34: ARG003 Unused class method argument: `args`
+ src/latch_cli/centromere/utils.py:67:34: ARG004 Unused static method argument: `args`
- src/latch_cli/centromere/utils.py:67:42: ARG003 Unused class method argument: `kwargs`
+ src/latch_cli/centromere/utils.py:67:42: ARG004 Unused static method argument: `kwargs`

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
ARG004	2	2	0	0	0
ARG003	2	0	2	0	0

cake-monotone · 2024-09-10T11:52:34Z

I'm sure there are other breaking changes besides the ruff-ecosystem results
I'll try to summarize them.

cake-monotone · 2024-09-11T09:42:46Z

Affected Rules

ARG003: unused-class-method-argument
ARG004: unused-static-method-argument
PLW0642: self-or-cls-assignment
PLW0211: bad-staticmethod-argument
N804: invalid-first-argument-name-for-class-method
PLR0913: too-many-arguments
PLR0917: too-many-positional-arguments

ARG003, ARG004

ARG003 (unused-class-method-argument) -> ARG004 (unused-static-method-argument)

PLW0642

__new__ methods will no longer be checked by PLW0642. This rule only applies to methods and class methods, not static methods.

N804, PLW0211

N804 (invalid-first-argument-name-for-class-method) -> PLW0211 (bad-staticmethod-argument)

PLR0913, PLR0917

PLR0913 (too-many-arguments) and PLR0917 (too-many-positional-arguments) behave differently for classmethod and staticmethod. In classmethod, the first argument cls is excluded from the argument count. In staticmethod, cls is not excluded, so the argument limit applies more strictly.

Example

You can view an example of how these rules are applied here:
https://play.ruff.rs/2df5f2b6-093f-4988-a562-c6101a1c71cc

class ClassDunderNew:
    @classmethod
    def __new__(
        cls,
        x,  # ARG003: Unused class method argument: `x`
    ):
        cls = 1  # PLW0642: Reassigned `cls` variable in class method

    @classmethod
    def __new__(  # PLR0913: Too many arguments in function definition (6 > 5)  # PLR0917: Too many positional arguments (6/5)
        self,  # N804: First argument of a class method should be named `cls`
        a1, a2, a3, a4, a5,
        a6,
    ):
        ...


class StaticDunderNew:
    @staticmethod
    def __new__(
        cls,
        x,  # ARG004: Unused static method argument: `x`
    ):
        cls = 1

    @staticmethod
    def __new__(  # PLR0913: Too many arguments in function definition (6 > 5)  # PLR0917: Too many positional arguments (6/5)
        self,  # PLW0211: First argument of a static method should not be named `self`
        a1, a2, a3, a4, a5,
    ):
        ...

cake-monotone · 2024-09-11T09:51:58Z

I reviewed all the code affected by function_type::classify. Let me know if I missed any affected rules.

Please check if these affected rules are acceptable. :)

…ified as `staticmethod`

dylwil3

Thanks so much!

I think this is pretty much good to go - had to tweak some things to rebase, and I decided we should special case the counting of arguments for the "too-many-(positional-)arguments" rules to treat __new__ like a classmethod, since it seems more intuitive there. Also added more test fixtures to track changes if we decide to further modify the treatment of __new__ later.

My only hesitation on merging is whether this should be considered breaking and wait until the next minor release - thoughts @AlexWaygood / @MichaReiser ?

Also to Alex: It'd be great if you could spot check the conflict resolution for PYI019 to make sure it's in line with what you had in mind for the recent changes to that rule. https://github.com/astral-sh/ruff/pull/13305/files#diff-bfce927fd2c71368a7fc9c045c1f51f03f4ac25e398ed8b3a571caf01f4efe06

MichaReiser · 2025-02-15T21:35:53Z

I'd prefer if this is a preview only change just because of the scope of it. It's definitely a breaking change otherwise

dylwil3 · 2025-02-16T19:25:33Z

I don't think making the change preview only makes sense in many cases, since I consider the old behavior a bug/incorrect. Here's what I ended up doing. Even though this is a long list, there are actually not so many behavior changes, so I wouldn't consider this breaking, personally. What do you think?

As suggested by Alex, I introduced 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.
The following rule behaviors and messages are totally unchanged:
- too-many-arguments (PLR0913)
- too-many-positional-arguments (PLR0917)
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)
The following rules are changed unconditionally (not gated behind preview) because I think 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) no longer applies to __new__
- unused-static-method-argument (ARG004) now applies to __new__
The only changes which differ based on preview are the following:
- 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): When preview is enabled, this now applies to __new__.

AlexWaygood

This looks excellent! Thanks @cake-monotone for the PR, and thanks @dylwil3 for driving it over the line!

crates/ruff_python_semantic/src/analyze/function_type.rs

crates/ruff_linter/src/rules/flake8_unused_arguments/rules/unused_arguments.rs

crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs

crates/ruff_python_semantic/src/analyze/function_type.rs

Co-authored-by: Alex Waygood <[email protected]>

AlexWaygood

argh sorry, a couple of tiny nits I missed pre-merge that might be good to address as a followup

AlexWaygood · 2025-02-16T20:26:49Z

crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs

+        // In preview, this violation is caught by `PLW0211` instead
+        function_type::FunctionType::NewMethod if checker.settings.preview.is_enabled() => {
+            return;
+        }


Oh, I guess it might be good to add a note about this to the rule's docs?

AlexWaygood · 2025-02-16T20:27:34Z

crates/ruff_linter/src/rules/flake8_unused_arguments/rules/unused_arguments.rs

+                        && !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


Ah shoot, my previous suggestion had a typo -- it should have been this :-(

Suggested change

// an implicit staticmethod, `__new__` methods must always have >= parameter

// an implicit staticmethod, `__new__` methods must always have >=1 parameters

…method-argument` and not `invalid-first-argument-name-for-class-method` (`PLW0211`/`N804`) (#16676) ## Summary This PR stabilizes the behavior changes introduced by #13305 that were gated behind preview. The change is that `__new__` methods are now no longer flagged by `invalid-first-argument-name-for-class-method` (`N804`) but instead by `bad-staticmethod-argument` (`PLW0211`) > __new__ methods are technically static methods, with cls as their first argument. However, Ruff currently classifies them as classmethod, which causes two issues: ## Test Plan There have been no new issues or PRs related to `N804` or `PLW0211` since the behavior change was released in Ruff 0.9.7 (about 3 weeks ago). This is a somewhat recent change but I don't think it's necessary to leave this in preview for another 2 months. The main reason why it was in preview is that it is breaking, not because it is a risky change.

cake-monotone requested a review from AlexWaygood as a code owner September 10, 2024 11:08

AlexWaygood self-assigned this Sep 10, 2024

AlexWaygood removed their assignment Sep 19, 2024

dylwil3 force-pushed the dunder-new-as-staticmethod branch from 1865ecb to 0e23922 Compare December 25, 2024 05:02

MichaReiser added rule Implementing or modifying a lint rule needs-decision Awaiting a decision from a maintainer labels Dec 26, 2024

cake-monotone and others added 6 commits February 15, 2025 12:19

Refactor method classification logic: __new__ methods are now class…

01f1ff1

…ified as `staticmethod`

snapshots for refactored classification logic

2991d14

nit: use eq not matches

bf8ef80

add fixtures to document behavior

7ae9a7f

special case argument count for __new__ to be more intuitive

c2eeb14

update snapshot

b7931ce

dylwil3 force-pushed the dunder-new-as-staticmethod branch from 0e23922 to b7931ce Compare February 15, 2025 18:49

dylwil3 removed the needs-decision Awaiting a decision from a maintainer label Feb 15, 2025

dylwil3 reviewed Feb 15, 2025

View reviewed changes

dylwil3 added 10 commits February 16, 2025 11:16

add FunctionType::NewMethod

4e09b27

modify fixture comment for self-or-cls-assignment

546b60d

test with preview enabled

e8dadd7

treat as class method for custom-type-var-for-self

a189be8

treat as static method for ARG rules - not gated behind preview

1bc314e

self-or-cls-assignment applies but message refers to __new__

649698b

too-many-arguments unchanged behavior

9b5ce4d

pep8 turned off in preview, message changed to use __new__ otherwise

7f670e7

bad-staticmethod-argument applies to __new__ in preview

21606b7

snapshots

66dd4f0

dylwil3 added 2 commits February 16, 2025 13:21

smaller diff for too-many-arguments

40f01d9

overrule clippy to keep rule documentation prettier

10b6fb6

AlexWaygood approved these changes Feb 16, 2025

View reviewed changes

dylwil3 and others added 3 commits February 16, 2025 14:04

method --> methods

6873374

Co-authored-by: Alex Waygood <[email protected]>

descriptive comment for unused-arguments logic

451f163

Co-authored-by: Alex Waygood <[email protected]>

comment to doc-comment and reword

3291367

Co-authored-by: Alex Waygood <[email protected]>

dylwil3 added the preview Related to preview mode features label Feb 16, 2025

dylwil3 changed the title ~~Refactor method classification logic: __new__ methods are now classified as staticmethod~~ Consider __new__ methods as special function type for enforcing class method or static method rules Feb 16, 2025

dylwil3 merged commit 96dd1b1 into astral-sh:main Feb 16, 2025
21 checks passed

AlexWaygood reviewed Feb 16, 2025

View reviewed changes

BrewTestBot mentioned this pull request Feb 20, 2025

ruff 0.9.7 Homebrew/homebrew-core#208398

Merged

MichaReiser mentioned this pull request Mar 12, 2025

[pylint/pep8-naming] Check __new__ argument name in bad-staticmethod-argument and not invalid-first-argument-name-for-class-method (PLW0211/N804) #16676

Merged

	// an implicit staticmethod, `__new__` methods must always have >= parameter
	// an implicit staticmethod, `__new__` methods must always have >=1 parameters

Comments

Conversation

cake-monotone commented Sep 10, 2024 • edited by dylwil3 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

github-actions bot commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

cake-monotone commented Sep 10, 2024

Uh oh!

cake-monotone commented Sep 11, 2024

Affected Rules

ARG003, ARG004

PLW0642

N804, PLW0211

PLR0913, PLR0917

Example

Uh oh!

cake-monotone commented Sep 11, 2024

Uh oh!

dylwil3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylwil3 commented Feb 16, 2025 • edited by AlexWaygood Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Feb 16, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood Feb 16, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cake-monotone commented Sep 10, 2024 •

edited by dylwil3

Loading

github-actions bot commented Sep 10, 2024 •

edited

Loading

`ruff-ecosystem` results

dylwil3 left a comment •

edited

Loading

MichaReiser commented Feb 15, 2025 •

edited

Loading

dylwil3 commented Feb 16, 2025 •

edited by AlexWaygood

Loading

AlexWaygood left a comment •

edited

Loading