-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 PYI064
#11325
Changes from all commits
7e566f5
aec54e8
1d79672
96eaabe
26c0ec0
ba16ffc
fcd8479
8a46f01
9d6d73e
e2b1741
15aa5ea
877ae3d
cb148cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
from typing import Final, Literal | ||
|
||
x: Final[Literal[True]] = True # PYI064 | ||
y: Final[Literal[None]] = None # PYI064 | ||
z: Final[Literal[ # PYI064 | ||
"this is a really long literal, that won't be rendered in the issue text" | ||
]] = "this is a really long literal, that won't be rendered in the issue text" | ||
|
||
# This should be fixable, and marked as safe | ||
w1: Final[Literal[123]] # PYI064 | ||
|
||
# This should not be fixable | ||
w2: Final[Literal[123]] = "random value" # PYI064 | ||
|
||
n1: Final[Literal[True, False]] = True # No issue here | ||
n2: Literal[True] = True # No issue here | ||
|
||
PlatformName = Literal["linux", "macos", "windows"] | ||
PLATFORMS: Final[set[PlatformName]] = {"linux", "macos", "windows"} # No issue here | ||
|
||
foo: Final[{1, 2, 3}] = {1, 2, 3} # No issue here | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now this mimics |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
from typing import Final, Literal | ||
|
||
x: Final[Literal[True]] # PYI064 | ||
y: Final[Literal[None]] = None # PYI064 | ||
z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 | ||
|
||
# This should be fixable, and marked as safe | ||
w1: Final[Literal[123]] # PYI064 | ||
|
||
# This should not be fixable | ||
w2: Final[Literal[123]] = "random value" # PYI064 | ||
|
||
n1: Final[Literal[True, False]] # No issue here | ||
n2: Literal[True] # No issue here | ||
|
||
PlatformName = Literal["linux", "macos", "windows"] | ||
PLATFORMS: Final[set[PlatformName]] = {"linux", "macos", "windows"} # No issue here | ||
|
||
foo: Final[{1, 2, 3}] = {1, 2, 3} # No issue here |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; | ||
use ruff_macros::{derive_message_formats, violation}; | ||
use ruff_python_ast::{self as ast, comparable::ComparableExpr}; | ||
use ruff_source_file::Locator; | ||
use ruff_text_size::{Ranged, TextSize}; | ||
|
||
use crate::checkers::ast::Checker; | ||
use crate::fix::snippet::SourceCodeSnippet; | ||
|
||
/// ## What it does | ||
/// Checks for redundant `Final[Literal[...]]` annotations. | ||
/// | ||
/// ## Why is this bad? | ||
/// A `Final[Literal[...]]` annotation can be replaced with `Final`; the literal | ||
/// use is unnecessary. | ||
/// | ||
/// ## Example | ||
/// | ||
/// ```python | ||
/// x: Final[Literal[42]] | ||
/// y: Final[Literal[42]] = 42 | ||
/// ``` | ||
/// | ||
/// Use instead: | ||
/// ```python | ||
/// x: Final = 42 | ||
/// y: Final = 42 | ||
/// ``` | ||
#[violation] | ||
pub struct RedundantFinalLiteral { | ||
literal: SourceCodeSnippet, | ||
} | ||
|
||
impl Violation for RedundantFinalLiteral { | ||
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; | ||
|
||
#[derive_message_formats] | ||
fn message(&self) -> String { | ||
let RedundantFinalLiteral { literal } = self; | ||
format!( | ||
"`Final[Literal[{literal}]]` can be replaced with a bare `Final`", | ||
literal = literal.truncated_display() | ||
) | ||
} | ||
|
||
fn fix_title(&self) -> Option<String> { | ||
Some("Replace with `Final`".to_string()) | ||
} | ||
} | ||
|
||
/// PYI064 | ||
pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::StmtAnnAssign) { | ||
if !checker.semantic().seen_typing() { | ||
return; | ||
} | ||
|
||
let ast::StmtAnnAssign { | ||
value: assign_value, | ||
annotation, | ||
.. | ||
} = ann_assign; | ||
|
||
let ast::Expr::Subscript(annotation) = &**annotation else { | ||
return; | ||
}; | ||
|
||
// Ensure it is `Final[Literal[...]]`. | ||
let ast::Expr::Subscript(ast::ExprSubscript { | ||
value, | ||
slice: literal, | ||
.. | ||
}) = &*annotation.slice | ||
else { | ||
return; | ||
}; | ||
if !checker.semantic().match_typing_expr(value, "Literal") { | ||
return; | ||
} | ||
|
||
// Discards tuples like `Literal[1, 2, 3]` and complex literals like `Literal[{1, 2}]`. | ||
if !matches!( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can potentially be moved to |
||
&**literal, | ||
ast::Expr::StringLiteral(_) | ||
| ast::Expr::BytesLiteral(_) | ||
| ast::Expr::NumberLiteral(_) | ||
| ast::Expr::BooleanLiteral(_) | ||
| ast::Expr::NoneLiteral(_) | ||
| ast::Expr::EllipsisLiteral(_) | ||
) { | ||
return; | ||
} | ||
|
||
let mut diagnostic = Diagnostic::new( | ||
RedundantFinalLiteral { | ||
literal: SourceCodeSnippet::from_str(checker.locator().slice(literal.range())), | ||
}, | ||
ann_assign.range(), | ||
); | ||
|
||
// The literal value and the assignment value being different doesn't make sense, so we skip | ||
// fixing in that case. | ||
if let Some(assign_value) = assign_value.as_ref() { | ||
if ComparableExpr::from(assign_value) == ComparableExpr::from(literal) { | ||
diagnostic.set_fix(generate_fix(annotation, None, checker.locator())); | ||
} | ||
} else { | ||
diagnostic.set_fix(generate_fix(annotation, Some(literal), checker.locator())); | ||
} | ||
|
||
checker.diagnostics.push(diagnostic); | ||
} | ||
|
||
/// Generate a fix to convert a `Final[Literal[...]]` annotation to a `Final` annotation. | ||
fn generate_fix( | ||
annotation: &ast::ExprSubscript, | ||
literal: Option<&ast::Expr>, | ||
locator: &Locator, | ||
) -> Fix { | ||
// Remove the `Literal[...]` part from `Final[Literal[...]]`. | ||
let deletion = Edit::range_deletion( | ||
annotation | ||
.slice | ||
.range() | ||
.sub_start(TextSize::new(1)) | ||
.add_end(TextSize::new(1)), | ||
); | ||
|
||
// If a literal was provided, insert an assignment. | ||
// | ||
// For example, change `x: Final[Literal[42]]` to `x: Final = 42`. | ||
if let Some(literal) = literal { | ||
let assignment = Edit::insertion( | ||
format!( | ||
" = {literal_source}", | ||
literal_source = locator.slice(literal) | ||
), | ||
annotation.end(), | ||
); | ||
Fix::safe_edits(deletion, std::iter::once(assignment)) | ||
} else { | ||
Fix::safe_edit(deletion) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
--- | ||
source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs | ||
--- | ||
PYI064.py:3:1: PYI064 [*] `Final[Literal[True]]` can be replaced with a bare `Final` | ||
| | ||
1 | from typing import Final, Literal | ||
2 | | ||
3 | x: Final[Literal[True]] = True # PYI064 | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 | ||
4 | y: Final[Literal[None]] = None # PYI064 | ||
5 | z: Final[Literal[ # PYI064 | ||
| | ||
= help: Replace with `Final` | ||
|
||
ℹ Safe fix | ||
1 1 | from typing import Final, Literal | ||
2 2 | | ||
3 |-x: Final[Literal[True]] = True # PYI064 | ||
3 |+x: Final = True # PYI064 | ||
4 4 | y: Final[Literal[None]] = None # PYI064 | ||
5 5 | z: Final[Literal[ # PYI064 | ||
6 6 | "this is a really long literal, that won't be rendered in the issue text" | ||
|
||
PYI064.py:4:1: PYI064 [*] `Final[Literal[None]]` can be replaced with a bare `Final` | ||
| | ||
3 | x: Final[Literal[True]] = True # PYI064 | ||
4 | y: Final[Literal[None]] = None # PYI064 | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 | ||
5 | z: Final[Literal[ # PYI064 | ||
6 | "this is a really long literal, that won't be rendered in the issue text" | ||
| | ||
= help: Replace with `Final` | ||
|
||
ℹ Safe fix | ||
1 1 | from typing import Final, Literal | ||
2 2 | | ||
3 3 | x: Final[Literal[True]] = True # PYI064 | ||
4 |-y: Final[Literal[None]] = None # PYI064 | ||
4 |+y: Final = None # PYI064 | ||
5 5 | z: Final[Literal[ # PYI064 | ||
6 6 | "this is a really long literal, that won't be rendered in the issue text" | ||
7 7 | ]] = "this is a really long literal, that won't be rendered in the issue text" | ||
|
||
PYI064.py:5:1: PYI064 [*] `Final[Literal[...]]` can be replaced with a bare `Final` | ||
| | ||
3 | x: Final[Literal[True]] = True # PYI064 | ||
4 | y: Final[Literal[None]] = None # PYI064 | ||
5 | / z: Final[Literal[ # PYI064 | ||
6 | | "this is a really long literal, that won't be rendered in the issue text" | ||
7 | | ]] = "this is a really long literal, that won't be rendered in the issue text" | ||
| |______________________________________________________________________________^ PYI064 | ||
8 | | ||
9 | # This should be fixable, and marked as safe | ||
| | ||
= help: Replace with `Final` | ||
|
||
ℹ Safe fix | ||
2 2 | | ||
3 3 | x: Final[Literal[True]] = True # PYI064 | ||
4 4 | y: Final[Literal[None]] = None # PYI064 | ||
5 |-z: Final[Literal[ # PYI064 | ||
6 |- "this is a really long literal, that won't be rendered in the issue text" | ||
7 |-]] = "this is a really long literal, that won't be rendered in the issue text" | ||
5 |+z: Final = "this is a really long literal, that won't be rendered in the issue text" | ||
8 6 | | ||
9 7 | # This should be fixable, and marked as safe | ||
10 8 | w1: Final[Literal[123]] # PYI064 | ||
|
||
PYI064.py:10:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Final` | ||
| | ||
9 | # This should be fixable, and marked as safe | ||
10 | w1: Final[Literal[123]] # PYI064 | ||
| ^^^^^^^^^^^^^^^^^^^^^^^ PYI064 | ||
11 | | ||
12 | # This should not be fixable | ||
| | ||
= help: Replace with `Final` | ||
|
||
ℹ Safe fix | ||
7 7 | ]] = "this is a really long literal, that won't be rendered in the issue text" | ||
8 8 | | ||
9 9 | # This should be fixable, and marked as safe | ||
10 |-w1: Final[Literal[123]] # PYI064 | ||
10 |+w1: Final = 123 # PYI064 | ||
11 11 | | ||
12 12 | # This should not be fixable | ||
13 13 | w2: Final[Literal[123]] = "random value" # PYI064 | ||
|
||
PYI064.py:13:1: PYI064 `Final[Literal[123]]` can be replaced with a bare `Final` | ||
| | ||
12 | # This should not be fixable | ||
13 | w2: Final[Literal[123]] = "random value" # PYI064 | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 | ||
14 | | ||
15 | n1: Final[Literal[True, False]] = True # No issue here | ||
| | ||
= help: Replace with `Final` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think an unsafe autofix is appropriate here (specifically when the assignment value is different from the literal value).
Is autofixing it to
w2: Final = 123
in unsafe mode fair?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ummmmmmm this kind of thing doesn't really make any sense, right? I think I'd lean towards not doing an autofix here -- "refuse the temptation to guess"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.