From 75766692979bbe97d8a3fafa6f3fcd02c9c10bea Mon Sep 17 00:00:00 2001 From: Dan Parizher <105245560+danparizher@users.noreply.github.com> Date: Fri, 24 Oct 2025 09:40:26 -0400 Subject: [PATCH] [`flake8-pyi`] Fix PYI034 to not trigger on metaclasses (`PYI034`) (#20881) ## Summary Fixes #20781 --------- Co-authored-by: Alex Waygood Co-authored-by: Brent Westbrook Co-authored-by: Brent Westbrook <36778786+ntBre@users.noreply.github.com> --- .../test/fixtures/flake8_pyi/PYI034.py | 26 +++++ .../test/fixtures/flake8_pyi/PYI034.pyi | 25 +++++ .../flake8_pyi/rules/non_self_return_type.rs | 30 +++++- ...__flake8_pyi__tests__PYI034_PYI034.py.snap | 3 + ..._flake8_pyi__tests__PYI034_PYI034.pyi.snap | 5 + .../ruff_python_semantic/src/analyze/class.rs | 97 ++++++++++++++++++- 6 files changed, 184 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.py index 52a612a79e..b35efc3c21 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.py @@ -359,3 +359,29 @@ class Generic5(list[PotentialTypeVar]): def __new__(cls: type[Generic5]) -> Generic5: ... def __enter__(self: Generic5) -> Generic5: ... + +# Test cases based on issue #20781 - metaclasses that triggers IsMetaclass::Maybe +class MetaclassInWhichSelfCannotBeUsed5(type(Protocol)): + def __new__( + cls, name: str, bases: tuple[type[Any], ...], attrs: dict[str, Any], **kwargs: Any + ) -> MetaclassInWhichSelfCannotBeUsed5: + new_class = super().__new__(cls, name, bases, attrs, **kwargs) + return new_class + + +import django.db.models.base + + +class MetaclassInWhichSelfCannotBeUsed6(django.db.models.base.ModelBase): + def __new__(cls, name: str, bases: tuple[Any, ...], attrs: dict[str, Any], **kwargs: Any) -> MetaclassInWhichSelfCannotBeUsed6: + ... + + +class MetaclassInWhichSelfCannotBeUsed7(django.db.models.base.ModelBase): + def __new__(cls, /, name: str, bases: tuple[object, ...], attrs: dict[str, object], **kwds: object) -> MetaclassInWhichSelfCannotBeUsed7: + ... + + +class MetaclassInWhichSelfCannotBeUsed8(django.db.models.base.ModelBase): + def __new__(cls, name: builtins.str, bases: tuple, attributes: dict, /, **kw) -> MetaclassInWhichSelfCannotBeUsed8: + ... diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.pyi index 9567b343ac..b22f76798b 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI034.pyi @@ -252,3 +252,28 @@ from some_module import PotentialTypeVar class Generic5(list[PotentialTypeVar]): def __new__(cls: type[Generic5]) -> Generic5: ... def __enter__(self: Generic5) -> Generic5: ... + + +# Test case based on issue #20781 - metaclass that triggers IsMetaclass::Maybe +class MetaclassInWhichSelfCannotBeUsed5(type(Protocol)): + def __new__( + cls, name: str, bases: tuple[type[Any], ...], attrs: dict[str, Any], **kwargs: Any + ) -> MetaclassInWhichSelfCannotBeUsed5: ... + + +import django.db.models.base + + +class MetaclassInWhichSelfCannotBeUsed6(django.db.models.base.ModelBase): + def __new__(cls, name: str, bases: tuple[Any, ...], attrs: dict[str, Any], **kwargs: Any) -> MetaclassInWhichSelfCannotBeUsed6: + ... + + +class MetaclassInWhichSelfCannotBeUsed7(django.db.models.base.ModelBase): + def __new__(cls, /, name: str, bases: tuple[object, ...], attrs: dict[str, object], **kwds: object) -> MetaclassInWhichSelfCannotBeUsed7: + ... + + +class MetaclassInWhichSelfCannotBeUsed8(django.db.models.base.ModelBase): + def __new__(cls, name: builtins.str, bases: tuple, attributes: dict, /, **kw) -> MetaclassInWhichSelfCannotBeUsed8: + ... diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs index 59d01ff834..d3b2f75b2d 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/non_self_return_type.rs @@ -50,6 +50,29 @@ use ruff_text_size::Ranged; /// 1. `__aiter__` methods that return `AsyncIterator`, despite the class /// inheriting directly from `AsyncIterator`. /// +/// The rule attempts to avoid flagging methods on metaclasses, since +/// [PEP 673] specifies that `Self` is disallowed in metaclasses. Ruff can +/// detect a class as being a metaclass if it inherits from a stdlib +/// metaclass such as `builtins.type` or `abc.ABCMeta`, and additionally +/// infers that a class may be a metaclass if it has a `__new__` method +/// with a similar signature to `type.__new__`. The heuristic used to +/// identify a metaclass-like `__new__` method signature is that it: +/// +/// 1. Has exactly 5 parameters (including `cls`) +/// 1. Has a second parameter annotated with `str` +/// 1. Has a third parameter annotated with a `tuple` type +/// 1. Has a fourth parameter annotated with a `dict` type +/// 1. Has a fifth parameter is keyword-variadic (`**kwargs`) +/// +/// For example, the following class would be detected as a metaclass, disabling +/// the rule: +/// +/// ```python +/// class MyMetaclass(django.db.models.base.ModelBase): +/// def __new__(cls, name: str, bases: tuple[Any, ...], attrs: dict[str, Any], **kwargs: Any) -> MyMetaclass: +/// ... +/// ``` +/// /// ## Example /// /// ```pyi @@ -87,6 +110,8 @@ use ruff_text_size::Ranged; /// /// ## References /// - [Python documentation: `typing.Self`](https://docs.python.org/3/library/typing.html#typing.Self) +/// +/// [PEP 673]: https://peps.python.org/pep-0673/#valid-locations-for-self #[derive(ViolationMetadata)] #[violation_metadata(stable_since = "v0.0.271")] pub(crate) struct NonSelfReturnType { @@ -143,7 +168,10 @@ pub(crate) fn non_self_return_type( }; // PEP 673 forbids the use of `typing(_extensions).Self` in metaclasses. - if analyze::class::is_metaclass(class_def, semantic).is_yes() { + if !matches!( + analyze::class::is_metaclass(class_def, semantic), + analyze::class::IsMetaclass::No + ) { return; } diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.py.snap index fd92d1055d..8893a346e1 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.py.snap @@ -451,6 +451,7 @@ help: Use `Self` as return type 359 + def __new__(cls) -> typing.Self: ... 360 | def __enter__(self: Generic5) -> Generic5: ... 361 | +362 | note: This is an unsafe fix and may change runtime behavior PYI034 [*] `__enter__` methods in classes like `Generic5` usually return `self` at runtime @@ -468,4 +469,6 @@ help: Use `Self` as return type - def __enter__(self: Generic5) -> Generic5: ... 360 + def __enter__(self) -> typing.Self: ... 361 | +362 | +363 | # Test cases based on issue #20781 - metaclasses that triggers IsMetaclass::Maybe note: This is an unsafe fix and may change runtime behavior diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.pyi.snap index 2018bb04f2..29c91b9a77 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI034_PYI034.pyi.snap @@ -431,6 +431,8 @@ help: Use `Self` as return type - def __new__(cls: type[Generic5]) -> Generic5: ... 253 + def __new__(cls) -> typing.Self: ... 254 | def __enter__(self: Generic5) -> Generic5: ... +255 | +256 | note: This is a display-only fix and is likely to be incorrect PYI034 [*] `__enter__` methods in classes like `Generic5` usually return `self` at runtime @@ -447,4 +449,7 @@ help: Use `Self` as return type 253 | def __new__(cls: type[Generic5]) -> Generic5: ... - def __enter__(self: Generic5) -> Generic5: ... 254 + def __enter__(self) -> typing.Self: ... +255 | +256 | +257 | # Test case based on issue #20781 - metaclass that triggers IsMetaclass::Maybe note: This is a display-only fix and is likely to be incorrect diff --git a/crates/ruff_python_semantic/src/analyze/class.rs b/crates/ruff_python_semantic/src/analyze/class.rs index 0cb3a8a7f6..14f6b2c983 100644 --- a/crates/ruff_python_semantic/src/analyze/class.rs +++ b/crates/ruff_python_semantic/src/analyze/class.rs @@ -317,6 +317,91 @@ impl IsMetaclass { } } +/// Check if a class has a metaclass-like `__new__` method signature. +/// +/// A metaclass-like `__new__` method signature has: +/// 1. Exactly 5 parameters (including cls) +/// 2. Second parameter annotated with `str` +/// 3. Third parameter annotated with a `tuple` type +/// 4. Fourth parameter annotated with a `dict` type +/// 5. Fifth parameter is keyword-variadic (`**kwargs`) +/// +/// For example: +/// +/// ```python +/// class MyMetaclass(django.db.models.base.ModelBase): +/// def __new__(cls, name: str, bases: tuple[Any, ...], attrs: dict[str, Any], **kwargs: Any) -> MyMetaclass: +/// ... +/// ``` +fn has_metaclass_new_signature(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> bool { + // Look for a __new__ method in the class body + for stmt in &class_def.body { + let ast::Stmt::FunctionDef(ast::StmtFunctionDef { + name, parameters, .. + }) = stmt + else { + continue; + }; + + if name != "__new__" { + continue; + } + + // Check if we have exactly 5 parameters (cls + 4 others) + if parameters.len() != 5 { + continue; + } + + // Check that there is no variadic parameter + if parameters.vararg.is_some() { + continue; + } + + // Check that the last parameter is keyword-variadic (**kwargs) + if parameters.kwarg.is_none() { + continue; + } + + // Check parameter annotations, skipping the first parameter (cls) + let mut param_iter = parameters.iter().skip(1); + + // Check second parameter (name: str) + let Some(second_param) = param_iter.next() else { + continue; + }; + if !second_param + .annotation() + .is_some_and(|annotation| semantic.match_builtin_expr(map_subscript(annotation), "str")) + { + continue; + } + + // Check third parameter (bases: tuple[...]) + let Some(third_param) = param_iter.next() else { + continue; + }; + if !third_param.annotation().is_some_and(|annotation| { + semantic.match_builtin_expr(map_subscript(annotation), "tuple") + }) { + continue; + } + + // Check fourth parameter (attrs: dict[...]) + let Some(fourth_param) = param_iter.next() else { + continue; + }; + if !fourth_param.annotation().is_some_and(|annotation| { + semantic.match_builtin_expr(map_subscript(annotation), "dict") + }) { + continue; + } + + return true; + } + + false +} + /// Returns `IsMetaclass::Yes` if the given class is definitely a metaclass, /// `IsMetaclass::No` if it's definitely *not* a metaclass, and /// `IsMetaclass::Maybe` otherwise. @@ -349,7 +434,17 @@ pub fn is_metaclass(class_def: &ast::StmtClassDef, semantic: &SemanticModel) -> match (is_base_class, maybe) { (true, true) => IsMetaclass::Maybe, (true, false) => IsMetaclass::Yes, - (false, _) => IsMetaclass::No, + (false, _) => { + // If it has >1 base class and a metaclass-like signature for `__new__`, + // then it might be a metaclass. + if class_def.bases().is_empty() { + IsMetaclass::No + } else if has_metaclass_new_signature(class_def, semantic) { + IsMetaclass::Maybe + } else { + IsMetaclass::No + } + } } }