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.

Sign up for GitHub

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

Jump to bottom

[flake8-pyi] Mark fix as unsafe when type annotation contains comments for duplicate-literal-member (PYI062) #14268

Merged
merged 1 commit into from
Nov 11, 2024
Merged

[flake8-pyi] Mark fix as unsafe when type annotation contains comments for duplicate-literal-member (PYI062) #14268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI062.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@
Literal[1, Literal[2], Literal[2]] # once
t.Literal[1, t.Literal[2, t.Literal[1]]] # once
typing_extensions.Literal[1, 1, 1] # twice
Literal[
1, # comment
Literal[ # another comment
1
]
] # once

# Ensure issue is only raised once, even on nested literals
MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
Expand Down
12 changes: 9 additions & 3 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI062.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,24 @@ from typing import Literal
import typing as t
import typing_extensions

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

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

z: Literal[{1, 3, 5}, "foobar", {1,3,5}] # PY062 on the set literal
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
t.Literal[1, t.Literal[2, t.Literal[1]]] # once
typing_extensions.Literal[1, 1, 1] # twice
Literal[
1, # comment
Literal[ # another comment
1
]
] # once

# Ensure issue is only raised once, even on nested literals
MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
Expand Down
64 changes: 36 additions & 28 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/duplicate_literal_member.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::collections::HashSet;

use rustc_hash::FxHashSet;

use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_diagnostics::{AlwaysFixableViolation, Applicability, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::{self as ast, Expr, ExprContext};
Expand All @@ -28,8 +28,10 @@ use crate::checkers::ast::Checker;
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as safe; however, the fix will flatten nested
/// literals into a single top-level literal.
/// This rule's fix is marked as safe, unless the type annotation contains comments.
///
/// Note that the fix will flatten nested literals into a single top-level
/// literal.
///
/// ## References
/// - [Python documentation: `typing.Literal`](https://docs.python.org/3/library/typing.html#typing.Literal)
Expand Down Expand Up @@ -73,33 +75,39 @@ pub(crate) fn duplicate_literal_member<'a>(checker: &mut Checker, expr: &'a Expr
// Traverse the literal, collect all diagnostic members.
traverse_literal(&mut check_for_duplicate_members, checker.semantic(), expr);

if diagnostics.is_empty() {
return;
}
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved

// If there's at least one diagnostic, create a fix to remove the duplicate members.
if !diagnostics.is_empty() {
if let Expr::Subscript(subscript) = expr {
let subscript = Expr::Subscript(ast::ExprSubscript {
slice: Box::new(if let [elt] = unique_nodes.as_slice() {
(*elt).clone()
} else {
Expr::Tuple(ast::ExprTuple {
elts: unique_nodes.into_iter().cloned().collect(),
range: TextRange::default(),
ctx: ExprContext::Load,
parenthesized: false,
})
}),
value: subscript.value.clone(),
range: TextRange::default(),
ctx: ExprContext::Load,
});
let fix = Fix::safe_edit(Edit::range_replacement(
checker.generator().expr(&subscript),
expr.range(),
));
for diagnostic in &mut diagnostics {
diagnostic.set_fix(fix.clone());
}
if let Expr::Subscript(subscript) = expr {
let subscript = Expr::Subscript(ast::ExprSubscript {
slice: Box::new(if let [elt] = unique_nodes.as_slice() {
(*elt).clone()
} else {
Expr::Tuple(ast::ExprTuple {
elts: unique_nodes.into_iter().cloned().collect(),
range: TextRange::default(),
ctx: ExprContext::Load,
parenthesized: false,
})
}),
value: subscript.value.clone(),
range: TextRange::default(),
ctx: ExprContext::Load,
});
let fix = Fix::applicable_edit(
Edit::range_replacement(checker.generator().expr(&subscript), expr.range()),
if checker.comment_ranges().intersects(expr.range()) {
Applicability::Unsafe
} else {
Applicability::Safe
},
);
Comment on lines +99 to +106
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. While I've never seen comments inside a Literal annotation in "real life", this does seem to align with the rough consensus in #9790. So this change seems good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it's very rare. My primary motivation for this type of changes is to give the right example for future uses. While developing new rules we often take existing rules as base, and being rigorous now might prevent issues in future rules where the comments do matter.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense.

for diagnostic in &mut diagnostics {
diagnostic.set_fix(fix.clone());
}
}
};

checker.diagnostics.append(&mut diagnostics);
}
77 changes: 52 additions & 25 deletions ...c/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI062_PYI062.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ PYI062.py:14:32: PYI062 [*] Duplicate literal member `2`
14 |+Literal[1, 2] # once
15 15 | t.Literal[1, t.Literal[2, t.Literal[1]]] # once
16 16 | typing_extensions.Literal[1, 1, 1] # twice
17 17 |
17 17 | Literal[

PYI062.py:15:37: PYI062 [*] Duplicate literal member `1`
|
Expand All @@ -216,6 +216,7 @@ PYI062.py:15:37: PYI062 [*] Duplicate literal member `1`
15 | t.Literal[1, t.Literal[2, t.Literal[1]]] # once
| ^ PYI062
16 | typing_extensions.Literal[1, 1, 1] # twice
17 | Literal[
|
= help: Remove duplicates

Expand All @@ -226,17 +227,17 @@ PYI062.py:15:37: PYI062 [*] Duplicate literal member `1`
15 |-t.Literal[1, t.Literal[2, t.Literal[1]]] # once
15 |+t.Literal[1, 2] # once
16 16 | typing_extensions.Literal[1, 1, 1] # twice
17 17 |
18 18 | # Ensure issue is only raised once, even on nested literals
17 17 | Literal[
18 18 | 1, # comment

PYI062.py:16:30: PYI062 [*] Duplicate literal member `1`
|
14 | Literal[1, Literal[2], Literal[2]] # once
15 | t.Literal[1, t.Literal[2, t.Literal[1]]] # once
16 | typing_extensions.Literal[1, 1, 1] # twice
| ^ PYI062
17 |
18 | # Ensure issue is only raised once, even on nested literals
17 | Literal[
18 | 1, # comment
|
= help: Remove duplicates

Expand All @@ -246,18 +247,18 @@ PYI062.py:16:30: PYI062 [*] Duplicate literal member `1`
15 15 | t.Literal[1, t.Literal[2, t.Literal[1]]] # once
16 |-typing_extensions.Literal[1, 1, 1] # twice
16 |+typing_extensions.Literal[1] # twice
17 17 |
18 18 | # Ensure issue is only raised once, even on nested literals
19 19 | MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
17 17 | Literal[
18 18 | 1, # comment
19 19 | Literal[ # another comment

PYI062.py:16:33: PYI062 [*] Duplicate literal member `1`
|
14 | Literal[1, Literal[2], Literal[2]] # once
15 | t.Literal[1, t.Literal[2, t.Literal[1]]] # once
16 | typing_extensions.Literal[1, 1, 1] # twice
| ^ PYI062
17 |
18 | # Ensure issue is only raised once, even on nested literals
17 | Literal[
18 | 1, # comment
|
= help: Remove duplicates

Expand All @@ -267,25 +268,51 @@ PYI062.py:16:33: PYI062 [*] Duplicate literal member `1`
15 15 | t.Literal[1, t.Literal[2, t.Literal[1]]] # once
16 |-typing_extensions.Literal[1, 1, 1] # twice
16 |+typing_extensions.Literal[1] # twice
17 17 |
18 18 | # Ensure issue is only raised once, even on nested literals
19 19 | MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
17 17 | Literal[
18 18 | 1, # comment
19 19 | Literal[ # another comment

PYI062.py:19:46: PYI062 [*] Duplicate literal member `True`
PYI062.py:20:9: PYI062 [*] Duplicate literal member `1`
|
18 | # Ensure issue is only raised once, even on nested literals
19 | MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
18 | 1, # comment
19 | Literal[ # another comment
20 | 1
| ^ PYI062
21 | ]
22 | ] # once
|
= help: Remove duplicates

ℹ Unsafe fix
14 14 | Literal[1, Literal[2], Literal[2]] # once
15 15 | t.Literal[1, t.Literal[2, t.Literal[1]]] # once
16 16 | typing_extensions.Literal[1, 1, 1] # twice
17 |-Literal[
18 |- 1, # comment
19 |- Literal[ # another comment
20 |- 1
21 |- ]
22 |-] # once
17 |+Literal[1] # once
23 18 |
24 19 | # Ensure issue is only raised once, even on nested literals
25 20 | MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062

PYI062.py:25:46: PYI062 [*] Duplicate literal member `True`
|
24 | # Ensure issue is only raised once, even on nested literals
25 | MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
| ^^^^ PYI062
20 |
21 | n: Literal["No", "duplicates", "here", 1, "1"]
26 |
27 | n: Literal["No", "duplicates", "here", 1, "1"]
|
= help: Remove duplicates

ℹ Safe fix
16 16 | typing_extensions.Literal[1, 1, 1] # twice
17 17 |
18 18 | # Ensure issue is only raised once, even on nested literals
19 |-MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
19 |+MyType = Literal["foo", True, False, "bar"] # PYI062
20 20 |
21 21 | n: Literal["No", "duplicates", "here", 1, "1"]
22 22 | ] # once
23 23 |
24 24 | # Ensure issue is only raised once, even on nested literals
25 |-MyType = Literal["foo", Literal[True, False, True], "bar"] # PYI062
25 |+MyType = Literal["foo", True, False, "bar"] # PYI062
26 26 |
27 27 | n: Literal["No", "duplicates", "here", 1, "1"]
Loading
Loading