Skip to content

Commit

Permalink
[flake8-pyi] Implement PYI064 (#11325)
Browse files Browse the repository at this point in the history
## Summary

Implements `Y064` from `flake8-pyi` and its autofix.

## Test Plan

`cargo test` / `cargo insta review`
  • Loading branch information
tusharsadhwani authored May 28, 2024
1 parent 9a3b9f9 commit e0169d8
Show file tree
Hide file tree
Showing 11 changed files with 389 additions and 5 deletions.
21 changes: 21 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py
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
19 changes: 19 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi
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
7 changes: 7 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1633,6 +1633,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::TSuffixedTypeAlias) {
flake8_pyi::rules::t_suffixed_type_alias(checker, target);
}
} else if checker
.semantic
.match_typing_expr(helpers::map_subscript(annotation), "Final")
{
if checker.enabled(Rule::RedundantFinalLiteral) {
flake8_pyi::rules::redundant_final_literal(checker, assign_stmt);
}
}
}
Stmt::TypeAlias(ast::StmtTypeAlias { name, .. }) => {
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 @@ -811,6 +811,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Pyi, "058") => (RuleGroup::Stable, rules::flake8_pyi::rules::GeneratorReturnFromIterMethod),
(Flake8Pyi, "059") => (RuleGroup::Preview, rules::flake8_pyi::rules::GenericNotLastBaseClass),
(Flake8Pyi, "062") => (RuleGroup::Preview, rules::flake8_pyi::rules::DuplicateLiteralMember),
(Flake8Pyi, "064") => (RuleGroup::Preview, rules::flake8_pyi::rules::RedundantFinalLiteral),

// 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 @@ -63,6 +63,8 @@ mod tests {
#[test_case(Rule::PatchVersionComparison, Path::new("PYI004.pyi"))]
#[test_case(Rule::QuotedAnnotationInStub, Path::new("PYI020.py"))]
#[test_case(Rule::QuotedAnnotationInStub, Path::new("PYI020.pyi"))]
#[test_case(Rule::RedundantFinalLiteral, Path::new("PYI064.py"))]
#[test_case(Rule::RedundantFinalLiteral, Path::new("PYI064.pyi"))]
#[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.py"))]
#[test_case(Rule::RedundantLiteralUnion, Path::new("PYI051.pyi"))]
#[test_case(Rule::RedundantNumericUnion, Path::new("PYI041.py"))]
Expand Down
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 @@ -21,6 +21,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_final_literal::*;
pub(crate) use redundant_literal_union::*;
pub(crate) use redundant_numeric_union::*;
pub(crate) use simple_defaults::*;
Expand Down Expand Up @@ -61,6 +62,7 @@ mod pass_in_class_body;
mod pass_statement_stub_body;
mod prefix_type_params;
mod quoted_annotation_in_stub;
mod redundant_final_literal;
mod redundant_literal_union;
mod redundant_numeric_union;
mod simple_defaults;
Expand Down
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!(
&**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`
Loading

0 comments on commit e0169d8

Please sign in to comment.