From c67955ac7c305e177f0aec2c863bdd6939549cf5 Mon Sep 17 00:00:00 2001 From: Daverball Date: Thu, 2 Jan 2025 15:52:30 +0100 Subject: [PATCH 1/5] [`flake8-type-checking`] Add a fix for `runtime-string-union` (`TC010`) --- .../fixtures/flake8_type_checking/TC010.pyi | 16 ++ .../fixtures/flake8_type_checking/TC010_1.py | 1 + .../fixtures/flake8_type_checking/TC010_2.py | 4 + .../fixtures/flake8_type_checking/TC010_3.py | 25 ++ .../src/rules/flake8_type_checking/mod.rs | 2 + .../rules/runtime_string_union.rs | 250 ++++++++++++++++-- ...tests__runtime-string-union_TC010.pyi.snap | 18 ++ ...ests__runtime-string-union_TC010_1.py.snap | 17 +- ...ests__runtime-string-union_TC010_2.py.snap | 110 +++++++- ...ests__runtime-string-union_TC010_3.py.snap | 125 +++++++++ ...g__tests__tc010_precedence_over_tc008.snap | 10 +- crates/ruff_python_semantic/src/model.rs | 14 + 12 files changed, 558 insertions(+), 34 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_3.py create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi new file mode 100644 index 0000000000000..ee4c4e56ea05b --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi @@ -0,0 +1,16 @@ +from typing import TypeVar + + +x: "int" | str # TC010 +x: ("int" | str) | "bool" # TC010 + + +def func(): + x: "int" | str # OK + + +z: list[str, str | "int"] = [] # TC010 + +type A = Value["int" | str] # OK + +OldS = TypeVar('OldS', int | 'str', str) # TC010 diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_1.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_1.py index 268173c56753a..c31ad6541d271 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_1.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_1.py @@ -5,6 +5,7 @@ x: "int" | str # TC010 x: ("int" | str) | "bool" # TC010 +x: b"int" | str # TC010 (unfixable) def func(): diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_2.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_2.py index ee4c4e56ea05b..724fd8ca46e8b 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_2.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_2.py @@ -3,6 +3,7 @@ x: "int" | str # TC010 x: ("int" | str) | "bool" # TC010 +x: b"int" | str # TC010 (unfixable) def func(): @@ -14,3 +15,6 @@ def func(): type A = Value["int" | str] # OK OldS = TypeVar('OldS', int | 'str', str) # TC010 + +x: ("int" # TC010 (unsafe fix) + " | str" | None) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_3.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_3.py new file mode 100644 index 0000000000000..3aaf3df6407b1 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010_3.py @@ -0,0 +1,25 @@ +from typing import TypeVar, TYPE_CHECKING + +import foo + +if TYPE_CHECKING: + from foo import Foo + + +x: "Foo" | str # TC010 +x: ("int" | str) | "Foo" # TC010 +x: b"Foo" | str # TC010 (unfixable) + + +def func(): + x: "Foo" | str # OK + + +z: list[str, str | "list[str]"] = [] # TC010 + +type A = Value["Foo" | str] # OK + +OldS = TypeVar('OldS', int | 'foo.Bar', str) # TC010 + +x: ("Foo" # TC010 (unsafe fix) + | str) 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 b38b03fcf8aa5..1341b2311649c 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -36,8 +36,10 @@ mod tests { #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("TC004_8.py"))] #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("TC004_9.py"))] #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote.py"))] + #[test_case(Rule::RuntimeStringUnion, Path::new("TC010.pyi"))] #[test_case(Rule::RuntimeStringUnion, Path::new("TC010_1.py"))] #[test_case(Rule::RuntimeStringUnion, Path::new("TC010_2.py"))] + #[test_case(Rule::RuntimeStringUnion, Path::new("TC010_3.py"))] #[test_case(Rule::TypingOnlyFirstPartyImport, Path::new("TC001.py"))] #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("TC003.py"))] #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("init_var.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs index ab849880897ca..946af7533a415 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs @@ -1,10 +1,16 @@ -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast as ast; -use ruff_python_ast::{Expr, Operator}; +use ruff_python_ast::{Expr, ExprContext, Operator}; +use ruff_python_parser::typing::parse_type_annotation; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +use crate::rules::flake8_type_checking::helpers::quote_annotation; +use crate::settings::types::PythonVersion; +use crate::settings::LinterSettings; +use crate::Locator; /// ## What it does /// Checks for the presence of string literals in `X | Y`-style union types. @@ -36,19 +42,41 @@ use crate::checkers::ast::Checker; /// var: "str | int" /// ``` /// +/// ## Fix safety +/// This fix is safe as long as the fix doesn't remove a comment, which can happen +/// when the union spans multiple lines. +/// /// ## References /// - [PEP 563 - Postponed Evaluation of Annotations](https://peps.python.org/pep-0563/) /// - [PEP 604 – Allow writing union types as `X | Y`](https://peps.python.org/pep-0604/) /// /// [PEP 604]: https://peps.python.org/pep-0604/ #[derive(ViolationMetadata)] -pub(crate) struct RuntimeStringUnion; +pub(crate) struct RuntimeStringUnion { + strategy: Option, +} impl Violation for RuntimeStringUnion { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { "Invalid string member in `X | Y`-style union type".to_string() } + + fn fix_title(&self) -> Option { + let Self { + strategy: Some(strategy), + .. + } = self + else { + return None; + }; + match strategy { + Strategy::RemoveQuotes => Some("Remove quotes".to_string()), + Strategy::ExtendQuotes => Some("Extend quotes".to_string()), + } + } } /// TC010 @@ -62,24 +90,131 @@ pub(crate) fn runtime_string_union(checker: &mut Checker, expr: &Expr) { } // Search for strings within the binary operator. - let mut strings = Vec::new(); - traverse_op(expr, &mut strings); + let mut string_results = Vec::new(); + let quotes_are_extendable = traverse_op( + checker.semantic(), + checker.locator(), + expr, + &mut string_results, + checker.settings, + ); + + if string_results.is_empty() { + return; + } + + if !checker.source_type.is_stub() + && quotes_are_extendable + && string_results + .iter() + .any(|result| !result.quotes_are_removable) + { + // all union members will share a single fix which extend the quotes + // to the smallest valid type expression + let edit = quote_annotation( + checker + .semantic() + .current_expression_id() + .expect("No current expression"), + checker.semantic(), + checker.stylist(), + checker.locator(), + ); + let parent = expr.range().start(); + let fix = if checker + .comment_ranges() + .has_comments(expr, checker.source()) + { + Fix::unsafe_edit(edit) + } else { + Fix::safe_edit(edit) + }; + + for result in string_results { + let mut diagnostic = Diagnostic::new( + RuntimeStringUnion { + strategy: Some(Strategy::ExtendQuotes), + }, + result.string.range(), + ); + diagnostic.set_parent(parent); + diagnostic.set_fix(fix.clone()); + checker.diagnostics.push(diagnostic); + } + return; + } - for string in strings { - checker - .diagnostics - .push(Diagnostic::new(RuntimeStringUnion, string.range())); + // all union members will have their own fix which removes the quotes + for result in string_results { + let strategy = if result.quotes_are_removable { + Some(Strategy::RemoveQuotes) + } else { + None + }; + let mut diagnostic = + Diagnostic::new(RuntimeStringUnion { strategy }, result.string.range()); + // we can only fix string literals, not bytes literals + if result.quotes_are_removable { + let string = result + .string + .as_string_literal_expr() + .expect("Expected string literal"); + let edit = Edit::range_replacement(string.value.to_string(), string.range()); + if checker + .comment_ranges() + .has_comments(string, checker.source()) + { + diagnostic.set_fix(Fix::unsafe_edit(edit)); + } else { + diagnostic.set_fix(Fix::safe_edit(edit)); + } + } + checker.diagnostics.push(diagnostic); } } +struct StringResult<'a> { + pub string: &'a Expr, + pub quotes_are_removable: bool, +} + /// Collect all string members in possibly-nested binary `|` expressions. -fn traverse_op<'a>(expr: &'a Expr, strings: &mut Vec<&'a Expr>) { +/// Returns whether or not the quotes can be expanded to the entire union +fn traverse_op<'a>( + semantic: &'_ SemanticModel, + locator: &'_ Locator, + expr: &'a Expr, + strings: &mut Vec>, + settings: &'_ LinterSettings, +) -> bool { match expr { - Expr::StringLiteral(_) => { - strings.push(expr); + Expr::StringLiteral(literal) => { + if let Ok(result) = parse_type_annotation(literal, locator.contents()) { + strings.push(StringResult { + string: expr, + quotes_are_removable: quotes_are_removable( + semantic, + result.expression(), + settings, + ), + }); + // the only time quotes can be extended is if all quoted expression + // can be parsed as forward references + true + } else { + strings.push(StringResult { + string: expr, + quotes_are_removable: false, + }); + false + } } Expr::BytesLiteral(_) => { - strings.push(expr); + strings.push(StringResult { + string: expr, + quotes_are_removable: false, + }); + false } Expr::BinOp(ast::ExprBinOp { left, @@ -87,9 +222,92 @@ fn traverse_op<'a>(expr: &'a Expr, strings: &mut Vec<&'a Expr>) { op: Operator::BitOr, .. }) => { - traverse_op(left, strings); - traverse_op(right, strings); + // we don't want short-circuiting here, since we need to collect + // string results from both branches + traverse_op(semantic, locator, left, strings, settings) + & traverse_op(semantic, locator, right, strings, settings) + } + _ => true, + } +} + +/// Traverses the type expression and checks if the expression can safely +/// be unquoted +fn quotes_are_removable(semantic: &SemanticModel, expr: &Expr, settings: &LinterSettings) -> bool { + match expr { + Expr::BinOp(ast::ExprBinOp { + left, right, op, .. + }) => { + match op { + Operator::BitOr => { + if !semantic.in_stub_file() && settings.target_version < PythonVersion::Py310 { + return false; + } + quotes_are_removable(semantic, left, settings) + && quotes_are_removable(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. + _ => false, + } + } + Expr::Starred(ast::ExprStarred { + value, + ctx: ExprContext::Load, + .. + }) => quotes_are_removable(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. + // E.g. stubs only generics. + if !semantic.in_stub_file() { + return false; + } + if !quotes_are_removable(semantic, value, settings) { + return false; + } + // 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_removable(semantic, &elts[0], settings); + } + } + return true; + } + quotes_are_removable(semantic, slice, settings) + } + Expr::Attribute(ast::ExprAttribute { value, .. }) => { + // for attributes we also don't know whether it's safe + if !semantic.in_stub_file() { + return false; + } + quotes_are_removable(semantic, value, settings) } - _ => {} + Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => { + for elt in elts { + if !quotes_are_removable(semantic, elt, settings) { + return false; + } + } + true + } + Expr::Name(name) => { + semantic.lookup_symbol(name.id.as_str()).is_none() + || semantic.simulate_runtime_load(name).is_some() + } + _ => true, } } + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Strategy { + /// The quotes should be removed. + RemoveQuotes, + /// The quotes should be extended to cover the entire union. + ExtendQuotes, +} diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap new file mode 100644 index 0000000000000..9f2db89714c92 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap @@ -0,0 +1,18 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +TC010.pyi:16:30: TC010 [*] Invalid string member in `X | Y`-style union type + | +14 | type A = Value["int" | str] # OK +15 | +16 | OldS = TypeVar('OldS', int | 'str', str) # TC010 + | ^^^^^ TC010 + | + = help: Remove quotes + +ℹ Safe fix +13 13 | +14 14 | type A = Value["int" | str] # OK +15 15 | +16 |-OldS = TypeVar('OldS', int | 'str', str) # TC010 + 16 |+OldS = TypeVar('OldS', int | str, str) # TC010 diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_1.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_1.py.snap index c4db736188e05..76b8d04f9348b 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_1.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_1.py.snap @@ -1,11 +1,18 @@ --- source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs -snapshot_kind: text --- -TC010_1.py:18:30: TC010 Invalid string member in `X | Y`-style union type +TC010_1.py:19:30: TC010 [*] Invalid string member in `X | Y`-style union type | -16 | type A = Value["int" | str] # OK -17 | -18 | OldS = TypeVar('OldS', int | 'str', str) # TC010 +17 | type A = Value["int" | str] # OK +18 | +19 | OldS = TypeVar('OldS', int | 'str', str) # TC010 | ^^^^^ TC010 | + = help: Remove quotes + +ℹ Safe fix +16 16 | +17 17 | type A = Value["int" | str] # OK +18 18 | +19 |-OldS = TypeVar('OldS', int | 'str', str) # TC010 + 19 |+OldS = TypeVar('OldS', int | str, str) # TC010 diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap index 2dc80f40e266f..35ecd9fdc9b54 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap @@ -1,40 +1,126 @@ --- source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs -snapshot_kind: text --- -TC010_2.py:4:4: TC010 Invalid string member in `X | Y`-style union type +TC010_2.py:4:4: TC010 [*] Invalid string member in `X | Y`-style union type | 4 | x: "int" | str # TC010 | ^^^^^ TC010 5 | x: ("int" | str) | "bool" # TC010 +6 | x: b"int" | str # TC010 (unfixable) | + = help: Remove quotes -TC010_2.py:5:5: TC010 Invalid string member in `X | Y`-style union type +ℹ Safe fix +1 1 | from typing import TypeVar +2 2 | +3 3 | +4 |-x: "int" | str # TC010 + 4 |+x: int | str # TC010 +5 5 | x: ("int" | str) | "bool" # TC010 +6 6 | x: b"int" | str # TC010 (unfixable) +7 7 | + +TC010_2.py:5:5: TC010 [*] Invalid string member in `X | Y`-style union type | 4 | x: "int" | str # TC010 5 | x: ("int" | str) | "bool" # TC010 | ^^^^^ TC010 +6 | x: b"int" | str # TC010 (unfixable) | + = help: Remove quotes + +ℹ Safe fix +2 2 | +3 3 | +4 4 | x: "int" | str # TC010 +5 |-x: ("int" | str) | "bool" # TC010 + 5 |+x: (int | str) | "bool" # TC010 +6 6 | x: b"int" | str # TC010 (unfixable) +7 7 | +8 8 | -TC010_2.py:5:20: TC010 Invalid string member in `X | Y`-style union type +TC010_2.py:5:20: TC010 [*] Invalid string member in `X | Y`-style union type | 4 | x: "int" | str # TC010 5 | x: ("int" | str) | "bool" # TC010 | ^^^^^^ TC010 +6 | x: b"int" | str # TC010 (unfixable) + | + = help: Remove quotes + +ℹ Unsafe fix +2 2 | +3 3 | +4 4 | x: "int" | str # TC010 +5 |-x: ("int" | str) | "bool" # TC010 + 5 |+x: ("int" | str) | bool # TC010 +6 6 | x: b"int" | str # TC010 (unfixable) +7 7 | +8 8 | + +TC010_2.py:6:4: TC010 Invalid string member in `X | Y`-style union type + | +4 | x: "int" | str # TC010 +5 | x: ("int" | str) | "bool" # TC010 +6 | x: b"int" | str # TC010 (unfixable) + | ^^^^^^ TC010 | -TC010_2.py:12:20: TC010 Invalid string member in `X | Y`-style union type +TC010_2.py:13:20: TC010 [*] Invalid string member in `X | Y`-style union type | -12 | z: list[str, str | "int"] = [] # TC010 +13 | z: list[str, str | "int"] = [] # TC010 | ^^^^^ TC010 -13 | -14 | type A = Value["int" | str] # OK +14 | +15 | type A = Value["int" | str] # OK | + = help: Remove quotes + +ℹ Safe fix +10 10 | x: "int" | str # OK +11 11 | +12 12 | +13 |-z: list[str, str | "int"] = [] # TC010 + 13 |+z: list[str, str | int] = [] # TC010 +14 14 | +15 15 | type A = Value["int" | str] # OK +16 16 | -TC010_2.py:16:30: TC010 Invalid string member in `X | Y`-style union type +TC010_2.py:17:30: TC010 [*] Invalid string member in `X | Y`-style union type | -14 | type A = Value["int" | str] # OK -15 | -16 | OldS = TypeVar('OldS', int | 'str', str) # TC010 +15 | type A = Value["int" | str] # OK +16 | +17 | OldS = TypeVar('OldS', int | 'str', str) # TC010 | ^^^^^ TC010 +18 | +19 | x: ("int" # TC010 (unsafe fix) | + = help: Remove quotes + +ℹ Safe fix +14 14 | +15 15 | type A = Value["int" | str] # OK +16 16 | +17 |-OldS = TypeVar('OldS', int | 'str', str) # TC010 + 17 |+OldS = TypeVar('OldS', int | str, str) # TC010 +18 18 | +19 19 | x: ("int" # TC010 (unsafe fix) +20 20 | " | str" | None) + +TC010_2.py:19:5: TC010 [*] Invalid string member in `X | Y`-style union type + | +17 | OldS = TypeVar('OldS', int | 'str', str) # TC010 +18 | +19 | x: ("int" # TC010 (unsafe fix) + | _____^ +20 | | " | str" | None) + | |____________^ TC010 + | + = help: Remove quotes + +ℹ Unsafe fix +16 16 | +17 17 | OldS = TypeVar('OldS', int | 'str', str) # TC010 +18 18 | +19 |-x: ("int" # TC010 (unsafe fix) +20 |- " | str" | None) + 19 |+x: (int | str | None) diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap new file mode 100644 index 0000000000000..85bf1fea166e6 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap @@ -0,0 +1,125 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +TC010_3.py:9:4: TC010 [*] Invalid string member in `X | Y`-style union type + | + 9 | x: "Foo" | str # TC010 + | ^^^^^ TC010 +10 | x: ("int" | str) | "Foo" # TC010 +11 | x: b"Foo" | str # TC010 (unfixable) + | + = help: Extend quotes + +ℹ Unsafe fix +6 6 | from foo import Foo +7 7 | +8 8 | +9 |-x: "Foo" | str # TC010 + 9 |+x: "Foo | str" # TC010 +10 10 | x: ("int" | str) | "Foo" # TC010 +11 11 | x: b"Foo" | str # TC010 (unfixable) +12 12 | + +TC010_3.py:10:5: TC010 [*] Invalid string member in `X | Y`-style union type + | + 9 | x: "Foo" | str # TC010 +10 | x: ("int" | str) | "Foo" # TC010 + | ^^^^^ TC010 +11 | x: b"Foo" | str # TC010 (unfixable) + | + = help: Extend quotes + +ℹ Unsafe fix +7 7 | +8 8 | +9 9 | x: "Foo" | str # TC010 +10 |-x: ("int" | str) | "Foo" # TC010 + 10 |+x: "int | str | Foo" # TC010 +11 11 | x: b"Foo" | str # TC010 (unfixable) +12 12 | +13 13 | + +TC010_3.py:10:20: TC010 [*] Invalid string member in `X | Y`-style union type + | + 9 | x: "Foo" | str # TC010 +10 | x: ("int" | str) | "Foo" # TC010 + | ^^^^^ TC010 +11 | x: b"Foo" | str # TC010 (unfixable) + | + = help: Extend quotes + +ℹ Unsafe fix +7 7 | +8 8 | +9 9 | x: "Foo" | str # TC010 +10 |-x: ("int" | str) | "Foo" # TC010 + 10 |+x: "int | str | Foo" # TC010 +11 11 | x: b"Foo" | str # TC010 (unfixable) +12 12 | +13 13 | + +TC010_3.py:11:4: TC010 Invalid string member in `X | Y`-style union type + | + 9 | x: "Foo" | str # TC010 +10 | x: ("int" | str) | "Foo" # TC010 +11 | x: b"Foo" | str # TC010 (unfixable) + | ^^^^^^ TC010 + | + +TC010_3.py:18:20: TC010 [*] Invalid string member in `X | Y`-style union type + | +18 | z: list[str, str | "list[str]"] = [] # TC010 + | ^^^^^^^^^^^ TC010 +19 | +20 | type A = Value["Foo" | str] # OK + | + = help: Extend quotes + +ℹ Safe fix +15 15 | x: "Foo" | str # OK +16 16 | +17 17 | +18 |-z: list[str, str | "list[str]"] = [] # TC010 + 18 |+z: list[str, "str | list[str]"] = [] # TC010 +19 19 | +20 20 | type A = Value["Foo" | str] # OK +21 21 | + +TC010_3.py:22:30: TC010 [*] Invalid string member in `X | Y`-style union type + | +20 | type A = Value["Foo" | str] # OK +21 | +22 | OldS = TypeVar('OldS', int | 'foo.Bar', str) # TC010 + | ^^^^^^^^^ TC010 +23 | +24 | x: ("Foo" # TC010 (unsafe fix) + | + = help: Extend quotes + +ℹ Safe fix +19 19 | +20 20 | type A = Value["Foo" | str] # OK +21 21 | +22 |-OldS = TypeVar('OldS', int | 'foo.Bar', str) # TC010 + 22 |+OldS = TypeVar('OldS', "int | foo.Bar", str) # TC010 +23 23 | +24 24 | x: ("Foo" # TC010 (unsafe fix) +25 25 | | str) + +TC010_3.py:24:5: TC010 [*] Invalid string member in `X | Y`-style union type + | +22 | OldS = TypeVar('OldS', int | 'foo.Bar', str) # TC010 +23 | +24 | x: ("Foo" # TC010 (unsafe fix) + | ^^^^^ TC010 +25 | | str) + | + = help: Extend quotes + +ℹ Unsafe fix +21 21 | +22 22 | OldS = TypeVar('OldS', int | 'foo.Bar', str) # TC010 +23 23 | +24 |-x: ("Foo" # TC010 (unsafe fix) +25 |- | str) + 24 |+x: ("Foo | str") diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__tc010_precedence_over_tc008.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__tc010_precedence_over_tc008.snap index 2d30b115db7e0..8a740dc0b6056 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__tc010_precedence_over_tc008.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__tc010_precedence_over_tc008.snap @@ -19,9 +19,17 @@ source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs 6 |+a: TypeAlias = int | None # TC008 7 7 | b: TypeAlias = 'int' | None # TC010 -:7:16: TC010 Invalid string member in `X | Y`-style union type +:7:16: TC010 [*] Invalid string member in `X | Y`-style union type | 6 | a: TypeAlias = 'int | None' # TC008 7 | b: TypeAlias = 'int' | None # TC010 | ^^^^^ TC010 | + = help: Remove quotes + +ℹ Safe fix +4 4 | from typing import TypeAlias +5 5 | +6 6 | a: TypeAlias = 'int | None' # TC008 +7 |-b: TypeAlias = 'int' | None # TC010 + 7 |+b: TypeAlias = int | None # TC010 diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index f43cf24de1ec5..eeabbd0fbb5a9 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1274,6 +1274,20 @@ impl<'a> SemanticModel<'a> { self.current_expressions().nth(2) } + /// Returns an [`Iterator`] over the current expression hierarchy represented as [`NodeId`], + /// from the current [`NodeId`] through to any parents. + pub fn current_expression_ids(&self) -> impl Iterator + '_ { + self.node_id + .iter() + .flat_map(|id| self.nodes.ancestor_ids(*id)) + .filter(|id| self.nodes[*id].is_expression()) + } + + /// Return the [`NodeId`] of the current [`Expr`], if any. + pub fn current_expression_id(&self) -> Option { + self.current_expression_ids().next() + } + /// Returns an [`Iterator`] over the current statement hierarchy represented as [`NodeId`], /// from the current [`NodeId`] through to any parents. pub fn current_statement_ids(&self) -> impl Iterator + '_ { From 29508e8a25685872e4b1bda3128da87320646c1c Mon Sep 17 00:00:00 2001 From: Daverball Date: Fri, 3 Jan 2025 15:29:24 +0100 Subject: [PATCH 2/5] Disable TC010 in stubs and clean up unnecessary code --- .../fixtures/flake8_type_checking/TC010.pyi | 16 ------- .../src/rules/flake8_type_checking/mod.rs | 1 - .../rules/runtime_string_union.rs | 45 +++++-------------- ...tests__runtime-string-union_TC010.pyi.snap | 18 -------- 4 files changed, 10 insertions(+), 70 deletions(-) delete mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi delete mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi deleted file mode 100644 index ee4c4e56ea05b..0000000000000 --- a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC010.pyi +++ /dev/null @@ -1,16 +0,0 @@ -from typing import TypeVar - - -x: "int" | str # TC010 -x: ("int" | str) | "bool" # TC010 - - -def func(): - x: "int" | str # OK - - -z: list[str, str | "int"] = [] # TC010 - -type A = Value["int" | str] # OK - -OldS = TypeVar('OldS', int | 'str', str) # TC010 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 1341b2311649c..2cedda7251cd0 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -36,7 +36,6 @@ mod tests { #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("TC004_8.py"))] #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("TC004_9.py"))] #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote.py"))] - #[test_case(Rule::RuntimeStringUnion, Path::new("TC010.pyi"))] #[test_case(Rule::RuntimeStringUnion, Path::new("TC010_1.py"))] #[test_case(Rule::RuntimeStringUnion, Path::new("TC010_2.py"))] #[test_case(Rule::RuntimeStringUnion, Path::new("TC010_3.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs index 946af7533a415..59cfbf6a8fdc8 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs @@ -85,7 +85,9 @@ pub(crate) fn runtime_string_union(checker: &mut Checker, expr: &Expr) { return; } - if !checker.semantic().execution_context().is_runtime() { + // The union is only problematic at runtime. Even though stub files are never + // executed, some of the nodes still end up having a runtime execution context + if checker.source_type.is_stub() || !checker.semantic().execution_context().is_runtime() { return; } @@ -103,8 +105,7 @@ pub(crate) fn runtime_string_union(checker: &mut Checker, expr: &Expr) { return; } - if !checker.source_type.is_stub() - && quotes_are_extendable + if quotes_are_extendable && string_results .iter() .any(|result| !result.quotes_are_removable) @@ -240,7 +241,7 @@ fn quotes_are_removable(semantic: &SemanticModel, expr: &Expr, settings: &Linter }) => { match op { Operator::BitOr => { - if !semantic.in_stub_file() && settings.target_version < PythonVersion::Py310 { + if settings.target_version < PythonVersion::Py310 { return false; } quotes_are_removable(semantic, left, settings) @@ -248,8 +249,7 @@ fn quotes_are_removable(semantic: &SemanticModel, expr: &Expr, settings: &Linter } // 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. + // walking subscript _ => false, } } @@ -258,35 +258,10 @@ fn quotes_are_removable(semantic: &SemanticModel, expr: &Expr, settings: &Linter ctx: ExprContext::Load, .. }) => quotes_are_removable(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. - // E.g. stubs only generics. - if !semantic.in_stub_file() { - return false; - } - if !quotes_are_removable(semantic, value, settings) { - return false; - } - // 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_removable(semantic, &elts[0], settings); - } - } - return true; - } - quotes_are_removable(semantic, slice, settings) - } - Expr::Attribute(ast::ExprAttribute { value, .. }) => { - // for attributes we also don't know whether it's safe - if !semantic.in_stub_file() { - return false; - } - quotes_are_removable(semantic, value, settings) + Expr::Subscript(_) | Expr::Attribute(_) => { + // Subscript or attribute accesses that are valid type expressions + // may fail at runtime, so we have to assume that they do. + return false; } Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => { for elt in elts { diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap deleted file mode 100644 index 9f2db89714c92..0000000000000 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010.pyi.snap +++ /dev/null @@ -1,18 +0,0 @@ ---- -source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs ---- -TC010.pyi:16:30: TC010 [*] Invalid string member in `X | Y`-style union type - | -14 | type A = Value["int" | str] # OK -15 | -16 | OldS = TypeVar('OldS', int | 'str', str) # TC010 - | ^^^^^ TC010 - | - = help: Remove quotes - -ℹ Safe fix -13 13 | -14 14 | type A = Value["int" | str] # OK -15 15 | -16 |-OldS = TypeVar('OldS', int | 'str', str) # TC010 - 16 |+OldS = TypeVar('OldS', int | str, str) # TC010 From d60d9239b16c88ea3ab382bb41abb7106d650fc8 Mon Sep 17 00:00:00 2001 From: Daverball Date: Fri, 3 Jan 2025 15:33:51 +0100 Subject: [PATCH 3/5] Remove unnecessary return --- .../flake8_type_checking/rules/runtime_string_union.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs index 59cfbf6a8fdc8..c7a67b9a98776 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs @@ -258,11 +258,9 @@ fn quotes_are_removable(semantic: &SemanticModel, expr: &Expr, settings: &Linter ctx: ExprContext::Load, .. }) => quotes_are_removable(semantic, value, settings), - Expr::Subscript(_) | Expr::Attribute(_) => { - // Subscript or attribute accesses that are valid type expressions - // may fail at runtime, so we have to assume that they do. - return false; - } + // Subscript or attribute accesses that are valid type expressions may fail + // at runtime, so we have to assume that they do, to keep code working. + Expr::Subscript(_) | Expr::Attribute(_) => false, Expr::List(ast::ExprList { elts, .. }) | Expr::Tuple(ast::ExprTuple { elts, .. }) => { for elt in elts { if !quotes_are_removable(semantic, elt, settings) { From e8581b6881c3fed7d954e5933c5fc710dbcd834f Mon Sep 17 00:00:00 2001 From: David Salvisberg Date: Wed, 8 Jan 2025 14:38:50 +0100 Subject: [PATCH 4/5] Fixes `simulate_runtime_load` call. --- .../flake8_type_checking/rules/runtime_string_union.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs index c7a67b9a98776..cf4942b021279 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs @@ -3,7 +3,7 @@ use ruff_macros::{derive_message_formats, ViolationMetadata}; use ruff_python_ast as ast; use ruff_python_ast::{Expr, ExprContext, Operator}; use ruff_python_parser::typing::parse_type_annotation; -use ruff_python_semantic::SemanticModel; +use ruff_python_semantic::{SemanticModel, TypingOnlyBindingsStatus}; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -271,7 +271,9 @@ fn quotes_are_removable(semantic: &SemanticModel, expr: &Expr, settings: &Linter } Expr::Name(name) => { semantic.lookup_symbol(name.id.as_str()).is_none() - || semantic.simulate_runtime_load(name).is_some() + || semantic + .simulate_runtime_load(name, TypingOnlyBindingsStatus::Disallowed) + .is_some() } _ => true, } From 3eb38f17439e5320523bd1feb1d075887652b6d5 Mon Sep 17 00:00:00 2001 From: David Salvisberg Date: Tue, 21 Jan 2025 13:42:11 +0100 Subject: [PATCH 5/5] Fixes false positives in comment intersection check for fix safety --- .../flake8_type_checking/rules/runtime_string_union.rs | 10 ++-------- ...ecking__tests__runtime-string-union_TC010_2.py.snap | 2 +- ...ecking__tests__runtime-string-union_TC010_3.py.snap | 6 +++--- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs index cf4942b021279..125c2c3085156 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_string_union.rs @@ -122,10 +122,7 @@ pub(crate) fn runtime_string_union(checker: &mut Checker, expr: &Expr) { checker.locator(), ); let parent = expr.range().start(); - let fix = if checker - .comment_ranges() - .has_comments(expr, checker.source()) - { + let fix = if checker.comment_ranges().intersects(expr.range()) { Fix::unsafe_edit(edit) } else { Fix::safe_edit(edit) @@ -161,10 +158,7 @@ pub(crate) fn runtime_string_union(checker: &mut Checker, expr: &Expr) { .as_string_literal_expr() .expect("Expected string literal"); let edit = Edit::range_replacement(string.value.to_string(), string.range()); - if checker - .comment_ranges() - .has_comments(string, checker.source()) - { + if checker.comment_ranges().intersects(string.range()) { diagnostic.set_fix(Fix::unsafe_edit(edit)); } else { diagnostic.set_fix(Fix::safe_edit(edit)); diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap index 1c9de36c0a122..8c32bcf5494e9 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_2.py.snap @@ -48,7 +48,7 @@ TC010_2.py:5:20: TC010 [*] Invalid string member in `X | Y`-style union type | = help: Remove quotes -ℹ Unsafe fix +ℹ Safe fix 2 2 | 3 3 | 4 4 | x: "int" | str # TC010 diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap index 8540e5cec713f..0aaf23c56683a 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-string-union_TC010_3.py.snap @@ -10,7 +10,7 @@ TC010_3.py:9:4: TC010 [*] Invalid string member in `X | Y`-style union type | = help: Extend quotes -ℹ Unsafe fix +ℹ Safe fix 6 6 | from foo import Foo 7 7 | 8 8 | @@ -29,7 +29,7 @@ TC010_3.py:10:5: TC010 [*] Invalid string member in `X | Y`-style union type | = help: Extend quotes -ℹ Unsafe fix +ℹ Safe fix 7 7 | 8 8 | 9 9 | x: "Foo" | str # TC010 @@ -48,7 +48,7 @@ TC010_3.py:10:20: TC010 [*] Invalid string member in `X | Y`-style union type | = help: Extend quotes -ℹ Unsafe fix +ℹ Safe fix 7 7 | 8 8 | 9 9 | x: "Foo" | str # TC010