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

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

@@ -0,0 +1,33 @@
+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]],
+):
+    ...
+
+
+# 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,29 @@
+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]],
+):
+    ...
+
+
+# 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,13 @@ 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,
+                    arguments.as_deref(),
+                );
+            }
             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,174 @@
+use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
+use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::identifier::Identifier;
+use ruff_python_ast::{self as ast, Arguments, Expr, StmtClassDef};
+use ruff_python_semantic::SemanticModel;
+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[]`, but `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).
+/// In a stub file, however, this rule is enforced purely 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]],
+/// ):
+///     ...
+/// ```
+///
+/// ```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 be the last base class".to_string())
+    }
+}
+
+/// PYI059
+pub(crate) fn generic_not_last_base_class(
+    checker: &mut Checker,
+    class_def: &StmtClassDef,
+    bases: Option<&Arguments>,
+) {
+    let Some(bases) = bases else {
+        return;
+    };
+
+    let semantic = checker.semantic();
+
+    let generic_base_indices: Vec<usize> = bases
+        .args
+        .iter()
+        .enumerate()
+        .filter_map(|(base_index, base)| {
+            if is_generic(base, semantic) {
+                return Some(base_index);
+            }
+            None
+        })
+        .collect();
+
+    if generic_base_indices.is_empty() {
+        return;
+    }
+
+    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;
+        }
+
+        let mut diagnostic = Diagnostic::new(GenericNotLastBaseClass, class_def.identifier());
+        diagnostic.set_fix(generate_fix(bases, base_index, checker.locator()));
+        checker.diagnostics.push(diagnostic);
+    } else {
+        // No fix if multiple generics are seen in the class bases.
+        checker.diagnostics.push(Diagnostic::new(
+            GenericNotLastBaseClass,
+            class_def.identifier(),
+        ));
+    }
+}
+
+/// Return `true` if the given expression resolves to `typing.Generic[...]`.
+fn is_generic(expr: &Expr, semantic: &SemanticModel) -> bool {
+    if !semantic.seen_typing() {
+        return false;
+    }
+
+    let Expr::Subscript(ast::ExprSubscript { value, .. }) = expr else {
+        return false;
+    };
+
+    let qualified_name = semantic.resolve_qualified_name(value);
+    qualified_name.as_ref().is_some_and(|qualified_name| {
+        semantic.match_typing_qualified_name(qualified_name, "Generic")
+    })
+}
+
+// let call_start = Edit::deletion(call.start(), argument.start());
+
+// // Delete from the start of the call to the start of the argument.
+
+// // Delete from the end of the argument to the end of the call.
+// let call_end = Edit::deletion(argument.end(), call.end());
+
+// // If this is a tuple, we also need to convert the inner argument to a list.
+// if argument.is_tuple_expr() {
+//     // Replace `(` with `[`.
+//     let argument_start = Edit::replacement(
+//         "[".to_string(),
+//         argument.start(),
+//         argument.start() + TextSize::from(1),
+//     );
+
+//     // Replace `)` with `]`.
+//     let argument_end = Edit::replacement(
+//         "]".to_string(),
+//         argument.end() - TextSize::from(1),
+//         argument.end(),
+//     );
+
+//     Fix::unsafe_edits(call_start, [argument_start, argument_end, call_end])
+// } else {
+//     Fix::unsafe_edits(call_start, [call_end])
+// }
+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 next_base = bases
+        .args
+        .get(generic_base_index + 1)
+        .expect("Generic base should never be the last base during auto-fix");
+
+    let deletion = Edit::deletion(generic_base.start(), next_base.start());
+    let insertion = Edit::insertion(
+        format!(", {}", locator.slice(generic_base.range())),
+        last_base.end(),
+    );
+    Fix::safe_edits(insertion, [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,54 @@
+---
+source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs
+---
+PYI059.py:8:7: 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 be the last base class
+
+ℹ 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:7: PYI059 [*] `Generic[]` should always be the last base class
+   |
+13 |         self._items.append(item)
+14 | 
+15 | class MyMapping(  # PYI059
+   |       ^^^^^^^^^ PYI059
+16 |     t.Generic[K, V],
+17 |     Iterable[Tuple[K, V]],
+   |
+   = help: Move `Generic[]` to be the last base class
+
+ℹ 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:24:7: PYI059 `Generic[]` should always be the last base class
+   |
+23 | # in case of multiple Generic[] inheritance, don't fix it.
+24 | class C(Generic[T], Generic[K, V]): ...  # PYI059
+   |       ^ PYI059
+   |
+   = help: Move `Generic[]` to be the last base class

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

@@ -0,0 +1,54 @@
+---
+source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs
+---
+PYI059.pyi:8:7: 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 |     def push(self, item: T) -> None: ...
+   |
+   = help: Move `Generic[]` to be the last base class
+
+ℹ 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 |     def push(self, item: T) -> None: ...
+11 11 | 
+
+PYI059.pyi:12:7: PYI059 [*] `Generic[]` should always be the last base class
+   |
+10 |     def push(self, item: T) -> None: ...
+11 | 
+12 | class MyMapping(  # PYI059
+   |       ^^^^^^^^^ PYI059
+13 |     t.Generic[K, V],
+14 |     Iterable[Tuple[K, V]],
+   |
+   = help: Move `Generic[]` to be the last base class
+
+ℹ Safe fix
+10 10 |     def push(self, item: T) -> None: ...
+11 11 | 
+12 12 | class MyMapping(  # PYI059
+13    |-    t.Generic[K, V],
+14 13 |     Iterable[Tuple[K, V]],
+15    |-    Container[Tuple[K, V]],
+   14 |+    Container[Tuple[K, V]], t.Generic[K, V],
+16 15 | ):
+17 16 |     ...
+18 17 | 
+
+PYI059.pyi:21:7: PYI059 `Generic[]` should always be the last base class
+   |
+20 | # in case of multiple Generic[] inheritance, don't fix it.
+21 | class C(Generic[T], Generic[K, V]): ...  # PYI059
+   |       ^ PYI059
+   |
+   = help: Move `Generic[]` to be the last base class