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

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,137 @@
+use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::helpers::map_subscript;
+use ruff_python_ast::{Expr, StmtClassDef};
+use ruff_source_file::Locator;
+use ruff_text_size::Ranged;
+
+use crate::checkers::ast::Checker;
+
+/// ## What it does
+/// Checks for classes inheriting from `typing.Generic[]` where `Generic[]` is
+/// not the last base class in the bases list.
+///
+/// ## Why is this bad?
+/// `Generic[]` not being the final class in the bases tuple can cause
+/// unexpected behaviour at runtime (See [this CPython issue][1] for 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: &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 raise issue.
+    if generic_base.range() == last_base.range() {
+        return;
+    }
+
+    let multiple_generic_bases = generic_base_iter.next().is_some();
+    let diagnostic = {
+        if multiple_generic_bases {
+            // No fix if multiple generics are seen in the class bases.
+            Diagnostic::new(GenericNotLastBaseClass, bases.range())
+        } else {
+            Diagnostic::new(GenericNotLastBaseClass, bases.range()).with_fix(generate_fix(
+                generic_base,
+                last_base,
+                checker.locator(),
+            ))
+        }
+    };
+
+    checker.diagnostics.push(diagnostic);
+}
+
+fn generate_fix(generic_base: &Expr, last_base: &Expr, locator: &Locator) -> Fix {
+    let comma_after_generic_base = generic_base.end().to_usize()
+        + locator
+            .after(generic_base.end())
+            .find(',')
+            .expect("Comma must always exist after generic base");
+
+    let last_whitespace = (comma_after_generic_base + 1)
+        + locator.contents()[comma_after_generic_base + 1..]
+            .bytes()
+            .position(|b| !b.is_ascii_whitespace())
+            .expect("Non whitespace character must always exist after Generic[]");
+
+    let comma_after_generic_base: u32 = comma_after_generic_base.try_into().unwrap();
+    let last_whitespace: u32 = last_whitespace.try_into().unwrap();
+
+    let base_deletion = Edit::deletion(generic_base.start(), generic_base.end());
+    let base_comma_deletion =
+        Edit::deletion(comma_after_generic_base.into(), last_whitespace.into());
+    let insertion = Edit::insertion(
+        format!(", {}", locator.slice(generic_base.range())),
+        last_base.end(),
+    );
+    Fix::safe_edits(insertion, [base_deletion, base_comma_deletion])
+}

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,114 @@
+---
+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
+   33 |+      # comment about Generic[T]  # PYI059
+34 34 |     # another comment?
+35    |-    ,  # comment about the comma?
+   35 |+    # comment about the comma?
+36 36 |     # part 1 of multiline comment 2
+37 37 |     # part 2 of multiline comment 2
+38    |-    int,  # comment about int
+   38 |+    int, Generic[T],  # comment about int
+39 39 |     # yet another comment?
+40 40 | ):  # and another one for good measure
+41 41 |     ...
+
+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