From 7e566f5c1d20f2568f55d3a6145739f0f2647c9b Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Tue, 7 May 2024 18:02:42 +0530 Subject: [PATCH 01/11] [flake8-pyi] Implement PYI064 --- .../test/fixtures/flake8_pyi/PYI064.py | 16 +++ .../test/fixtures/flake8_pyi/PYI064.pyi | 14 +++ .../src/checkers/ast/analyze/statement.rs | 7 ++ crates/ruff_linter/src/codes.rs | 1 + .../ruff_linter/src/rules/flake8_pyi/mod.rs | 2 + .../src/rules/flake8_pyi/rules/mod.rs | 2 + .../rules/redundant_final_literal.rs | 111 ++++++++++++++++++ ...__flake8_pyi__tests__PYI064_PYI064.py.snap | 107 +++++++++++++++++ ..._flake8_pyi__tests__PYI064_PYI064.pyi.snap | 102 ++++++++++++++++ crates/ruff_python_ast/src/helpers.rs | 9 +- crates/ruff_python_semantic/src/model.rs | 9 ++ 11 files changed, 375 insertions(+), 5 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py new file mode 100644 index 0000000000000..32b50a2d68461 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py @@ -0,0 +1,16 @@ +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 be fixable, but marked as unsafe +w2: Final[Literal[123]] = "random value" # PYI064 + +n1: Final[Literal[True, False]] = True # No issue here +n2: Literal[True] = True # No issue here diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi new file mode 100644 index 0000000000000..a7835e993ec34 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi @@ -0,0 +1,14 @@ +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 be fixable, but marked as unsafe +w2: Final[Literal[123]] = "random value" # PYI064 + +n1: Final[Literal[True, False]] # No issue here +n2: Literal[True] # No issue here diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index cc9fd59ed2b9f..ffe3597f12d21 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -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, .. }) => { diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index d6f675f19db1f..4c02a7b14c0c5 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -809,6 +809,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Pyi, "056") => (RuleGroup::Stable, rules::flake8_pyi::rules::UnsupportedMethodCallOnAll), (Flake8Pyi, "058") => (RuleGroup::Stable, rules::flake8_pyi::rules::GeneratorReturnFromIterMethod), (Flake8Pyi, "059") => (RuleGroup::Preview, rules::flake8_pyi::rules::GenericNotLastBaseClass), + (Flake8Pyi, "064") => (RuleGroup::Preview, rules::flake8_pyi::rules::RedundantFinalLiteral), // flake8-pytest-style (Flake8PytestStyle, "001") => (RuleGroup::Stable, rules::flake8_pytest_style::rules::PytestFixtureIncorrectParenthesesStyle), diff --git a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs index 75a5387db3c08..fcfed4a58042b 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs @@ -61,6 +61,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"))] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs index dbf28eefd7327..6e11c94ec4c12 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs @@ -20,6 +20,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::*; @@ -59,6 +60,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; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs new file mode 100644 index 0000000000000..4438ac0dc5899 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -0,0 +1,111 @@ +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_text_size::Ranged; + +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[x]]` annotation can be replaced with just `Final`. +/// +/// ## Example +/// +/// ```python +/// x: Final[Literal[42]] +/// # or, +/// x: Final[Literal[42]] = 42 +/// ``` +/// +/// Use instead: +/// ```python +/// x: 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; + if let Some(literal) = literal.full_display() { + format!("`Final[Literal[{literal}]]` can be replaced with a bare `Final`") + } else { + format!("`Final[Literal[...]] can be replaced with a bare `Final`") + } + } + + fn fix_title(&self) -> Option { + let RedundantFinalLiteral { literal } = self; + if let Some(literal) = literal.full_display() { + Some(format!( + "Replace `Final[Literal[{literal}]]` with a bare `Final`" + )) + } else { + Some(format!("Replace `Final[Literal[...]] with a bare `Final`")) + } + } +} + +/// PYI064 +pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::StmtAnnAssign) { + let ast::StmtAnnAssign { + value: assign_value, + annotation, + .. + } = ann_assign; + + let ast::Expr::Subscript(ast::ExprSubscript { + slice: literal_slice, + .. + }) = &**annotation + else { + return; + }; + let ast::Expr::Subscript(ast::ExprSubscript { slice: literal, .. }) = &**literal_slice else { + return; + }; + + // If the Literal contains multiple elements, don't raise issue + if let ast::Expr::Tuple(_) = &**literal { + return; + } + + checker.diagnostics.push( + Diagnostic::new( + RedundantFinalLiteral { + literal: SourceCodeSnippet::from_str(checker.locator().slice(literal.range())), + }, + ann_assign.range(), + ) + .with_fix(generate_fix(annotation, assign_value.as_deref(), literal)), + ); +} + +fn generate_fix( + annotation: &ast::Expr, + assign_value: Option<&ast::Expr>, + literal: &ast::Expr, +) -> Fix { + let deletion = Edit::range_deletion(annotation.range()); + let insertion = Edit::insertion(format!("Final"), annotation.start()); + + let Some(assign_value) = assign_value else { + // TODO: modify the assign side + return Fix::safe_edits(deletion, [insertion]); + }; + + if ComparableExpr::from(assign_value) != ComparableExpr::from(literal) { + // TODO: modify the assign side + return Fix::unsafe_edits(deletion, [insertion]); + } + + Fix::safe_edits(deletion, [insertion]) +} diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap new file mode 100644 index 0000000000000..17345d4f1c9d0 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap @@ -0,0 +1,107 @@ +--- +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 `Final[Literal[True]]` with a bare `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 `Final[Literal[None]]` with a bare `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 `Final[Literal[...]] with a bare `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 be fixable, but marked as unsafe + | + = help: Replace `Final[Literal[123]]` with a bare `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 # PYI064 +11 11 | +12 12 | # This should be fixable, but marked as unsafe +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 be fixable, but marked as unsafe +13 | w2: Final[Literal[123]] = "random value" # PYI064 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 +14 | +15 | n1: Final[Literal[True, False]] = True # No issue here + | + = help: Replace `Final[Literal[123]]` with a bare `Final` + +ℹ Unsafe fix +10 10 | w1: Final[Literal[123]] # PYI064 +11 11 | +12 12 | # This should be fixable, but marked as unsafe +13 |-w2: Final[Literal[123]] = "random value" # PYI064 + 13 |+w2: Final = "random value" # PYI064 +14 14 | +15 15 | n1: Final[Literal[True, False]] = True # No issue here +16 16 | n2: Literal[True] = True # No issue here diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap new file mode 100644 index 0000000000000..7af3d0bb6eaaa --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap @@ -0,0 +1,102 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- +PYI064.pyi: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]] # PYI064 + | ^^^^^^^^^^^^^^^^^^^^^^^ PYI064 +4 | y: Final[Literal[None]] = None # PYI064 +5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 + | + = help: Replace `Final[Literal[True]]` with a bare `Final` + +ℹ Safe fix +1 1 | from typing import Final, Literal +2 2 | +3 |-x: Final[Literal[True]] # PYI064 + 3 |+x: Final # PYI064 +4 4 | y: Final[Literal[None]] = None # PYI064 +5 5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 +6 6 | + +PYI064.pyi:4:1: PYI064 [*] `Final[Literal[None]]` can be replaced with a bare `Final` + | +3 | x: Final[Literal[True]] # PYI064 +4 | y: Final[Literal[None]] = None # PYI064 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 +5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 + | + = help: Replace `Final[Literal[None]]` with a bare `Final` + +ℹ Safe fix +1 1 | from typing import Final, Literal +2 2 | +3 3 | x: Final[Literal[True]] # PYI064 +4 |-y: Final[Literal[None]] = None # PYI064 + 4 |+y: Final = None # PYI064 +5 5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 +6 6 | +7 7 | # This should be fixable, and marked as safe + +PYI064.pyi:5:1: PYI064 [*] `Final[Literal[...]] can be replaced with a bare `Final` + | +3 | x: Final[Literal[True]] # PYI064 +4 | y: Final[Literal[None]] = None # PYI064 +5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 +6 | +7 | # This should be fixable, and marked as safe + | + = help: Replace `Final[Literal[...]] with a bare `Final` + +ℹ Safe fix +2 2 | +3 3 | x: Final[Literal[True]] # PYI064 +4 4 | y: Final[Literal[None]] = None # PYI064 +5 |-z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 + 5 |+z: Final # PYI064 +6 6 | +7 7 | # This should be fixable, and marked as safe +8 8 | w1: Final[Literal[123]] # PYI064 + +PYI064.pyi:8:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Final` + | + 7 | # This should be fixable, and marked as safe + 8 | w1: Final[Literal[123]] # PYI064 + | ^^^^^^^^^^^^^^^^^^^^^^^ PYI064 + 9 | +10 | # This should be fixable, but marked as unsafe + | + = help: Replace `Final[Literal[123]]` with a bare `Final` + +ℹ Safe fix +5 5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 +6 6 | +7 7 | # This should be fixable, and marked as safe +8 |-w1: Final[Literal[123]] # PYI064 + 8 |+w1: Final # PYI064 +9 9 | +10 10 | # This should be fixable, but marked as unsafe +11 11 | w2: Final[Literal[123]] = "random value" # PYI064 + +PYI064.pyi:11:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Final` + | +10 | # This should be fixable, but marked as unsafe +11 | w2: Final[Literal[123]] = "random value" # PYI064 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 +12 | +13 | n1: Final[Literal[True, False]] # No issue here + | + = help: Replace `Final[Literal[123]]` with a bare `Final` + +ℹ Unsafe fix +8 8 | w1: Final[Literal[123]] # PYI064 +9 9 | +10 10 | # This should be fixable, but marked as unsafe +11 |-w2: Final[Literal[123]] = "random value" # PYI064 + 11 |+w2: Final = "random value" # PYI064 +12 12 | +13 13 | n1: Final[Literal[True, False]] # No issue here +14 14 | n2: Literal[True] # No issue here diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 7213355ff6af8..100189e596335 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -659,15 +659,14 @@ pub fn map_callable(decorator: &Expr) -> &Expr { } } -/// Given an [`Expr`] that can be callable or not (like a decorator, which could -/// be used with or without explicit call syntax), return the underlying -/// callable. +/// Given an [`Expr`] that can be a [`ExprSubscript`][ast::ExprSubscript] or not +/// (like an annotation that may be generic or not), return the underlying expr. pub fn map_subscript(expr: &Expr) -> &Expr { if let Expr::Subscript(ast::ExprSubscript { value, .. }) = expr { - // Ex) `Iterable[T]` + // Ex) `Iterable[T]` => return `Iterable` value } else { - // Ex) `Iterable` + // Ex) `Iterable` => return `Iterable` expr } } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 8e0c0c1f47d29..ea6ca0e0e2e9a 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1350,6 +1350,15 @@ impl<'a> SemanticModel<'a> { false } + /// Return `true` if the model is in a nested literal expression (e.g., the inner `Literal` in + /// `Literal[Literal[int, str], float]`). + pub fn in_nested_literal(&self) -> bool { + // Ex) `Literal[Literal[int, str], float]` + self.current_expression_grandparent() + .and_then(Expr::as_subscript_expr) + .is_some_and(|parent| self.match_typing_expr(&parent.value, "Literal")) + } + /// Returns `true` if `left` and `right` are in the same branches of an `if`, `match`, or /// `try` statement. /// From aec54e8cca3150c5f2d2d11031f1d6ee7ee15be7 Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Tue, 7 May 2024 23:36:25 +0530 Subject: [PATCH 02/11] implement assignment changes in autofix --- .../rules/redundant_final_literal.rs | 25 +++++++++++++++---- ...__flake8_pyi__tests__PYI064_PYI064.py.snap | 4 +-- ..._flake8_pyi__tests__PYI064_PYI064.pyi.snap | 8 +++--- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index 4438ac0dc5899..0a45b748bb744 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -85,11 +85,17 @@ pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::S }, ann_assign.range(), ) - .with_fix(generate_fix(annotation, assign_value.as_deref(), literal)), + .with_fix(generate_fix( + checker, + annotation, + assign_value.as_deref(), + literal, + )), ); } fn generate_fix( + checker: &Checker, annotation: &ast::Expr, assign_value: Option<&ast::Expr>, literal: &ast::Expr, @@ -98,13 +104,22 @@ fn generate_fix( let insertion = Edit::insertion(format!("Final"), annotation.start()); let Some(assign_value) = assign_value else { - // TODO: modify the assign side - return Fix::safe_edits(deletion, [insertion]); + // If no assignment exists, add our own, same as the literal value. + let literal_source = checker.locator().slice(literal.range()); + let assignment = Edit::insertion(format!(" = {literal_source}"), annotation.end()); + return Fix::safe_edits(deletion, [insertion, assignment]); }; if ComparableExpr::from(assign_value) != ComparableExpr::from(literal) { - // TODO: modify the assign side - return Fix::unsafe_edits(deletion, [insertion]); + // In this case, assume that the value in the literal annotation + // is the correct one. + let literal_source = checker.locator().slice(literal.range()); + let assign_replacement = Edit::replacement( + literal_source.to_string(), + assign_value.start(), + assign_value.end(), + ); + return Fix::unsafe_edits(deletion, [insertion, assign_replacement]); } Fix::safe_edits(deletion, [insertion]) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap index 17345d4f1c9d0..48f54fca02463 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap @@ -81,7 +81,7 @@ PYI064.py:10:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 8 8 | 9 9 | # This should be fixable, and marked as safe 10 |-w1: Final[Literal[123]] # PYI064 - 10 |+w1: Final # PYI064 + 10 |+w1: Final = 123 # PYI064 11 11 | 12 12 | # This should be fixable, but marked as unsafe 13 13 | w2: Final[Literal[123]] = "random value" # PYI064 @@ -101,7 +101,7 @@ PYI064.py:13:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 11 11 | 12 12 | # This should be fixable, but marked as unsafe 13 |-w2: Final[Literal[123]] = "random value" # PYI064 - 13 |+w2: Final = "random value" # PYI064 + 13 |+w2: Final = 123 # PYI064 14 14 | 15 15 | n1: Final[Literal[True, False]] = True # No issue here 16 16 | n2: Literal[True] = True # No issue here diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap index 7af3d0bb6eaaa..d98b5446462fa 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap @@ -16,7 +16,7 @@ PYI064.pyi:3:1: PYI064 [*] `Final[Literal[True]]` can be replaced with a bare `F 1 1 | from typing import Final, Literal 2 2 | 3 |-x: Final[Literal[True]] # PYI064 - 3 |+x: Final # PYI064 + 3 |+x: Final = True # PYI064 4 4 | y: Final[Literal[None]] = None # PYI064 5 5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 6 6 | @@ -56,7 +56,7 @@ PYI064.pyi:5:1: PYI064 [*] `Final[Literal[...]] can be replaced with a bare `Fin 3 3 | x: Final[Literal[True]] # PYI064 4 4 | y: Final[Literal[None]] = None # PYI064 5 |-z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 - 5 |+z: Final # PYI064 + 5 |+z: Final = "this is a really long literal, that won't be rendered in the issue text" # PYI064 6 6 | 7 7 | # This should be fixable, and marked as safe 8 8 | w1: Final[Literal[123]] # PYI064 @@ -76,7 +76,7 @@ PYI064.pyi:8:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 6 6 | 7 7 | # This should be fixable, and marked as safe 8 |-w1: Final[Literal[123]] # PYI064 - 8 |+w1: Final # PYI064 + 8 |+w1: Final = 123 # PYI064 9 9 | 10 10 | # This should be fixable, but marked as unsafe 11 11 | w2: Final[Literal[123]] = "random value" # PYI064 @@ -96,7 +96,7 @@ PYI064.pyi:11:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `F 9 9 | 10 10 | # This should be fixable, but marked as unsafe 11 |-w2: Final[Literal[123]] = "random value" # PYI064 - 11 |+w2: Final = "random value" # PYI064 + 11 |+w2: Final = 123 # PYI064 12 12 | 13 13 | n1: Final[Literal[True, False]] # No issue here 14 14 | n2: Literal[True] # No issue here From 1d79672eb369baf30626afc08f164bd794fb541e Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Tue, 7 May 2024 23:42:16 +0530 Subject: [PATCH 03/11] update schema --- ruff.schema.json | 1 + 1 file changed, 1 insertion(+) diff --git a/ruff.schema.json b/ruff.schema.json index 4b374adba80b8..e6e98770d96c5 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3574,6 +3574,7 @@ "PYI056", "PYI058", "PYI059", + "PYI064", "Q", "Q0", "Q00", From 96eaabe987b035059dfdda40c47fef643f4dd509 Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Tue, 7 May 2024 23:43:14 +0530 Subject: [PATCH 04/11] fix is always available as of now --- .../src/rules/flake8_pyi/rules/redundant_final_literal.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index 0a45b748bb744..e28d399997192 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -30,7 +30,7 @@ pub struct RedundantFinalLiteral { } impl Violation for RedundantFinalLiteral { - const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always; #[derive_message_formats] fn message(&self) -> String { From 26c0ec01cf4e30861c8ebf22c29e65d606548e6f Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Tue, 7 May 2024 23:48:37 +0530 Subject: [PATCH 05/11] schema fix again --- ruff.schema.json | 1 + 1 file changed, 1 insertion(+) diff --git a/ruff.schema.json b/ruff.schema.json index e6e98770d96c5..8f02e5d216aef 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3574,6 +3574,7 @@ "PYI056", "PYI058", "PYI059", + "PYI06", "PYI064", "Q", "Q0", From fcd8479957f3457e24ace750d015ef15a032b92b Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Wed, 8 May 2024 00:13:31 +0530 Subject: [PATCH 06/11] make the weird case non autofixable --- .../test/fixtures/flake8_pyi/PYI064.py | 2 +- .../test/fixtures/flake8_pyi/PYI064.pyi | 2 +- .../rules/redundant_final_literal.rs | 51 ++++++++----------- ...__flake8_pyi__tests__PYI064_PYI064.py.snap | 18 ++----- ..._flake8_pyi__tests__PYI064_PYI064.pyi.snap | 18 ++----- 5 files changed, 32 insertions(+), 59 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py index 32b50a2d68461..d8925ced741f9 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py @@ -9,7 +9,7 @@ # This should be fixable, and marked as safe w1: Final[Literal[123]] # PYI064 -# This should be fixable, but marked as unsafe +# This should not be fixable w2: Final[Literal[123]] = "random value" # PYI064 n1: Final[Literal[True, False]] = True # No issue here diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi index a7835e993ec34..964da3b62fc83 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi @@ -7,7 +7,7 @@ z: Final[Literal["this is a really long literal, that won't be rendered in the i # This should be fixable, and marked as safe w1: Final[Literal[123]] # PYI064 -# This should be fixable, but marked as unsafe +# This should not be fixable w2: Final[Literal[123]] = "random value" # PYI064 n1: Final[Literal[True, False]] # No issue here diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index e28d399997192..23ae4b0f805da 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -30,7 +30,7 @@ pub struct RedundantFinalLiteral { } impl Violation for RedundantFinalLiteral { - const FIX_AVAILABILITY: FixAvailability = FixAvailability::Always; + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] fn message(&self) -> String { @@ -78,49 +78,42 @@ pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::S return; } - checker.diagnostics.push( - Diagnostic::new( - RedundantFinalLiteral { - literal: SourceCodeSnippet::from_str(checker.locator().slice(literal.range())), - }, - ann_assign.range(), - ) - .with_fix(generate_fix( + 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 don't do an autofix if that happens. + if !assign_value.as_ref().is_some_and(|assign_value| { + ComparableExpr::from(assign_value) != ComparableExpr::from(literal) + }) { + diagnostic.set_fix(generate_fix( checker, annotation, - assign_value.as_deref(), literal, - )), - ); + assign_value.is_none(), + )); + } + checker.diagnostics.push(diagnostic); } fn generate_fix( checker: &Checker, annotation: &ast::Expr, - assign_value: Option<&ast::Expr>, literal: &ast::Expr, + add_assignment: bool, ) -> Fix { let deletion = Edit::range_deletion(annotation.range()); - let insertion = Edit::insertion(format!("Final"), annotation.start()); + let mut insertions = vec![Edit::insertion(format!("Final"), annotation.start())]; - let Some(assign_value) = assign_value else { + if add_assignment { // If no assignment exists, add our own, same as the literal value. let literal_source = checker.locator().slice(literal.range()); let assignment = Edit::insertion(format!(" = {literal_source}"), annotation.end()); - return Fix::safe_edits(deletion, [insertion, assignment]); + insertions.push(assignment) }; - if ComparableExpr::from(assign_value) != ComparableExpr::from(literal) { - // In this case, assume that the value in the literal annotation - // is the correct one. - let literal_source = checker.locator().slice(literal.range()); - let assign_replacement = Edit::replacement( - literal_source.to_string(), - assign_value.start(), - assign_value.end(), - ); - return Fix::unsafe_edits(deletion, [insertion, assign_replacement]); - } - - Fix::safe_edits(deletion, [insertion]) + Fix::safe_edits(deletion, insertions) } diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap index 48f54fca02463..25d80ab022234 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap @@ -72,7 +72,7 @@ PYI064.py:10:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 10 | w1: Final[Literal[123]] # PYI064 | ^^^^^^^^^^^^^^^^^^^^^^^ PYI064 11 | -12 | # This should be fixable, but marked as unsafe +12 | # This should not be fixable | = help: Replace `Final[Literal[123]]` with a bare `Final` @@ -83,25 +83,15 @@ PYI064.py:10:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 10 |-w1: Final[Literal[123]] # PYI064 10 |+w1: Final = 123 # PYI064 11 11 | -12 12 | # This should be fixable, but marked as unsafe +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` +PYI064.py:13:1: PYI064 `Final[Literal[123]]` can be replaced with a bare `Final` | -12 | # This should be fixable, but marked as unsafe +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 `Final[Literal[123]]` with a bare `Final` - -ℹ Unsafe fix -10 10 | w1: Final[Literal[123]] # PYI064 -11 11 | -12 12 | # This should be fixable, but marked as unsafe -13 |-w2: Final[Literal[123]] = "random value" # PYI064 - 13 |+w2: Final = 123 # PYI064 -14 14 | -15 15 | n1: Final[Literal[True, False]] = True # No issue here -16 16 | n2: Literal[True] = True # No issue here diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap index d98b5446462fa..1bc662004de7a 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap @@ -67,7 +67,7 @@ PYI064.pyi:8:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 8 | w1: Final[Literal[123]] # PYI064 | ^^^^^^^^^^^^^^^^^^^^^^^ PYI064 9 | -10 | # This should be fixable, but marked as unsafe +10 | # This should not be fixable | = help: Replace `Final[Literal[123]]` with a bare `Final` @@ -78,25 +78,15 @@ PYI064.pyi:8:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 8 |-w1: Final[Literal[123]] # PYI064 8 |+w1: Final = 123 # PYI064 9 9 | -10 10 | # This should be fixable, but marked as unsafe +10 10 | # This should not be fixable 11 11 | w2: Final[Literal[123]] = "random value" # PYI064 -PYI064.pyi:11:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Final` +PYI064.pyi:11:1: PYI064 `Final[Literal[123]]` can be replaced with a bare `Final` | -10 | # This should be fixable, but marked as unsafe +10 | # This should not be fixable 11 | w2: Final[Literal[123]] = "random value" # PYI064 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 12 | 13 | n1: Final[Literal[True, False]] # No issue here | = help: Replace `Final[Literal[123]]` with a bare `Final` - -ℹ Unsafe fix -8 8 | w1: Final[Literal[123]] # PYI064 -9 9 | -10 10 | # This should be fixable, but marked as unsafe -11 |-w2: Final[Literal[123]] = "random value" # PYI064 - 11 |+w2: Final = 123 # PYI064 -12 12 | -13 13 | n1: Final[Literal[True, False]] # No issue here -14 14 | n2: Literal[True] # No issue here From 8a46f0198e03758a9b1e9d619f0fef8a7044ab16 Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Wed, 8 May 2024 00:17:17 +0530 Subject: [PATCH 07/11] clippy --- .../src/rules/flake8_pyi/rules/redundant_final_literal.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index 23ae4b0f805da..4d8790309a92a 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -112,7 +112,7 @@ fn generate_fix( // If no assignment exists, add our own, same as the literal value. let literal_source = checker.locator().slice(literal.range()); let assignment = Edit::insertion(format!(" = {literal_source}"), annotation.end()); - insertions.push(assignment) + insertions.push(assignment); }; Fix::safe_edits(deletion, insertions) From 9d6d73e66d9750225d1dd4eaf90cd3a7427b8c67 Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Wed, 8 May 2024 00:53:50 +0530 Subject: [PATCH 08/11] add test for non literal final --- .../resources/test/fixtures/flake8_pyi/PYI064.py | 3 +++ .../resources/test/fixtures/flake8_pyi/PYI064.pyi | 3 +++ .../flake8_pyi/rules/redundant_final_literal.rs | 13 +++++++++++-- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py index d8925ced741f9..011e41ba7bb45 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py @@ -14,3 +14,6 @@ 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 diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi index 964da3b62fc83..8661b7b6d015f 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi @@ -12,3 +12,6 @@ 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 \ No newline at end of file diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index 4d8790309a92a..497b454e16ba3 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -73,8 +73,17 @@ pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::S return; }; - // If the Literal contains multiple elements, don't raise issue - if let ast::Expr::Tuple(_) = &**literal { + // 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; } From e2b1741298669005d80b12781775ba0aead59272 Mon Sep 17 00:00:00 2001 From: Tushar Sadhwani Date: Wed, 8 May 2024 01:19:27 +0530 Subject: [PATCH 09/11] add more tests --- .../resources/test/fixtures/flake8_pyi/PYI064.py | 2 ++ .../resources/test/fixtures/flake8_pyi/PYI064.pyi | 4 +++- .../flake8_pyi/rules/redundant_final_literal.rs | 14 +++++++++++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py index 011e41ba7bb45..198c02a3d643f 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.py @@ -17,3 +17,5 @@ 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 diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi index 8661b7b6d015f..feb8f7926bca1 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI064.pyi @@ -14,4 +14,6 @@ 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 \ No newline at end of file +PLATFORMS: Final[set[PlatformName]] = {"linux", "macos", "windows"} # No issue here + +foo: Final[{1, 2, 3}] = {1, 2, 3} # No issue here diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index 497b454e16ba3..64753ad87001c 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -1,6 +1,6 @@ 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_python_ast::{self as ast, comparable::ComparableExpr, helpers::map_subscript}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -56,6 +56,10 @@ impl Violation for RedundantFinalLiteral { /// 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, @@ -69,6 +73,14 @@ pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::S else { return; }; + + // Ensure it is `Final[Literal[...]]` + if !checker + .semantic() + .match_typing_expr(map_subscript(literal_slice), "Literal") + { + return; + } let ast::Expr::Subscript(ast::ExprSubscript { slice: literal, .. }) = &**literal_slice else { return; }; From 877ae3d77168a7d1b5b879b0248b7234036e5494 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 28 May 2024 19:51:16 -0400 Subject: [PATCH 10/11] Simplify fix and message --- .../rules/redundant_final_literal.rs | 116 +++++++++--------- ...__flake8_pyi__tests__PYI064_PYI064.py.snap | 12 +- ..._flake8_pyi__tests__PYI064_PYI064.pyi.snap | 12 +- 3 files changed, 71 insertions(+), 69 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index 64753ad87001c..3810ee1ab79ca 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -1,28 +1,30 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::{self as ast, comparable::ComparableExpr, helpers::map_subscript}; -use ruff_text_size::Ranged; +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. +/// Checks for redundant `Final[Literal[...]]` annotations. /// /// ## Why is this bad? -/// A `Final[Literal[x]]` annotation can be replaced with just `Final`. +/// A `Final[Literal[...]]` annotation can be replaced with `Final`; the literal +/// use is unnecessary. /// /// ## Example /// /// ```python /// x: Final[Literal[42]] -/// # or, -/// x: Final[Literal[42]] = 42 +/// y: Final[Literal[42]] = 42 /// ``` /// /// Use instead: /// ```python /// x: Final = 42 +/// y: Final = 42 /// ``` #[violation] pub struct RedundantFinalLiteral { @@ -35,22 +37,14 @@ impl Violation for RedundantFinalLiteral { #[derive_message_formats] fn message(&self) -> String { let RedundantFinalLiteral { literal } = self; - if let Some(literal) = literal.full_display() { - format!("`Final[Literal[{literal}]]` can be replaced with a bare `Final`") - } else { - format!("`Final[Literal[...]] can be replaced with a bare `Final`") - } + format!( + "`Final[Literal[{literal}]]` can be replaced with a bare `Final`", + literal = literal.truncated_display() + ) } fn fix_title(&self) -> Option { - let RedundantFinalLiteral { literal } = self; - if let Some(literal) = literal.full_display() { - Some(format!( - "Replace `Final[Literal[{literal}]]` with a bare `Final`" - )) - } else { - Some(format!("Replace `Final[Literal[...]] with a bare `Final`")) - } + Some("Replace with `Final`".to_string()) } } @@ -66,27 +60,24 @@ pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::S .. } = ann_assign; + let ast::Expr::Subscript(annotation) = &**annotation else { + return; + }; + + // Ensure it is `Final[Literal[...]]`. let ast::Expr::Subscript(ast::ExprSubscript { - slice: literal_slice, + value, + slice: literal, .. - }) = &**annotation + }) = &*annotation.slice else { return; }; - - // Ensure it is `Final[Literal[...]]` - if !checker - .semantic() - .match_typing_expr(map_subscript(literal_slice), "Literal") - { + if !checker.semantic().match_typing_expr(value, "Literal") { return; } - let ast::Expr::Subscript(ast::ExprSubscript { slice: literal, .. }) = &**literal_slice else { - return; - }; - // Discards tuples like `Literal[1, 2, 3]` - // and complex literals like `Literal[{1, 2}]` + // Discards tuples like `Literal[1, 2, 3]` and complex literals like `Literal[{1, 2}]`. if !matches!( &**literal, ast::Expr::StringLiteral(_) @@ -105,36 +96,47 @@ pub(crate) fn redundant_final_literal(checker: &mut Checker, ann_assign: &ast::S }, ann_assign.range(), ); - // The literal value and the assignment value being different doesn't - // make sense, so we don't do an autofix if that happens. - if !assign_value.as_ref().is_some_and(|assign_value| { - ComparableExpr::from(assign_value) != ComparableExpr::from(literal) - }) { - diagnostic.set_fix(generate_fix( - checker, - annotation, - literal, - assign_value.is_none(), - )); + + // 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( - checker: &Checker, - annotation: &ast::Expr, - literal: &ast::Expr, - add_assignment: bool, + annotation: &ast::ExprSubscript, + literal: Option<&ast::Expr>, + locator: &Locator, ) -> Fix { - let deletion = Edit::range_deletion(annotation.range()); - let mut insertions = vec![Edit::insertion(format!("Final"), annotation.start())]; - - if add_assignment { - // If no assignment exists, add our own, same as the literal value. - let literal_source = checker.locator().slice(literal.range()); - let assignment = Edit::insertion(format!(" = {literal_source}"), annotation.end()); - insertions.push(assignment); - }; + // 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)), + ); - Fix::safe_edits(deletion, insertions) + if let Some(literal) = literal { + // If a literal was provided, insert an assignment. + let assignment = Edit::insertion( + format!( + " = {literal_source}", + literal_source = locator.slice(literal) + ), + annotation.end(), + ); + Fix::safe_edits(deletion, std::iter::once(assignment)) + } else { + // If no literal exists, we can remove the assignment. + Fix::safe_edit(deletion) + } } diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap index 25d80ab022234..4e441d5012a1f 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.py.snap @@ -10,7 +10,7 @@ PYI064.py:3:1: PYI064 [*] `Final[Literal[True]]` can be replaced with a bare `Fi 4 | y: Final[Literal[None]] = None # PYI064 5 | z: Final[Literal[ # PYI064 | - = help: Replace `Final[Literal[True]]` with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 1 1 | from typing import Final, Literal @@ -29,7 +29,7 @@ PYI064.py:4:1: PYI064 [*] `Final[Literal[None]]` can be replaced with a bare `Fi 5 | z: Final[Literal[ # PYI064 6 | "this is a really long literal, that won't be rendered in the issue text" | - = help: Replace `Final[Literal[None]]` with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 1 1 | from typing import Final, Literal @@ -41,7 +41,7 @@ PYI064.py:4:1: PYI064 [*] `Final[Literal[None]]` can be replaced with a bare `Fi 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` +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 @@ -52,7 +52,7 @@ PYI064.py:5:1: PYI064 [*] `Final[Literal[...]] can be replaced with a bare `Fina 8 | 9 | # This should be fixable, and marked as safe | - = help: Replace `Final[Literal[...]] with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 2 2 | @@ -74,7 +74,7 @@ PYI064.py:10:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 11 | 12 | # This should not be fixable | - = help: Replace `Final[Literal[123]]` with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 7 7 | ]] = "this is a really long literal, that won't be rendered in the issue text" @@ -94,4 +94,4 @@ PYI064.py:13:1: PYI064 `Final[Literal[123]]` can be replaced with a bare `Final` 14 | 15 | n1: Final[Literal[True, False]] = True # No issue here | - = help: Replace `Final[Literal[123]]` with a bare `Final` + = help: Replace with `Final` diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap index 1bc662004de7a..2c25d8adc5f36 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI064_PYI064.pyi.snap @@ -10,7 +10,7 @@ PYI064.pyi:3:1: PYI064 [*] `Final[Literal[True]]` can be replaced with a bare `F 4 | y: Final[Literal[None]] = None # PYI064 5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 | - = help: Replace `Final[Literal[True]]` with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 1 1 | from typing import Final, Literal @@ -28,7 +28,7 @@ PYI064.pyi:4:1: PYI064 [*] `Final[Literal[None]]` can be replaced with a bare `F | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PYI064 5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 | - = help: Replace `Final[Literal[None]]` with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 1 1 | from typing import Final, Literal @@ -40,7 +40,7 @@ PYI064.pyi:4:1: PYI064 [*] `Final[Literal[None]]` can be replaced with a bare `F 6 6 | 7 7 | # This should be fixable, and marked as safe -PYI064.pyi:5:1: PYI064 [*] `Final[Literal[...]] can be replaced with a bare `Final` +PYI064.pyi:5:1: PYI064 [*] `Final[Literal[...]]` can be replaced with a bare `Final` | 3 | x: Final[Literal[True]] # PYI064 4 | y: Final[Literal[None]] = None # PYI064 @@ -49,7 +49,7 @@ PYI064.pyi:5:1: PYI064 [*] `Final[Literal[...]] can be replaced with a bare `Fin 6 | 7 | # This should be fixable, and marked as safe | - = help: Replace `Final[Literal[...]] with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 2 2 | @@ -69,7 +69,7 @@ PYI064.pyi:8:1: PYI064 [*] `Final[Literal[123]]` can be replaced with a bare `Fi 9 | 10 | # This should not be fixable | - = help: Replace `Final[Literal[123]]` with a bare `Final` + = help: Replace with `Final` ℹ Safe fix 5 5 | z: Final[Literal["this is a really long literal, that won't be rendered in the issue text"]] # PYI064 @@ -89,4 +89,4 @@ PYI064.pyi:11:1: PYI064 `Final[Literal[123]]` can be replaced with a bare `Final 12 | 13 | n1: Final[Literal[True, False]] # No issue here | - = help: Replace `Final[Literal[123]]` with a bare `Final` + = help: Replace with `Final` From cb148cc40d7fb6423042fd490756f2563fa89897 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 28 May 2024 19:53:25 -0400 Subject: [PATCH 11/11] Fix comment --- .../src/rules/flake8_pyi/rules/redundant_final_literal.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs index 3810ee1ab79ca..8f93ba0d5d968 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_final_literal.rs @@ -125,8 +125,10 @@ fn generate_fix( .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 { - // If a literal was provided, insert an assignment. let assignment = Edit::insertion( format!( " = {literal_source}", @@ -136,7 +138,6 @@ fn generate_fix( ); Fix::safe_edits(deletion, std::iter::once(assignment)) } else { - // If no literal exists, we can remove the assignment. Fix::safe_edit(deletion) } }