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 PYI062 (duplicate-literal-member) #11269

Merged
merged 12 commits into from
May 7, 2024
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from typing import Literal

x: Literal[True, False, True, False] # PYI062 twice here

y: Literal[1, print("hello"), 3, Literal[4, 1]] # PYI062 on the last 1
Copy link
Member

Choose a reason for hiding this comment

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

Can you add test cases where the inner literal is entirely redundant? e.g.

Literal[1, Literal[1]]
Literal[1, 2, Literal[1, 2]]
Literal[1, Literal[1], Literal[1]]
Literal[1, Literal[2], Literal[2]]
Literal[1, Literal[2, Literal[1]]]

Copy link
Member

Choose a reason for hiding this comment

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

These will be more important for when we add a fix, I think.

Copy link
Member

Choose a reason for hiding this comment

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

It's also good to have coverage for cases like Literal[1, 1, 1] where we should raise the diagnostic twice.


z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PY062 on the set literal

n: Literal["No", "duplicates", "here", 1, "1"]
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
from typing import Literal

x: Literal[True, False, True, False] # PY062 twice here

y: Literal[1, print("hello"), 3, Literal[4, 1]] # PY062 on the last 1

z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PY062 on the set literal

n: Literal["No", "duplicates", "here", 1, "1"]
15 changes: 15 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
}
}

// Ex) Literal[...]
if checker.enabled(Rule::DuplicateLiteralMember) {
if !checker.semantic.in_nested_literal() {
flake8_pyi::rules::duplicate_literal_member(checker, expr);
}
}

if checker.enabled(Rule::NeverUnion) {
ruff::rules::never_union(checker, expr);
}
Expand Down Expand Up @@ -1194,6 +1201,14 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
ruff::rules::never_union(checker, expr);
}

// Avoid duplicate checks if the parent is a literal, since this rule already
// traverses nested literals.
if checker.enabled(Rule::DuplicateLiteralMember) {
if !checker.semantic.in_nested_literal() {
flake8_pyi::rules::duplicate_literal_member(checker, expr);
}
}

AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
// Avoid duplicate checks if the parent is a union, since these rules already
// traverse nested unions.
if !checker.semantic.in_nested_union() {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -808,6 +808,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "055") => (RuleGroup::Stable, rules::flake8_pyi::rules::UnnecessaryTypeUnion),
(Flake8Pyi, "056") => (RuleGroup::Stable, rules::flake8_pyi::rules::UnsupportedMethodCallOnAll),
(Flake8Pyi, "058") => (RuleGroup::Stable, rules::flake8_pyi::rules::GeneratorReturnFromIterMethod),
(Flake8Pyi, "062") => (RuleGroup::Stable, rules::flake8_pyi::rules::DuplicateLiteralMember),
tusharsadhwani marked this conversation as resolved.
Show resolved Hide resolved

// flake8-pytest-style
(Flake8PytestStyle, "001") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestFixtureIncorrectParenthesesStyle),
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ mod tests {
#[test_case(Rule::CustomTypeVarReturnType, Path::new("PYI019.pyi"))]
#[test_case(Rule::DocstringInStub, Path::new("PYI021.py"))]
#[test_case(Rule::DocstringInStub, Path::new("PYI021.pyi"))]
#[test_case(Rule::DuplicateLiteralMember, Path::new("PYI062.py"))]
#[test_case(Rule::DuplicateLiteralMember, Path::new("PYI062.pyi"))]
#[test_case(Rule::DuplicateUnionMember, Path::new("PYI016.py"))]
#[test_case(Rule::DuplicateUnionMember, Path::new("PYI016.pyi"))]
#[test_case(Rule::EllipsisInNonEmptyClassBody, Path::new("PYI013.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
use std::collections::HashSet;

use rustc_hash::FxHashSet;

use ruff_diagnostics::{Diagnostic, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::Expr;
use ruff_python_semantic::analyze::typing::traverse_literal;
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for duplicate literal members.
tusharsadhwani marked this conversation as resolved.
Show resolved Hide resolved
///
/// ## Why is this bad?
/// Duplicate literal members are redundant and should be removed.
///
/// ## Example
/// ```python
/// foo: Literal[True, False, True]
/// ```
///
/// Use instead:
/// ```python
/// foo: Literal[True, False]
/// ```
tusharsadhwani marked this conversation as resolved.
Show resolved Hide resolved
///
/// ## References
/// - [Python documentation: `typing.Literal`](https://docs.python.org/3/library/typing.html#typing.Literal)
#[violation]
pub struct DuplicateLiteralMember {
duplicate_name: String,
}

impl Violation for DuplicateLiteralMember {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
format!("Duplicate literal member `{}`", self.duplicate_name)
}
}

/// PYI062
pub(crate) fn duplicate_literal_member<'a>(checker: &mut Checker, expr: &'a Expr) {
let mut seen_nodes: HashSet<ComparableExpr<'_>, _> = FxHashSet::default();
let mut diagnostics: Vec<Diagnostic> = Vec::new();

// Adds a member to `literal_exprs` if it is a `Literal` annotation
let mut check_for_duplicate_members = |expr: &'a Expr, _: &'a Expr| {
// If we've already seen this literal member, raise a violation.
if !seen_nodes.insert(expr.into()) {
let diagnostic = Diagnostic::new(
DuplicateLiteralMember {
duplicate_name: checker.generator().expr(expr),
},
expr.range(),
);
diagnostics.push(diagnostic);
tusharsadhwani marked this conversation as resolved.
Show resolved Hide resolved
}
};

// Traverse the literal, collect all diagnostic members
traverse_literal(&mut check_for_duplicate_members, checker.semantic(), expr);
checker.diagnostics.append(&mut diagnostics);
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ pub(crate) use complex_assignment_in_stub::*;
pub(crate) use complex_if_statement_in_stub::*;
pub(crate) use custom_type_var_return_type::*;
pub(crate) use docstring_in_stubs::*;
pub(crate) use duplicate_literal_member::*;
pub(crate) use duplicate_union_member::*;
pub(crate) use ellipsis_in_non_empty_class_body::*;
pub(crate) use exit_annotations::*;
Expand Down Expand Up @@ -44,6 +45,7 @@ mod complex_assignment_in_stub;
mod complex_if_statement_in_stub;
mod custom_type_var_return_type;
mod docstring_in_stubs;
mod duplicate_literal_member;
mod duplicate_union_member;
mod ellipsis_in_non_empty_class_body;
mod exit_annotations;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
---
source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs
---
PYI062.py:3:25: PYI062 Duplicate literal member `True`
|
1 | from typing import Literal
2 |
3 | x: Literal[True, False, True, False] # PYI062 twice here
| ^^^^ PYI062
4 |
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PYI062 on the last 1
|

PYI062.py:3:31: PYI062 Duplicate literal member `False`
|
1 | from typing import Literal
2 |
3 | x: Literal[True, False, True, False] # PYI062 twice here
| ^^^^^ PYI062
4 |
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PYI062 on the last 1
|

PYI062.py:5:45: PYI062 Duplicate literal member `1`
|
3 | x: Literal[True, False, True, False] # PYI062 twice here
4 |
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PYI062 on the last 1
| ^ PYI062
6 |
7 | z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PY062 on the set literal
|

PYI062.py:7:33: PYI062 Duplicate literal member `{1, 3, 5}`
|
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PYI062 on the last 1
6 |
7 | z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PY062 on the set literal
| ^^^^^^^ PYI062
8 |
9 | n: Literal["No", "duplicates", "here", 1, "1"]
|
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
---
source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs
---
PYI062.pyi:3:25: PYI062 Duplicate literal member `True`
|
1 | from typing import Literal
2 |
3 | x: Literal[True, False, True, False] # PY062 twice here
| ^^^^ PYI062
4 |
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PY062 on the last 1
|

PYI062.pyi:3:31: PYI062 Duplicate literal member `False`
|
1 | from typing import Literal
2 |
3 | x: Literal[True, False, True, False] # PY062 twice here
| ^^^^^ PYI062
4 |
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PY062 on the last 1
|

PYI062.pyi:5:45: PYI062 Duplicate literal member `1`
|
3 | x: Literal[True, False, True, False] # PY062 twice here
4 |
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PY062 on the last 1
| ^ PYI062
6 |
7 | z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PY062 on the set literal
|

PYI062.pyi:7:33: PYI062 Duplicate literal member `{1, 3, 5}`
|
5 | y: Literal[1, print("hello"), 3, Literal[4, 1]] # PY062 on the last 1
6 |
7 | z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PY062 on the set literal
| ^^^^^^^ PYI062
8 |
9 | n: Literal["No", "duplicates", "here", 1, "1"]
|
38 changes: 38 additions & 0 deletions crates/ruff_python_semantic/src/analyze/typing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,44 @@ where
inner(func, semantic, expr, None);
}

/// Traverse a "literal" type annotation, applying `func` to each literal member.
///
/// The function is called with each expression in the literal (excluding declarations of nested
/// literals) and the parent expression.
pub fn traverse_literal<'a, F>(func: &mut F, semantic: &SemanticModel, expr: &'a Expr)
where
F: FnMut(&'a Expr, &'a Expr),
{
fn inner<'a, F>(
func: &mut F,
semantic: &SemanticModel,
expr: &'a Expr,
parent: Option<&'a Expr>,
) where
F: FnMut(&'a Expr, &'a Expr),
{
// Ex) Literal[x, y]
if let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr {
if semantic.match_typing_expr(value, "Literal") {
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() {
// Traverse each element of the tuple within the literal recursively to handle cases
// such as `Literal[..., Literal[...]]
elts.iter()
.for_each(|elt| inner(func, semantic, elt, Some(expr)));
return;
}
}
}

// Otherwise, call the function on expression, if it's not the top-level expression.
if let Some(parent) = parent {
func(expr, parent);
}
}

inner(func, semantic, expr, None);
}

/// Abstraction for a type checker, conservatively checks for the intended type(s).
pub trait TypeChecker {
/// Check annotation expression to match the intended type(s).
Expand Down
9 changes: 9 additions & 0 deletions crates/ruff_python_semantic/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1350,6 +1350,15 @@ impl<'a> SemanticModel<'a> {
false
}

/// Return `true` if the model is in a nested literal expression (e.g., the inner `Literal` in
/// `Literal[Literal[int, str], float]`).
pub fn in_nested_literal(&self) -> bool {
// Ex) `Literal[Literal[int, str], float]`
self.current_expression_grandparent()
.and_then(Expr::as_subscript_expr)
.is_some_and(|parent| self.match_typing_expr(&parent.value, "Literal"))
}

/// Returns `true` if `left` and `right` are in the same branches of an `if`, `match`, or
/// `try` statement.
///
Expand Down
2 changes: 2 additions & 0 deletions ruff.schema.json
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved

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

Loading