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 PYI064 #11325

Merged
merged 13 commits into from
May 28, 2024
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
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 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?

Copy link
Member

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.


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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now this mimics flake8-pyi behaviour, however I think raising the issue here should be fine?

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!(
Copy link
Contributor Author

@tusharsadhwani tusharsadhwani May 7, 2024

Choose a reason for hiding this comment

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

can potentially be moved to ruff_python_ast/src/helpers.rs, as a is_literal_expr helper.

&**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
Loading