From ce8110332c94dcda5402323cbfb8bcd2cfdd8da4 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Wed, 22 Jan 2025 20:19:13 -0500 Subject: [PATCH] [`pyupgrade`] Handle multiple base classes for PEP 695 generics (`UP046`) (#15659) ## Summary Addresses the second follow up to #15565 in #15642. This was easier than expected by using this cool destructuring syntax I hadn't used before, and by assuming [PYI059](https://docs.astral.sh/ruff/rules/generic-not-last-base-class/) (`generic-not-last-base-class`). ## Test Plan Using an existing test, plus two new tests combining multiple base classes and multiple generics. It looks like I deleted a relevant test, which I did, but I meant to rename this in #15565. It looks like instead I copied it and renamed the copy. --------- Co-authored-by: Alex Waygood --- .../test/fixtures/pyupgrade/UP046_0.py | 56 ++++++- .../rules/pep695/non_pep695_generic_class.rs | 76 ++++++--- ...__rules__pyupgrade__tests__UP046_0.py.snap | 153 ++++++++++++++++++ 3 files changed, 257 insertions(+), 28 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP046_0.py b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP046_0.py index 6af12c47f0df9..1d3bcc58be03f 100644 --- a/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP046_0.py +++ b/crates/ruff_linter/resources/test/fixtures/pyupgrade/UP046_0.py @@ -49,6 +49,57 @@ class MultipleBaseClasses(list, Generic[T]): var: T +# these are just for the MoreBaseClasses and MultipleBaseAndGenerics cases +class Base1: ... + + +class Base2: ... + + +class Base3: ... + + +class MoreBaseClasses(Base1, Base2, Base3, Generic[T]): + var: T + + +class MultipleBaseAndGenerics(Base1, Base2, Base3, Generic[S, T, *Ts, P]): + var: S + typ: T + tup: tuple[*Ts] + pep: P + + +class A(Generic[T]): ... + + +class B(A[S], Generic[S]): + var: S + + +class C(A[S], Generic[S, T]): + var: tuple[S, T] + + +class D(A[int], Generic[T]): + var: T + + +class NotLast(Generic[T], Base1): + var: T + + +class Sandwich(Base1, Generic[T], Base2): + var: T + + +# runtime `TypeError` to inherit from `Generic` multiple times, but we still +# emit a diagnostic +class TooManyGenerics(Generic[T], Generic[S]): + var: T + var: S + + # These cases are not handled class D(Generic[T, T]): # duplicate generic variable, runtime error pass @@ -71,11 +122,6 @@ def more_generic(u: U, t: T) -> tuple[U, T]: return (u, t) -# TODO(brent) we should also handle multiple base classes -class Multiple(NotGeneric, Generic[T]): - pass - - # TODO(brent) default requires 3.13 V = TypeVar("V", default=Any, bound=str) diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/non_pep695_generic_class.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/non_pep695_generic_class.rs index 3a46fa1521476..5c29f31cfc163 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/non_pep695_generic_class.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/pep695/non_pep695_generic_class.rs @@ -1,11 +1,12 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast::visitor::Visitor; -use ruff_python_ast::StmtClassDef; -use ruff_python_ast::{Expr, ExprSubscript}; +use ruff_python_ast::{Arguments, ExprSubscript, StmtClassDef}; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::fix::edits::{remove_argument, Parentheses}; use crate::settings::types::PythonVersion; use super::{check_type_vars, in_nested_context, DisplayTypeVars, TypeVarReferenceVisitor}; @@ -21,10 +22,9 @@ use super::{check_type_vars, in_nested_context, DisplayTypeVars, TypeVarReferenc /// /// ## Known problems /// -/// The rule currently skips generic classes with multiple base classes. It also skips -/// generic classes nested inside of other -/// functions or classes. Finally, this rule skips type parameters with the `default` argument -/// introduced in [PEP 696] and implemented in Python 3.13. +/// The rule currently skips generic classes nested inside of other functions or classes. It also +/// skips type parameters with the `default` argument introduced in [PEP 696] and implemented in +/// Python 3.13. /// /// This rule can only offer a fix if all of the generic types in the class definition are defined /// in the current module. For external type parameters, a diagnostic is emitted without a suggested @@ -65,6 +65,11 @@ use super::{check_type_vars, in_nested_context, DisplayTypeVars, TypeVarReferenc /// [`unused-private-type-var`](unused-private-type-var.md) for a rule to clean up unused /// private type variables. /// +/// This rule will correctly handle classes with multiple base classes, as long as the single +/// `Generic` base class is at the end of the argument list, as checked by +/// [`generic-not-last-base-class`](generic-not-last-base-class.md). If a `Generic` base class is +/// found outside of the last position, a diagnostic is emitted without a suggested fix. +/// /// This rule only applies to generic classes and does not include generic functions. See /// [`non-pep695-generic-function`](non-pep695-generic-function.md) for the function version. /// @@ -118,22 +123,12 @@ pub(crate) fn non_pep695_generic_class(checker: &mut Checker, class_def: &StmtCl return; }; - // TODO(brent) only accept a single, Generic argument for now. I think it should be fine to have - // other arguments, but this simplifies the fix just to delete the argument list for now - let [Expr::Subscript(ExprSubscript { - value, - slice, - range, - .. - })] = arguments.args.as_ref() + let Some((generic_idx, generic_expr @ ExprSubscript { slice, range, .. })) = + find_generic(arguments, checker.semantic()) else { return; }; - if !checker.semantic().match_typing_expr(value, "Generic") { - return; - } - let mut diagnostic = Diagnostic::new( NonPEP695GenericClass { name: name.to_string(), @@ -141,6 +136,19 @@ pub(crate) fn non_pep695_generic_class(checker: &mut Checker, class_def: &StmtCl *range, ); + // only handle the case where Generic is at the end of the argument list, in line with PYI059 + // (generic-not-last-base-class). If it comes elsewhere, it results in a runtime error. In stubs + // it's not *strictly* necessary for `Generic` to come last in the bases tuple, but it would + // cause more complication for us to handle stubs specially, and probably isn't worth the + // bother. we still offer a diagnostic here but not a fix + // + // because `find_generic` also finds the *first* Generic argument, this has the additional + // benefit of bailing out with a diagnostic if multiple Generic arguments are present + if generic_idx != arguments.len() - 1 { + checker.diagnostics.push(diagnostic); + return; + } + let mut visitor = TypeVarReferenceVisitor { vars: vec![], semantic: checker.semantic(), @@ -179,12 +187,34 @@ pub(crate) fn non_pep695_generic_class(checker: &mut Checker, class_def: &StmtCl source: checker.source(), }; - diagnostic.set_fix(Fix::unsafe_edit(Edit::replacement( - type_params.to_string(), - name.end(), - arguments.end(), - ))); + diagnostic.try_set_fix(|| { + let removal_edit = remove_argument( + generic_expr, + arguments, + Parentheses::Remove, + checker.source(), + )?; + Ok(Fix::unsafe_edits( + Edit::insertion(type_params.to_string(), name.end()), + [removal_edit], + )) + }); } checker.diagnostics.push(diagnostic); } + +/// Search `class_bases` for a `typing.Generic` base class. Returns the `Generic` expression (if +/// any), along with its index in the class's bases tuple. +fn find_generic<'a>( + class_bases: &'a Arguments, + semantic: &SemanticModel, +) -> Option<(usize, &'a ExprSubscript)> { + class_bases.args.iter().enumerate().find_map(|(idx, expr)| { + expr.as_subscript_expr().and_then(|sub_expr| { + semantic + .match_typing_expr(&sub_expr.value, "Generic") + .then_some((idx, sub_expr)) + }) + }) +} diff --git a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP046_0.py.snap b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP046_0.py.snap index 2b643d7ecd8ae..c15a773704cd2 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP046_0.py.snap +++ b/crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP046_0.py.snap @@ -124,3 +124,156 @@ UP046_0.py:41:24: UP046 [*] Generic class `MultipleGenerics` uses `Generic` subc 42 42 | var: S 43 43 | typ: T 44 44 | tup: tuple[*Ts] + +UP046_0.py:48:33: UP046 [*] Generic class `MultipleBaseClasses` uses `Generic` subclass instead of type parameters + | +48 | class MultipleBaseClasses(list, Generic[T]): + | ^^^^^^^^^^ UP046 +49 | var: T + | + = help: Use type parameters + +ℹ Unsafe fix +45 45 | pep: P +46 46 | +47 47 | +48 |-class MultipleBaseClasses(list, Generic[T]): + 48 |+class MultipleBaseClasses[T: float](list): +49 49 | var: T +50 50 | +51 51 | + +UP046_0.py:62:44: UP046 [*] Generic class `MoreBaseClasses` uses `Generic` subclass instead of type parameters + | +62 | class MoreBaseClasses(Base1, Base2, Base3, Generic[T]): + | ^^^^^^^^^^ UP046 +63 | var: T + | + = help: Use type parameters + +ℹ Unsafe fix +59 59 | class Base3: ... +60 60 | +61 61 | +62 |-class MoreBaseClasses(Base1, Base2, Base3, Generic[T]): + 62 |+class MoreBaseClasses[T: float](Base1, Base2, Base3): +63 63 | var: T +64 64 | +65 65 | + +UP046_0.py:66:52: UP046 [*] Generic class `MultipleBaseAndGenerics` uses `Generic` subclass instead of type parameters + | +66 | class MultipleBaseAndGenerics(Base1, Base2, Base3, Generic[S, T, *Ts, P]): + | ^^^^^^^^^^^^^^^^^^^^^ UP046 +67 | var: S +68 | typ: T + | + = help: Use type parameters + +ℹ Unsafe fix +63 63 | var: T +64 64 | +65 65 | +66 |-class MultipleBaseAndGenerics(Base1, Base2, Base3, Generic[S, T, *Ts, P]): + 66 |+class MultipleBaseAndGenerics[S: (str, bytes), T: float, *Ts, **P](Base1, Base2, Base3): +67 67 | var: S +68 68 | typ: T +69 69 | tup: tuple[*Ts] + +UP046_0.py:73:9: UP046 [*] Generic class `A` uses `Generic` subclass instead of type parameters + | +73 | class A(Generic[T]): ... + | ^^^^^^^^^^ UP046 + | + = help: Use type parameters + +ℹ Unsafe fix +70 70 | pep: P +71 71 | +72 72 | +73 |-class A(Generic[T]): ... + 73 |+class A[T: float]: ... +74 74 | +75 75 | +76 76 | class B(A[S], Generic[S]): + +UP046_0.py:76:15: UP046 [*] Generic class `B` uses `Generic` subclass instead of type parameters + | +76 | class B(A[S], Generic[S]): + | ^^^^^^^^^^ UP046 +77 | var: S + | + = help: Use type parameters + +ℹ Unsafe fix +73 73 | class A(Generic[T]): ... +74 74 | +75 75 | +76 |-class B(A[S], Generic[S]): + 76 |+class B[S: (str, bytes)](A[S]): +77 77 | var: S +78 78 | +79 79 | + +UP046_0.py:80:15: UP046 [*] Generic class `C` uses `Generic` subclass instead of type parameters + | +80 | class C(A[S], Generic[S, T]): + | ^^^^^^^^^^^^^ UP046 +81 | var: tuple[S, T] + | + = help: Use type parameters + +ℹ Unsafe fix +77 77 | var: S +78 78 | +79 79 | +80 |-class C(A[S], Generic[S, T]): + 80 |+class C[S: (str, bytes), T: float](A[S]): +81 81 | var: tuple[S, T] +82 82 | +83 83 | + +UP046_0.py:84:17: UP046 [*] Generic class `D` uses `Generic` subclass instead of type parameters + | +84 | class D(A[int], Generic[T]): + | ^^^^^^^^^^ UP046 +85 | var: T + | + = help: Use type parameters + +ℹ Unsafe fix +81 81 | var: tuple[S, T] +82 82 | +83 83 | +84 |-class D(A[int], Generic[T]): + 84 |+class D[T: float](A[int]): +85 85 | var: T +86 86 | +87 87 | + +UP046_0.py:88:15: UP046 Generic class `NotLast` uses `Generic` subclass instead of type parameters + | +88 | class NotLast(Generic[T], Base1): + | ^^^^^^^^^^ UP046 +89 | var: T + | + = help: Use type parameters + +UP046_0.py:92:23: UP046 Generic class `Sandwich` uses `Generic` subclass instead of type parameters + | +92 | class Sandwich(Base1, Generic[T], Base2): + | ^^^^^^^^^^ UP046 +93 | var: T + | + = help: Use type parameters + +UP046_0.py:98:23: UP046 Generic class `TooManyGenerics` uses `Generic` subclass instead of type parameters + | + 96 | # runtime `TypeError` to inherit from `Generic` multiple times, but we still + 97 | # emit a diagnostic + 98 | class TooManyGenerics(Generic[T], Generic[S]): + | ^^^^^^^^^^ UP046 + 99 | var: T +100 | var: S + | + = help: Use type parameters