From 3e3ea9fc5bf563c9033fb6c715fd813b1aa008b0 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Mon, 11 Nov 2024 13:21:07 +0100 Subject: [PATCH 1/3] [`flake8-pyi`] Improve autofix for nested and mixed type unions `unnecessary-type-union` (`PYI055`) --- .../test/fixtures/flake8_pyi/PYI055.py | 3 + .../test/fixtures/flake8_pyi/PYI055.pyi | 5 +- .../rules/unnecessary_type_union.rs | 215 +++++++++++------- ...__flake8_pyi__tests__PYI055_PYI055.py.snap | 169 ++++++++------ ..._flake8_pyi__tests__PYI055_PYI055.pyi.snap | 31 ++- 5 files changed, 260 insertions(+), 163 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.py index 2a9c2d77871cc..10c7bb8c1dc3a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.py @@ -30,6 +30,9 @@ def func(): # PYI055 x: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker y: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker + z: Union[ # comment + type[requests_mock.Mocker], # another comment + type[httpretty], type[str]] = requests_mock.Mocker def func(): diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.pyi index 530f395dfa359..61814216b1381 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.pyi @@ -16,10 +16,13 @@ z: Union[float, complex] def func(arg: type[int, float] | str) -> None: ... -# OK +# PYI055 item: type[requests_mock.Mocker] | type[httpretty] = requests_mock.Mocker def func(): # PYI055 item: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker item2: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker + item3: Union[ # comment + type[requests_mock.Mocker], # another comment + type[httpretty], type[str]] = requests_mock.Mocker diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs index e960d4c92a974..5b4c2ccfff72b 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs @@ -1,5 +1,5 @@ use ast::ExprContext; -use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::helpers::pep_604_union; use ruff_python_ast::name::Name; @@ -25,10 +25,18 @@ use crate::checkers::ast::Checker; /// ```pyi /// field: type[int | float] | str /// ``` +/// +/// ## Fix safety +/// +/// This rule's fix is marked as safe in most cases; however, the fix will +/// flatten nested unions type expressions into a single top-level union. +/// +/// The fix is marked as unsafe when comments are present within the type +/// expression. #[violation] pub struct UnnecessaryTypeUnion { members: Vec, - is_pep604_union: bool, + union_kind: UnionKind, } impl Violation for UnnecessaryTypeUnion { @@ -36,10 +44,9 @@ impl Violation for UnnecessaryTypeUnion { #[derive_message_formats] fn message(&self) -> String { - let union_str = if self.is_pep604_union { - self.members.join(" | ") - } else { - format!("Union[{}]", self.members.join(", ")) + let union_str = match self.union_kind { + UnionKind::PEP604 => self.members.join(" | "), + UnionKind::TypingUnion => format!("Union[{}]", self.members.join(", ")), }; format!( @@ -70,88 +77,48 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr) let mut type_exprs: Vec<&Expr> = Vec::new(); let mut other_exprs: Vec<&Expr> = Vec::new(); - let mut collect_type_exprs = |expr: &'a Expr, _parent: &'a Expr| match expr { - Expr::Subscript(ast::ExprSubscript { slice, value, .. }) => { - if semantic.match_builtin_expr(value, "type") { - type_exprs.push(slice); - } else { - other_exprs.push(expr); + let mut union_kind = UnionKind::TypingUnion; + let mut collect_type_exprs = |expr: &'a Expr, parent: &'a Expr| { + if matches!(parent, Expr::BinOp(_)) { + union_kind = UnionKind::PEP604; + } + match expr { + Expr::Subscript(ast::ExprSubscript { slice, value, .. }) => { + if semantic.match_builtin_expr(value, "type") { + type_exprs.push(slice); + } else { + other_exprs.push(expr); + } } + _ => other_exprs.push(expr), } - _ => other_exprs.push(expr), }; traverse_union(&mut collect_type_exprs, semantic, union); - if type_exprs.len() > 1 { - let type_members: Vec = type_exprs - .clone() - .into_iter() - .map(|type_expr| Name::new(checker.locator().slice(type_expr))) - .collect(); - - let mut diagnostic = Diagnostic::new( - UnnecessaryTypeUnion { - members: type_members.clone(), - is_pep604_union: subscript.is_none(), - }, - union.range(), - ); - - if semantic.has_builtin_binding("type") { - let content = if let Some(subscript) = subscript { - let types = &Expr::Subscript(ast::ExprSubscript { - value: Box::new(Expr::Name(ast::ExprName { - id: Name::new_static("type"), - ctx: ExprContext::Load, - range: TextRange::default(), - })), - slice: Box::new(Expr::Subscript(ast::ExprSubscript { - value: subscript.value.clone(), - slice: Box::new(Expr::Tuple(ast::ExprTuple { - elts: type_members - .into_iter() - .map(|type_member| { - Expr::Name(ast::ExprName { - id: type_member, - ctx: ExprContext::Load, - range: TextRange::default(), - }) - }) - .collect(), - ctx: ExprContext::Load, - range: TextRange::default(), - parenthesized: true, - })), - ctx: ExprContext::Load, - range: TextRange::default(), - })), - ctx: ExprContext::Load, - range: TextRange::default(), - }); - - if other_exprs.is_empty() { - checker.generator().expr(types) - } else { - let mut exprs = Vec::new(); - exprs.push(types); - exprs.extend(other_exprs); - - let union = Expr::Subscript(ast::ExprSubscript { - value: subscript.value.clone(), - slice: Box::new(Expr::Tuple(ast::ExprTuple { - elts: exprs.into_iter().cloned().collect(), - ctx: ExprContext::Load, - range: TextRange::default(), - parenthesized: true, - })), - ctx: ExprContext::Load, - range: TextRange::default(), - }); + // Return if zero or one `type` expressions are found. + if type_exprs.len() <= 1 { + return; + } - checker.generator().expr(&union) - } - } else { + let type_members: Vec = type_exprs + .clone() + .into_iter() + .map(|type_expr| Name::new(checker.locator().slice(type_expr))) + .collect(); + + let mut diagnostic = Diagnostic::new( + UnnecessaryTypeUnion { + members: type_members.clone(), + union_kind, + }, + union.range(), + ); + + if semantic.has_builtin_binding("type") { + // Construct the content for the [`Fix`] based on if we encountered a PEP604 union. + let content = match union_kind { + UnionKind::PEP604 => { let elts: Vec = type_exprs.into_iter().cloned().collect(); let types = Expr::Subscript(ast::ExprSubscript { value: Box::new(Expr::Name(ast::ExprName { @@ -172,14 +139,86 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr) .collect(); checker.generator().expr(&pep_604_union(&elts)) } - }; + } + UnionKind::TypingUnion => { + if let Some(subscript) = subscript { + let types = &Expr::Subscript(ast::ExprSubscript { + value: Box::new(Expr::Name(ast::ExprName { + id: Name::new_static("type"), + ctx: ExprContext::Load, + range: TextRange::default(), + })), + slice: Box::new(Expr::Subscript(ast::ExprSubscript { + value: subscript.value.clone(), + slice: Box::new(Expr::Tuple(ast::ExprTuple { + elts: type_members + .into_iter() + .map(|type_member| { + Expr::Name(ast::ExprName { + id: type_member, + ctx: ExprContext::Load, + range: TextRange::default(), + }) + }) + .collect(), + ctx: ExprContext::Load, + range: TextRange::default(), + parenthesized: true, + })), + ctx: ExprContext::Load, + range: TextRange::default(), + })), + ctx: ExprContext::Load, + range: TextRange::default(), + }); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - content, - union.range(), - ))); - } + if other_exprs.is_empty() { + checker.generator().expr(types) + } else { + let mut exprs = Vec::new(); + exprs.push(types); + exprs.extend(other_exprs); + + let union = Expr::Subscript(ast::ExprSubscript { + value: subscript.value.clone(), + slice: Box::new(Expr::Tuple(ast::ExprTuple { + elts: exprs.into_iter().cloned().collect(), + ctx: ExprContext::Load, + range: TextRange::default(), + parenthesized: true, + })), + ctx: ExprContext::Load, + range: TextRange::default(), + }); + + checker.generator().expr(&union) + } + } else { + return; + } + } + }; + + // Mark [`Fix`] as unsafe when comments are in range. + let applicability = if checker.comment_ranges().intersects(union.range()) { + Applicability::Unsafe + } else { + Applicability::Safe + }; - checker.diagnostics.push(diagnostic); + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement(content, union.range()), + applicability, + )); } + + checker.diagnostics.push(diagnostic); +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum UnionKind { + /// E.g., `typing.Union[int, str]` + TypingUnion, + /// E.g., `int | str` + PEP604, } diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.py.snap index e93e79a12d48a..d7fed5bd1e418 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.py.snap @@ -8,6 +8,7 @@ PYI055.py:31:8: PYI055 [*] Multiple `type` members in a union. Combine them into 31 | x: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 32 | y: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +33 | z: Union[ # comment | = help: Combine multiple `type` members @@ -18,8 +19,8 @@ PYI055.py:31:8: PYI055 [*] Multiple `type` members in a union. Combine them into 31 |- x: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker 31 |+ x: type[requests_mock.Mocker | httpretty | str] = requests_mock.Mocker 32 32 | y: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker -33 33 | -34 34 | +33 33 | z: Union[ # comment +34 34 | type[requests_mock.Mocker], # another comment PYI055.py:32:8: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[Union[requests_mock.Mocker, httpretty, str]]`. | @@ -27,6 +28,8 @@ PYI055.py:32:8: PYI055 [*] Multiple `type` members in a union. Combine them into 31 | x: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker 32 | y: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 +33 | z: Union[ # comment +34 | type[requests_mock.Mocker], # another comment | = help: Combine multiple `type` members @@ -36,109 +39,131 @@ PYI055.py:32:8: PYI055 [*] Multiple `type` members in a union. Combine them into 31 31 | x: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker 32 |- y: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker 32 |+ y: type[Union[requests_mock.Mocker, httpretty, str]] = requests_mock.Mocker -33 33 | -34 34 | -35 35 | def func(): +33 33 | z: Union[ # comment +34 34 | type[requests_mock.Mocker], # another comment +35 35 | type[httpretty], type[str]] = requests_mock.Mocker -PYI055.py:39:8: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[Union[requests_mock.Mocker, httpretty, str]]`. +PYI055.py:33:8: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[Union[requests_mock.Mocker, httpretty, str]]`. | -38 | # PYI055 -39 | x: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +31 | x: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker +32 | y: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +33 | z: Union[ # comment + | ________^ +34 | | type[requests_mock.Mocker], # another comment +35 | | type[httpretty], type[str]] = requests_mock.Mocker + | |___________________________________^ PYI055 + | + = help: Combine multiple `type` members + +ℹ Unsafe fix +30 30 | # PYI055 +31 31 | x: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker +32 32 | y: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +33 |- z: Union[ # comment +34 |- type[requests_mock.Mocker], # another comment +35 |- type[httpretty], type[str]] = requests_mock.Mocker + 33 |+ z: type[Union[requests_mock.Mocker, httpretty, str]] = requests_mock.Mocker +36 34 | +37 35 | +38 36 | def func(): + +PYI055.py:42:8: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[Union[requests_mock.Mocker, httpretty, str]]`. + | +41 | # PYI055 +42 | x: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 | = help: Combine multiple `type` members ℹ Safe fix -36 36 | from typing import Union as U -37 37 | -38 38 | # PYI055 -39 |- x: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker - 39 |+ x: type[Union[requests_mock.Mocker, httpretty, str]] = requests_mock.Mocker +39 39 | from typing import Union as U 40 40 | -41 41 | -42 42 | def convert_union(union: UnionType) -> _T | None: +41 41 | # PYI055 +42 |- x: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker + 42 |+ x: type[Union[requests_mock.Mocker, httpretty, str]] = requests_mock.Mocker +43 43 | +44 44 | +45 45 | def convert_union(union: UnionType) -> _T | None: -PYI055.py:44:9: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. +PYI055.py:47:9: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. | -42 | def convert_union(union: UnionType) -> _T | None: -43 | converters: tuple[ -44 | type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T], ... # PYI055 +45 | def convert_union(union: UnionType) -> _T | None: +46 | converters: tuple[ +47 | type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T], ... # PYI055 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 -45 | ] = union.__args__ -46 | ... +48 | ] = union.__args__ +49 | ... | = help: Combine multiple `type` members ℹ Safe fix -41 41 | -42 42 | def convert_union(union: UnionType) -> _T | None: -43 43 | converters: tuple[ -44 |- type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T], ... # PYI055 - 44 |+ type[_T | Converter[_T]] | Converter[_T] | Callable[[str], _T], ... # PYI055 -45 45 | ] = union.__args__ -46 46 | ... -47 47 | +44 44 | +45 45 | def convert_union(union: UnionType) -> _T | None: +46 46 | converters: tuple[ +47 |- type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T], ... # PYI055 + 47 |+ type[_T | Converter[_T]] | Converter[_T] | Callable[[str], _T], ... # PYI055 +48 48 | ] = union.__args__ +49 49 | ... +50 50 | -PYI055.py:50:15: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. +PYI055.py:53:15: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. | -48 | def convert_union(union: UnionType) -> _T | None: -49 | converters: tuple[ -50 | Union[type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T]], ... # PYI055 +51 | def convert_union(union: UnionType) -> _T | None: +52 | converters: tuple[ +53 | Union[type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T]], ... # PYI055 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 -51 | ] = union.__args__ -52 | ... +54 | ] = union.__args__ +55 | ... | = help: Combine multiple `type` members ℹ Safe fix -47 47 | -48 48 | def convert_union(union: UnionType) -> _T | None: -49 49 | converters: tuple[ -50 |- Union[type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T]], ... # PYI055 - 50 |+ Union[type[_T | Converter[_T]] | Converter[_T] | Callable[[str], _T]], ... # PYI055 -51 51 | ] = union.__args__ -52 52 | ... -53 53 | +50 50 | +51 51 | def convert_union(union: UnionType) -> _T | None: +52 52 | converters: tuple[ +53 |- Union[type[_T] | type[Converter[_T]] | Converter[_T] | Callable[[str], _T]], ... # PYI055 + 53 |+ Union[type[_T | Converter[_T]] | Converter[_T] | Callable[[str], _T]], ... # PYI055 +54 54 | ] = union.__args__ +55 55 | ... +56 56 | -PYI055.py:56:15: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. +PYI055.py:59:15: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. | -54 | def convert_union(union: UnionType) -> _T | None: -55 | converters: tuple[ -56 | Union[type[_T] | type[Converter[_T]]] | Converter[_T] | Callable[[str], _T], ... # PYI055 +57 | def convert_union(union: UnionType) -> _T | None: +58 | converters: tuple[ +59 | Union[type[_T] | type[Converter[_T]]] | Converter[_T] | Callable[[str], _T], ... # PYI055 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 -57 | ] = union.__args__ -58 | ... +60 | ] = union.__args__ +61 | ... | = help: Combine multiple `type` members ℹ Safe fix -53 53 | -54 54 | def convert_union(union: UnionType) -> _T | None: -55 55 | converters: tuple[ -56 |- Union[type[_T] | type[Converter[_T]]] | Converter[_T] | Callable[[str], _T], ... # PYI055 - 56 |+ Union[type[_T | Converter[_T]]] | Converter[_T] | Callable[[str], _T], ... # PYI055 -57 57 | ] = union.__args__ -58 58 | ... -59 59 | +56 56 | +57 57 | def convert_union(union: UnionType) -> _T | None: +58 58 | converters: tuple[ +59 |- Union[type[_T] | type[Converter[_T]]] | Converter[_T] | Callable[[str], _T], ... # PYI055 + 59 |+ Union[type[_T | Converter[_T]]] | Converter[_T] | Callable[[str], _T], ... # PYI055 +60 60 | ] = union.__args__ +61 61 | ... +62 62 | -PYI055.py:62:15: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. +PYI055.py:65:15: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[_T | Converter[_T]]`. | -60 | def convert_union(union: UnionType) -> _T | None: -61 | converters: tuple[ -62 | Union[type[_T] | type[Converter[_T]] | str] | Converter[_T] | Callable[[str], _T], ... # PYI055 +63 | def convert_union(union: UnionType) -> _T | None: +64 | converters: tuple[ +65 | Union[type[_T] | type[Converter[_T]] | str] | Converter[_T] | Callable[[str], _T], ... # PYI055 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 -63 | ] = union.__args__ -64 | ... +66 | ] = union.__args__ +67 | ... | = help: Combine multiple `type` members ℹ Safe fix -59 59 | -60 60 | def convert_union(union: UnionType) -> _T | None: -61 61 | converters: tuple[ -62 |- Union[type[_T] | type[Converter[_T]] | str] | Converter[_T] | Callable[[str], _T], ... # PYI055 - 62 |+ Union[type[_T | Converter[_T]] | str] | Converter[_T] | Callable[[str], _T], ... # PYI055 -63 63 | ] = union.__args__ -64 64 | ... - - +62 62 | +63 63 | def convert_union(union: UnionType) -> _T | None: +64 64 | converters: tuple[ +65 |- Union[type[_T] | type[Converter[_T]] | str] | Converter[_T] | Callable[[str], _T], ... # PYI055 + 65 |+ Union[type[_T | Converter[_T]] | str] | Converter[_T] | Callable[[str], _T], ... # PYI055 +66 66 | ] = union.__args__ +67 67 | ... diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.pyi.snap index 0e41288be5073..c94c341181514 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI055_PYI055.pyi.snap @@ -127,7 +127,7 @@ PYI055.pyi:10:15: PYI055 [*] Multiple `type` members in a union. Combine them in PYI055.pyi:20:7: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[requests_mock.Mocker | httpretty]`. | -19 | # OK +19 | # PYI055 20 | item: type[requests_mock.Mocker] | type[httpretty] = requests_mock.Mocker | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 21 | @@ -138,7 +138,7 @@ PYI055.pyi:20:7: PYI055 [*] Multiple `type` members in a union. Combine them int ℹ Safe fix 17 17 | def func(arg: type[int, float] | str) -> None: ... 18 18 | -19 19 | # OK +19 19 | # PYI055 20 |-item: type[requests_mock.Mocker] | type[httpretty] = requests_mock.Mocker 20 |+item: type[requests_mock.Mocker | httpretty] = requests_mock.Mocker 21 21 | @@ -152,6 +152,7 @@ PYI055.pyi:24:11: PYI055 [*] Multiple `type` members in a union. Combine them in 24 | item: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 25 | item2: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +26 | item3: Union[ # comment | = help: Combine multiple `type` members @@ -162,6 +163,8 @@ PYI055.pyi:24:11: PYI055 [*] Multiple `type` members in a union. Combine them in 24 |- item: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker 24 |+ item: type[requests_mock.Mocker | httpretty | str] = requests_mock.Mocker 25 25 | item2: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +26 26 | item3: Union[ # comment +27 27 | type[requests_mock.Mocker], # another comment PYI055.pyi:25:12: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[Union[requests_mock.Mocker, httpretty, str]]`. | @@ -169,6 +172,8 @@ PYI055.pyi:25:12: PYI055 [*] Multiple `type` members in a union. Combine them in 24 | item: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker 25 | item2: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055 +26 | item3: Union[ # comment +27 | type[requests_mock.Mocker], # another comment | = help: Combine multiple `type` members @@ -178,5 +183,27 @@ PYI055.pyi:25:12: PYI055 [*] Multiple `type` members in a union. Combine them in 24 24 | item: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker 25 |- item2: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker 25 |+ item2: type[Union[requests_mock.Mocker, httpretty, str]] = requests_mock.Mocker +26 26 | item3: Union[ # comment +27 27 | type[requests_mock.Mocker], # another comment +28 28 | type[httpretty], type[str]] = requests_mock.Mocker +PYI055.pyi:26:12: PYI055 [*] Multiple `type` members in a union. Combine them into one, e.g., `type[Union[requests_mock.Mocker, httpretty, str]]`. + | +24 | item: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker +25 | item2: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +26 | item3: Union[ # comment + | ____________^ +27 | | type[requests_mock.Mocker], # another comment +28 | | type[httpretty], type[str]] = requests_mock.Mocker + | |___________________________________^ PYI055 + | + = help: Combine multiple `type` members +ℹ Unsafe fix +23 23 | # PYI055 +24 24 | item: type[requests_mock.Mocker] | type[httpretty] | type[str] = requests_mock.Mocker +25 25 | item2: Union[type[requests_mock.Mocker], type[httpretty], type[str]] = requests_mock.Mocker +26 |- item3: Union[ # comment +27 |- type[requests_mock.Mocker], # another comment +28 |- type[httpretty], type[str]] = requests_mock.Mocker + 26 |+ item3: type[Union[requests_mock.Mocker, httpretty, str]] = requests_mock.Mocker From 0b5ee1c124f4312a5bf2d91fdb6ecf2688e29a1c Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Mon, 11 Nov 2024 18:03:05 +0100 Subject: [PATCH 2/3] Remove unnecessary return --- .../rules/unnecessary_type_union.rs | 101 +++++++++--------- 1 file changed, 53 insertions(+), 48 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs index 5b4c2ccfff72b..da38daef17ad2 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs @@ -70,15 +70,22 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr) // Check if `union` is a PEP604 union (e.g. `float | int`) or a `typing.Union[float, int]` let subscript = union.as_subscript_expr(); - if subscript.is_some_and(|subscript| !semantic.match_typing_expr(&subscript.value, "Union")) { - return; - } + let mut union_kind = match subscript { + Some(subscript) => { + if !semantic.match_typing_expr(&subscript.value, "Union") { + return; + } + UnionKind::TypingUnion + } + None => UnionKind::PEP604, + }; let mut type_exprs: Vec<&Expr> = Vec::new(); let mut other_exprs: Vec<&Expr> = Vec::new(); - let mut union_kind = UnionKind::TypingUnion; let mut collect_type_exprs = |expr: &'a Expr, parent: &'a Expr| { + // If a PEP604-style union is used within a `typing.Union`, then the fix can + // use PEP604-style unions. if matches!(parent, Expr::BinOp(_)) { union_kind = UnionKind::PEP604; } @@ -141,60 +148,58 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr) } } UnionKind::TypingUnion => { - if let Some(subscript) = subscript { - let types = &Expr::Subscript(ast::ExprSubscript { - value: Box::new(Expr::Name(ast::ExprName { - id: Name::new_static("type"), - ctx: ExprContext::Load, - range: TextRange::default(), - })), - slice: Box::new(Expr::Subscript(ast::ExprSubscript { - value: subscript.value.clone(), - slice: Box::new(Expr::Tuple(ast::ExprTuple { - elts: type_members - .into_iter() - .map(|type_member| { - Expr::Name(ast::ExprName { - id: type_member, - ctx: ExprContext::Load, - range: TextRange::default(), - }) + // When subscript is None, it uses the pervious match case. + let subscript = subscript.unwrap(); + let types = &Expr::Subscript(ast::ExprSubscript { + value: Box::new(Expr::Name(ast::ExprName { + id: Name::new_static("type"), + ctx: ExprContext::Load, + range: TextRange::default(), + })), + slice: Box::new(Expr::Subscript(ast::ExprSubscript { + value: subscript.value.clone(), + slice: Box::new(Expr::Tuple(ast::ExprTuple { + elts: type_members + .into_iter() + .map(|type_member| { + Expr::Name(ast::ExprName { + id: type_member, + ctx: ExprContext::Load, + range: TextRange::default(), }) - .collect(), - ctx: ExprContext::Load, - range: TextRange::default(), - parenthesized: true, - })), + }) + .collect(), ctx: ExprContext::Load, range: TextRange::default(), + parenthesized: true, })), ctx: ExprContext::Load, range: TextRange::default(), - }); + })), + ctx: ExprContext::Load, + range: TextRange::default(), + }); - if other_exprs.is_empty() { - checker.generator().expr(types) - } else { - let mut exprs = Vec::new(); - exprs.push(types); - exprs.extend(other_exprs); - - let union = Expr::Subscript(ast::ExprSubscript { - value: subscript.value.clone(), - slice: Box::new(Expr::Tuple(ast::ExprTuple { - elts: exprs.into_iter().cloned().collect(), - ctx: ExprContext::Load, - range: TextRange::default(), - parenthesized: true, - })), + if other_exprs.is_empty() { + checker.generator().expr(types) + } else { + let mut exprs = Vec::new(); + exprs.push(types); + exprs.extend(other_exprs); + + let union = Expr::Subscript(ast::ExprSubscript { + value: subscript.value.clone(), + slice: Box::new(Expr::Tuple(ast::ExprTuple { + elts: exprs.into_iter().cloned().collect(), ctx: ExprContext::Load, range: TextRange::default(), - }); + parenthesized: true, + })), + ctx: ExprContext::Load, + range: TextRange::default(), + }); - checker.generator().expr(&union) - } - } else { - return; + checker.generator().expr(&union) } } }; From 5e1d5b82747f89fe9d91051e852ddf2262c2279a Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Tue, 12 Nov 2024 12:47:52 +0100 Subject: [PATCH 3/3] Avoid clone. --- .../src/rules/flake8_pyi/rules/unnecessary_type_union.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs index da38daef17ad2..459d37a649b19 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unnecessary_type_union.rs @@ -109,8 +109,7 @@ pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr) } let type_members: Vec = type_exprs - .clone() - .into_iter() + .iter() .map(|type_expr| Name::new(checker.locator().slice(type_expr))) .collect();