From 9f38dbd06ed8f93d2221b482beeaacd0280e460a Mon Sep 17 00:00:00 2001 From: Victor Hugo Gomes Date: Wed, 2 Aug 2023 16:37:40 -0300 Subject: [PATCH] [`flake8-pyi`] Implement PYI051 (#6215) ## Summary Checks for the presence of redundant `Literal` types and builtin super types in an union. See [original source](https://github.com/PyCQA/flake8-pyi/blob/2a86db8271dc500247a8a69419536240334731cf/pyi.py#L1261). This implementation has a couple of differences from the original. The first one is, we support the `complex` and `float` builtin types. The second is, when reporting diagnostic for a `Literal` with multiple members of the same type, we print the entire `Literal` while `flak8` only prints the `Literal` with its first member. For example: ```python from typing import Literal x: Literal[1, 2] | int ``` Ruff will show `Literal[1, 2]` while flake8 only shows `Literal[1]`. ```shell $ ruff crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:4:18: PYI051 `Literal["foo"]` is redundant in an union with `str` crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:5:37: PYI051 `Literal[b"bar", b"foo"]` is redundant in an union with `bytes` crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:6:37: PYI051 `Literal[5]` is redundant in an union with `int` crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:6:67: PYI051 `Literal["foo"]` is redundant in an union with `str` crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:7:37: PYI051 `Literal[b"str_bytes"]` is redundant in an union with `bytes` crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:7:51: PYI051 `Literal[42]` is redundant in an union with `int` crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:9:31: PYI051 `Literal[1J]` is redundant in an union with `complex` crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:9:53: PYI051 `Literal[3.14]` is redundant in an union with `float` Found 8 errors. ``` ```shell $ flake8 crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:4:18: Y051 "Literal['foo']" is redundant in a union with "str" crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:5:37: Y051 "Literal[b'bar']" is redundant in a union with "bytes" crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:6:37: Y051 "Literal[5]" is redundant in a unionwith "int" crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:6:67: Y051 "Literal['foo']" is redundant in a union with "str" crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:7:37: Y051 "Literal[b'str_bytes']" is redundantin a union with "bytes" crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi:7:51: Y051 "Literal[42]" is redundant in a union with "int" ``` While implementing this rule, I found a bug in the `is_unchecked_union` check. This is the new check. https://github.com/astral-sh/ruff/blob/1ab86bad35e5edd1ba4cd82b0eb4c78416509dac/crates/ruff/src/checkers/ast/analyze/expression.rs#L85-L102 The purpose of the check was to prevent rules from navigating through nested `Union`s, as they already handle nested `Union`s. The way it was implemented, this was not happening, the rules were getting executed more than one time and sometimes were receiving expressions that were not `Union`. For example, with the following code: ```python typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]] ``` The rules were receiving the expressions in the following order: - `typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]]` - `Literal[5]` - `typing.Union[Literal["foo"], str]]` This was causing `PYI030` to report redundant information, for example: ```python typing.Union[Literal[5], int, typing.Union[Literal["foo"], Literal["bar"]]] ``` This is the `PYI030` output for this code: ```shell PYI030 Multiple literal members in a union. Use a single literal, e.g. `Literal[5, "foo", "bar"]` YI030 Multiple literal members in a union. Use a single literal, e.g.`Literal[5, "foo"]` ``` If I haven't misinterpreted the rule, that looks incorrect. I didn't have the time to check the `PYI016` rule. The last thing is, I couldn't find a reason for the "Why is this bad?" section for `PYI051`. Ref: #848 ## Test Plan Snapshots and manual runs of flake8. \ --- .../test/fixtures/flake8_pyi/PYI051.py | 17 ++ .../test/fixtures/flake8_pyi/PYI051.pyi | 17 ++ .../src/checkers/ast/analyze/expression.rs | 22 ++- crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_pyi/mod.rs | 2 + crates/ruff/src/rules/flake8_pyi/rules/mod.rs | 2 + .../rules/redundant_literal_union.rs | 155 ++++++++++++++++++ ...__flake8_pyi__tests__PYI051_PYI051.py.snap | 4 + ..._flake8_pyi__tests__PYI051_PYI051.pyi.snap | 90 ++++++++++ ruff.schema.json | 1 + 10 files changed, 308 insertions(+), 3 deletions(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi create mode 100644 crates/ruff/src/rules/flake8_pyi/rules/redundant_literal_union.rs create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.py.snap create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.pyi.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.py new file mode 100644 index 0000000000..543c12974c --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.py @@ -0,0 +1,17 @@ +import typing +from typing import Literal, TypeAlias, Union + +A: str | Literal["foo"] +B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str] +C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]] +D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int] + +def func(x: complex | Literal[1J], y: Union[Literal[3.14], float]): ... + +# OK +A: Literal["foo"] +B: TypeAlias = Literal[b"bar", b"foo"] +C: TypeAlias = typing.Union[Literal[5], Literal["foo"]] +D: TypeAlias = Literal[b"str_bytes", 42] + +def func(x: Literal[1J], y: Literal[3.14]): ... diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi new file mode 100644 index 0000000000..543c12974c --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi @@ -0,0 +1,17 @@ +import typing +from typing import Literal, TypeAlias, Union + +A: str | Literal["foo"] +B: TypeAlias = typing.Union[Literal[b"bar", b"foo"], bytes, str] +C: TypeAlias = typing.Union[Literal[5], int, typing.Union[Literal["foo"], str]] +D: TypeAlias = typing.Union[Literal[b"str_bytes", 42], bytes, int] + +def func(x: complex | Literal[1J], y: Union[Literal[3.14], float]): ... + +# OK +A: Literal["foo"] +B: TypeAlias = Literal[b"bar", b"foo"] +C: TypeAlias = typing.Union[Literal[5], Literal["foo"]] +D: TypeAlias = Literal[b"str_bytes", 42] + +def func(x: Literal[1J], y: Literal[3.14]): ... diff --git a/crates/ruff/src/checkers/ast/analyze/expression.rs b/crates/ruff/src/checkers/ast/analyze/expression.rs index 42f552ced0..38a2638d84 100644 --- a/crates/ruff/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff/src/checkers/ast/analyze/expression.rs @@ -74,9 +74,13 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { } // Ex) Union[...] - if checker.any_enabled(&[Rule::UnnecessaryLiteralUnion, Rule::DuplicateUnionMember]) { - // Determine if the current expression is an union - // Avoid duplicate checks if the parent is an `Union[...]` since these rules traverse nested unions + if checker.any_enabled(&[ + Rule::UnnecessaryLiteralUnion, + Rule::DuplicateUnionMember, + Rule::RedundantLiteralUnion, + ]) { + // Avoid duplicate checks if the parent is an `Union[...]` since these rules + // traverse nested unions. let is_unchecked_union = checker .semantic .expr_grandparent() @@ -92,6 +96,9 @@ 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) { + flake8_pyi::rules::redundant_literal_union(checker, expr); + } } } @@ -1081,6 +1088,15 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { { flake8_pyi::rules::unnecessary_literal_union(checker, expr); } + if checker.enabled(Rule::RedundantLiteralUnion) + // Avoid duplicate checks if the parent is an `|` + && !matches!( + checker.semantic.expr_parent(), + Some(Expr::BinOp(ast::ExprBinOp { op: Operator::BitOr, ..})) + ) + { + flake8_pyi::rules::redundant_literal_union(checker, expr); + } } } Expr::UnaryOp(ast::ExprUnaryOp { diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 9018dcc946..64e795cde8 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -658,6 +658,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Pyi, "048") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::StubBodyMultipleStatements), (Flake8Pyi, "049") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnusedPrivateTypedDict), (Flake8Pyi, "050") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::NoReturnArgumentAnnotationInStub), + (Flake8Pyi, "051") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::RedundantLiteralUnion), (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), diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index 6c363fdc6b..ec790aec90 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/mod.rs @@ -101,6 +101,8 @@ mod tests { #[test_case(Rule::UnusedPrivateTypeAlias, Path::new("PYI047.pyi"))] #[test_case(Rule::UnusedPrivateTypedDict, Path::new("PYI049.py"))] #[test_case(Rule::UnusedPrivateTypedDict, Path::new("PYI049.pyi"))] + #[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.py"))] + #[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.pyi"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs index 6ee3f89e8a..f5e8fda7e7 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs @@ -17,6 +17,7 @@ pub(crate) use pass_in_class_body::*; pub(crate) use pass_statement_stub_body::*; pub(crate) use prefix_type_params::*; pub(crate) use quoted_annotation_in_stub::*; +pub(crate) use redundant_literal_union::*; pub(crate) use redundant_numeric_union::*; pub(crate) use simple_defaults::*; pub(crate) use str_or_repr_defined_in_stub::*; @@ -50,6 +51,7 @@ mod pass_in_class_body; mod pass_statement_stub_body; mod prefix_type_params; mod quoted_annotation_in_stub; +mod redundant_literal_union; mod redundant_numeric_union; mod simple_defaults; mod str_or_repr_defined_in_stub; diff --git a/crates/ruff/src/rules/flake8_pyi/rules/redundant_literal_union.rs b/crates/ruff/src/rules/flake8_pyi/rules/redundant_literal_union.rs new file mode 100644 index 0000000000..51e333fc50 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/rules/redundant_literal_union.rs @@ -0,0 +1,155 @@ +use rustc_hash::FxHashSet; +use std::fmt; + +use ast::{Constant, Ranged}; +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::{self as ast, Expr}; +use ruff_python_semantic::SemanticModel; + +use crate::{checkers::ast::Checker, rules::flake8_pyi::helpers::traverse_union}; + +/// ## What it does +/// Checks for the presence of redundant `Literal` types and builtin super +/// types in an union. +/// +/// ## Why is this bad? +/// The use of `Literal` types in a union with the builtin super type of one of +/// its literal members is redundant, as the super type is strictly more +/// general than the `Literal` type. +/// +/// For example, `Literal["A"] | str` is equivalent to `str`, and +/// `Literal[1] | int` is equivalent to `int`, as `str` and `int` are the super +/// types of `"A"` and `1` respectively. +/// +/// ## Example +/// ```python +/// from typing import Literal +/// +/// A: Literal["A"] | str +/// ``` +/// +/// Use instead: +/// ```python +/// from typing import Literal +/// +/// A: Literal["A"] +/// ``` +#[violation] +pub struct RedundantLiteralUnion { + literal: String, + builtin_type: ExprType, +} + +impl Violation for RedundantLiteralUnion { + #[derive_message_formats] + fn message(&self) -> String { + let RedundantLiteralUnion { + literal, + builtin_type, + } = self; + format!("`Literal[{literal}]` is redundant in a union with `{builtin_type}`",) + } +} + +/// PYI051 +pub(crate) fn redundant_literal_union<'a>(checker: &mut Checker, union: &'a Expr) { + let mut literal_exprs = Vec::new(); + let mut builtin_types_in_union = FxHashSet::default(); + + // Adds a member to `literal_exprs` for each value in a `Literal`, and any builtin types + // to `builtin_types_in_union`. + let mut func = |expr: &'a Expr, _| { + if let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr { + if checker.semantic().match_typing_expr(value, "Literal") { + if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { + literal_exprs.extend(elts.iter()); + } else { + literal_exprs.push(slice); + } + } + return; + } + + let Some(builtin_type) = match_builtin_type(expr, checker.semantic()) else { + return; + }; + builtin_types_in_union.insert(builtin_type); + }; + + traverse_union(&mut func, checker.semantic(), union, None); + + for literal_expr in literal_exprs { + let Some(constant_type) = match_constant_type(literal_expr) else { + continue; + }; + + if builtin_types_in_union.contains(&constant_type) { + checker.diagnostics.push(Diagnostic::new( + RedundantLiteralUnion { + literal: checker.locator().slice(literal_expr.range()).to_string(), + builtin_type: constant_type, + }, + literal_expr.range(), + )); + } + } +} + +#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)] +enum ExprType { + Int, + Str, + Bool, + Float, + Bytes, + Complex, +} + +impl fmt::Display for ExprType { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + Self::Int => fmt.write_str("int"), + Self::Str => fmt.write_str("str"), + Self::Bool => fmt.write_str("bool"), + Self::Float => fmt.write_str("float"), + Self::Bytes => fmt.write_str("bytes"), + Self::Complex => fmt.write_str("complex"), + } + } +} + +/// Return the [`ExprType`] of an [`Expr]` if it is a builtin type (e.g. `int`, `bool`, `float`, +/// `str`, `bytes`, or `complex`). +fn match_builtin_type(expr: &Expr, model: &SemanticModel) -> Option { + let name = expr.as_name_expr()?; + let result = match name.id.as_str() { + "int" => ExprType::Int, + "bool" => ExprType::Bool, + "str" => ExprType::Str, + "float" => ExprType::Float, + "bytes" => ExprType::Bytes, + "complex" => ExprType::Complex, + _ => return None, + }; + if !model.is_builtin(name.id.as_str()) { + return None; + } + Some(result) +} + +/// Return the [`ExprType`] of an [`Expr]` if it is a constant (e.g., an `int`, like `1`, or a +/// `bool`, like `True`). +fn match_constant_type(expr: &Expr) -> Option { + let constant = expr.as_constant_expr()?; + let result = match constant.value { + Constant::Bool(_) => ExprType::Bool, + Constant::Str(_) => ExprType::Str, + Constant::Bytes(_) => ExprType::Bytes, + Constant::Int(_) => ExprType::Int, + Constant::Float(_) => ExprType::Float, + Constant::Complex { .. } => ExprType::Complex, + _ => return None, + }; + Some(result) +} diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.py.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.py.snap new file mode 100644 index 0000000000..d1aa2e9116 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +--- + diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.pyi.snap new file mode 100644 index 0000000000..14b0115513 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI051_PYI051.pyi.snap @@ -0,0 +1,90 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +--- +PYI051.pyi:4:18: PYI051 `Literal["foo"]` is redundant in a union with `str` + | +2 | from typing import Literal, TypeAlias, Union +3 | +4 | A: str | Literal["foo"] + | ^^^^^ PYI051 +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.pyi:5:37: PYI051 `Literal[b"bar"]` 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.pyi: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.pyi: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.pyi: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.pyi: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.pyi: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.pyi:9:31: PYI051 `Literal[1J]` is redundant in a union with `complex` + | + 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 + | + +PYI051.pyi: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 + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 87ddf7002c..96ff8fe0a8 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2408,6 +2408,7 @@ "PYI049", "PYI05", "PYI050", + "PYI051", "PYI052", "PYI053", "PYI054",