Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[`flake8-type-checking`] Fixes `quote_type_expression` (#14634)

This commit is contained in:
David Salvisberg 2024-11-27 18:58:48 +01:00 committed by GitHub
parent 6fcbe8efb4
commit 8a7ba5d2df
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
9 changed files with 216 additions and 157 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
crates/ruff_linter/resources/test/fixtures/flake8_type_checking/TC006.py vendored
View File

@ -60,3 +60,23 @@ def f():
| None,
3.0
)
def f():
# Regression test for #14554
import typing
typing.cast(M-())
def f():
# Simple case with Literal that should lead to nested quotes
from typing import cast, Literal
cast(Literal["A"], 'A')
def f():
# Really complex case with nested forward references
from typing import cast, Annotated, Literal
cast(list[Annotated["list['Literal[\"A\"]']", "Foo"]], ['A'])

2
crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote3.py vendored
View File

@ -58,7 +58,7 @@ def f():
from typing import Literal
from third_party import Type
def test_string_contains_opposite_quote_do_not_fix(self, type1: Type[Literal["'"]], type2: Type[Literal["\'"]]):
def test_string_contains_opposite_quote(self, type1: Type[Literal["'"]], type2: Type[Literal["\'"]]):
pass

190
crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs
View File

@ -1,19 +1,19 @@
use std::cmp::Reverse;
use anyhow::Result;
use ruff_diagnostics::Edit;
use ruff_python_ast::helpers::{map_callable, map_subscript};
use ruff_python_ast::name::QualifiedName;
use ruff_python_ast::visitor::source_order::{SourceOrderVisitor, TraversalSignal};
use ruff_python_ast::visitor::transformer::{walk_expr, Transformer};
use ruff_python_ast::{self as ast, Decorator, Expr};
use ruff_python_codegen::{Generator, Stylist};
use ruff_python_parser::typing::parse_type_annotation;
use ruff_python_semantic::{
analyze, Binding, BindingKind, Modules, NodeId, ResolvedReference, ScopeKind, SemanticModel,
};
use ruff_text_size::Ranged;
use ruff_text_size::{Ranged, TextRange};
use crate::rules::flake8_type_checking::settings::Settings;
use crate::Locator;
/// Returns `true` if the [`ResolvedReference`] is in a typing-only context _or_ a runtime-evaluated
/// context (with quoting enabled).
@ -229,7 +229,8 @@ pub(crate) fn quote_annotation(
node_id: NodeId,
semantic: &SemanticModel,
stylist: &Stylist,
) -> Result<Edit> {
locator: &Locator,
) -> Edit {
let expr = semantic.expression(node_id).expect("Expression not found");
if let Some(parent_id) = semantic.parent_expression_id(node_id) {
match semantic.expression(parent_id) {
@ -238,7 +239,7 @@ pub(crate) fn quote_annotation(
// If we're quoting the value of a subscript, we need to quote the entire
// expression. For example, when quoting `DataFrame` in `DataFrame[int]`, we
// should generate `"DataFrame[int]"`.
return quote_annotation(parent_id, semantic, stylist);
return quote_annotation(parent_id, semantic, stylist, locator);
}
}
Some(Expr::Attribute(parent)) => {
@ -246,7 +247,7 @@ pub(crate) fn quote_annotation(
// If we're quoting the value of an attribute, we need to quote the entire
// expression. For example, when quoting `DataFrame` in `pd.DataFrame`, we
// should generate `"pd.DataFrame"`.
return quote_annotation(parent_id, semantic, stylist);
return quote_annotation(parent_id, semantic, stylist, locator);
}
}
Some(Expr::Call(parent)) => {
@ -254,7 +255,7 @@ pub(crate) fn quote_annotation(
// If we're quoting the function of a call, we need to quote the entire
// expression. For example, when quoting `DataFrame` in `DataFrame()`, we
// should generate `"DataFrame()"`.
return quote_annotation(parent_id, semantic, stylist);
return quote_annotation(parent_id, semantic, stylist, locator);
}
}
Some(Expr::BinOp(parent)) => {
@ -262,14 +263,14 @@ pub(crate) fn quote_annotation(
// If we're quoting the left or right side of a binary operation, we need to
// quote the entire expression. For example, when quoting `DataFrame` in
// `DataFrame | Series`, we should generate `"DataFrame | Series"`.
return quote_annotation(parent_id, semantic, stylist);
return quote_annotation(parent_id, semantic, stylist, locator);
}
}
_ => {}
}
}
quote_type_expression(expr, semantic, stylist)
quote_type_expression(expr, semantic, stylist, locator)
}
/// Wrap a type expression in quotes.
@ -284,17 +285,12 @@ pub(crate) fn quote_type_expression(
expr: &Expr,
semantic: &SemanticModel,
stylist: &Stylist,
) -> Result<Edit> {
locator: &Locator,
) -> Edit {
// Quote the entire expression.
let quote = stylist.quote();
let mut quote_annotator = QuoteAnnotator::new(semantic, stylist);
quote_annotator.visit_expr(expr);
let annotation = quote_annotator.into_annotation()?;
let quote_annotator = QuoteAnnotator::new(semantic, stylist, locator);
Ok(Edit::range_replacement(
format!("{quote}{annotation}{quote}"),
expr.range(),
))
Edit::range_replacement(quote_annotator.into_annotation(expr), expr.range())
}
/// Filter out any [`Edit`]s that are completely contained by any other [`Edit`].
@ -317,144 +313,90 @@ pub(crate) fn filter_contained(edits: Vec<Edit>) -> Vec<Edit> {
filtered
}
#[derive(Copy, PartialEq, Clone)]
enum QuoteAnnotatorState {
Literal,
AnnotatedFirst,
AnnotatedRest,
Other,
}
pub(crate) struct QuoteAnnotator<'a> {
stylist: &'a Stylist<'a>,
semantic: &'a SemanticModel<'a>,
state: Vec<QuoteAnnotatorState>,
annotation: String,
cannot_fix: bool,
stylist: &'a Stylist<'a>,
locator: &'a Locator<'a>,
}
impl<'a> QuoteAnnotator<'a> {
fn new(semantic: &'a SemanticModel<'a>, stylist: &'a Stylist<'a>) -> Self {
fn new(
semantic: &'a SemanticModel<'a>,
stylist: &'a Stylist<'a>,
locator: &'a Locator<'a>,
) -> Self {
Self {
stylist,
semantic,
state: Vec::new(),
annotation: String::new(),
cannot_fix: false,
stylist,
locator,
}
}
fn into_annotation(self) -> Result<String> {
if self.cannot_fix {
Err(anyhow::anyhow!(
"Cannot quote annotation because it already contains opposite quote or escape character"
))
} else {
Ok(self.annotation)
fn into_annotation(self, expr: &Expr) -> String {
let mut expr_without_forward_references = expr.clone();
self.visit_expr(&mut expr_without_forward_references);
let generator = Generator::from(self.stylist);
// we first generate the annotation with the inverse quote, so we can
// generate the string literal with the preferred quote
let subgenerator = Generator::new(
self.stylist.indentation(),
self.stylist.quote().opposite(),
self.stylist.line_ending(),
);
let annotation = subgenerator.expr(&expr_without_forward_references);
generator.expr(&Expr::from(ast::StringLiteral {
range: TextRange::default(),
value: annotation.into_boxed_str(),
flags: ast::StringLiteralFlags::default(),
}))
}
fn visit_annotated_slice(&self, slice: &mut Expr) {
// we only want to walk the first tuple element if it exists,
// anything else should not be transformed
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice {
if !elts.is_empty() {
self.visit_expr(&mut elts[0]);
}
}
}
}
impl<'a> SourceOrderVisitor<'a> for QuoteAnnotator<'a> {
fn enter_node(&mut self, _node: ast::AnyNodeRef<'a>) -> TraversalSignal {
if self.cannot_fix {
TraversalSignal::Skip
} else {
TraversalSignal::Traverse
}
}
fn visit_expr(&mut self, expr: &'a Expr) {
let generator = Generator::from(self.stylist);
impl Transformer for QuoteAnnotator<'_> {
fn visit_expr(&self, expr: &mut Expr) {
match expr {
Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
let value_expr = generator.expr(value);
self.annotation.push_str(&value_expr);
self.annotation.push('[');
if let Some(qualified_name) = self.semantic.resolve_qualified_name(value) {
if self
.semantic
.match_typing_qualified_name(&qualified_name, "Literal")
{
self.state.push(QuoteAnnotatorState::Literal);
// we don't want to modify anything inside `Literal`
// so skip visiting this subscripts' slice
} else if self
.semantic
.match_typing_qualified_name(&qualified_name, "Annotated")
{
self.state.push(QuoteAnnotatorState::AnnotatedFirst);
self.visit_annotated_slice(slice);
} else {
self.state.push(QuoteAnnotatorState::Other);
self.visit_expr(slice);
}
}
self.visit_expr(slice);
self.state.pop();
self.annotation.push(']');
}
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
let Some((first, remaining)) = elts.split_first() else {
return;
};
self.visit_expr(first);
if let Some(last) = self.state.last_mut() {
if *last == QuoteAnnotatorState::AnnotatedFirst {
*last = QuoteAnnotatorState::AnnotatedRest;
}
}
for expr in remaining {
self.annotation.push_str(", ");
Expr::StringLiteral(literal) => {
// try to parse the forward reference and replace the string
// literal node with the parsed expression, if we fail to
// parse the forward reference, we just keep treating this
// like a regular string literal
if let Ok(annotation) = parse_type_annotation(literal, self.locator.contents()) {
*expr = annotation.expression().clone();
// we need to visit the parsed expression too
// since it may contain forward references itself
self.visit_expr(expr);
}
self.state.pop();
}
// For the following expressions, we just need to make sure to visit the nested
// expressions using the quote annotator and not use the generator. This is so that any
// subscript elements nested within them are identified and quoted correctly.
Expr::List(ast::ExprList { elts, .. }) => {
let mut first = true;
self.annotation.push('[');
for expr in elts {
if first {
first = false;
} else {
self.annotation.push_str(", ");
}
self.visit_expr(expr);
}
self.annotation.push(']');
}
Expr::BinOp(ast::ExprBinOp {
left, op, right, ..
}) => {
debug_assert_eq!(*op, ast::Operator::BitOr);
self.visit_expr(left);
self.annotation.push_str(" | ");
self.visit_expr(right);
}
_ => {
let source = match self.state.last().copied() {
Some(QuoteAnnotatorState::Literal | QuoteAnnotatorState::AnnotatedRest) => {
let mut source = generator.expr(expr);
let opposite_quote = &self.stylist.quote().opposite().as_char().to_string();
// If the quotes we are going to insert in this source already exists set the auto quote outcome
// to failed. Because this means we are inserting quotes that are in the string and they collect.
if source.contains(opposite_quote) || source.contains('\\') {
self.cannot_fix = true;
}
source = source.replace(self.stylist.quote().as_char(), opposite_quote);
source
}
None
| Some(QuoteAnnotatorState::AnnotatedFirst | QuoteAnnotatorState::Other) => {
let mut source = generator.expr(expr);
source = source.replace(self.stylist.quote().as_char(), "");
source = source.replace(self.stylist.quote().opposite().as_char(), "");
source
}
};
self.annotation.push_str(&source);
walk_expr(self, expr);
}
}
}

33
crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_cast_value.rs
View File

@ -1,6 +1,6 @@
use ruff_python_ast::Expr;
use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation};
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Fix};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_text_size::Ranged;
@ -34,16 +34,14 @@ use crate::rules::flake8_type_checking::helpers::quote_type_expression;
#[derive(ViolationMetadata)]
pub(crate) struct RuntimeCastValue;
impl Violation for RuntimeCastValue {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
impl AlwaysFixableViolation for RuntimeCastValue {
#[derive_message_formats]
fn message(&self) -> String {
"Add quotes to type expression in `typing.cast()`".to_string()
}
fn fix_title(&self) -> Option<String> {
Some("Add quotes".to_string())
fn fix_title(&self) -> String {
"Add quotes".to_string()
}
}
@ -54,16 +52,19 @@ pub(crate) fn runtime_cast_value(checker: &mut Checker, type_expr: &Expr) {
}
let mut diagnostic = Diagnostic::new(RuntimeCastValue, type_expr.range());
let edit = quote_type_expression(type_expr, checker.semantic(), checker.stylist()).ok();
if let Some(edit) = edit {
if checker
.comment_ranges()
.has_comments(type_expr, checker.source())
{
diagnostic.set_fix(Fix::unsafe_edit(edit));
} else {
diagnostic.set_fix(Fix::safe_edit(edit));
}
let edit = quote_type_expression(
type_expr,
checker.semantic(),
checker.stylist(),
checker.locator(),
);
if checker
.comment_ranges()
.has_comments(type_expr, checker.source())
{
diagnostic.set_fix(Fix::unsafe_edit(edit));
} else {
diagnostic.set_fix(Fix::safe_edit(edit));
}
checker.diagnostics.push(diagnostic);
}

15
crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs
View File

@ -213,7 +213,7 @@ pub(crate) fn runtime_import_in_type_checking_block(
// Generate a diagnostic for every import, but share a fix across all imports within the same
// statement (excluding those that are ignored).
Action::Quote => {
let fix = quote_imports(checker, node_id, &imports).ok();
let fix = quote_imports(checker, node_id, &imports);
for ImportBinding {
import,
@ -232,9 +232,7 @@ pub(crate) fn runtime_import_in_type_checking_block(
if let Some(range) = parent_range {
diagnostic.set_parent(range.start());
}
if let Some(fix) = fix.as_ref() {
diagnostic.set_fix(fix.clone());
}
diagnostic.set_fix(fix.clone());
diagnostics.push(diagnostic);
}
}
@ -267,7 +265,7 @@ pub(crate) fn runtime_import_in_type_checking_block(
}
/// Generate a [`Fix`] to quote runtime usages for imports in a type-checking block.
fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> Result<Fix> {
fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> Fix {
let quote_reference_edits = filter_contained(
imports
.iter()
@ -279,20 +277,21 @@ fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding])
reference.expression_id()?,
checker.semantic(),
checker.stylist(),
checker.locator(),
))
} else {
None
}
})
})
.collect::<Result<Vec<_>>>()?,
.collect::<Vec<_>>(),
);
let mut rest = quote_reference_edits.into_iter();
let head = rest.next().expect("Expected at least one reference");
Ok(Fix::unsafe_edits(head, rest).isolate(Checker::isolation(
Fix::unsafe_edits(head, rest).isolate(Checker::isolation(
checker.semantic().parent_statement_id(node_id),
)))
))
}
/// Generate a [`Fix`] to remove runtime imports from a type-checking block.

11
crates/ruff_linter/src/rules/flake8_type_checking/rules/type_alias_quotes.rs
View File

@ -153,14 +153,17 @@ pub(crate) fn unquoted_type_alias(checker: &Checker, binding: &Binding) -> Optio
// Eventually we may try to be more clever and come up with the
// minimal set of subexpressions that need to be quoted.
let parent = expr.range().start();
let edit = quote_type_expression(expr, checker.semantic(), checker.stylist());
let edit = quote_type_expression(
expr,
checker.semantic(),
checker.stylist(),
checker.locator(),
);
let mut diagnostics = Vec::with_capacity(names.len());
for name in names {
let mut diagnostic = Diagnostic::new(UnquotedTypeAlias, name.range());
diagnostic.set_parent(parent);
if let Ok(edit) = edit.as_ref() {
diagnostic.set_fix(Fix::unsafe_edit(edit.clone()));
}
diagnostic.set_fix(Fix::unsafe_edit(edit.clone()));
diagnostics.push(diagnostic);
}
Some(diagnostics)

3
crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs
View File

@ -509,13 +509,14 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
reference.expression_id()?,
checker.semantic(),
checker.stylist(),
checker.locator(),
))
} else {
None
}
})
})
.collect::<Result<Vec<_>>>()?,
.collect::<Vec<_>>(),
);
Ok(Fix::unsafe_edits(

44
crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote3.py.snap
View File

@ -152,18 +152,38 @@ quote3.py:40:37: TC002 [*] Move third-party import `django.contrib.auth.models`
44 47 |
45 48 |
quote3.py:59:29: TC002 Move third-party import `third_party.Type` into a type-checking block
quote3.py:59:29: TC002 [*] Move third-party import `third_party.Type` into a type-checking block
|
57 | def f():
58 | from typing import Literal
59 | from third_party import Type
| ^^^^ TC002
60 |
61 | def test_string_contains_opposite_quote_do_not_fix(self, type1: Type[Literal["'"]], type2: Type[Literal["\'"]]):
61 | def test_string_contains_opposite_quote(self, type1: Type[Literal["'"]], type2: Type[Literal["\'"]]):
|
= help: Move into type-checking block
quote3.py:67:29: TC002 Move third-party import `third_party.Type` into a type-checking block
ℹ Unsafe fix
1 |+from typing import TYPE_CHECKING
2 |+
3 |+if TYPE_CHECKING:
4 |+ from third_party import Type
1 5 | def f():
2 6 | from typing import Literal, Union
3 7 |
--------------------------------------------------------------------------------
56 60 |
57 61 | def f():
58 62 | from typing import Literal
59 |- from third_party import Type
60 63 |
61 |- def test_string_contains_opposite_quote(self, type1: Type[Literal["'"]], type2: Type[Literal["\'"]]):
64 |+ def test_string_contains_opposite_quote(self, type1: 'Type[Literal["\'"]]', type2: 'Type[Literal["\'"]]'):
62 65 | pass
63 66 |
64 67 |
quote3.py:67:29: TC002 [*] Move third-party import `third_party.Type` into a type-checking block
|
65 | def f():
66 | from typing import Literal
@ -173,3 +193,21 @@ quote3.py:67:29: TC002 Move third-party import `third_party.Type` into a type-ch
69 | def test_quote_contains_backslash(self, type1: Type[Literal["\n"]], type2: Type[Literal["\""]]):
|
= help: Move into type-checking block
ℹ Unsafe fix
1 |+from typing import TYPE_CHECKING
2 |+
3 |+if TYPE_CHECKING:
4 |+ from third_party import Type
1 5 | def f():
2 6 | from typing import Literal, Union
3 7 |
--------------------------------------------------------------------------------
64 68 |
65 69 | def f():
66 70 | from typing import Literal
67 |- from third_party import Type
68 71 |
69 |- def test_quote_contains_backslash(self, type1: Type[Literal["\n"]], type2: Type[Literal["\""]]):
72 |+ def test_quote_contains_backslash(self, type1: 'Type[Literal["\\n"]]', type2: 'Type[Literal[\'"\']]'):
70 73 | pass

55
crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-cast-value_TC006.py.snap
View File

@ -136,3 +136,58 @@ TC006.py:59:9: TC006 [*] Add quotes to type expression in `typing.cast()`
59 |+ "int | None",
61 60 | 3.0
62 61 | )
63 62 |
TC006.py:68:17: TC006 [*] Add quotes to type expression in `typing.cast()`
|
66 | # Regression test for #14554
67 | import typing
68 | typing.cast(M-())
| ^^^^ TC006
|
= help: Add quotes
ℹ Safe fix
65 65 | def f():
66 66 | # Regression test for #14554
67 67 | import typing
68 |- typing.cast(M-())
68 |+ typing.cast("M - ()")
69 69 |
70 70 |
71 71 | def f():
TC006.py:75:10: TC006 [*] Add quotes to type expression in `typing.cast()`
|
73 | from typing import cast, Literal
74 |
75 | cast(Literal["A"], 'A')
| ^^^^^^^^^^^^ TC006
|
= help: Add quotes
ℹ Safe fix
72 72 | # Simple case with Literal that should lead to nested quotes
73 73 | from typing import cast, Literal
74 74 |
75 |- cast(Literal["A"], 'A')
75 |+ cast("Literal['A']", 'A')
76 76 |
77 77 |
78 78 | def f():
TC006.py:82:10: TC006 [*] Add quotes to type expression in `typing.cast()`
|
80 | from typing import cast, Annotated, Literal
81 |
82 | cast(list[Annotated["list['Literal[\"A\"]']", "Foo"]], ['A'])
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ TC006
|
= help: Add quotes
ℹ Safe fix
79 79 | # Really complex case with nested forward references
80 80 | from typing import cast, Annotated, Literal
81 81 |
82 |- cast(list[Annotated["list['Literal[\"A\"]']", "Foo"]], ['A'])
82 |+ cast("list[Annotated[list[Literal['A']], 'Foo']]", ['A'])