[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,146 @@
+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::{Arguments, 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 generic_base_indices: Vec<usize> = bases
+        .args
+        .iter()
+        .enumerate()
+        .filter_map(|(base_index, base)| {
+            if semantic.match_typing_expr(map_subscript(base), "Generic") {
+                return Some(base_index);
+            }
+            None
+        })
+        .collect();
+
+    if generic_base_indices.is_empty() {
+        return;
+    }
+
+    let diagnostic = {
+        if generic_base_indices.len() == 1 {
+            let base_index = generic_base_indices[0];
+            if base_index == bases.args.len() - 1 {
+                // Don't raise issue for the last base.
+                return;
+            }
+
+            Diagnostic::new(GenericNotLastBaseClass, bases.range()).with_fix(generate_fix(
+                bases,
+                base_index,
+                checker.locator(),
+            ))
+        } else {
+            // No fix if multiple generics are seen in the class bases.
+            Diagnostic::new(GenericNotLastBaseClass, bases.range())
+        }
+    };
+
+    checker.diagnostics.push(diagnostic);
+}
+
+fn generate_fix(bases: &Arguments, generic_base_index: usize, locator: &Locator) -> Fix {
+    let last_base = bases.args.last().expect("Last base should always exist");
+    let generic_base = bases
+        .args
+        .get(generic_base_index)
+        .expect("Generic base should always exist");
+
+    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;