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
19 changes: 19 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI062.py
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
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}] # PYI062 on the set literal

Literal[1, Literal[1]] # once
Literal[1, 2, Literal[1, 2]] # twice
Literal[1, Literal[1], Literal[1]] # twice
Literal[1, Literal[2], Literal[2]] # once
Literal[1, Literal[2, Literal[1]]] # once
Literal[1, 1, 1] # twice

# Ensure issue is only raised once, even on nested literals
MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062

n: Literal["No", "duplicates", "here", 1, "1"]
19 changes: 19 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI062.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
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

Literal[1, Literal[1]] # once
Literal[1, 2, Literal[1, 2]] # twice
Literal[1, Literal[1], Literal[1]] # twice
Literal[1, Literal[2], Literal[2]] # once
Literal[1, Literal[2, Literal[1]]] # once
Literal[1, 1, 1] # twice

# Ensure issue is only raised once, even on nested literals
MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062

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 @@ -97,6 +97,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 @@ -1193,6 +1200,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::Preview, rules::flake8_pyi::rules::DuplicateLiteralMember),

// 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,138 @@
---
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}] # PYI062 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}] # PYI062 on the set literal
| ^^^^^^^ PYI062
8 |
9 | Literal[1, Literal[1]] # once
|

PYI062.py:9:20: PYI062 Duplicate literal member `1`
|
7 | z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PYI062 on the set literal
8 |
9 | Literal[1, Literal[1]] # once
| ^ PYI062
10 | Literal[1, 2, Literal[1, 2]] # twice
11 | Literal[1, Literal[1], Literal[1]] # twice
|

PYI062.py:10:23: PYI062 Duplicate literal member `1`
|
9 | Literal[1, Literal[1]] # once
10 | Literal[1, 2, Literal[1, 2]] # twice
| ^ PYI062
11 | Literal[1, Literal[1], Literal[1]] # twice
12 | Literal[1, Literal[2], Literal[2]] # once
|

PYI062.py:10:26: PYI062 Duplicate literal member `2`
|
9 | Literal[1, Literal[1]] # once
10 | Literal[1, 2, Literal[1, 2]] # twice
| ^ PYI062
11 | Literal[1, Literal[1], Literal[1]] # twice
12 | Literal[1, Literal[2], Literal[2]] # once
|

PYI062.py:11:20: PYI062 Duplicate literal member `1`
|
9 | Literal[1, Literal[1]] # once
10 | Literal[1, 2, Literal[1, 2]] # twice
11 | Literal[1, Literal[1], Literal[1]] # twice
| ^ PYI062
12 | Literal[1, Literal[2], Literal[2]] # once
13 | Literal[1, Literal[2, Literal[1]]] # once
|

PYI062.py:11:32: PYI062 Duplicate literal member `1`
|
9 | Literal[1, Literal[1]] # once
10 | Literal[1, 2, Literal[1, 2]] # twice
11 | Literal[1, Literal[1], Literal[1]] # twice
| ^ PYI062
12 | Literal[1, Literal[2], Literal[2]] # once
13 | Literal[1, Literal[2, Literal[1]]] # once
|

PYI062.py:12:32: PYI062 Duplicate literal member `2`
|
10 | Literal[1, 2, Literal[1, 2]] # twice
11 | Literal[1, Literal[1], Literal[1]] # twice
12 | Literal[1, Literal[2], Literal[2]] # once
| ^ PYI062
13 | Literal[1, Literal[2, Literal[1]]] # once
14 | Literal[1, 1, 1] # twice
|

PYI062.py:13:31: PYI062 Duplicate literal member `1`
|
11 | Literal[1, Literal[1], Literal[1]] # twice
12 | Literal[1, Literal[2], Literal[2]] # once
13 | Literal[1, Literal[2, Literal[1]]] # once
| ^ PYI062
14 | Literal[1, 1, 1] # twice
|

PYI062.py:14:12: PYI062 Duplicate literal member `1`
|
12 | Literal[1, Literal[2], Literal[2]] # once
13 | Literal[1, Literal[2, Literal[1]]] # once
14 | Literal[1, 1, 1] # twice
| ^ PYI062
15 |
16 | # Ensure issue is only raised once, even on nested literals
|

PYI062.py:14:15: PYI062 Duplicate literal member `1`
|
12 | Literal[1, Literal[2], Literal[2]] # once
13 | Literal[1, Literal[2, Literal[1]]] # once
14 | Literal[1, 1, 1] # twice
| ^ PYI062
15 |
16 | # Ensure issue is only raised once, even on nested literals
|

PYI062.py:17:46: PYI062 Duplicate literal member `True`
|
16 | # Ensure issue is only raised once, even on nested literals
17 | MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
| ^^^^ PYI062
18 |
19 | n: Literal["No", "duplicates", "here", 1, "1"]
|
Loading
Loading