From 2a36055b71233bccb9241bfe97f322d24c740e06 Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Mon, 17 Jul 2023 19:33:54 -0300 Subject: [PATCH 1/6] Implement PYI026 ## What it does Checks for `typehint.TypeAlias` annotation in type aliases. ## Why is this bad? It makes harder for someone reading the code to differentiate between a value assignment and a type alias. ## Example ```python IntOrStr = int | str ``` Use instead: ```python IntOrStr: TypeAlias = int | str ``` --- .../test/fixtures/flake8_pyi/PYI026.py | 18 +++++++ .../test/fixtures/flake8_pyi/PYI026.pyi | 18 +++++++ crates/ruff/src/checkers/ast/mod.rs | 4 ++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/rules/flake8_pyi/mod.rs | 2 + .../rules/flake8_pyi/rules/simple_defaults.rs | 47 +++++++++++++++++ ...__flake8_pyi__tests__PYI026_PYI026.py.snap | 4 ++ ..._flake8_pyi__tests__PYI026_PYI026.pyi.snap | 52 +++++++++++++++++++ ruff.schema.json | 2 +- 9 files changed, 147 insertions(+), 1 deletion(-) create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.py.snap create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py new file mode 100644 index 0000000000000..7442ab5742dae --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py @@ -0,0 +1,18 @@ +import typing +from typing import TypeAlias, Literal, Any + +NewAny = Any +OptinalStr = typing.Optional[str] +Foo = Literal["foo"] +IntOrStr = int | str +AliasNone = None + +NewAny: typing.TypeAlias = Any +OptinalStr: TypeAlias = typing.Optional[str] +Foo: typing.TypeAlias = Literal["foo"] +IntOrStr: TypeAlias = int | str +AliasNone: typing.TypeAlias = None + +# these are ok +VarAlias = str +AliasFoo = Foo diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi new file mode 100644 index 0000000000000..7442ab5742dae --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi @@ -0,0 +1,18 @@ +import typing +from typing import TypeAlias, Literal, Any + +NewAny = Any +OptinalStr = typing.Optional[str] +Foo = Literal["foo"] +IntOrStr = int | str +AliasNone = None + +NewAny: typing.TypeAlias = Any +OptinalStr: TypeAlias = typing.Optional[str] +Foo: typing.TypeAlias = Literal["foo"] +IntOrStr: TypeAlias = int | str +AliasNone: typing.TypeAlias = None + +# these are ok +VarAlias = str +AliasFoo = Foo diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 61f09c69504ea..80cbf99c5b805 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1549,6 +1549,7 @@ where Rule::UnprefixedTypeParam, Rule::AssignmentDefaultInStub, Rule::UnannotatedAssignmentInStub, + Rule::TypeAliasCheck, ]) { // Ignore assignments in function bodies; those are covered by other rules. if !self @@ -1567,6 +1568,9 @@ where self, targets, value, ); } + if self.enabled(Rule::TypeAliasCheck) { + flake8_pyi::rules::type_alias_check(self, value, targets); + } } } } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index b77dccb198269..8657e97791341 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -633,6 +633,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Pyi, "021") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::DocstringInStub), (Flake8Pyi, "024") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::CollectionsNamedTuple), (Flake8Pyi, "025") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnaliasedCollectionsAbcSetImport), + (Flake8Pyi, "026") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::TypeAliasCheck), (Flake8Pyi, "029") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::StrOrReprDefinedInStub), (Flake8Pyi, "030") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnnecessaryLiteralUnion), (Flake8Pyi, "032") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::AnyEqNeAnnotation), diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index b931f17c0b9b2..13de1a91c0b4f 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/mod.rs @@ -87,6 +87,8 @@ mod tests { #[test_case(Rule::UnrecognizedVersionInfoCheck, Path::new("PYI003.pyi"))] #[test_case(Rule::WrongTupleLengthVersionComparison, Path::new("PYI005.py"))] #[test_case(Rule::WrongTupleLengthVersionComparison, Path::new("PYI005.pyi"))] + #[test_case(Rule::TypeAliasCheck, Path::new("PYI026.py"))] + #[test_case(Rule::TypeAliasCheck, Path::new("PYI026.pyi"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs index 6470c817250d3..33c2d06e045a6 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs @@ -97,6 +97,20 @@ impl Violation for UnassignedSpecialVariableInStub { } } +#[violation] +pub struct TypeAliasCheck { + name: String, + value: String, +} + +impl Violation for TypeAliasCheck { + #[derive_message_formats] + fn message(&self) -> String { + let TypeAliasCheck { name, value } = self; + format!("Use `typing.TypeAlias` for type aliases in `{name}`, e.g. \"{name}: typing.TypeAlias = {value}\"") + } +} + fn is_allowed_negated_math_attribute(call_path: &CallPath) -> bool { matches!(call_path.as_slice(), ["math", "inf" | "e" | "pi" | "tau"]) } @@ -523,3 +537,36 @@ pub(crate) fn unassigned_special_variable_in_stub( stmt.range(), )); } + +/// PIY026 +pub(crate) fn type_alias_check(checker: &mut Checker, value: &Expr, targets: &[Expr]) { + let is_any = |expr: &Expr| { + let Expr::Name(ast::ExprName { id, .. }) = expr else { + return false; + }; + id == "Any" + }; + + let target = &targets[0]; + if is_special_assignment(target, checker.semantic()) { + return; + } + if matches!(value, Expr::Name(_)) && !is_any(value) { + return; + } + if !is_valid_pep_604_union(value) { + return; + } + + let Expr::Name(ast::ExprName { id, .. }) = target else { + return; + }; + + checker.diagnostics.push(Diagnostic::new( + TypeAliasCheck { + name: id.to_string(), + value: checker.generator().expr(value), + }, + target.range(), + )); +} diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.py.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.py.snap new file mode 100644 index 0000000000000..d1aa2e9116558 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.py.snap @@ -0,0 +1,4 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +--- + diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap new file mode 100644 index 0000000000000..c4575d3d0e2a0 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap @@ -0,0 +1,52 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +--- +PYI026.pyi:4:1: PYI026 Use `typing.TypeAlias` for type aliases in `NewAny`, e.g. "NewAny: typing.TypeAlias = Any" + | +2 | from typing import TypeAlias, Literal, Any +3 | +4 | NewAny = Any + | ^^^^^^ PYI026 +5 | OptinalStr = typing.Optional[str] +6 | Foo = Literal["foo"] + | + +PYI026.pyi:5:1: PYI026 Use `typing.TypeAlias` for type aliases in `OptinalStr`, e.g. "OptinalStr: typing.TypeAlias = typing.Optional[str]" + | +4 | NewAny = Any +5 | OptinalStr = typing.Optional[str] + | ^^^^^^^^^^ PYI026 +6 | Foo = Literal["foo"] +7 | IntOrStr = int | str + | + +PYI026.pyi:6:1: PYI026 Use `typing.TypeAlias` for type aliases in `Foo`, e.g. "Foo: typing.TypeAlias = Literal["foo"]" + | +4 | NewAny = Any +5 | OptinalStr = typing.Optional[str] +6 | Foo = Literal["foo"] + | ^^^ PYI026 +7 | IntOrStr = int | str +8 | AliasNone = None + | + +PYI026.pyi:7:1: PYI026 Use `typing.TypeAlias` for type aliases in `IntOrStr`, e.g. "IntOrStr: typing.TypeAlias = int | str" + | +5 | OptinalStr = typing.Optional[str] +6 | Foo = Literal["foo"] +7 | IntOrStr = int | str + | ^^^^^^^^ PYI026 +8 | AliasNone = None + | + +PYI026.pyi:8:1: PYI026 Use `typing.TypeAlias` for type aliases in `AliasNone`, e.g. "AliasNone: typing.TypeAlias = None" + | + 6 | Foo = Literal["foo"] + 7 | IntOrStr = int | str + 8 | AliasNone = None + | ^^^^^^^^^ PYI026 + 9 | +10 | NewAny: typing.TypeAlias = Any + | + + diff --git a/ruff.schema.json b/ruff.schema.json index a20e05447c8bb..8017dec0d6122 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2351,6 +2351,7 @@ "PYI021", "PYI024", "PYI025", + "PYI026", "PYI029", "PYI03", "PYI030", @@ -2409,7 +2410,6 @@ "RUF011", "RUF012", "RUF013", - "RUF014", "RUF015", "RUF016", "RUF1", From eb3e65498e0bf5fe31bb71d34ad8ded77ca78f13 Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Mon, 17 Jul 2023 19:55:52 -0300 Subject: [PATCH 2/6] add docs --- .../rules/flake8_pyi/rules/simple_defaults.rs | 26 ++++++++++++++++--- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs index 33c2d06e045a6..8af8cb33892af 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs @@ -97,6 +97,22 @@ impl Violation for UnassignedSpecialVariableInStub { } } +/// ## What it does +/// Checks for `typehint.TypeAlias` annotation in type aliases. +/// +/// ## Why is this bad? +/// It makes harder for someone reading the code to differentiate +/// between a value assignment and a type alias. +/// +/// ## Example +/// ```python +/// IntOrStr = int | str +/// ``` +/// +/// Use instead: +/// ```python +/// IntOrStr: TypeAlias = int | str +/// ``` #[violation] pub struct TypeAliasCheck { name: String, @@ -413,9 +429,10 @@ pub(crate) fn argument_simple_defaults(checker: &mut Checker, arguments: &Argume /// PYI015 pub(crate) fn assignment_default_in_stub(checker: &mut Checker, targets: &[Expr], value: &Expr) { - let [target] = targets else { + if targets.len() != 1 { return; - }; + } + let target = &targets[0]; if !target.is_name_expr() { return; } @@ -484,9 +501,10 @@ pub(crate) fn unannotated_assignment_in_stub( targets: &[Expr], value: &Expr, ) { - let [target] = targets else { + if targets.len() != 1 { return; - }; + } + let target = &targets[0]; let Expr::Name(ast::ExprName { id, .. }) = target else { return; }; From 341f6007b245a8211cd2fe40478b4eb75d0c99fb Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Tue, 18 Jul 2023 17:09:27 -0300 Subject: [PATCH 3/6] fix typo and add new test case --- crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py | 5 +++-- crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py index 7442ab5742dae..afdbfe6480693 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.py @@ -2,15 +2,16 @@ from typing import TypeAlias, Literal, Any NewAny = Any -OptinalStr = typing.Optional[str] +OptionalStr = typing.Optional[str] Foo = Literal["foo"] IntOrStr = int | str AliasNone = None NewAny: typing.TypeAlias = Any -OptinalStr: TypeAlias = typing.Optional[str] +OptionalStr: TypeAlias = typing.Optional[str] Foo: typing.TypeAlias = Literal["foo"] IntOrStr: TypeAlias = int | str +IntOrFloat: Foo = int | float AliasNone: typing.TypeAlias = None # these are ok diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi index 7442ab5742dae..afdbfe6480693 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi @@ -2,15 +2,16 @@ import typing from typing import TypeAlias, Literal, Any NewAny = Any -OptinalStr = typing.Optional[str] +OptionalStr = typing.Optional[str] Foo = Literal["foo"] IntOrStr = int | str AliasNone = None NewAny: typing.TypeAlias = Any -OptinalStr: TypeAlias = typing.Optional[str] +OptionalStr: TypeAlias = typing.Optional[str] Foo: typing.TypeAlias = Literal["foo"] IntOrStr: TypeAlias = int | str +IntOrFloat: Foo = int | float AliasNone: typing.TypeAlias = None # these are ok From 8a348472b840830e4a2f4a4dfc9c196cf198d457 Mon Sep 17 00:00:00 2001 From: LaBatata101 Date: Tue, 18 Jul 2023 23:10:34 -0300 Subject: [PATCH 4/6] review feedback - fix a bug in `is_valid_pep_604_union` where it was returning `true` when only receiving an `Expr::Name`, `Expr::Subscript`, `Expr::Attribute` or `Expr::Constant(ast::ExprConstant {value: Constant::None, ..})` - don't run rule for multiple targets, e.g., `x = y = 1` - change the verification for when the rule should run to match the flake8-pyi implementation - implement autofix --- .../test/fixtures/flake8_pyi/PYI026.pyi | 3 +- .../rules/flake8_pyi/rules/simple_defaults.rs | 98 ++++++++----- ..._flake8_pyi__tests__PYI026_PYI026.pyi.snap | 133 +++++++++++++----- 3 files changed, 164 insertions(+), 70 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi index afdbfe6480693..87cb0ffe0d3d5 100644 --- a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI026.pyi @@ -1,5 +1,4 @@ -import typing -from typing import TypeAlias, Literal, Any +from typing import Literal, Any NewAny = Any OptionalStr = typing.Optional[str] diff --git a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs index 8af8cb33892af..a697d7301972d 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs @@ -9,6 +9,7 @@ use ruff_python_ast::source_code::Locator; use ruff_python_semantic::{ScopeKind, SemanticModel}; use crate::checkers::ast::Checker; +use crate::importer::ImportRequest; use crate::registry::AsRule; #[violation] @@ -101,7 +102,7 @@ impl Violation for UnassignedSpecialVariableInStub { /// Checks for `typehint.TypeAlias` annotation in type aliases. /// /// ## Why is this bad? -/// It makes harder for someone reading the code to differentiate +/// It makes it harder for someone reading the code to differentiate /// between a value assignment and a type alias. /// /// ## Example @@ -119,11 +120,15 @@ pub struct TypeAliasCheck { value: String, } -impl Violation for TypeAliasCheck { +impl AlwaysAutofixableViolation for TypeAliasCheck { #[derive_message_formats] fn message(&self) -> String { let TypeAliasCheck { name, value } = self; - format!("Use `typing.TypeAlias` for type aliases in `{name}`, e.g. \"{name}: typing.TypeAlias = {value}\"") + format!("Use `typing.TypeAlias` for type alias `{name}`, e.g. \"{name}: typing.TypeAlias = {value}\"") + } + + fn autofix_title(&self) -> String { + "Add `typing.TypeAlias` annotation".to_string() } } @@ -262,6 +267,20 @@ fn is_valid_default_value_with_annotation( false } +/// Returns `true` if an [`Expr`] appears to be a valid PEP 604 union member. +fn is_valid_pep_604_union_member(value: &Expr) -> bool { + matches!( + value, + Expr::Name(_) + | Expr::Subscript(_) + | Expr::Attribute(_) + | Expr::Constant(ast::ExprConstant { + value: Constant::None, + .. + }) + ) +} + /// Returns `true` if an [`Expr`] appears to be a valid PEP 604 union. (e.g. `int | None`) fn is_valid_pep_604_union(annotation: &Expr) -> bool { match annotation { @@ -270,14 +289,10 @@ fn is_valid_pep_604_union(annotation: &Expr) -> bool { op: Operator::BitOr, right, range: _, - }) => is_valid_pep_604_union(left) && is_valid_pep_604_union(right), - Expr::Name(_) - | Expr::Subscript(_) - | Expr::Attribute(_) - | Expr::Constant(ast::ExprConstant { - value: Constant::None, - .. - }) => true, + }) => { + (is_valid_pep_604_union_member(left) || is_valid_pep_604_union(left)) + && is_valid_pep_604_union_member(right) + } _ => false, } } @@ -353,6 +368,20 @@ fn is_enum(bases: &[Expr], semantic: &SemanticModel) -> bool { }); } +/// Returns `true` if an [`Expr`] is valid for a type alias check. +fn validate_type_alias_value(value: &Expr, semantic: &SemanticModel) -> bool { + is_valid_pep_604_union(value) + || semantic.match_typing_expr(value, "Any") + || matches!( + value, + Expr::Subscript(_) + | Expr::Constant(ast::ExprConstant { + value: Constant::None, + .. + }), + ) +} + /// PYI011 pub(crate) fn typed_argument_simple_defaults(checker: &mut Checker, arguments: &Arguments) { for ArgWithDefault { @@ -429,10 +458,9 @@ pub(crate) fn argument_simple_defaults(checker: &mut Checker, arguments: &Argume /// PYI015 pub(crate) fn assignment_default_in_stub(checker: &mut Checker, targets: &[Expr], value: &Expr) { - if targets.len() != 1 { + let [target] = targets else { return; - } - let target = &targets[0]; + }; if !target.is_name_expr() { return; } @@ -501,10 +529,9 @@ pub(crate) fn unannotated_assignment_in_stub( targets: &[Expr], value: &Expr, ) { - if targets.len() != 1 { + let [target] = targets else { return; - } - let target = &targets[0]; + }; let Expr::Name(ast::ExprName { id, .. }) = target else { return; }; @@ -558,33 +585,36 @@ pub(crate) fn unassigned_special_variable_in_stub( /// PIY026 pub(crate) fn type_alias_check(checker: &mut Checker, value: &Expr, targets: &[Expr]) { - let is_any = |expr: &Expr| { - let Expr::Name(ast::ExprName { id, .. }) = expr else { - return false; - }; - id == "Any" - }; - - let target = &targets[0]; - if is_special_assignment(target, checker.semantic()) { - return; - } - if matches!(value, Expr::Name(_)) && !is_any(value) { + let [target] = targets else { return; - } - if !is_valid_pep_604_union(value) { + }; + if !validate_type_alias_value(value, checker.semantic()) { return; } let Expr::Name(ast::ExprName { id, .. }) = target else { - return; + return; }; - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( TypeAliasCheck { name: id.to_string(), value: checker.generator().expr(value), }, target.range(), - )); + ); + if checker.patch(diagnostic.kind.rule()) { + diagnostic.try_set_fix(|| { + let (import_edit, binding) = checker.importer.get_or_import_symbol( + &ImportRequest::import("typing", "TypeAlias"), + target.start(), + checker.semantic(), + )?; + Ok(Fix::suggested_edits( + Edit::range_replacement(format!("{id}: {binding}"), target.range()), + [import_edit], + )) + }); + } + checker.diagnostics.push(diagnostic); } diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap index c4575d3d0e2a0..4ec013ee26bc0 100644 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap @@ -1,52 +1,117 @@ --- source: crates/ruff/src/rules/flake8_pyi/mod.rs --- -PYI026.pyi:4:1: PYI026 Use `typing.TypeAlias` for type aliases in `NewAny`, e.g. "NewAny: typing.TypeAlias = Any" +PYI026.pyi:3:1: PYI026 [*] Use `typing.TypeAlias` for type alias `NewAny`, e.g. "NewAny: typing.TypeAlias = Any" | -2 | from typing import TypeAlias, Literal, Any -3 | -4 | NewAny = Any +1 | from typing import Literal, Any +2 | +3 | NewAny = Any | ^^^^^^ PYI026 -5 | OptinalStr = typing.Optional[str] -6 | Foo = Literal["foo"] +4 | OptionalStr = typing.Optional[str] +5 | Foo = Literal["foo"] | + = help: Add `typing.TypeAlias` annotation -PYI026.pyi:5:1: PYI026 Use `typing.TypeAlias` for type aliases in `OptinalStr`, e.g. "OptinalStr: typing.TypeAlias = typing.Optional[str]" +ℹ Suggested fix +1 |-from typing import Literal, Any + 1 |+from typing import Literal, Any, TypeAlias +2 2 | +3 |-NewAny = Any + 3 |+NewAny: TypeAlias = Any +4 4 | OptionalStr = typing.Optional[str] +5 5 | Foo = Literal["foo"] +6 6 | IntOrStr = int | str + +PYI026.pyi:4:1: PYI026 [*] Use `typing.TypeAlias` for type alias `OptionalStr`, e.g. "OptionalStr: typing.TypeAlias = typing.Optional[str]" | -4 | NewAny = Any -5 | OptinalStr = typing.Optional[str] - | ^^^^^^^^^^ PYI026 -6 | Foo = Literal["foo"] -7 | IntOrStr = int | str +3 | NewAny = Any +4 | OptionalStr = typing.Optional[str] + | ^^^^^^^^^^^ PYI026 +5 | Foo = Literal["foo"] +6 | IntOrStr = int | str | + = help: Add `typing.TypeAlias` annotation + +ℹ Suggested fix +1 |-from typing import Literal, Any + 1 |+from typing import Literal, Any, TypeAlias +2 2 | +3 3 | NewAny = Any +4 |-OptionalStr = typing.Optional[str] + 4 |+OptionalStr: TypeAlias = typing.Optional[str] +5 5 | Foo = Literal["foo"] +6 6 | IntOrStr = int | str +7 7 | AliasNone = None -PYI026.pyi:6:1: PYI026 Use `typing.TypeAlias` for type aliases in `Foo`, e.g. "Foo: typing.TypeAlias = Literal["foo"]" +PYI026.pyi:5:1: PYI026 [*] Use `typing.TypeAlias` for type alias `Foo`, e.g. "Foo: typing.TypeAlias = Literal["foo"]" | -4 | NewAny = Any -5 | OptinalStr = typing.Optional[str] -6 | Foo = Literal["foo"] +3 | NewAny = Any +4 | OptionalStr = typing.Optional[str] +5 | Foo = Literal["foo"] | ^^^ PYI026 -7 | IntOrStr = int | str -8 | AliasNone = None +6 | IntOrStr = int | str +7 | AliasNone = None | + = help: Add `typing.TypeAlias` annotation + +ℹ Suggested fix +1 |-from typing import Literal, Any + 1 |+from typing import Literal, Any, TypeAlias +2 2 | +3 3 | NewAny = Any +4 4 | OptionalStr = typing.Optional[str] +5 |-Foo = Literal["foo"] + 5 |+Foo: TypeAlias = Literal["foo"] +6 6 | IntOrStr = int | str +7 7 | AliasNone = None +8 8 | -PYI026.pyi:7:1: PYI026 Use `typing.TypeAlias` for type aliases in `IntOrStr`, e.g. "IntOrStr: typing.TypeAlias = int | str" +PYI026.pyi:6:1: PYI026 [*] Use `typing.TypeAlias` for type alias `IntOrStr`, e.g. "IntOrStr: typing.TypeAlias = int | str" | -5 | OptinalStr = typing.Optional[str] -6 | Foo = Literal["foo"] -7 | IntOrStr = int | str +4 | OptionalStr = typing.Optional[str] +5 | Foo = Literal["foo"] +6 | IntOrStr = int | str | ^^^^^^^^ PYI026 -8 | AliasNone = None - | - -PYI026.pyi:8:1: PYI026 Use `typing.TypeAlias` for type aliases in `AliasNone`, e.g. "AliasNone: typing.TypeAlias = None" - | - 6 | Foo = Literal["foo"] - 7 | IntOrStr = int | str - 8 | AliasNone = None - | ^^^^^^^^^ PYI026 - 9 | -10 | NewAny: typing.TypeAlias = Any - | +7 | AliasNone = None + | + = help: Add `typing.TypeAlias` annotation + +ℹ Suggested fix +1 |-from typing import Literal, Any + 1 |+from typing import Literal, Any, TypeAlias +2 2 | +3 3 | NewAny = Any +4 4 | OptionalStr = typing.Optional[str] +5 5 | Foo = Literal["foo"] +6 |-IntOrStr = int | str + 6 |+IntOrStr: TypeAlias = int | str +7 7 | AliasNone = None +8 8 | +9 9 | NewAny: typing.TypeAlias = Any + +PYI026.pyi:7:1: PYI026 [*] Use `typing.TypeAlias` for type alias `AliasNone`, e.g. "AliasNone: typing.TypeAlias = None" + | +5 | Foo = Literal["foo"] +6 | IntOrStr = int | str +7 | AliasNone = None + | ^^^^^^^^^ PYI026 +8 | +9 | NewAny: typing.TypeAlias = Any + | + = help: Add `typing.TypeAlias` annotation + +ℹ Suggested fix +1 |-from typing import Literal, Any + 1 |+from typing import Literal, Any, TypeAlias +2 2 | +3 3 | NewAny = Any +4 4 | OptionalStr = typing.Optional[str] +5 5 | Foo = Literal["foo"] +6 6 | IntOrStr = int | str +7 |-AliasNone = None + 7 |+AliasNone: TypeAlias = None +8 8 | +9 9 | NewAny: typing.TypeAlias = Any +10 10 | OptionalStr: TypeAlias = typing.Optional[str] From 18d6b12e075066682c2702671234b2816b7fd905 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 19 Jul 2023 21:28:07 -0400 Subject: [PATCH 5/6] Minor tweaks --- crates/ruff/src/checkers/ast/mod.rs | 8 ++- crates/ruff/src/codes.rs | 2 +- crates/ruff/src/rules/flake8_pyi/mod.rs | 4 +- .../rules/flake8_pyi/rules/simple_defaults.rs | 61 +++++++++++-------- ..._flake8_pyi__tests__PYI026_PYI026.pyi.snap | 10 +-- 5 files changed, 49 insertions(+), 36 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 2c3db15c99648..35e0fed7dcafd 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1557,7 +1557,7 @@ where Rule::UnprefixedTypeParam, Rule::AssignmentDefaultInStub, Rule::UnannotatedAssignmentInStub, - Rule::TypeAliasCheck, + Rule::TypeAliasWithoutAnnotation, ]) { // Ignore assignments in function bodies; those are covered by other rules. if !self @@ -1576,8 +1576,10 @@ where self, targets, value, ); } - if self.enabled(Rule::TypeAliasCheck) { - flake8_pyi::rules::type_alias_check(self, value, targets); + if self.enabled(Rule::TypeAliasWithoutAnnotation) { + flake8_pyi::rules::type_alias_without_annotation( + self, value, targets, + ); } } } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 9727132601336..101a2ea94389d 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -633,7 +633,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Flake8Pyi, "021") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::DocstringInStub), (Flake8Pyi, "024") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::CollectionsNamedTuple), (Flake8Pyi, "025") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnaliasedCollectionsAbcSetImport), - (Flake8Pyi, "026") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::TypeAliasCheck), + (Flake8Pyi, "026") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::TypeAliasWithoutAnnotation), (Flake8Pyi, "029") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::StrOrReprDefinedInStub), (Flake8Pyi, "030") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::UnnecessaryLiteralUnion), (Flake8Pyi, "032") => (RuleGroup::Unspecified, rules::flake8_pyi::rules::AnyEqNeAnnotation), diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index 13de1a91c0b4f..be8d717f25c1d 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/mod.rs @@ -87,8 +87,8 @@ mod tests { #[test_case(Rule::UnrecognizedVersionInfoCheck, Path::new("PYI003.pyi"))] #[test_case(Rule::WrongTupleLengthVersionComparison, Path::new("PYI005.py"))] #[test_case(Rule::WrongTupleLengthVersionComparison, Path::new("PYI005.pyi"))] - #[test_case(Rule::TypeAliasCheck, Path::new("PYI026.py"))] - #[test_case(Rule::TypeAliasCheck, Path::new("PYI026.pyi"))] + #[test_case(Rule::TypeAliasWithoutAnnotation, Path::new("PYI026.py"))] + #[test_case(Rule::TypeAliasWithoutAnnotation, Path::new("PYI026.pyi"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs index a697d7301972d..fc780b6cb4efa 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs @@ -99,32 +99,39 @@ impl Violation for UnassignedSpecialVariableInStub { } /// ## What it does -/// Checks for `typehint.TypeAlias` annotation in type aliases. +/// Checks for type alias definitions that are not annotated with +/// `typing.TypeAlias`. /// /// ## Why is this bad? -/// It makes it harder for someone reading the code to differentiate -/// between a value assignment and a type alias. +/// In Python, a type alias is defined by assigning a type to a variable (e.g., +/// `Vector = list[float]`). +/// +/// It's best to annotate type aliases with the `typing.TypeAlias` type to +/// make it clear that the statement is a type alias declaration, as opposed +/// to a normal variable assignment. /// /// ## Example /// ```python -/// IntOrStr = int | str +/// Vector = list[float] /// ``` /// /// Use instead: /// ```python -/// IntOrStr: TypeAlias = int | str +/// from typing import TypeAlias +/// +/// Vector: TypeAlias = list[float] /// ``` #[violation] -pub struct TypeAliasCheck { +pub struct TypeAliasWithoutAnnotation { name: String, value: String, } -impl AlwaysAutofixableViolation for TypeAliasCheck { +impl AlwaysAutofixableViolation for TypeAliasWithoutAnnotation { #[derive_message_formats] fn message(&self) -> String { - let TypeAliasCheck { name, value } = self; - format!("Use `typing.TypeAlias` for type alias `{name}`, e.g. \"{name}: typing.TypeAlias = {value}\"") + let TypeAliasWithoutAnnotation { name, value } = self; + format!("Use `typing.TypeAlias` for type alias, e.g., `{name}: typing.TypeAlias = {value}`") } fn autofix_title(&self) -> String { @@ -368,18 +375,21 @@ fn is_enum(bases: &[Expr], semantic: &SemanticModel) -> bool { }); } -/// Returns `true` if an [`Expr`] is valid for a type alias check. -fn validate_type_alias_value(value: &Expr, semantic: &SemanticModel) -> bool { - is_valid_pep_604_union(value) +/// Returns `true` if an [`Expr`] is a value that should be annotated with `typing.TypeAlias`. +/// +/// This is relatively conservative, as it's hard to reliably detect whether a right-hand side is a +/// valid type alias. In particular, this function checks for uses of `typing.Any`, `None`, +/// parameterized generics, and PEP 604-style unions. +fn is_annotatable_type_alias(value: &Expr, semantic: &SemanticModel) -> bool { + matches!( + value, + Expr::Subscript(_) + | Expr::Constant(ast::ExprConstant { + value: Constant::None, + .. + }), + ) || is_valid_pep_604_union(value) || semantic.match_typing_expr(value, "Any") - || matches!( - value, - Expr::Subscript(_) - | Expr::Constant(ast::ExprConstant { - value: Constant::None, - .. - }), - ) } /// PYI011 @@ -584,20 +594,21 @@ pub(crate) fn unassigned_special_variable_in_stub( } /// PIY026 -pub(crate) fn type_alias_check(checker: &mut Checker, value: &Expr, targets: &[Expr]) { +pub(crate) fn type_alias_without_annotation(checker: &mut Checker, value: &Expr, targets: &[Expr]) { let [target] = targets else { return; }; - if !validate_type_alias_value(value, checker.semantic()) { - return; - } let Expr::Name(ast::ExprName { id, .. }) = target else { return; }; + if !is_annotatable_type_alias(value, checker.semantic()) { + return; + } + let mut diagnostic = Diagnostic::new( - TypeAliasCheck { + TypeAliasWithoutAnnotation { name: id.to_string(), value: checker.generator().expr(value), }, diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap index 4ec013ee26bc0..075d94d02c900 100644 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI026_PYI026.pyi.snap @@ -1,7 +1,7 @@ --- source: crates/ruff/src/rules/flake8_pyi/mod.rs --- -PYI026.pyi:3:1: PYI026 [*] Use `typing.TypeAlias` for type alias `NewAny`, e.g. "NewAny: typing.TypeAlias = Any" +PYI026.pyi:3:1: PYI026 [*] Use `typing.TypeAlias` for type alias, e.g., `NewAny: typing.TypeAlias = Any` | 1 | from typing import Literal, Any 2 | @@ -22,7 +22,7 @@ PYI026.pyi:3:1: PYI026 [*] Use `typing.TypeAlias` for type alias `NewAny`, e.g. 5 5 | Foo = Literal["foo"] 6 6 | IntOrStr = int | str -PYI026.pyi:4:1: PYI026 [*] Use `typing.TypeAlias` for type alias `OptionalStr`, e.g. "OptionalStr: typing.TypeAlias = typing.Optional[str]" +PYI026.pyi:4:1: PYI026 [*] Use `typing.TypeAlias` for type alias, e.g., `OptionalStr: typing.TypeAlias = typing.Optional[str]` | 3 | NewAny = Any 4 | OptionalStr = typing.Optional[str] @@ -43,7 +43,7 @@ PYI026.pyi:4:1: PYI026 [*] Use `typing.TypeAlias` for type alias `OptionalStr`, 6 6 | IntOrStr = int | str 7 7 | AliasNone = None -PYI026.pyi:5:1: PYI026 [*] Use `typing.TypeAlias` for type alias `Foo`, e.g. "Foo: typing.TypeAlias = Literal["foo"]" +PYI026.pyi:5:1: PYI026 [*] Use `typing.TypeAlias` for type alias, e.g., `Foo: typing.TypeAlias = Literal["foo"]` | 3 | NewAny = Any 4 | OptionalStr = typing.Optional[str] @@ -66,7 +66,7 @@ PYI026.pyi:5:1: PYI026 [*] Use `typing.TypeAlias` for type alias `Foo`, e.g. "Fo 7 7 | AliasNone = None 8 8 | -PYI026.pyi:6:1: PYI026 [*] Use `typing.TypeAlias` for type alias `IntOrStr`, e.g. "IntOrStr: typing.TypeAlias = int | str" +PYI026.pyi:6:1: PYI026 [*] Use `typing.TypeAlias` for type alias, e.g., `IntOrStr: typing.TypeAlias = int | str` | 4 | OptionalStr = typing.Optional[str] 5 | Foo = Literal["foo"] @@ -89,7 +89,7 @@ PYI026.pyi:6:1: PYI026 [*] Use `typing.TypeAlias` for type alias `IntOrStr`, e.g 8 8 | 9 9 | NewAny: typing.TypeAlias = Any -PYI026.pyi:7:1: PYI026 [*] Use `typing.TypeAlias` for type alias `AliasNone`, e.g. "AliasNone: typing.TypeAlias = None" +PYI026.pyi:7:1: PYI026 [*] Use `typing.TypeAlias` for type alias, e.g., `AliasNone: typing.TypeAlias = None` | 5 | Foo = Literal["foo"] 6 | IntOrStr = int | str From bf80cfd539af7feecd58efc66a2335d12260bf12 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 19 Jul 2023 21:33:04 -0400 Subject: [PATCH 6/6] Tweak is_valid_pep_604_union --- .../rules/flake8_pyi/rules/simple_defaults.rs | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs index fc780b6cb4efa..f7313d756f0b6 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/simple_defaults.rs @@ -274,34 +274,41 @@ fn is_valid_default_value_with_annotation( false } -/// Returns `true` if an [`Expr`] appears to be a valid PEP 604 union member. -fn is_valid_pep_604_union_member(value: &Expr) -> bool { - matches!( - value, - Expr::Name(_) +/// Returns `true` if an [`Expr`] appears to be a valid PEP 604 union. (e.g. `int | None`) +fn is_valid_pep_604_union(annotation: &Expr) -> bool { + /// Returns `true` if an [`Expr`] appears to be a valid PEP 604 union member. + fn is_valid_pep_604_union_member(value: &Expr) -> bool { + match value { + Expr::BinOp(ast::ExprBinOp { + left, + op: Operator::BitOr, + right, + range: _, + }) => is_valid_pep_604_union_member(left) && is_valid_pep_604_union_member(right), + Expr::Name(_) | Expr::Subscript(_) | Expr::Attribute(_) | Expr::Constant(ast::ExprConstant { value: Constant::None, .. - }) - ) -} - -/// Returns `true` if an [`Expr`] appears to be a valid PEP 604 union. (e.g. `int | None`) -fn is_valid_pep_604_union(annotation: &Expr) -> bool { - match annotation { - Expr::BinOp(ast::ExprBinOp { - left, - op: Operator::BitOr, - right, - range: _, - }) => { - (is_valid_pep_604_union_member(left) || is_valid_pep_604_union(left)) - && is_valid_pep_604_union_member(right) + }) => true, + _ => false, } - _ => false, } + + // The top-level expression must be a bit-or operation. + let Expr::BinOp(ast::ExprBinOp { + left, + op: Operator::BitOr, + right, + range: _, + }) = annotation + else { + return false; + }; + + // The left and right operands must be valid union members. + is_valid_pep_604_union_member(left) && is_valid_pep_604_union_member(right) } /// Returns `true` if an [`Expr`] appears to be a valid default value without an annotation.