[flake8-pyi] Implement PYI059 (generic-not-last-base-class) (#1…

…1233)

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py

@@ -0,0 +1,54 @@
+from typing import Container, Generic, Iterable, List, Sized, Tuple, TypeVar
+import typing as t
+
+T = TypeVar('T')
+K = TypeVar('K')
+V = TypeVar('V')
+
+class LinkedList(Generic[T], Sized):  # PYI059
+    def __init__(self) -> None:
+        self._items: List[T] = []
+
+    def push(self, item: T) -> None:
+        self._items.append(item)
+
+class MyMapping(  # PYI059
+    t.Generic[K, V],
+    Iterable[Tuple[K, V]],
+    Container[Tuple[K, V]],
+):
+    ...
+
+
+# Inheriting from just `Generic` is a TypeError, but it's probably fine
+# to flag this issue in this case as well, since after fixing the error
+# the Generic's position issue persists.
+class Foo(Generic, LinkedList):  # PYI059
+    pass
+
+
+class Foo(  # comment about the bracket
+    # Part 1 of multiline comment 1
+    # Part 2 of multiline comment 1
+    Generic[T]  # comment about Generic[T]  # PYI059
+    # another comment?
+    ,  # comment about the comma?
+    # part 1 of multiline comment 2
+    # part 2 of multiline comment 2
+    int,  # comment about int
+    # yet another comment?
+):  # and another one for good measure
+    ...
+
+
+# in case of multiple Generic[] inheritance, don't fix it.
+class C(Generic[T], Generic[K, V]): ...  # PYI059
+
+
+# Negative cases
+class MyList(Sized, Generic[T]):  # Generic already in last place
+    def __init__(self) -> None:
+        self._items: List[T] = []
+
+class SomeGeneric(Generic[T]):  # Only one generic
+    pass

crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.pyi

@@ -0,0 +1,48 @@
+from typing import Container, Generic, Iterable, Sized, Tuple, TypeVar
+import typing as t
+
+T = TypeVar('T')
+K = TypeVar('K')
+V = TypeVar('V')
+
+class LinkedList(Generic[T], Sized):  # PYI059
+    def __init__(self) -> None: ...
+    def push(self, item: T) -> None: ...
+
+class MyMapping(  # PYI059
+    t.Generic[K, V],
+    Iterable[Tuple[K, V]],
+    Container[Tuple[K, V]],
+):
+    ...
+
+# Inheriting from just `Generic` is a TypeError, but it's probably fine
+# to flag this issue in this case as well, since after fixing the error
+# the Generic's position issue persists.
+class Foo(Generic, LinkedList): ...  # PYI059
+
+
+class Foo(  # comment about the bracket
+    # Part 1 of multiline comment 1
+    # Part 2 of multiline comment 1
+    Generic[T]  # comment about Generic[T]  # PYI059
+    # another comment?
+    ,  # comment about the comma?
+    # part 1 of multiline comment 2
+    # part 2 of multiline comment 2
+    int,  # comment about int
+    # yet another comment?
+):  # and another one for good measure
+    ...
+
+
+# in case of multiple Generic[] inheritance, don't fix it.
+class C(Generic[T], Generic[K, V]): ...  # PYI059
+
+
+# Negative cases
+class MyList(Sized, Generic[T]):  # Generic already in last place
+    def __init__(self) -> None: ...
+
+class SomeGeneric(Generic[T]):  # Only one generic
+    ...

crates/ruff_linter/src/checkers/ast/analyze/statement.rs

@@ -478,6 +478,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
             if checker.enabled(Rule::EllipsisInNonEmptyClassBody) {
                 flake8_pyi::rules::ellipsis_in_non_empty_class_body(checker, body);
             }
+            if checker.enabled(Rule::GenericNotLastBaseClass) {
+                flake8_pyi::rules::generic_not_last_base_class(checker, class_def);
+            }
             if checker.enabled(Rule::PytestIncorrectMarkParenthesesStyle) {
                 flake8_pytest_style::rules::marks(checker, decorator_list);
             }

crates/ruff_linter/src/codes.rs

@@ -808,6 +808,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Flake8Pyi, "055") => (RuleGroup::Stable, rules::flake8_pyi::rules::UnnecessaryTypeUnion),
         (Flake8Pyi, "056") => (RuleGroup::Stable, rules::flake8_pyi::rules::UnsupportedMethodCallOnAll),
         (Flake8Pyi, "058") => (RuleGroup::Stable, rules::flake8_pyi::rules::GeneratorReturnFromIterMethod),
+        (Flake8Pyi, "059") => (RuleGroup::Preview, rules::flake8_pyi::rules::GenericNotLastBaseClass),
 
         // flake8-pytest-style
         (Flake8PytestStyle, "001") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestFixtureIncorrectParenthesesStyle),

crates/ruff_linter/src/rules/flake8_pyi/mod.rs

@@ -41,6 +41,8 @@ mod tests {
     #[test_case(Rule::FutureAnnotationsInStub, Path::new("PYI044.pyi"))]
     #[test_case(Rule::GeneratorReturnFromIterMethod, Path::new("PYI058.py"))]
     #[test_case(Rule::GeneratorReturnFromIterMethod, Path::new("PYI058.pyi"))]
+    #[test_case(Rule::GenericNotLastBaseClass, Path::new("PYI059.py"))]
+    #[test_case(Rule::GenericNotLastBaseClass, Path::new("PYI059.pyi"))]
     #[test_case(Rule::IterMethodReturnIterable, Path::new("PYI045.py"))]
     #[test_case(Rule::IterMethodReturnIterable, Path::new("PYI045.pyi"))]
     #[test_case(Rule::NoReturnArgumentAnnotationInStub, Path::new("PYI050.py"))]

crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs

@@ -0,0 +1,122 @@
+use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::{self as ast, helpers::map_subscript};
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+use crate::fix::edits::{add_argument, remove_argument, Parentheses};
+
+/// ## What it does
+/// Checks for classes inheriting from `typing.Generic[]` where `Generic[]` is
+/// not the last base class in the bases tuple.
+///
+/// ## Why is this bad?
+/// If `Generic[]` is not the final class in the bases tuple, unexpected
+/// behaviour can occur at runtime (See [this CPython issue][1] for an example).
+/// The rule is also applied to stub files, but, unlike at runtime,
+/// in stubs it is purely enforced for stylistic consistency.
+///
+/// For example:
+/// ```python
+/// class LinkedList(Generic[T], Sized):
+///     def push(self, item: T) -> None:
+///         self._items.append(item)
+///
+/// class MyMapping(
+///     Generic[K, V],
+///     Iterable[Tuple[K, V]],
+///     Container[Tuple[K, V]],
+/// ):
+///     ...
+/// ```
+///
+/// Use instead:
+/// ```python
+/// class LinkedList(Sized, Generic[T]):
+///     def push(self, item: T) -> None:
+///         self._items.append(item)
+///
+/// class MyMapping(
+///     Iterable[Tuple[K, V]],
+///     Container[Tuple[K, V]],
+///     Generic[K, V],
+/// ):
+///     ...
+/// ```
+/// ## References
+/// - [`typing.Generic` documentation](https://docs.python.org/3/library/typing.html#typing.Generic)
+///
+/// [1]: https://github.com/python/cpython/issues/106102
+#[violation]
+pub struct GenericNotLastBaseClass;
+
+impl Violation for GenericNotLastBaseClass {
+    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
+
+    #[derive_message_formats]
+    fn message(&self) -> String {
+        format!("`Generic[]` should always be the last base class")
+    }
+
+    fn fix_title(&self) -> Option<String> {
+        Some("Move `Generic[]` to the end".to_string())
+    }
+}
+
+/// PYI059
+pub(crate) fn generic_not_last_base_class(checker: &mut Checker, class_def: &ast::StmtClassDef) {
+    let Some(bases) = class_def.arguments.as_deref() else {
+        return;
+    };
+
+    let semantic = checker.semantic();
+    if !semantic.seen_typing() {
+        return;
+    }
+
+    let Some(last_base) = bases.args.last() else {
+        return;
+    };
+
+    let mut generic_base_iter = bases
+        .args
+        .iter()
+        .filter(|base| semantic.match_typing_expr(map_subscript(base), "Generic"));
+
+    let Some(generic_base) = generic_base_iter.next() else {
+        return;
+    };
+
+    // If `Generic[]` exists, but is the last base, don't emit a diagnostic.
+    if generic_base.range() == last_base.range() {
+        return;
+    }
+
+    let mut diagnostic = Diagnostic::new(GenericNotLastBaseClass, bases.range());
+
+    // No fix if multiple `Generic[]`s are seen in the class bases.
+    if generic_base_iter.next().is_none() {
+        diagnostic.try_set_fix(|| generate_fix(generic_base, bases, checker));
+    }
+
+    checker.diagnostics.push(diagnostic);
+}
+
+fn generate_fix(
+    generic_base: &ast::Expr,
+    arguments: &ast::Arguments,
+    checker: &Checker,
+) -> anyhow::Result<Fix> {
+    let locator = checker.locator();
+    let source = locator.contents();
+
+    let deletion = remove_argument(generic_base, arguments, Parentheses::Preserve, source)?;
+    let insertion = add_argument(
+        locator.slice(generic_base),
+        arguments,
+        checker.indexer().comment_ranges(),
+        source,
+    );
+
+    Ok(Fix::safe_edits(deletion, [insertion]))
+}

crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs

@@ -10,6 +10,7 @@ pub(crate) use duplicate_union_member::*;
 pub(crate) use ellipsis_in_non_empty_class_body::*;
 pub(crate) use exit_annotations::*;
 pub(crate) use future_annotations_in_stub::*;
+pub(crate) use generic_not_last_base_class::*;
 pub(crate) use iter_method_return_iterable::*;
 pub(crate) use no_return_argument_annotation::*;
 pub(crate) use non_empty_stub_body::*;
@@ -48,6 +49,7 @@ mod duplicate_union_member;
 mod ellipsis_in_non_empty_class_body;
 mod exit_annotations;
 mod future_annotations_in_stub;
+mod generic_not_last_base_class;
 mod iter_method_return_iterable;
 mod no_return_argument_annotation;
 mod non_empty_stub_body;

crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.py.snap

@@ -0,0 +1,113 @@
+---
+source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs
+---
+PYI059.py:8:17: PYI059 [*] `Generic[]` should always be the last base class
+   |
+ 6 | V = TypeVar('V')
+ 7 | 
+ 8 | class LinkedList(Generic[T], Sized):  # PYI059
+   |                 ^^^^^^^^^^^^^^^^^^^ PYI059
+ 9 |     def __init__(self) -> None:
+10 |         self._items: List[T] = []
+   |
+   = help: Move `Generic[]` to the end
+
+ℹ Safe fix
+5 5 | K = TypeVar('K')
+6 6 | V = TypeVar('V')
+7 7 | 
+8   |-class LinkedList(Generic[T], Sized):  # PYI059
+  8 |+class LinkedList(Sized, Generic[T]):  # PYI059
+9 9 |     def __init__(self) -> None:
+10 10 |         self._items: List[T] = []
+11 11 | 
+
+PYI059.py:15:16: PYI059 [*] `Generic[]` should always be the last base class
+   |
+13 |           self._items.append(item)
+14 |   
+15 |   class MyMapping(  # PYI059
+   |  ________________^
+16 | |     t.Generic[K, V],
+17 | |     Iterable[Tuple[K, V]],
+18 | |     Container[Tuple[K, V]],
+19 | | ):
+   | |_^ PYI059
+20 |       ...
+   |
+   = help: Move `Generic[]` to the end
+
+ℹ Safe fix
+13 13 |         self._items.append(item)
+14 14 | 
+15 15 | class MyMapping(  # PYI059
+16    |-    t.Generic[K, V],
+17 16 |     Iterable[Tuple[K, V]],
+18    |-    Container[Tuple[K, V]],
+   17 |+    Container[Tuple[K, V]], t.Generic[K, V],
+19 18 | ):
+20 19 |     ...
+21 20 | 
+
+PYI059.py:26:10: PYI059 [*] `Generic[]` should always be the last base class
+   |
+24 | # to flag this issue in this case as well, since after fixing the error
+25 | # the Generic's position issue persists.
+26 | class Foo(Generic, LinkedList):  # PYI059
+   |          ^^^^^^^^^^^^^^^^^^^^^ PYI059
+27 |     pass
+   |
+   = help: Move `Generic[]` to the end
+
+ℹ Safe fix
+23 23 | # Inheriting from just `Generic` is a TypeError, but it's probably fine
+24 24 | # to flag this issue in this case as well, since after fixing the error
+25 25 | # the Generic's position issue persists.
+26    |-class Foo(Generic, LinkedList):  # PYI059
+   26 |+class Foo(LinkedList, Generic):  # PYI059
+27 27 |     pass
+28 28 | 
+29 29 | 
+
+PYI059.py:30:10: PYI059 [*] `Generic[]` should always be the last base class
+   |
+30 |   class Foo(  # comment about the bracket
+   |  __________^
+31 | |     # Part 1 of multiline comment 1
+32 | |     # Part 2 of multiline comment 1
+33 | |     Generic[T]  # comment about Generic[T]  # PYI059
+34 | |     # another comment?
+35 | |     ,  # comment about the comma?
+36 | |     # part 1 of multiline comment 2
+37 | |     # part 2 of multiline comment 2
+38 | |     int,  # comment about int
+39 | |     # yet another comment?
+40 | | ):  # and another one for good measure
+   | |_^ PYI059
+41 |       ...
+   |
+   = help: Move `Generic[]` to the end
+
+ℹ Safe fix
+30 30 | class Foo(  # comment about the bracket
+31 31 |     # Part 1 of multiline comment 1
+32 32 |     # Part 2 of multiline comment 1
+33    |-    Generic[T]  # comment about Generic[T]  # PYI059
+34    |-    # another comment?
+35    |-    ,  # comment about the comma?
+   33 |+    # comment about the comma?
+36 34 |     # part 1 of multiline comment 2
+37 35 |     # part 2 of multiline comment 2
+38    |-    int,  # comment about int
+   36 |+    int, Generic[T],  # comment about int
+39 37 |     # yet another comment?
+40 38 | ):  # and another one for good measure
+41 39 |     ...
+
+PYI059.py:45:8: PYI059 `Generic[]` should always be the last base class
+   |
+44 | # in case of multiple Generic[] inheritance, don't fix it.
+45 | class C(Generic[T], Generic[K, V]): ...  # PYI059
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI059
+   |
+   = help: Move `Generic[]` to the end