From bc3f4fa3bcae3e61b31f02c106e7cc705e66a1ff Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Tue, 7 May 2024 15:37:56 +0530 Subject: [PATCH] [`flake8-pyi`] Implement `PYI059` (`generic-not-last-base-class`) (#11233) --- .../test/fixtures/flake8_pyi/PYI059.py | 54 ++++++++ .../test/fixtures/flake8_pyi/PYI059.pyi | 48 +++++++ .../src/checkers/ast/analyze/statement.rs | 3 + crates/ruff_linter/src/codes.rs | 1 + .../ruff_linter/src/rules/flake8_pyi/mod.rs | 2 + .../rules/generic_not_last_base_class.rs | 122 ++++++++++++++++++ .../src/rules/flake8_pyi/rules/mod.rs | 2 + ...__flake8_pyi__tests__PYI059_PYI059.py.snap | 113 ++++++++++++++++ ..._flake8_pyi__tests__PYI059_PYI059.pyi.snap | 112 ++++++++++++++++ ruff.schema.json | 1 + 10 files changed, 458 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.pyi create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.pyi.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.py new file mode 100644 index 0000000000000..0e4d877d4b485 --- /dev/null +++ b/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 diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI059.pyi new file mode 100644 index 0000000000000..1e0cbd14cbd31 --- /dev/null +++ b/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 + ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index fcdc1af2c3828..cc9fd59ed2b9f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/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); } diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 07f610a9cc078..d6f675f19db1f 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/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), diff --git a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs index 004aacd280bfe..75a5387db3c08 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs +++ b/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"))] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/generic_not_last_base_class.rs new file mode 100644 index 0000000000000..c08f74870a867 --- /dev/null +++ b/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 { + 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 { + 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])) +} diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs index 851b0660d113e..dbf28eefd7327 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs +++ b/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; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.py.snap new file mode 100644 index 0000000000000..bcb28618ff3c8 --- /dev/null +++ b/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 diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.pyi.snap new file mode 100644 index 0000000000000..eec9fa9f22c4c --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI059_PYI059.pyi.snap @@ -0,0 +1,112 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- +PYI059.pyi: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 | def push(self, item: T) -> None: ... + | + = 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 | def push(self, item: T) -> None: ... +11 11 | + +PYI059.pyi:12:16: PYI059 [*] `Generic[]` should always be the last base class + | +10 | def push(self, item: T) -> None: ... +11 | +12 | class MyMapping( # PYI059 + | ________________^ +13 | | t.Generic[K, V], +14 | | Iterable[Tuple[K, V]], +15 | | Container[Tuple[K, V]], +16 | | ): + | |_^ PYI059 +17 | ... + | + = help: Move `Generic[]` to the end + +ℹ 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:22:10: PYI059 [*] `Generic[]` should always be the last base class + | +20 | # to flag this issue in this case as well, since after fixing the error +21 | # the Generic's position issue persists. +22 | class Foo(Generic, LinkedList): ... # PYI059 + | ^^^^^^^^^^^^^^^^^^^^^ PYI059 + | + = help: Move `Generic[]` to the end + +ℹ Safe fix +19 19 | # Inheriting from just `Generic` is a TypeError, but it's probably fine +20 20 | # to flag this issue in this case as well, since after fixing the error +21 21 | # the Generic's position issue persists. +22 |-class Foo(Generic, LinkedList): ... # PYI059 + 22 |+class Foo(LinkedList, Generic): ... # PYI059 +23 23 | +24 24 | +25 25 | class Foo( # comment about the bracket + +PYI059.pyi:25:10: PYI059 [*] `Generic[]` should always be the last base class + | +25 | class Foo( # comment about the bracket + | __________^ +26 | | # Part 1 of multiline comment 1 +27 | | # Part 2 of multiline comment 1 +28 | | Generic[T] # comment about Generic[T] # PYI059 +29 | | # another comment? +30 | | , # comment about the comma? +31 | | # part 1 of multiline comment 2 +32 | | # part 2 of multiline comment 2 +33 | | int, # comment about int +34 | | # yet another comment? +35 | | ): # and another one for good measure + | |_^ PYI059 +36 | ... + | + = help: Move `Generic[]` to the end + +ℹ Safe fix +25 25 | class Foo( # comment about the bracket +26 26 | # Part 1 of multiline comment 1 +27 27 | # Part 2 of multiline comment 1 +28 |- Generic[T] # comment about Generic[T] # PYI059 +29 |- # another comment? +30 |- , # comment about the comma? + 28 |+ # comment about the comma? +31 29 | # part 1 of multiline comment 2 +32 30 | # part 2 of multiline comment 2 +33 |- int, # comment about int + 31 |+ int, Generic[T], # comment about int +34 32 | # yet another comment? +35 33 | ): # and another one for good measure +36 34 | ... + +PYI059.pyi:40:8: PYI059 `Generic[]` should always be the last base class + | +39 | # in case of multiple Generic[] inheritance, don't fix it. +40 | class C(Generic[T], Generic[K, V]): ... # PYI059 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI059 + | + = help: Move `Generic[]` to the end diff --git a/ruff.schema.json b/ruff.schema.json index 21a4165b5b315..4b374adba80b8 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3573,6 +3573,7 @@ "PYI055", "PYI056", "PYI058", + "PYI059", "Q", "Q0", "Q00",