Skip to content

Commit

Permalink
[flake8-pyi] Implement PYI051 (#6215)
Browse files Browse the repository at this point in the history
## 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.
\
  • Loading branch information
LaBatata101 authored Aug 2, 2023
1 parent 7c5791f commit 9f38dbd
Show file tree
Hide file tree
Showing 10 changed files with 308 additions and 3 deletions.
17 changes: 17 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.py
Original file line number Diff line number Diff line change
@@ -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]): ...
17 changes: 17 additions & 0 deletions crates/ruff/resources/test/fixtures/flake8_pyi/PYI051.pyi
Original file line number Diff line number Diff line change
@@ -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]): ...
22 changes: 19 additions & 3 deletions crates/ruff/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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);
}
}
}

Expand Down Expand Up @@ -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 {
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 @@ -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),
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 @@ -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(
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 @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
155 changes: 155 additions & 0 deletions crates/ruff/src/rules/flake8_pyi/rules/redundant_literal_union.rs
Original file line number Diff line number Diff line change
@@ -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<ExprType> {
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<ExprType> {
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)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff/src/rules/flake8_pyi/mod.rs
---

Original file line number Diff line number Diff line change
@@ -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
|


1 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 9f38dbd

Please sign in to comment.