Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flake8-pyi] Implement PYI055 #6316

Merged
merged 7 commits into from
Aug 4, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI055.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import builtins
from typing import Union


w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
x: type[int] | type[str] | type[float]
y: builtins.type[int] | type[str] | builtins.type[complex]
z: Union[type[float], type[complex]]


def func(arg: type[int] | str | type[float]) -> None: ...

# OK
x: type[int, str, float]
y: builtins.type[int, str, complex]
z: Union[float, complex]


def func(arg: type[int, float] | str) -> None: ...
19 changes: 19 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI055.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import builtins
from typing import Union


w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
x: type[int] | type[str] | type[float]
y: builtins.type[int] | type[str] | builtins.type[complex]
z: Union[type[float], type[complex]]


def func(arg: type[int] | str | type[float]) -> None: ...

# OK
x: type[int, str, float]
y: builtins.type[int, str, complex]
z: Union[float, complex]


def func(arg: type[int, float] | str) -> None: ...
25 changes: 18 additions & 7 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
Rule::UnnecessaryLiteralUnion,
Rule::DuplicateUnionMember,
Rule::RedundantLiteralUnion,
Rule::UnnecessaryTypeUnion,
]) {
// Avoid duplicate checks if the parent is an `Union[...]` since these rules
// traverse nested unions.
Expand All @@ -96,9 +97,12 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::DuplicateUnionMember) {
flake8_pyi::rules::duplicate_union_member(checker, expr);
}
if checker.is_stub && checker.enabled(Rule::RedundantLiteralUnion) {
if checker.enabled(Rule::RedundantLiteralUnion) {
flake8_pyi::rules::redundant_literal_union(checker, expr);
}
if checker.enabled(Rule::UnnecessaryTypeUnion) {
flake8_pyi::rules::unnecessary_type_union(checker, expr);
}
}
}

Expand Down Expand Up @@ -179,9 +183,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
&& checker.semantic.in_annotation()
&& !checker.settings.pyupgrade.keep_runtime_typing
{
flake8_future_annotations::rules::future_rewritable_type_annotation(
checker, expr,
);
flake8_future_annotations::rules::future_rewritable_type_annotation(checker, expr);
}
}
if checker.enabled(Rule::NonPEP585Annotation) {
Expand Down Expand Up @@ -387,9 +389,8 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
.enabled(Rule::StringDotFormatExtraPositionalArguments)
{
pyflakes::rules::string_dot_format_extra_positional_arguments(
checker,
&summary, args, location,
);
checker, &summary, args, location,
);
}
if checker.enabled(Rule::StringDotFormatMissingArguments) {
pyflakes::rules::string_dot_format_missing_argument(
Expand Down Expand Up @@ -1075,6 +1076,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
);
}
}

if checker.enabled(Rule::DuplicateUnionMember)
&& checker.semantic.in_type_definition()
// Avoid duplicate checks if the parent is an `|`
Expand Down Expand Up @@ -1103,6 +1105,15 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
{
flake8_pyi::rules::redundant_literal_union(checker, expr);
}
if checker.enabled(Rule::UnnecessaryTypeUnion)
// Avoid duplicate checks if the parent is an `|`
&& !matches!(
checker.semantic.expr_parent(),
Some(Expr::BinOp(ast::ExprBinOp { op: Operator::BitOr, ..}))
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider moving this check into a shared block like we do for the Union[...] case above?

Copy link
Contributor Author

@LaBatata101 LaBatata101 Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be nice, removing the duplicated code. Does the variable name is_unchecked_union fit in here as well?

{
flake8_pyi::rules::unnecessary_type_union(checker, expr);
}
}
Expr::UnaryOp(ast::ExprUnaryOp {
op,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "052") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnannotatedAssignmentInStub),
(Flake8Pyi, "054") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::NumericLiteralTooLong),
(Flake8Pyi, "053") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::StringOrBytesTooLong),
(Flake8Pyi, "055") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnnecessaryTypeUnion),
(Flake8Pyi, "056") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnsupportedMethodCallOnAll),

// flake8-pytest-style
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@ mod tests {
#[test_case(Rule::UnusedPrivateTypedDict, Path::new("PYI049.pyi"))]
#[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.py"))]
#[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.pyi"))]
#[test_case(Rule::UnnecessaryTypeUnion, Path::new("PYI055.py"))]
#[test_case(Rule::UnnecessaryTypeUnion, Path::new("PYI055.pyi"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ pub(crate) use type_alias_naming::*;
pub(crate) use type_comment_in_stub::*;
pub(crate) use unaliased_collections_abc_set_import::*;
pub(crate) use unnecessary_literal_union::*;
pub(crate) use unnecessary_type_union::*;
pub(crate) use unrecognized_platform::*;
pub(crate) use unrecognized_version_info::*;
pub(crate) use unsupported_method_call_on_all::*;
Expand Down Expand Up @@ -63,6 +64,7 @@ mod type_alias_naming;
mod type_comment_in_stub;
mod unaliased_collections_abc_set_import;
mod unnecessary_literal_union;
mod unnecessary_type_union;
mod unrecognized_platform;
mod unrecognized_version_info;
mod unsupported_method_call_on_all;
Expand Down
82 changes: 82 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/rules/unnecessary_type_union.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{Expr, Ranged};

use crate::{checkers::ast::Checker, rules::flake8_pyi::helpers::traverse_union};

/// ## What it does
/// Checks for the presence of multiple `type`s in a union.
///
/// ## Why is this bad?
/// The `type` built-in function accepts unions, and it is
/// clearer to explicitly specify them as a single `type`.
///
/// ## Example
/// ```python
/// field: type[int] | type[float]
/// ```
///
/// Use instead:
/// ```python
/// field: type[int | float]
/// ```
#[violation]
pub struct UnnecessaryTypeUnion {
members: Vec<String>,
is_pep604_union: bool,
}

impl Violation for UnnecessaryTypeUnion {
#[derive_message_formats]
fn message(&self) -> String {
let union_str = if self.is_pep604_union {
format!("{}", self.members.join(" | "))
} else {
format!("Union[{}]", self.members.join(", "))
};

format!(
"Multiple `type` members in a union. Combine them into one, e.g. `type[{union_str}]`"
)
}
}

/// PYI055
pub(crate) fn unnecessary_type_union<'a>(checker: &mut Checker, union: &'a Expr) {
let mut type_exprs = Vec::new();

// Check if `union` is a PEP604 union (e.g. `float | int`) or a `typing.Union[float, int]`
let is_pep604_union = !union.as_subscript_expr().is_some_and(|subscript| {
checker
.semantic()
.match_typing_expr(&subscript.value, "Union")
});

let mut collect_type_exprs = |expr: &'a Expr, _| {
let Some(subscript) = expr.as_subscript_expr() else {
return;
};
let Some(call_path) = checker.semantic().resolve_call_path(subscript.value.as_ref()) else {
return;
};

if matches!(call_path.as_slice(), ["" | "builtins", "type"]) {
type_exprs.push(&subscript.slice);
}
};

traverse_union(&mut collect_type_exprs, checker.semantic(), union, None);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How was using this helper? I wrote it and would appreciate any feedback :)

Copy link
Contributor Author

@LaBatata101 LaBatata101 Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was very easy to use, good job!


if type_exprs.len() > 1 {
checker.diagnostics.push(Diagnostic::new(
UnnecessaryTypeUnion {
members: type_exprs
.into_iter()
.map(|type_expr| checker.locator().slice(type_expr.range()).to_string())
.collect(),
is_pep604_union,
},
union.range(),
));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,62 @@ PYI051.py:4:18: PYI051 `Literal["foo"]` is redundant in a union with `str`
6 | C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]
|

PYI051.py:5:37: PYI051 `Literal[b"bar"]` is redundant in a union with `bytes`
zanieb marked this conversation as resolved.
Show resolved Hide resolved
|
4 | A: str | Literal["foo"]
5 | B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str]
| ^^^^^^ PYI051
6 | C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
|

PYI051.py:5:45: PYI051 `Literal[b"foo"]` is redundant in a union with `bytes`
|
4 | A: str | Literal["foo"]
5 | B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str]
| ^^^^^^ PYI051
6 | C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
|

PYI051.py:6:37: PYI051 `Literal[5]` is redundant in a union with `int`
|
4 | A: str | Literal["foo"]
5 | B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str]
6 | C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]
| ^ PYI051
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
|

PYI051.py:6:67: PYI051 `Literal["foo"]` is redundant in a union with `str`
|
4 | A: str | Literal["foo"]
5 | B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str]
6 | C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]
| ^^^^^ PYI051
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
|

PYI051.py:7:37: PYI051 `Literal[b"str_bytes"]` is redundant in a union with `bytes`
|
5 | B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str]
6 | C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
| ^^^^^^^^^^^^ PYI051
8 |
9 | def func(x: complex | Literal[1J], y: Union[Literal[3.14], float]): ...
|

PYI051.py:7:51: PYI051 `Literal[42]` is redundant in a union with `int`
|
5 | B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str]
6 | C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
| ^^ PYI051
8 |
9 | def func(x: complex | Literal[1J], y: Union[Literal[3.14], float]): ...
|

PYI051.py:9:31: PYI051 `Literal[1J]` is redundant in a union with `complex`
|
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
Expand All @@ -21,4 +77,14 @@ PYI051.py:9:31: PYI051 `Literal[1J]` is redundant in a union with `complex`
11 | # OK
|

PYI051.py:9:53: PYI051 `Literal[3.14]` is redundant in a union with `float`
|
7 | D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int]
8 |
9 | def func(x: complex | Literal[1J], y: Union[Literal[3.14], float]): ...
| ^^^^ PYI051
10 |
11 | # OK
|


Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---
PYI055.py:5:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | str | complex]`
|
5 | w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
6 | x: type[int] | type[str] | type[float]
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
|

PYI055.py:6:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | str | float]`
|
5 | w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
6 | x: type[int] | type[str] | type[float]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
8 | z: Union[type[float], type[complex]]
|

PYI055.py:7:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | str | complex]`
|
5 | w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
6 | x: type[int] | type[str] | type[float]
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
8 | z: Union[type[float], type[complex]]
|

PYI055.py:8:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[Union[float, complex]]`
|
6 | x: type[int] | type[str] | type[float]
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
8 | z: Union[type[float], type[complex]]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
|

PYI055.py:11:15: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | float]`
|
11 | def func(arg: type[int] | str | type[float]) -> None: ...
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
12 |
13 | # OK
|


Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---
PYI055.pyi:5:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | str | complex]`
|
5 | w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
6 | x: type[int] | type[str] | type[float]
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
|

PYI055.pyi:6:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | str | float]`
|
5 | w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
6 | x: type[int] | type[str] | type[float]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
8 | z: Union[type[float], type[complex]]
|

PYI055.pyi:7:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | str | complex]`
|
5 | w: builtins.type[int] | builtins.type[str] | builtins.type[complex]
6 | x: type[int] | type[str] | type[float]
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
8 | z: Union[type[float], type[complex]]
|

PYI055.pyi:8:4: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[Union[float, complex]]`
|
6 | x: type[int] | type[str] | type[float]
7 | y: builtins.type[int] | type[str] | builtins.type[complex]
8 | z: Union[type[float], type[complex]]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
|

PYI055.pyi:11:15: PYI055 Multiple `type` members in a union. Combine them into one, e.g. `type[int | float]`
|
11 | def func(arg: type[int] | str | type[float]) -> None: ...
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI055
12 |
13 | # OK
|


Loading