From 89d35c2842f330e62b6e63f61a85d4a60b0720a8 Mon Sep 17 00:00:00 2001 From: David Salvisberg Date: Wed, 8 Jan 2025 13:39:38 +0100 Subject: [PATCH] [`flake8-type-checking`] Avoid false positives for `|` in `TC008` --- .../TC008_union_syntax_pre_py310.py | 11 ++++ .../src/rules/flake8_type_checking/mod.rs | 18 +++++++ .../rules/type_alias_quotes.rs | 53 +++++++++++++++---- ...alias_TC008_union_syntax_pre_py310.py.snap | 36 +++++++++++++ 4 files changed, 109 insertions(+), 9 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC008_union_syntax_pre_py310.py create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__pre_py310_quoted-type-alias_TC008_union_syntax_pre_py310.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC008_union_syntax_pre_py310.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC008_union_syntax_pre_py310.py new file mode 100644 index 0000000000000..c7870ef9b9fa6 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC008_union_syntax_pre_py310.py @@ -0,0 +1,11 @@ +from __future__ import annotations + +from typing import Annotated, TypeAlias, TYPE_CHECKING + +a: TypeAlias = 'int | None' # OK +b: TypeAlias = 'Annotated[int, 1 | 2]' # False negative in runtime context + +if TYPE_CHECKING: + c: TypeAlias = 'int | None' # OK + d: TypeAlias = 'Annotated[int, 1 | 2]' # TC008 + e: TypeAlias = 'Annotated[int, 1 + 2]' # TC008 diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs index 3535901c90b34..ae376b3f14995 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -81,6 +81,24 @@ mod tests { Ok(()) } + #[test_case(Rule::QuotedTypeAlias, Path::new("TC008_union_syntax_pre_py310.py"))] + fn type_alias_rules_pre_py310(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "pre_py310_{}_{}", + rule_code.as_ref(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("flake8_type_checking").join(path).as_path(), + &settings::LinterSettings { + target_version: settings::types::PythonVersion::Py39, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote.py"))] #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("quote.py"))] #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote2.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/type_alias_quotes.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/type_alias_quotes.rs index e2f3c7a04e6c7..b1c56d34dde34 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/type_alias_quotes.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/type_alias_quotes.rs @@ -1,4 +1,3 @@ -use crate::registry::Rule; use ast::{ExprContext, Operator}; use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; @@ -9,7 +8,10 @@ use ruff_python_stdlib::typing::{is_pep_593_generic_type, is_standard_library_li use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::registry::Rule; use crate::rules::flake8_type_checking::helpers::quote_type_expression; +use crate::settings::types::PythonVersion; +use crate::settings::LinterSettings; /// ## What it does /// Checks if [PEP 613] explicit type aliases contain references to @@ -279,7 +281,7 @@ pub(crate) fn quoted_type_alias( // explicit type aliases require some additional checks to avoid false positives if checker.semantic().in_annotated_type_alias_value() - && quotes_are_unremovable(checker.semantic(), expr) + && quotes_are_unremovable(checker.semantic(), expr, checker.settings) { return; } @@ -300,16 +302,35 @@ pub(crate) fn quoted_type_alias( /// Traverses the type expression and checks if the expression can safely /// be unquoted -fn quotes_are_unremovable(semantic: &SemanticModel, expr: &Expr) -> bool { +fn quotes_are_unremovable( + semantic: &SemanticModel, + expr: &Expr, + settings: &LinterSettings, +) -> bool { match expr { - Expr::BinOp(ast::ExprBinOp { left, right, .. }) => { - quotes_are_unremovable(semantic, left) || quotes_are_unremovable(semantic, right) + Expr::BinOp(ast::ExprBinOp { + left, right, op, .. + }) => { + match op { + Operator::BitOr => { + if settings.target_version < PythonVersion::Py310 { + return true; + } + quotes_are_unremovable(semantic, left, settings) + || quotes_are_unremovable(semantic, right, settings) + } + // for now we'll treat uses of other operators as unremovable quotes + // since that would make it an invalid type expression anyways. We skip + // walking the nested non-type expressions from `typing.Annotated`, so + // we don't produce false negatives in this branch. + _ => true, + } } Expr::Starred(ast::ExprStarred { value, ctx: ExprContext::Load, .. - }) => quotes_are_unremovable(semantic, value), + }) => quotes_are_unremovable(semantic, value, settings), Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { // for subscripts we don't know whether it's safe to do at runtime // since the operation may only be available at type checking time. @@ -317,18 +338,32 @@ fn quotes_are_unremovable(semantic: &SemanticModel, expr: &Expr) -> bool { if !semantic.in_type_checking_block() { return true; } - quotes_are_unremovable(semantic, value) || quotes_are_unremovable(semantic, slice) + if quotes_are_unremovable(semantic, value, settings) { + return true; + } + // for `typing.Annotated`, only analyze the first argument, since the rest may + // contain arbitrary expressions. + if let Some(qualified_name) = semantic.resolve_qualified_name(value) { + if semantic.match_typing_qualified_name(&qualified_name, "Annotated") { + if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { + return !elts.is_empty() + && quotes_are_unremovable(semantic, &elts[0], settings); + } + } + return false; + } + quotes_are_unremovable(semantic, slice, settings) } Expr::Attribute(ast::ExprAttribute { value, .. }) => { // for attributes we also don't know whether it's safe if !semantic.in_type_checking_block() { return true; } - quotes_are_unremovable(semantic, value) + quotes_are_unremovable(semantic, value, settings) } Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => { for elt in elts { - if quotes_are_unremovable(semantic, elt) { + if quotes_are_unremovable(semantic, elt, settings) { return true; } } diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__pre_py310_quoted-type-alias_TC008_union_syntax_pre_py310.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__pre_py310_quoted-type-alias_TC008_union_syntax_pre_py310.py.snap new file mode 100644 index 0000000000000..d686f260ed693 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__pre_py310_quoted-type-alias_TC008_union_syntax_pre_py310.py.snap @@ -0,0 +1,36 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +TC008_union_syntax_pre_py310.py:10:20: TC008 [*] Remove quotes from type alias + | + 8 | if TYPE_CHECKING: + 9 | c: TypeAlias = 'int | None' # OK +10 | d: TypeAlias = 'Annotated[int, 1 | 2]' # TC008 + | ^^^^^^^^^^^^^^^^^^^^^^^ TC008 +11 | e: TypeAlias = 'Annotated[int, 1 + 2]' # TC008 + | + = help: Remove quotes + +ℹ Safe fix +7 7 | +8 8 | if TYPE_CHECKING: +9 9 | c: TypeAlias = 'int | None' # OK +10 |- d: TypeAlias = 'Annotated[int, 1 | 2]' # TC008 + 10 |+ d: TypeAlias = Annotated[int, 1 | 2] # TC008 +11 11 | e: TypeAlias = 'Annotated[int, 1 + 2]' # TC008 + +TC008_union_syntax_pre_py310.py:11:20: TC008 [*] Remove quotes from type alias + | + 9 | c: TypeAlias = 'int | None' # OK +10 | d: TypeAlias = 'Annotated[int, 1 | 2]' # TC008 +11 | e: TypeAlias = 'Annotated[int, 1 + 2]' # TC008 + | ^^^^^^^^^^^^^^^^^^^^^^^ TC008 + | + = help: Remove quotes + +ℹ Safe fix +8 8 | if TYPE_CHECKING: +9 9 | c: TypeAlias = 'int | None' # OK +10 10 | d: TypeAlias = 'Annotated[int, 1 | 2]' # TC008 +11 |- e: TypeAlias = 'Annotated[int, 1 + 2]' # TC008 + 11 |+ e: TypeAlias = Annotated[int, 1 + 2] # TC008