From 9b3ac0a8bb96397355be178a92d8e52911080e9f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 9 Dec 2023 22:08:01 -0500 Subject: [PATCH 1/5] Add quote support --- .../fixtures/flake8_type_checking/quote.py | 40 +++++ crates/ruff_linter/src/checkers/ast/mod.rs | 96 +++++++---- .../src/rules/flake8_type_checking/helpers.rs | 25 ++- .../src/rules/flake8_type_checking/mod.rs | 1 + .../rules/typing_only_runtime_import.rs | 122 ++++++++++++-- .../rules/flake8_type_checking/settings.rs | 2 +- ...ping-only-third-party-import_quote.py.snap | 156 ++++++++++++++++++ crates/ruff_python_semantic/src/model.rs | 148 ++++++++++++----- crates/ruff_python_semantic/src/reference.rs | 54 +++++- 9 files changed, 550 insertions(+), 94 deletions(-) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py new file mode 100644 index 0000000000000..26d5f9d341957 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py @@ -0,0 +1,40 @@ +def f(): + from pandas import DataFrame + + def baz() -> DataFrame: + ... + + +def f(): + from pandas import DataFrame + + def baz() -> DataFrame[int]: + ... + + +def f(): + from pandas import DataFrame + + def baz() -> DataFrame["int"]: + ... + + +def f(): + import pandas as pd + + def baz() -> pd.DataFrame: + ... + + +def f(): + import pandas as pd + + def baz() -> pd.DataFrame.Extra: + ... + + +def f(): + import pandas as pd + + def baz() -> pd.DataFrame | int: + ... diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 5d525550f5375..41f5dcc17f43c 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -515,8 +515,10 @@ where .chain(¶meters.kwonlyargs) { if let Some(expr) = ¶meter_with_default.parameter.annotation { - if runtime_annotation || singledispatch { - self.visit_runtime_annotation(expr); + if singledispatch { + self.visit_runtime_required_annotation(expr); + } else if runtime_annotation { + self.visit_runtime_evaluated_annotation(expr); } else { self.visit_annotation(expr); }; @@ -529,7 +531,7 @@ where if let Some(arg) = ¶meters.vararg { if let Some(expr) = &arg.annotation { if runtime_annotation { - self.visit_runtime_annotation(expr); + self.visit_runtime_evaluated_annotation(expr); } else { self.visit_annotation(expr); }; @@ -538,7 +540,7 @@ where if let Some(arg) = ¶meters.kwarg { if let Some(expr) = &arg.annotation { if runtime_annotation { - self.visit_runtime_annotation(expr); + self.visit_runtime_evaluated_annotation(expr); } else { self.visit_annotation(expr); }; @@ -546,7 +548,7 @@ where } for expr in returns { if runtime_annotation { - self.visit_runtime_annotation(expr); + self.visit_runtime_evaluated_annotation(expr); } else { self.visit_annotation(expr); }; @@ -677,40 +679,64 @@ where value, .. }) => { - // If we're in a class or module scope, then the annotation needs to be - // available at runtime. - // See: https://docs.python.org/3/reference/simple_stmts.html#annotated-assignment-statements - let runtime_annotation = if self.semantic.future_annotations() { - self.semantic + enum AnnotationKind { + RuntimeRequired, + RuntimeEvaluated, + TypingOnly, + } + + fn annotation_kind( + semantic: &SemanticModel, + settings: &LinterSettings, + ) -> AnnotationKind { + // If the annotation is in a class, and that class is marked as + // runtime-evaluated, treat the annotation as runtime-required. + // TODO(charlie): We could also include function calls here. + if semantic .current_scope() .kind .as_class() .is_some_and(|class_def| { - flake8_type_checking::helpers::runtime_evaluated_class( + flake8_type_checking::helpers::runtime_required_class( class_def, - &self - .settings - .flake8_type_checking - .runtime_evaluated_base_classes, - &self - .settings - .flake8_type_checking - .runtime_evaluated_decorators, - &self.semantic, + &settings.flake8_type_checking.runtime_evaluated_base_classes, + &settings.flake8_type_checking.runtime_evaluated_decorators, + semantic, ) }) - } else { - matches!( - self.semantic.current_scope().kind, + { + return AnnotationKind::RuntimeRequired; + } + + // If `__future__` annotations are enabled, then annotations are never evaluated + // at runtime, so we can treat them as typing-only. + if semantic.future_annotations() { + return AnnotationKind::TypingOnly; + } + + // Otherwise, if we're in a class or module scope, then the annotation needs to + // be available at runtime. + // See: https://docs.python.org/3/reference/simple_stmts.html#annotated-assignment-statements + if matches!( + semantic.current_scope().kind, ScopeKind::Class(_) | ScopeKind::Module - ) - }; + ) { + return AnnotationKind::RuntimeEvaluated; + } - if runtime_annotation { - self.visit_runtime_annotation(annotation); - } else { - self.visit_annotation(annotation); + AnnotationKind::TypingOnly + } + + match annotation_kind(&self.semantic, self.settings) { + AnnotationKind::RuntimeRequired => { + self.visit_runtime_required_annotation(annotation); + } + AnnotationKind::RuntimeEvaluated => { + self.visit_runtime_evaluated_annotation(annotation); + } + AnnotationKind::TypingOnly => self.visit_annotation(annotation), } + if let Some(expr) = value { if self.semantic.match_typing_expr(annotation, "TypeAlias") { self.visit_type_definition(expr); @@ -1527,10 +1553,18 @@ impl<'a> Checker<'a> { self.semantic.flags = snapshot; } + /// Visit an [`Expr`], and treat it as a runtime-evaluated type annotation. + fn visit_runtime_evaluated_annotation(&mut self, expr: &'a Expr) { + let snapshot = self.semantic.flags; + self.semantic.flags |= SemanticModelFlags::RUNTIME_EVALUATED_ANNOTATION; + self.visit_type_definition(expr); + self.semantic.flags = snapshot; + } + /// Visit an [`Expr`], and treat it as a runtime-required type annotation. - fn visit_runtime_annotation(&mut self, expr: &'a Expr) { + fn visit_runtime_required_annotation(&mut self, expr: &'a Expr) { let snapshot = self.semantic.flags; - self.semantic.flags |= SemanticModelFlags::RUNTIME_ANNOTATION; + self.semantic.flags |= SemanticModelFlags::RUNTIME_REQUIRED_ANNOTATION; self.visit_type_definition(expr); self.semantic.flags = snapshot; } diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs index 0a51e151f4703..38309950ac6fc 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs @@ -12,28 +12,37 @@ pub(crate) fn is_valid_runtime_import(binding: &Binding, semantic: &SemanticMode binding.context.is_runtime() && binding .references() - .any(|reference_id| semantic.reference(reference_id).context().is_runtime()) + .map(|reference_id| semantic.reference(reference_id)) + .any(|reference| { + // Are we in a typing-only context _or_ a runtime-evaluated context? We're + // willing to quote runtime-evaluated, but not runtime-required annotations. + !(reference.in_type_checking_block() + || reference.in_typing_only_annotation() + || reference.in_runtime_evaluated_annotation() + || reference.in_complex_string_type_definition() + || reference.in_simple_string_type_definition()) + }) } else { false } } -pub(crate) fn runtime_evaluated_class( +pub(crate) fn runtime_required_class( class_def: &ast::StmtClassDef, base_classes: &[String], decorators: &[String], semantic: &SemanticModel, ) -> bool { - if runtime_evaluated_base_class(class_def, base_classes, semantic) { + if runtime_required_base_class(class_def, base_classes, semantic) { return true; } - if runtime_evaluated_decorators(class_def, decorators, semantic) { + if runtime_required_decorators(class_def, decorators, semantic) { return true; } false } -fn runtime_evaluated_base_class( +fn runtime_required_base_class( class_def: &ast::StmtClassDef, base_classes: &[String], semantic: &SemanticModel, @@ -45,7 +54,7 @@ fn runtime_evaluated_base_class( seen: &mut FxHashSet, ) -> bool { class_def.bases().iter().any(|expr| { - // If the base class is itself runtime-evaluated, then this is too. + // If the base class is itself runtime-required, then this is too. // Ex) `class Foo(BaseModel): ...` if semantic .resolve_call_path(map_subscript(expr)) @@ -58,7 +67,7 @@ fn runtime_evaluated_base_class( return true; } - // If the base class extends a runtime-evaluated class, then this does too. + // If the base class extends a runtime-required class, then this does too. // Ex) `class Bar(BaseModel): ...; class Foo(Bar): ...` if let Some(id) = semantic.lookup_attribute(map_subscript(expr)) { if seen.insert(id) { @@ -86,7 +95,7 @@ fn runtime_evaluated_base_class( inner(class_def, base_classes, semantic, &mut FxHashSet::default()) } -fn runtime_evaluated_decorators( +fn runtime_required_decorators( class_def: &ast::StmtClassDef, decorators: &[String], semantic: &SemanticModel, 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 82b24755f4277..dd494af115e7f 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -37,6 +37,7 @@ mod tests { #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("TCH003.py"))] #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("snapshot.py"))] #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("TCH002.py"))] + #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("quote.py"))] #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("singledispatch.py"))] #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("strict.py"))] #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("typing_modules_1.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 1ce6336d3b158..897469b7a43a4 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -3,9 +3,14 @@ use std::borrow::Cow; use anyhow::Result; use rustc_hash::FxHashMap; -use ruff_diagnostics::{Diagnostic, DiagnosticKind, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Diagnostic, DiagnosticKind, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::{AnyImport, Binding, Imported, NodeId, ResolvedReferenceId, Scope}; +use ruff_python_ast::Expr; +use ruff_python_codegen::Stylist; +use ruff_python_semantic::{ + AnyImport, Binding, Imported, NodeId, ResolvedReferenceId, Scope, SemanticModel, +}; +use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; @@ -253,13 +258,19 @@ pub(crate) fn typing_only_runtime_import( }; if binding.context.is_runtime() - && binding.references().all(|reference_id| { - checker - .semantic() - .reference(reference_id) - .context() - .is_typing() - }) + && binding + .references() + .map(|reference_id| checker.semantic().reference(reference_id)) + .all(|reference| { + // All references should be in a typing context _or_ a runtime-evaluated + // annotation (as opposed to a runtime-required annotation), which we can + // quote. + reference.in_type_checking_block() + || reference.in_typing_only_annotation() + || reference.in_runtime_evaluated_annotation() + || reference.in_complex_string_type_definition() + || reference.in_simple_string_type_definition() + }) { let qualified_name = import.qualified_name(); @@ -310,6 +321,7 @@ pub(crate) fn typing_only_runtime_import( let import = ImportBinding { import, reference_id, + binding, range: binding.range(), parent_range: binding.parent_range(checker.semantic()), }; @@ -380,6 +392,8 @@ pub(crate) fn typing_only_runtime_import( struct ImportBinding<'a> { /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). import: AnyImport<'a>, + /// The binding for the imported symbol. + binding: &'a Binding<'a>, /// The first reference to the imported symbol. reference_id: ResolvedReferenceId, /// The trimmed range of the import (e.g., `List` in `from typing import List`). @@ -482,9 +496,91 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> checker.source_type, )?; - Ok( - Fix::unsafe_edits(remove_import_edit, add_import_edit.into_edits()).isolate( - Checker::isolation(checker.semantic().parent_statement_id(node_id)), - ), + // Step 3) Quote any runtime usages of the referenced symbol. + let quote_reference_edits = imports + .iter() + .flat_map(|ImportBinding { binding, .. }| { + binding.references.iter().filter_map(|reference_id| { + let reference = checker.semantic().reference(*reference_id); + if reference.context().is_runtime() { + Some(quote_annotation( + reference.node_id()?, + checker.semantic(), + checker.locator(), + checker.stylist(), + )) + } else { + None + } + }) + }) + .collect::>>()?; + + Ok(Fix::unsafe_edits( + remove_import_edit, + add_import_edit + .into_edits() + .into_iter() + .chain(quote_reference_edits), ) + .isolate(Checker::isolation( + checker.semantic().parent_statement_id(node_id), + ))) +} + +/// Wrap a type annotation in quotes. +/// +/// This requires more than just wrapping the reference itself in quotes. For example: +/// - When quoting `Series` in `Series[pd.Timestamp]`, we want `"Series[pd.Timestamp]"`. +/// - When quoting `kubernetes` in `kubernetes.SecurityContext`, we want `"kubernetes.SecurityContext"`. +/// - When quoting `Series` in `Series["pd.Timestamp"]`, we want `"Series[pd.Timestamp]"`. +/// - When quoting `Series` in `Series[Literal["pd.Timestamp"]]`, we want `"Series[Literal['pd.Timestamp']]"`. +fn quote_annotation( + node_id: NodeId, + semantic: &SemanticModel, + locator: &Locator, + stylist: &Stylist, +) -> Result { + let expr = semantic.expression(node_id); + if let Some(parent_id) = semantic.parent_expression_id(node_id) { + match semantic.expression(parent_id) { + Expr::Subscript(parent) => { + if expr == parent.value.as_ref() { + // 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, locator, stylist); + } + } + Expr::Attribute(parent) => { + if expr == parent.value.as_ref() { + // 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, locator, stylist); + } + } + Expr::BinOp(parent) => { + if parent.op.is_bit_or() { + // 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, locator, stylist); + } + } + _ => {} + } + } + + let annotation = locator.slice(expr); + + // If the annotation already contains a quote, avoid attempting to re-quote it. + if annotation.contains('\'') || annotation.contains('"') { + return Err(anyhow::anyhow!("Annotation already contains a quote")); + } + + // If we're quoting a name, we need to quote the entire expression. + let quote = stylist.quote(); + let annotation = format!("{quote}{annotation}{quote}"); + Ok(Edit::range_replacement(annotation, expr.range())) } diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs index 425f02fe550a4..811163860da55 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs @@ -14,7 +14,7 @@ impl Default for Settings { fn default() -> Self { Self { strict: false, - exempt_modules: vec!["typing".to_string()], + exempt_modules: vec!["typing".to_string(), "typing_extensions".to_string()], runtime_evaluated_base_classes: vec![], runtime_evaluated_decorators: vec![], } diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap new file mode 100644 index 0000000000000..cdd862fa0626a --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap @@ -0,0 +1,156 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +quote.py:2:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block + | +1 | def f(): +2 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +3 | +4 | def baz() -> DataFrame: + | + = help: Move into type-checking block + +ℹ Unsafe fix +1 |-def f(): + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: +2 4 | from pandas import DataFrame + 5 |+def f(): +3 6 | +4 |- def baz() -> DataFrame: + 7 |+ def baz() -> "DataFrame": +5 8 | ... +6 9 | +7 10 | + +quote.py:9:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block + | + 8 | def f(): + 9 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +10 | +11 | def baz() -> DataFrame[int]: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ from pandas import DataFrame +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +6 10 | +7 11 | +8 12 | def f(): +9 |- from pandas import DataFrame +10 13 | +11 |- def baz() -> DataFrame[int]: + 14 |+ def baz() -> "DataFrame[int]": +12 15 | ... +13 16 | +14 17 | + +quote.py:16:24: TCH002 Move third-party import `pandas.DataFrame` into a type-checking block + | +15 | def f(): +16 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +17 | +18 | def baz() -> DataFrame["int"]: + | + = help: Move into type-checking block + +quote.py:23:22: TCH002 [*] Move third-party import `pandas` into a type-checking block + | +22 | def f(): +23 | import pandas as pd + | ^^ TCH002 +24 | +25 | def baz() -> pd.DataFrame: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ import pandas as pd +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +20 24 | +21 25 | +22 26 | def f(): +23 |- import pandas as pd +24 27 | +25 |- def baz() -> pd.DataFrame: + 28 |+ def baz() -> "pd.DataFrame": +26 29 | ... +27 30 | +28 31 | + +quote.py:30:22: TCH002 [*] Move third-party import `pandas` into a type-checking block + | +29 | def f(): +30 | import pandas as pd + | ^^ TCH002 +31 | +32 | def baz() -> pd.DataFrame.Extra: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ import pandas as pd +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +27 31 | +28 32 | +29 33 | def f(): +30 |- import pandas as pd +31 34 | +32 |- def baz() -> pd.DataFrame.Extra: + 35 |+ def baz() -> "pd.DataFrame.Extra": +33 36 | ... +34 37 | +35 38 | + +quote.py:37:22: TCH002 [*] Move third-party import `pandas` into a type-checking block + | +36 | def f(): +37 | import pandas as pd + | ^^ TCH002 +38 | +39 | def baz() -> pd.DataFrame | int: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ import pandas as pd +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +34 38 | +35 39 | +36 40 | def f(): +37 |- import pandas as pd +38 41 | +39 |- def baz() -> pd.DataFrame | int: + 42 |+ def baz() -> "pd.DataFrame | int": +40 43 | ... + + diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 9cb3cebaa07ff..b1a63fd1f9d8c 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -291,9 +291,12 @@ impl<'a> SemanticModel<'a> { if let Some(binding_id) = self.scopes.global().get(name.id.as_str()) { if !self.bindings[binding_id].is_unbound() { // Mark the binding as used. - let reference_id = - self.resolved_references - .push(ScopeId::global(), name.range, self.flags); + let reference_id = self.resolved_references.push( + ScopeId::global(), + self.node_id, + name.range, + self.flags, + ); self.bindings[binding_id].references.push(reference_id); // Mark any submodule aliases as used. @@ -302,6 +305,7 @@ impl<'a> SemanticModel<'a> { { let reference_id = self.resolved_references.push( ScopeId::global(), + self.node_id, name.range, self.flags, ); @@ -356,18 +360,24 @@ impl<'a> SemanticModel<'a> { if let Some(binding_id) = scope.get(name.id.as_str()) { // Mark the binding as used. - let reference_id = - self.resolved_references - .push(self.scope_id, name.range, self.flags); + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + name.range, + self.flags, + ); self.bindings[binding_id].references.push(reference_id); // Mark any submodule aliases as used. if let Some(binding_id) = self.resolve_submodule(name.id.as_str(), scope_id, binding_id) { - let reference_id = - self.resolved_references - .push(self.scope_id, name.range, self.flags); + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + name.range, + self.flags, + ); self.bindings[binding_id].references.push(reference_id); } @@ -431,9 +441,12 @@ impl<'a> SemanticModel<'a> { // The `x` in `print(x)` should resolve to the `x` in `x = 1`. BindingKind::UnboundException(Some(binding_id)) => { // Mark the binding as used. - let reference_id = - self.resolved_references - .push(self.scope_id, name.range, self.flags); + let reference_id = self.resolved_references.push( + self.scope_id, + self.node_id, + name.range, + self.flags, + ); self.bindings[binding_id].references.push(reference_id); // Mark any submodule aliases as used. @@ -442,6 +455,7 @@ impl<'a> SemanticModel<'a> { { let reference_id = self.resolved_references.push( self.scope_id, + self.node_id, name.range, self.flags, ); @@ -979,6 +993,32 @@ impl<'a> SemanticModel<'a> { &self.nodes[node_id] } + /// Return the [`Expr`] corresponding to the given [`NodeId`]. + #[inline] + pub fn expression(&self, node_id: NodeId) -> &'a Expr { + self.nodes + .ancestor_ids(node_id) + .find_map(|id| self.nodes[id].as_expression()) + .expect("No expression found") + } + + /// Given a [`Expr`], return its parent, if any. + #[inline] + pub fn parent_expression(&self, node_id: NodeId) -> Option<&'a Expr> { + self.nodes + .ancestor_ids(node_id) + .filter_map(|id| self.nodes[id].as_expression()) + .nth(1) + } + + /// Given a [`NodeId`], return the [`NodeId`] of the parent expression, if any. + pub fn parent_expression_id(&self, node_id: NodeId) -> Option { + self.nodes + .ancestor_ids(node_id) + .filter(|id| self.nodes[*id].is_expression()) + .nth(1) + } + /// Return the [`Stmt`] corresponding to the given [`NodeId`]. #[inline] pub fn statement(&self, node_id: NodeId) -> &'a Stmt { @@ -1169,17 +1209,17 @@ impl<'a> SemanticModel<'a> { /// Add a reference to the given [`BindingId`] in the local scope. pub fn add_local_reference(&mut self, binding_id: BindingId, range: TextRange) { - let reference_id = self - .resolved_references - .push(self.scope_id, range, self.flags); + let reference_id = + self.resolved_references + .push(self.scope_id, self.node_id, range, self.flags); self.bindings[binding_id].references.push(reference_id); } /// Add a reference to the given [`BindingId`] in the global scope. pub fn add_global_reference(&mut self, binding_id: BindingId, range: TextRange) { - let reference_id = self - .resolved_references - .push(ScopeId::global(), range, self.flags); + let reference_id = + self.resolved_references + .push(ScopeId::global(), self.node_id, range, self.flags); self.bindings[binding_id].references.push(reference_id); } @@ -1282,10 +1322,16 @@ impl<'a> SemanticModel<'a> { .intersects(SemanticModelFlags::TYPING_ONLY_ANNOTATION) } - /// Return `true` if the model is in a runtime-required type annotation. - pub const fn in_runtime_annotation(&self) -> bool { + /// Return `true` if the context is in a runtime-evaluated type annotation. + pub const fn in_runtime_evaluated_annotation(&self) -> bool { self.flags - .intersects(SemanticModelFlags::RUNTIME_ANNOTATION) + .intersects(SemanticModelFlags::RUNTIME_EVALUATED_ANNOTATION) + } + + /// Return `true` if the context is in a runtime-required type annotation. + pub const fn in_runtime_required_annotation(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::RUNTIME_REQUIRED_ANNOTATION) } /// Return `true` if the model is in a type definition. @@ -1457,8 +1503,9 @@ impl ShadowedBinding { bitflags! { /// Flags indicating the current model state. #[derive(Debug, Default, Copy, Clone, Eq, PartialEq)] - pub struct SemanticModelFlags: u16 { - /// The model is in a typing-time-only type annotation. + pub struct SemanticModelFlags: u32 { + /// The model is in a type annotation that will only be evaluated when running a type + /// checker. /// /// For example, the model could be visiting `int` in: /// ```python @@ -1473,7 +1520,7 @@ bitflags! { /// are any annotated assignments in module or class scopes. const TYPING_ONLY_ANNOTATION = 1 << 0; - /// The model is in a runtime type annotation. + /// The model is in a type annotation that will be evaluated at runtime. /// /// For example, the model could be visiting `int` in: /// ```python @@ -1487,7 +1534,27 @@ bitflags! { /// If `from __future__ import annotations` is used, all annotations are evaluated at /// typing time. Otherwise, all function argument annotations are evaluated at runtime, as /// are any annotated assignments in module or class scopes. - const RUNTIME_ANNOTATION = 1 << 1; + const RUNTIME_EVALUATED_ANNOTATION = 1 << 1; + + /// The model is in a type annotation that is _required_ to be available at runtime. + /// + /// For example, the context could be visiting `int` in: + /// ```python + /// from pydantic import BaseModel + /// + /// class Foo(BaseModel): + /// x: int + /// ``` + /// + /// In this case, Pydantic requires that the type annotation be available at runtime + /// in order to perform runtime type-checking. + /// + /// Unlike [`RUNTIME_EVALUATED_ANNOTATION`], annotations that are marked as + /// [`RUNTIME_REQUIRED_ANNOTATION`] cannot be deferred to typing time via conversion to a + /// forward reference (e.g., by wrapping the type in quotes), as the annotations are not + /// only required by the Python interpreter, but by runtime type checkers too. + const RUNTIME_REQUIRED_ANNOTATION = 1 << 2; + /// The model is in a type definition. /// @@ -1501,7 +1568,7 @@ bitflags! { /// All type annotations are also type definitions, but the converse is not true. /// In our example, `int` is a type definition but not a type annotation, as it /// doesn't appear in a type annotation context, but rather in a type definition. - const TYPE_DEFINITION = 1 << 2; + const TYPE_DEFINITION = 1 << 3; /// The model is in a (deferred) "simple" string type definition. /// @@ -1512,7 +1579,7 @@ bitflags! { /// /// "Simple" string type definitions are those that consist of a single string literal, /// as opposed to an implicitly concatenated string literal. - const SIMPLE_STRING_TYPE_DEFINITION = 1 << 3; + const SIMPLE_STRING_TYPE_DEFINITION = 1 << 4; /// The model is in a (deferred) "complex" string type definition. /// @@ -1523,7 +1590,7 @@ bitflags! { /// /// "Complex" string type definitions are those that consist of a implicitly concatenated /// string literals. These are uncommon but valid. - const COMPLEX_STRING_TYPE_DEFINITION = 1 << 4; + const COMPLEX_STRING_TYPE_DEFINITION = 1 << 5; /// The model is in a (deferred) `__future__` type definition. /// @@ -1536,7 +1603,7 @@ bitflags! { /// /// `__future__`-style type annotations are only enabled if the `annotations` feature /// is enabled via `from __future__ import annotations`. - const FUTURE_TYPE_DEFINITION = 1 << 5; + const FUTURE_TYPE_DEFINITION = 1 << 6; /// The model is in an exception handler. /// @@ -1547,7 +1614,7 @@ bitflags! { /// except Exception: /// x: int = 1 /// ``` - const EXCEPTION_HANDLER = 1 << 6; + const EXCEPTION_HANDLER = 1 << 7; /// The model is in an f-string. /// @@ -1555,7 +1622,7 @@ bitflags! { /// ```python /// f'{x}' /// ``` - const F_STRING = 1 << 7; + const F_STRING = 1 << 8; /// The model is in a boolean test. /// @@ -1567,7 +1634,7 @@ bitflags! { /// /// The implication is that the actual value returned by the current expression is /// not used, only its truthiness. - const BOOLEAN_TEST = 1 << 8; + const BOOLEAN_TEST = 1 << 9; /// The model is in a `typing::Literal` annotation. /// @@ -1576,7 +1643,7 @@ bitflags! { /// def f(x: Literal["A", "B", "C"]): /// ... /// ``` - const TYPING_LITERAL = 1 << 9; + const TYPING_LITERAL = 1 << 10; /// The model is in a subscript expression. /// @@ -1584,7 +1651,7 @@ bitflags! { /// ```python /// x["a"]["b"] /// ``` - const SUBSCRIPT = 1 << 10; + const SUBSCRIPT = 1 << 11; /// The model is in a type-checking block. /// @@ -1596,7 +1663,7 @@ bitflags! { /// if TYPE_CHECKING: /// x: int = 1 /// ``` - const TYPE_CHECKING_BLOCK = 1 << 11; + const TYPE_CHECKING_BLOCK = 1 << 12; /// The model has traversed past the "top-of-file" import boundary. /// @@ -1609,7 +1676,7 @@ bitflags! { /// /// x: int = 1 /// ``` - const IMPORT_BOUNDARY = 1 << 12; + const IMPORT_BOUNDARY = 1 << 13; /// The model has traversed past the `__future__` import boundary. /// @@ -1624,7 +1691,7 @@ bitflags! { /// /// Python considers it a syntax error to import from `__future__` after /// any other non-`__future__`-importing statements. - const FUTURES_BOUNDARY = 1 << 13; + const FUTURES_BOUNDARY = 1 << 14; /// `__future__`-style type annotations are enabled in this model. /// @@ -1636,7 +1703,7 @@ bitflags! { /// def f(x: int) -> int: /// ... /// ``` - const FUTURE_ANNOTATIONS = 1 << 14; + const FUTURE_ANNOTATIONS = 1 << 15; /// The model is in a type parameter definition. /// @@ -1646,10 +1713,11 @@ bitflags! { /// /// Record = TypeVar("Record") /// - const TYPE_PARAM_DEFINITION = 1 << 15; + const TYPE_PARAM_DEFINITION = 1 << 16; /// The context is in any type annotation. - const ANNOTATION = Self::TYPING_ONLY_ANNOTATION.bits() | Self::RUNTIME_ANNOTATION.bits(); + const ANNOTATION = Self::TYPING_ONLY_ANNOTATION.bits() | Self::RUNTIME_EVALUATED_ANNOTATION.bits() | Self::RUNTIME_REQUIRED_ANNOTATION.bits(); + /// The context is in any string type definition. const STRING_TYPE_DEFINITION = Self::SIMPLE_STRING_TYPE_DEFINITION.bits() diff --git a/crates/ruff_python_semantic/src/reference.rs b/crates/ruff_python_semantic/src/reference.rs index a963895b21e69..ae6d2825d0504 100644 --- a/crates/ruff_python_semantic/src/reference.rs +++ b/crates/ruff_python_semantic/src/reference.rs @@ -8,11 +8,14 @@ use ruff_text_size::{Ranged, TextRange}; use crate::context::ExecutionContext; use crate::scope::ScopeId; -use crate::{Exceptions, SemanticModelFlags}; +use crate::{Exceptions, NodeId, SemanticModelFlags}; /// A resolved read reference to a name in a program. #[derive(Debug, Clone)] pub struct ResolvedReference { + /// The expression that the reference occurs in. `None` if the reference is a global + /// reference or a reference via an augmented assignment. + node_id: Option, /// The scope in which the reference is defined. scope_id: ScopeId, /// The range of the reference in the source code. @@ -22,6 +25,11 @@ pub struct ResolvedReference { } impl ResolvedReference { + /// The expression that the reference occurs in. + pub const fn node_id(&self) -> Option { + self.node_id + } + /// The scope in which the reference is defined. pub const fn scope_id(&self) -> ScopeId { self.scope_id @@ -35,6 +43,48 @@ impl ResolvedReference { ExecutionContext::Runtime } } + + /// Return `true` if the context is in a typing-only type annotation. + pub const fn in_typing_only_annotation(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::TYPING_ONLY_ANNOTATION) + } + + /// Return `true` if the context is in a runtime-required type annotation. + pub const fn in_runtime_evaluated_annotation(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::RUNTIME_EVALUATED_ANNOTATION) + } + + /// Return `true` if the context is in a "simple" string type definition. + pub const fn in_simple_string_type_definition(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::SIMPLE_STRING_TYPE_DEFINITION) + } + + /// Return `true` if the context is in a "complex" string type definition. + pub const fn in_complex_string_type_definition(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::COMPLEX_STRING_TYPE_DEFINITION) + } + + /// Return `true` if the context is in a `__future__` type definition. + pub const fn in_future_type_definition(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::FUTURE_TYPE_DEFINITION) + } + + /// Return `true` if the context is in any kind of deferred type definition. + pub const fn in_deferred_type_definition(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::DEFERRED_TYPE_DEFINITION) + } + + /// Return `true` if the context is in a type-checking block. + pub const fn in_type_checking_block(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::TYPE_CHECKING_BLOCK) + } } impl Ranged for ResolvedReference { @@ -57,10 +107,12 @@ impl ResolvedReferences { pub(crate) fn push( &mut self, scope_id: ScopeId, + node_id: Option, range: TextRange, flags: SemanticModelFlags, ) -> ResolvedReferenceId { self.0.push(ResolvedReference { + node_id, scope_id, range, flags, From 176d325d3774be90277ee2d1bb6e104591e1510f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 10 Dec 2023 17:43:23 -0500 Subject: [PATCH 2/5] Add setting --- .../rules/flake8_type_checking/settings.rs | 20 +++++++++ crates/ruff_workspace/src/options.rs | 44 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs index 811163860da55..fa266694ce285 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs @@ -1,6 +1,7 @@ //! Settings for the `flake8-type-checking` plugin. use ruff_macros::CacheKey; +use serde::{Deserialize, Serialize}; #[derive(Debug, CacheKey)] pub struct Settings { @@ -8,6 +9,7 @@ pub struct Settings { pub exempt_modules: Vec, pub runtime_evaluated_base_classes: Vec, pub runtime_evaluated_decorators: Vec, + pub annotation_strategy: AnnotationStrategy, } impl Default for Settings { @@ -17,6 +19,24 @@ impl Default for Settings { exempt_modules: vec!["typing".to_string(), "typing_extensions".to_string()], runtime_evaluated_base_classes: vec![], runtime_evaluated_decorators: vec![], + annotation_strategy: AnnotationStrategy::Preserve, } } } + +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, CacheKey)] +#[serde(deny_unknown_fields, rename_all = "kebab-case")] +#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] +pub enum AnnotationStrategy { + /// Avoid changing the semantics of runtime-evaluated annotations (e.g., by quoting them, or + /// inserting `from __future__ import annotations`). Imports will be classified as typing-only + /// or runtime-required based exclusively on the existing type annotations. + #[default] + Preserve, + /// Quote runtime-evaluated annotations, if doing so would enable the corresponding import to + /// be moved into an `if TYPE_CHECKING:` block. + Quote, + /// Insert `from __future__ import annotations` at the top of the file, if doing so would enable + /// an import to be moved into an `if TYPE_CHECKING:` block. + Future, +} diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 10e5c6717696e..4aae814dd273c 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -12,6 +12,7 @@ use ruff_linter::rules::flake8_pytest_style::settings::SettingsError; use ruff_linter::rules::flake8_pytest_style::types; use ruff_linter::rules::flake8_quotes::settings::Quote; use ruff_linter::rules::flake8_tidy_imports::settings::{ApiBan, Strictness}; +use ruff_linter::rules::flake8_type_checking::settings::AnnotationStrategy; use ruff_linter::rules::isort::settings::RelativeImportsOrder; use ruff_linter::rules::isort::{ImportSection, ImportType}; use ruff_linter::rules::pydocstyle::settings::Convention; @@ -1642,6 +1643,48 @@ pub struct Flake8TypeCheckingOptions { "# )] pub runtime_evaluated_decorators: Option>, + + /// The strategy to use when analyzing type annotations that, by default, + /// Python would evaluate at runtime. + /// + /// For example, in the following, Python requires that `Sequence` be + /// available at runtime, despite the fact that it's only used in a type + /// annotation: + /// ```python + /// from collections.abc import Sequence + /// + /// def func(value: Sequence[int]) -> None: + /// ... + /// ``` + /// + /// In other words, moving `from collections.abc import Sequence` into an + /// `if TYPE_CHECKING:` block above would cause a runtime error. + /// + /// By default, Ruff will respect such runtime evaluations (`"preserve"`), + /// but supports two alternative strategies for handling them: + /// + /// - `"quote"`: Add quotes around the annotation (e.g., `"Sequence[int]"`), to + /// prevent Python from evaluating it at runtime. Adding quotes around + /// `Sequence` above will allow Ruff to move the import into an + /// `if TYPE_CHECKING:` block. + /// - `"future"`: Add a `from __future__ import annotations` import at the + /// top of the file, which will cause Python to treat all annotations as + /// strings, rather than evaluating them at runtime. + /// + /// Setting `annotation-strategy` to `"quote"` or `"future"` will allow + /// Ruff to move more imports into type-checking blocks, but may cause + /// issues with other tools that expect annotations to be evaluated at + /// runtime. + #[option( + default = "[]", + value_type = r#""preserve" | "quote" | "future""#, + example = r#" + # Add quotes around type annotations, if doing so would allow + # an import to be moved into a type-checking block. + annotation-strategy = "quote" + "# + )] + pub annotation_strategy: Option, } impl Flake8TypeCheckingOptions { @@ -1653,6 +1696,7 @@ impl Flake8TypeCheckingOptions { .unwrap_or_else(|| vec!["typing".to_string()]), runtime_evaluated_base_classes: self.runtime_evaluated_base_classes.unwrap_or_default(), runtime_evaluated_decorators: self.runtime_evaluated_decorators.unwrap_or_default(), + annotation_strategy: self.annotation_strategy.unwrap_or_default(), } } } From 81966e0a81dc8e3165bf3f991e2cfae2d862bb68 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 11 Dec 2023 11:11:39 -0500 Subject: [PATCH 3/5] Properly support runtime annotations --- .../fixtures/flake8_type_checking/quote.py | 10 ++ .../checkers/ast/analyze/deferred_scopes.rs | 1 + .../src/rules/flake8_type_checking/helpers.rs | 76 ++++++++- .../src/rules/flake8_type_checking/imports.rs | 22 +++ .../src/rules/flake8_type_checking/mod.rs | 21 +++ .../runtime_import_in_type_checking_block.rs | 156 +++++++++++++---- .../rules/typing_only_runtime_import.rs | 112 +++---------- .../rules/flake8_type_checking/settings.rs | 4 +- ...mport-in-type-checking-block_quote.py.snap | 22 +++ ...ping-only-third-party-import_quote.py.snap | 158 ++++++++++++++++++ ...mport-in-type-checking-block_quote.py.snap | 29 ++++ ...ping-only-third-party-import_quote.py.snap | 152 ----------------- pyproject.toml | 3 + ruff.schema.json | 36 ++++ 14 files changed, 531 insertions(+), 271 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/imports.rs create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py index 26d5f9d341957..041bb3d2b7391 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py @@ -38,3 +38,13 @@ def f(): def baz() -> pd.DataFrame | int: ... + + +def f(): + from typing import TYPE_CHECKING + + if TYPE_CHECKING: + from pandas import DataFrame + + def func(value: DataFrame): + ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs index 27c9a6b7c79e2..66d0ad25273e9 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs @@ -59,6 +59,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) { flake8_type_checking::helpers::is_valid_runtime_import( binding, &checker.semantic, + &checker.settings.flake8_type_checking, ) }) .collect() diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs index 38309950ac6fc..f502d3519274e 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs @@ -1,10 +1,20 @@ +use crate::rules::flake8_type_checking::settings::Settings; +use anyhow::Result; +use ruff_diagnostics::Edit; use ruff_python_ast::call_path::from_qualified_name; use ruff_python_ast::helpers::{map_callable, map_subscript}; use ruff_python_ast::{self as ast, Expr}; -use ruff_python_semantic::{Binding, BindingId, BindingKind, SemanticModel}; +use ruff_python_codegen::Stylist; +use ruff_python_semantic::{Binding, BindingId, BindingKind, NodeId, SemanticModel}; +use ruff_source_file::Locator; +use ruff_text_size::Ranged; use rustc_hash::FxHashSet; -pub(crate) fn is_valid_runtime_import(binding: &Binding, semantic: &SemanticModel) -> bool { +pub(crate) fn is_valid_runtime_import( + binding: &Binding, + semantic: &SemanticModel, + settings: &Settings, +) -> bool { if matches!( binding.kind, BindingKind::Import(..) | BindingKind::FromImport(..) | BindingKind::SubmoduleImport(..) @@ -18,9 +28,10 @@ pub(crate) fn is_valid_runtime_import(binding: &Binding, semantic: &SemanticMode // willing to quote runtime-evaluated, but not runtime-required annotations. !(reference.in_type_checking_block() || reference.in_typing_only_annotation() - || reference.in_runtime_evaluated_annotation() || reference.in_complex_string_type_definition() - || reference.in_simple_string_type_definition()) + || reference.in_simple_string_type_definition() + || (settings.annotation_strategy.is_quote() + && reference.in_runtime_evaluated_annotation())) }) } else { false @@ -183,3 +194,60 @@ pub(crate) fn is_singledispatch_implementation( is_singledispatch_interface(function_def, semantic) }) } + +/// Wrap a type annotation in quotes. +/// +/// This requires more than just wrapping the reference itself in quotes. For example: +/// - When quoting `Series` in `Series[pd.Timestamp]`, we want `"Series[pd.Timestamp]"`. +/// - When quoting `kubernetes` in `kubernetes.SecurityContext`, we want `"kubernetes.SecurityContext"`. +/// - When quoting `Series` in `Series["pd.Timestamp"]`, we want `"Series[pd.Timestamp]"`. +/// - When quoting `Series` in `Series[Literal["pd.Timestamp"]]`, we want `"Series[Literal['pd.Timestamp']]"`. +pub(crate) fn quote_annotation( + node_id: NodeId, + semantic: &SemanticModel, + locator: &Locator, + stylist: &Stylist, +) -> Result { + let expr = semantic.expression(node_id); + if let Some(parent_id) = semantic.parent_expression_id(node_id) { + match semantic.expression(parent_id) { + Expr::Subscript(parent) => { + if expr == parent.value.as_ref() { + // 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, locator, stylist); + } + } + Expr::Attribute(parent) => { + if expr == parent.value.as_ref() { + // 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, locator, stylist); + } + } + Expr::BinOp(parent) => { + if parent.op.is_bit_or() { + // 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, locator, stylist); + } + } + _ => {} + } + } + + let annotation = locator.slice(expr); + + // If the annotation already contains a quote, avoid attempting to re-quote it. + if annotation.contains('\'') || annotation.contains('"') { + return Err(anyhow::anyhow!("Annotation already contains a quote")); + } + + // If we're quoting a name, we need to quote the entire expression. + let quote = stylist.quote(); + let annotation = format!("{quote}{annotation}{quote}"); + Ok(Edit::range_replacement(annotation, expr.range())) +} diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/imports.rs b/crates/ruff_linter/src/rules/flake8_type_checking/imports.rs new file mode 100644 index 0000000000000..92c65cf275e1c --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/imports.rs @@ -0,0 +1,22 @@ +use ruff_python_semantic::{AnyImport, Binding, ResolvedReferenceId}; +use ruff_text_size::{Ranged, TextRange}; + +/// An import with its surrounding context. +pub(crate) struct ImportBinding<'a> { + /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). + pub(crate) import: AnyImport<'a>, + /// The binding for the imported symbol. + pub(crate) binding: &'a Binding<'a>, + /// The first reference to the imported symbol. + pub(crate) reference_id: ResolvedReferenceId, + /// The trimmed range of the import (e.g., `List` in `from typing import List`). + pub(crate) range: TextRange, + /// The range of the import's parent statement. + pub(crate) parent_range: Option, +} + +impl Ranged for ImportBinding<'_> { + fn range(&self) -> TextRange { + self.range + } +} 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 dd494af115e7f..754b7e8ae74ac 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -1,5 +1,6 @@ //! Rules from [flake8-type-checking](https://pypi.org/project/flake8-type-checking/). pub(crate) mod helpers; +mod imports; pub(crate) mod rules; pub mod settings; @@ -12,6 +13,7 @@ mod tests { use test_case::test_case; use crate::registry::{Linter, Rule}; + use crate::rules::flake8_type_checking::settings::AnnotationStrategy; use crate::test::{test_path, test_snippet}; use crate::{assert_messages, settings}; @@ -33,6 +35,7 @@ mod tests { #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("TCH004_7.py"))] #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("TCH004_8.py"))] #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("TCH004_9.py"))] + #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote.py"))] #[test_case(Rule::TypingOnlyFirstPartyImport, Path::new("TCH001.py"))] #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("TCH003.py"))] #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("snapshot.py"))] @@ -52,6 +55,24 @@ mod tests { Ok(()) } + #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote.py"))] + #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("quote.py"))] + fn quote(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!("quote_{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let diagnostics = test_path( + Path::new("flake8_type_checking").join(path).as_path(), + &settings::LinterSettings { + flake8_type_checking: super::settings::Settings { + annotation_strategy: AnnotationStrategy::Quote, + ..Default::default() + }, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } + #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("strict.py"))] fn strict(rule_code: Rule, path: &Path) -> Result<()> { let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index dea0f4007e0cd..6a766392f1403 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -5,13 +5,15 @@ use rustc_hash::FxHashMap; use ruff_diagnostics::{Diagnostic, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::{AnyImport, Imported, NodeId, ResolvedReferenceId, Scope}; -use ruff_text_size::{Ranged, TextRange}; +use ruff_python_semantic::{Imported, NodeId, Scope}; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::fix; use crate::importer::ImportedMembers; +use crate::rules::flake8_type_checking::helpers::quote_annotation; +use crate::rules::flake8_type_checking::imports::ImportBinding; /// ## What it does /// Checks for runtime imports defined in a type-checking block. @@ -20,6 +22,11 @@ use crate::importer::ImportedMembers; /// The type-checking block is not executed at runtime, so the import will not /// be available at runtime. /// +/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// annotations will be wrapped in quotes if doing so would enable the +/// corresponding import to remain in the type-checking block. +/// +/// /// ## Example /// ```python /// from typing import TYPE_CHECKING @@ -41,11 +48,15 @@ use crate::importer::ImportedMembers; /// foo.bar() /// ``` /// +/// ## Options +/// - `flake8-type-checking.annotation-strategy` +/// /// ## References /// - [PEP 535](https://peps.python.org/pep-0563/#runtime-annotation-resolution-and-type-checking) #[violation] pub struct RuntimeImportInTypeCheckingBlock { qualified_name: String, + strategy: Strategy, } impl Violation for RuntimeImportInTypeCheckingBlock { @@ -53,14 +64,26 @@ impl Violation for RuntimeImportInTypeCheckingBlock { #[derive_message_formats] fn message(&self) -> String { - let RuntimeImportInTypeCheckingBlock { qualified_name } = self; - format!( - "Move import `{qualified_name}` out of type-checking block. Import is used for more than type hinting." - ) + let Self { + qualified_name, + strategy, + } = self; + match strategy { + Strategy::MoveImport => format!( + "Move import `{qualified_name}` out of type-checking block. Import is used for more than type hinting." + ), + Strategy::QuoteUsages => format!( + "Quote references to `{qualified_name}`. Import is not available at runtime." + ), + } } fn fix_title(&self) -> Option { - Some("Move out of type-checking block".to_string()) + let Self { strategy, .. } = self; + match strategy { + Strategy::MoveImport => Some("Move out of type-checking block".to_string()), + Strategy::QuoteUsages => Some("Quote references".to_string()), + } } } @@ -71,7 +94,8 @@ pub(crate) fn runtime_import_in_type_checking_block( diagnostics: &mut Vec, ) { // Collect all runtime imports by statement. - let mut errors_by_statement: FxHashMap> = FxHashMap::default(); + let mut moves_by_statement: FxHashMap> = FxHashMap::default(); + let mut quotes_by_statement: FxHashMap> = FxHashMap::default(); let mut ignores_by_statement: FxHashMap> = FxHashMap::default(); for binding_id in scope.binding_ids() { @@ -101,6 +125,7 @@ pub(crate) fn runtime_import_in_type_checking_block( let import = ImportBinding { import, reference_id, + binding, range: binding.range(), parent_range: binding.parent_range(checker.semantic()), }; @@ -118,15 +143,32 @@ pub(crate) fn runtime_import_in_type_checking_block( .or_default() .push(import); } else { - errors_by_statement.entry(node_id).or_default().push(import); + // Determine whether the member should be fixed by moving the import out of the + // type-checking block, or by quoting its references. + if checker + .settings + .flake8_type_checking + .annotation_strategy + .is_quote() + && binding.references().all(|reference_id| { + let reference = checker.semantic().reference(reference_id); + reference.context().is_typing() + || reference.in_runtime_evaluated_annotation() + }) + { + println!("quote"); + quotes_by_statement.entry(node_id).or_default().push(import); + } else { + moves_by_statement.entry(node_id).or_default().push(import); + } } } } // Generate a diagnostic for every import, but share a fix across all imports within the same // statement (excluding those that are ignored). - for (node_id, imports) in errors_by_statement { - let fix = fix_imports(checker, node_id, &imports).ok(); + for (node_id, imports) in moves_by_statement { + let fix = move_imports(checker, node_id, &imports).ok(); for ImportBinding { import, @@ -138,6 +180,36 @@ pub(crate) fn runtime_import_in_type_checking_block( let mut diagnostic = Diagnostic::new( RuntimeImportInTypeCheckingBlock { qualified_name: import.qualified_name(), + strategy: Strategy::MoveImport, + }, + range, + ); + if let Some(range) = parent_range { + diagnostic.set_parent(range.start()); + } + if let Some(fix) = fix.as_ref() { + diagnostic.set_fix(fix.clone()); + } + diagnostics.push(diagnostic); + } + } + + // Generate a diagnostic for every import, but share a fix across all imports within the same + // statement (excluding those that are ignored). + for (node_id, imports) in quotes_by_statement { + let fix = quote_imports(checker, node_id, &imports).ok(); + + for ImportBinding { + import, + range, + parent_range, + .. + } in imports + { + let mut diagnostic = Diagnostic::new( + RuntimeImportInTypeCheckingBlock { + qualified_name: import.qualified_name(), + strategy: Strategy::QuoteUsages, }, range, ); @@ -163,6 +235,7 @@ pub(crate) fn runtime_import_in_type_checking_block( let mut diagnostic = Diagnostic::new( RuntimeImportInTypeCheckingBlock { qualified_name: import.qualified_name(), + strategy: Strategy::MoveImport, }, range, ); @@ -173,26 +246,38 @@ pub(crate) fn runtime_import_in_type_checking_block( } } -/// A runtime-required import with its surrounding context. -struct ImportBinding<'a> { - /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). - import: AnyImport<'a>, - /// The first reference to the imported symbol. - reference_id: ResolvedReferenceId, - /// The trimmed range of the import (e.g., `List` in `from typing import List`). - range: TextRange, - /// The range of the import's parent statement. - parent_range: Option, -} - -impl Ranged for ImportBinding<'_> { - fn range(&self) -> TextRange { - self.range - } +/// 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 { + let mut quote_reference_edits = imports + .iter() + .flat_map(|ImportBinding { binding, .. }| { + binding.references.iter().filter_map(|reference_id| { + let reference = checker.semantic().reference(*reference_id); + if reference.context().is_runtime() { + Some(quote_annotation( + reference.node_id()?, + checker.semantic(), + checker.locator(), + checker.stylist(), + )) + } else { + None + } + }) + }) + .collect::>>()?; + let quote_reference_edit = quote_reference_edits + .pop() + .expect("Expected at least one reference"); + Ok( + Fix::unsafe_edits(quote_reference_edit, quote_reference_edits).isolate(Checker::isolation( + checker.semantic().parent_statement_id(node_id), + )), + ) } /// Generate a [`Fix`] to remove runtime imports from a type-checking block. -fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> Result { +fn move_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> Result { let statement = checker.semantic().statement(node_id); let parent = checker.semantic().parent_statement(node_id); @@ -236,3 +321,18 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> ), ) } + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Strategy { + /// The import should be moved out of the type-checking block. + /// + /// This is required when at least one reference to the symbol is in a runtime-required context. + /// For example, given `from foo import Bar`, `x = Bar()` would be runtime-required. + MoveImport, + /// All usages of the import should be wrapped in quotes. + /// + /// This is acceptable when all references to the symbol are in a runtime-evaluated, but not + /// runtime-required context. For example, given `from foo import Bar`, `x: Bar` would be + /// runtime-evaluated, but not runtime-required. + QuoteUsages, +} diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 897469b7a43a4..b0184f4642d2b 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -3,20 +3,17 @@ use std::borrow::Cow; use anyhow::Result; use rustc_hash::FxHashMap; -use ruff_diagnostics::{Diagnostic, DiagnosticKind, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Diagnostic, DiagnosticKind, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::Expr; -use ruff_python_codegen::Stylist; -use ruff_python_semantic::{ - AnyImport, Binding, Imported, NodeId, ResolvedReferenceId, Scope, SemanticModel, -}; -use ruff_source_file::Locator; -use ruff_text_size::{Ranged, TextRange}; +use ruff_python_semantic::{Binding, Imported, NodeId, Scope}; +use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::fix; use crate::importer::ImportedMembers; +use crate::rules::flake8_type_checking::helpers::quote_annotation; +use crate::rules::flake8_type_checking::imports::ImportBinding; use crate::rules::isort::{categorize, ImportSection, ImportType}; /// ## What it does @@ -29,6 +26,10 @@ use crate::rules::isort::{categorize, ImportSection, ImportType}; /// instead be imported conditionally under an `if TYPE_CHECKING:` block to /// minimize runtime overhead. /// +/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// annotations will be wrapped in quotes if doing so would enable the +/// corresponding import to be moved into an `if TYPE_CHECKING:` block. +/// /// If a class _requires_ that type annotations be available at runtime (as is /// the case for Pydantic, SQLAlchemy, and other libraries), consider using /// the [`flake8-type-checking.runtime-evaluated-base-classes`] and @@ -61,6 +62,7 @@ use crate::rules::isort::{categorize, ImportSection, ImportType}; /// ``` /// /// ## Options +/// - `flake8-type-checking.annotation-strategy` /// - `flake8-type-checking.runtime-evaluated-base-classes` /// - `flake8-type-checking.runtime-evaluated-decorators` /// @@ -97,6 +99,10 @@ impl Violation for TypingOnlyFirstPartyImport { /// instead be imported conditionally under an `if TYPE_CHECKING:` block to /// minimize runtime overhead. /// +/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// annotations will be wrapped in quotes if doing so would enable the +/// corresponding import to be moved into an `if TYPE_CHECKING:` block. +/// /// If a class _requires_ that type annotations be available at runtime (as is /// the case for Pydantic, SQLAlchemy, and other libraries), consider using /// the [`flake8-type-checking.runtime-evaluated-base-classes`] and @@ -129,6 +135,7 @@ impl Violation for TypingOnlyFirstPartyImport { /// ``` /// /// ## Options +/// - `flake8-type-checking.annotation-strategy` /// - `flake8-type-checking.runtime-evaluated-base-classes` /// - `flake8-type-checking.runtime-evaluated-decorators` /// @@ -165,6 +172,10 @@ impl Violation for TypingOnlyThirdPartyImport { /// instead be imported conditionally under an `if TYPE_CHECKING:` block to /// minimize runtime overhead. /// +/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// annotations will be wrapped in quotes if doing so would enable the +/// corresponding import to be moved into an `if TYPE_CHECKING:` block. +/// /// If a class _requires_ that type annotations be available at runtime (as is /// the case for Pydantic, SQLAlchemy, and other libraries), consider using /// the [`flake8-type-checking.runtime-evaluated-base-classes`] and @@ -197,6 +208,7 @@ impl Violation for TypingOnlyThirdPartyImport { /// ``` /// /// ## Options +/// - `flake8-type-checking.annotation-strategy` /// - `flake8-type-checking.runtime-evaluated-base-classes` /// - `flake8-type-checking.runtime-evaluated-decorators` /// @@ -267,9 +279,14 @@ pub(crate) fn typing_only_runtime_import( // quote. reference.in_type_checking_block() || reference.in_typing_only_annotation() - || reference.in_runtime_evaluated_annotation() || reference.in_complex_string_type_definition() || reference.in_simple_string_type_definition() + || (checker + .settings + .flake8_type_checking + .annotation_strategy + .is_quote() + && reference.in_runtime_evaluated_annotation()) }) { let qualified_name = import.qualified_name(); @@ -388,26 +405,6 @@ pub(crate) fn typing_only_runtime_import( } } -/// A runtime-required import with its surrounding context. -struct ImportBinding<'a> { - /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). - import: AnyImport<'a>, - /// The binding for the imported symbol. - binding: &'a Binding<'a>, - /// The first reference to the imported symbol. - reference_id: ResolvedReferenceId, - /// The trimmed range of the import (e.g., `List` in `from typing import List`). - range: TextRange, - /// The range of the import's parent statement. - parent_range: Option, -} - -impl Ranged for ImportBinding<'_> { - fn range(&self) -> TextRange { - self.range - } -} - /// Return the [`Rule`] for the given import type. fn rule_for(import_type: ImportType) -> Rule { match import_type { @@ -527,60 +524,3 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> checker.semantic().parent_statement_id(node_id), ))) } - -/// Wrap a type annotation in quotes. -/// -/// This requires more than just wrapping the reference itself in quotes. For example: -/// - When quoting `Series` in `Series[pd.Timestamp]`, we want `"Series[pd.Timestamp]"`. -/// - When quoting `kubernetes` in `kubernetes.SecurityContext`, we want `"kubernetes.SecurityContext"`. -/// - When quoting `Series` in `Series["pd.Timestamp"]`, we want `"Series[pd.Timestamp]"`. -/// - When quoting `Series` in `Series[Literal["pd.Timestamp"]]`, we want `"Series[Literal['pd.Timestamp']]"`. -fn quote_annotation( - node_id: NodeId, - semantic: &SemanticModel, - locator: &Locator, - stylist: &Stylist, -) -> Result { - let expr = semantic.expression(node_id); - if let Some(parent_id) = semantic.parent_expression_id(node_id) { - match semantic.expression(parent_id) { - Expr::Subscript(parent) => { - if expr == parent.value.as_ref() { - // 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, locator, stylist); - } - } - Expr::Attribute(parent) => { - if expr == parent.value.as_ref() { - // 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, locator, stylist); - } - } - Expr::BinOp(parent) => { - if parent.op.is_bit_or() { - // 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, locator, stylist); - } - } - _ => {} - } - } - - let annotation = locator.slice(expr); - - // If the annotation already contains a quote, avoid attempting to re-quote it. - if annotation.contains('\'') || annotation.contains('"') { - return Err(anyhow::anyhow!("Annotation already contains a quote")); - } - - // If we're quoting a name, we need to quote the entire expression. - let quote = stylist.quote(); - let annotation = format!("{quote}{annotation}{quote}"); - Ok(Edit::range_replacement(annotation, expr.range())) -} diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs index fa266694ce285..5380d16babb58 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs @@ -24,7 +24,9 @@ impl Default for Settings { } } -#[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, CacheKey)] +#[derive( + Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, CacheKey, is_macro::Is, +)] #[serde(deny_unknown_fields, rename_all = "kebab-case")] #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] pub enum AnnotationStrategy { diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap new file mode 100644 index 0000000000000..7d935c57a185e --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap @@ -0,0 +1,22 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +quote.py:47:28: TCH004 [*] Quote references to `pandas.DataFrame`. Import is not available at runtime. + | +46 | if TYPE_CHECKING: +47 | from pandas import DataFrame + | ^^^^^^^^^ TCH004 +48 | +49 | def func(value: DataFrame): + | + = help: Quote references + +ℹ Unsafe fix +46 46 | if TYPE_CHECKING: +47 47 | from pandas import DataFrame +48 48 | +49 |- def func(value: DataFrame): + 49 |+ def func(value: "DataFrame"): +50 50 | ... + + diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap new file mode 100644 index 0000000000000..14df699c48d49 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap @@ -0,0 +1,158 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +quote.py:2:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block + | +1 | def f(): +2 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +3 | +4 | def baz() -> DataFrame: + | + = help: Move into type-checking block + +ℹ Unsafe fix +1 |-def f(): + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: +2 4 | from pandas import DataFrame + 5 |+def f(): +3 6 | +4 |- def baz() -> DataFrame: + 7 |+ def baz() -> "DataFrame": +5 8 | ... +6 9 | +7 10 | + +quote.py:9:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block + | + 8 | def f(): + 9 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +10 | +11 | def baz() -> DataFrame[int]: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ from pandas import DataFrame +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +6 10 | +7 11 | +8 12 | def f(): +9 |- from pandas import DataFrame +10 13 | +11 |- def baz() -> DataFrame[int]: + 14 |+ def baz() -> "DataFrame[int]": +12 15 | ... +13 16 | +14 17 | + +quote.py:16:24: TCH002 Move third-party import `pandas.DataFrame` into a type-checking block + | +15 | def f(): +16 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +17 | +18 | def baz() -> DataFrame["int"]: + | + = help: Move into type-checking block + +quote.py:23:22: TCH002 [*] Move third-party import `pandas` into a type-checking block + | +22 | def f(): +23 | import pandas as pd + | ^^ TCH002 +24 | +25 | def baz() -> pd.DataFrame: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ import pandas as pd +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +20 24 | +21 25 | +22 26 | def f(): +23 |- import pandas as pd +24 27 | +25 |- def baz() -> pd.DataFrame: + 28 |+ def baz() -> "pd.DataFrame": +26 29 | ... +27 30 | +28 31 | + +quote.py:30:22: TCH002 [*] Move third-party import `pandas` into a type-checking block + | +29 | def f(): +30 | import pandas as pd + | ^^ TCH002 +31 | +32 | def baz() -> pd.DataFrame.Extra: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ import pandas as pd +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +27 31 | +28 32 | +29 33 | def f(): +30 |- import pandas as pd +31 34 | +32 |- def baz() -> pd.DataFrame.Extra: + 35 |+ def baz() -> "pd.DataFrame.Extra": +33 36 | ... +34 37 | +35 38 | + +quote.py:37:22: TCH002 [*] Move third-party import `pandas` into a type-checking block + | +36 | def f(): +37 | import pandas as pd + | ^^ TCH002 +38 | +39 | def baz() -> pd.DataFrame | int: + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ import pandas as pd +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +34 38 | +35 39 | +36 40 | def f(): +37 |- import pandas as pd +38 41 | +39 |- def baz() -> pd.DataFrame | int: + 42 |+ def baz() -> "pd.DataFrame | int": +40 43 | ... +41 44 | +42 45 | + + diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap new file mode 100644 index 0000000000000..73d0d948de4a0 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap @@ -0,0 +1,29 @@ +--- +source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +--- +quote.py:47:28: TCH004 [*] Move import `pandas.DataFrame` out of type-checking block. Import is used for more than type hinting. + | +46 | if TYPE_CHECKING: +47 | from pandas import DataFrame + | ^^^^^^^^^ TCH004 +48 | +49 | def func(value: DataFrame): + | + = help: Move out of type-checking block + +ℹ Unsafe fix + 1 |+from pandas import DataFrame +1 2 | def f(): +2 3 | from pandas import DataFrame +3 4 | +-------------------------------------------------------------------------------- +44 45 | from typing import TYPE_CHECKING +45 46 | +46 47 | if TYPE_CHECKING: +47 |- from pandas import DataFrame + 48 |+ pass +48 49 | +49 50 | def func(value: DataFrame): +50 51 | ... + + diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap index cdd862fa0626a..6c5ead27428ce 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__typing-only-third-party-import_quote.py.snap @@ -1,156 +1,4 @@ --- source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs --- -quote.py:2:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block - | -1 | def f(): -2 | from pandas import DataFrame - | ^^^^^^^^^ TCH002 -3 | -4 | def baz() -> DataFrame: - | - = help: Move into type-checking block - -ℹ Unsafe fix -1 |-def f(): - 1 |+from typing import TYPE_CHECKING - 2 |+ - 3 |+if TYPE_CHECKING: -2 4 | from pandas import DataFrame - 5 |+def f(): -3 6 | -4 |- def baz() -> DataFrame: - 7 |+ def baz() -> "DataFrame": -5 8 | ... -6 9 | -7 10 | - -quote.py:9:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block - | - 8 | def f(): - 9 | from pandas import DataFrame - | ^^^^^^^^^ TCH002 -10 | -11 | def baz() -> DataFrame[int]: - | - = help: Move into type-checking block - -ℹ Unsafe fix - 1 |+from typing import TYPE_CHECKING - 2 |+ - 3 |+if TYPE_CHECKING: - 4 |+ from pandas import DataFrame -1 5 | def f(): -2 6 | from pandas import DataFrame -3 7 | --------------------------------------------------------------------------------- -6 10 | -7 11 | -8 12 | def f(): -9 |- from pandas import DataFrame -10 13 | -11 |- def baz() -> DataFrame[int]: - 14 |+ def baz() -> "DataFrame[int]": -12 15 | ... -13 16 | -14 17 | - -quote.py:16:24: TCH002 Move third-party import `pandas.DataFrame` into a type-checking block - | -15 | def f(): -16 | from pandas import DataFrame - | ^^^^^^^^^ TCH002 -17 | -18 | def baz() -> DataFrame["int"]: - | - = help: Move into type-checking block - -quote.py:23:22: TCH002 [*] Move third-party import `pandas` into a type-checking block - | -22 | def f(): -23 | import pandas as pd - | ^^ TCH002 -24 | -25 | def baz() -> pd.DataFrame: - | - = help: Move into type-checking block - -ℹ Unsafe fix - 1 |+from typing import TYPE_CHECKING - 2 |+ - 3 |+if TYPE_CHECKING: - 4 |+ import pandas as pd -1 5 | def f(): -2 6 | from pandas import DataFrame -3 7 | --------------------------------------------------------------------------------- -20 24 | -21 25 | -22 26 | def f(): -23 |- import pandas as pd -24 27 | -25 |- def baz() -> pd.DataFrame: - 28 |+ def baz() -> "pd.DataFrame": -26 29 | ... -27 30 | -28 31 | - -quote.py:30:22: TCH002 [*] Move third-party import `pandas` into a type-checking block - | -29 | def f(): -30 | import pandas as pd - | ^^ TCH002 -31 | -32 | def baz() -> pd.DataFrame.Extra: - | - = help: Move into type-checking block - -ℹ Unsafe fix - 1 |+from typing import TYPE_CHECKING - 2 |+ - 3 |+if TYPE_CHECKING: - 4 |+ import pandas as pd -1 5 | def f(): -2 6 | from pandas import DataFrame -3 7 | --------------------------------------------------------------------------------- -27 31 | -28 32 | -29 33 | def f(): -30 |- import pandas as pd -31 34 | -32 |- def baz() -> pd.DataFrame.Extra: - 35 |+ def baz() -> "pd.DataFrame.Extra": -33 36 | ... -34 37 | -35 38 | - -quote.py:37:22: TCH002 [*] Move third-party import `pandas` into a type-checking block - | -36 | def f(): -37 | import pandas as pd - | ^^ TCH002 -38 | -39 | def baz() -> pd.DataFrame | int: - | - = help: Move into type-checking block - -ℹ Unsafe fix - 1 |+from typing import TYPE_CHECKING - 2 |+ - 3 |+if TYPE_CHECKING: - 4 |+ import pandas as pd -1 5 | def f(): -2 6 | from pandas import DataFrame -3 7 | --------------------------------------------------------------------------------- -34 38 | -35 39 | -36 40 | def f(): -37 |- import pandas as pd -38 41 | -39 |- def baz() -> pd.DataFrame | int: - 42 |+ def baz() -> "pd.DataFrame | int": -40 43 | ... - diff --git a/pyproject.toml b/pyproject.toml index 981dca4ca291b..2d2b49ebe0807 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -99,3 +99,6 @@ version_files = [ "crates/ruff_shrinking/Cargo.toml", "scripts/benchmarks/pyproject.toml", ] + +[tool.ruff.flake8-type-checking] +annotation-strategy = "quote" diff --git a/ruff.schema.json b/ruff.schema.json index cbff63adcd807..afa8572f28e98 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -699,6 +699,31 @@ }, "additionalProperties": false, "definitions": { + "AnnotationStrategy": { + "oneOf": [ + { + "description": "Avoid changing the semantics of runtime-evaluated annotations (e.g., by quoting them, or inserting `from __future__ import annotations`). Imports will be classified as typing-only or runtime-required based exclusively on the existing type annotations.", + "type": "string", + "enum": [ + "preserve" + ] + }, + { + "description": "Quote runtime-evaluated annotations, if doing so would enable the corresponding import to be moved into an `if TYPE_CHECKING:` block.", + "type": "string", + "enum": [ + "quote" + ] + }, + { + "description": "Insert `from __future__ import annotations` at the top of the file, if doing so would enable an import to be moved into an `if TYPE_CHECKING:` block.", + "type": "string", + "enum": [ + "future" + ] + } + ] + }, "ApiBan": { "type": "object", "required": [ @@ -1184,6 +1209,17 @@ "Flake8TypeCheckingOptions": { "type": "object", "properties": { + "annotation-strategy": { + "description": "The strategy to use when analyzing type annotations that, by default, Python would evaluate at runtime.\n\nFor example, in the following, Python requires that `Sequence` be available at runtime, despite the fact that it's only used in a type annotation: ```python from collections.abc import Sequence\n\ndef func(value: Sequence[int]) -> None: ... ```\n\nIn other words, moving `from collections.abc import Sequence` into an `if TYPE_CHECKING:` block above would cause a runtime error.\n\nBy default, Ruff will respect such runtime evaluations (`\"preserve\"`), but supports two alternative strategies for handling them:\n\n- `\"quote\"`: Add quotes around the annotation (e.g., `\"Sequence[int]\"`), to prevent Python from evaluating it at runtime. Adding quotes around `Sequence` above will allow Ruff to move the import into an `if TYPE_CHECKING:` block. - `\"future\"`: Add a `from __future__ import annotations` import at the top of the file, which will cause Python to treat all annotations as strings, rather than evaluating them at runtime.\n\nSetting `annotation-strategy` to `\"quote\"` or `\"future\"` will allow Ruff to move more imports into type-checking blocks, but may cause issues with other tools that expect annotations to be evaluated at runtime.", + "anyOf": [ + { + "$ref": "#/definitions/AnnotationStrategy" + }, + { + "type": "null" + } + ] + }, "exempt-modules": { "description": "Exempt certain modules from needing to be moved into type-checking blocks.", "type": [ From 4a38344a5284d90ee777c823edd1bf8b4a1dbf6d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 11 Dec 2023 11:41:03 -0500 Subject: [PATCH 4/5] Use boolean --- .../fixtures/flake8_type_checking/quote.py | 17 ++++++++ .../src/rules/flake8_type_checking/helpers.rs | 18 ++++++-- .../src/rules/flake8_type_checking/mod.rs | 3 +- .../runtime_import_in_type_checking_block.rs | 12 ++---- .../rules/typing_only_runtime_import.rs | 18 +++----- .../rules/flake8_type_checking/settings.rs | 24 +---------- ...mport-in-type-checking-block_quote.py.snap | 22 +++++----- ...ping-only-third-party-import_quote.py.snap | 41 ++++++++++++++++++ ...mport-in-type-checking-block_quote.py.snap | 26 +++++------ crates/ruff_workspace/src/options.rs | 37 +++++++--------- pyproject.toml | 3 -- ruff.schema.json | 43 +++---------------- 12 files changed, 133 insertions(+), 131 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py index 041bb3d2b7391..67332e3010f24 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_type_checking/quote.py @@ -40,6 +40,23 @@ def baz() -> pd.DataFrame | int: ... + +def f(): + from pandas import DataFrame + + def baz() -> DataFrame(): + ... + + +def f(): + from typing import Literal + + from pandas import DataFrame + + def baz() -> DataFrame[Literal["int"]]: + ... + + def f(): from typing import TYPE_CHECKING diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs index f502d3519274e..55b333d3f83e2 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs @@ -1,5 +1,6 @@ -use crate::rules::flake8_type_checking::settings::Settings; use anyhow::Result; +use rustc_hash::FxHashSet; + use ruff_diagnostics::Edit; use ruff_python_ast::call_path::from_qualified_name; use ruff_python_ast::helpers::{map_callable, map_subscript}; @@ -8,7 +9,8 @@ use ruff_python_codegen::Stylist; use ruff_python_semantic::{Binding, BindingId, BindingKind, NodeId, SemanticModel}; use ruff_source_file::Locator; use ruff_text_size::Ranged; -use rustc_hash::FxHashSet; + +use crate::rules::flake8_type_checking::settings::Settings; pub(crate) fn is_valid_runtime_import( binding: &Binding, @@ -30,7 +32,7 @@ pub(crate) fn is_valid_runtime_import( || reference.in_typing_only_annotation() || reference.in_complex_string_type_definition() || reference.in_simple_string_type_definition() - || (settings.annotation_strategy.is_quote() + || (settings.quote_annotations && reference.in_runtime_evaluated_annotation())) }) } else { @@ -202,6 +204,8 @@ pub(crate) fn is_singledispatch_implementation( /// - When quoting `kubernetes` in `kubernetes.SecurityContext`, we want `"kubernetes.SecurityContext"`. /// - When quoting `Series` in `Series["pd.Timestamp"]`, we want `"Series[pd.Timestamp]"`. /// - When quoting `Series` in `Series[Literal["pd.Timestamp"]]`, we want `"Series[Literal['pd.Timestamp']]"`. +/// +/// In general, when expanding a component of a call chain, we want to quote the entire call chain. pub(crate) fn quote_annotation( node_id: NodeId, semantic: &SemanticModel, @@ -227,6 +231,14 @@ pub(crate) fn quote_annotation( return quote_annotation(parent_id, semantic, locator, stylist); } } + Expr::Call(parent) => { + if expr == parent.func.as_ref() { + // 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, locator, stylist); + } + } Expr::BinOp(parent) => { if parent.op.is_bit_or() { // If we're quoting the left or right side of a binary operation, we need to 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 754b7e8ae74ac..0f97bc708a446 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -13,7 +13,6 @@ mod tests { use test_case::test_case; use crate::registry::{Linter, Rule}; - use crate::rules::flake8_type_checking::settings::AnnotationStrategy; use crate::test::{test_path, test_snippet}; use crate::{assert_messages, settings}; @@ -63,7 +62,7 @@ mod tests { Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { flake8_type_checking: super::settings::Settings { - annotation_strategy: AnnotationStrategy::Quote, + quote_annotations: true, ..Default::default() }, ..settings::LinterSettings::for_rule(rule_code) diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index 6a766392f1403..08a639f3a427a 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -22,11 +22,10 @@ use crate::rules::flake8_type_checking::imports::ImportBinding; /// The type-checking block is not executed at runtime, so the import will not /// be available at runtime. /// -/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// If [`flake8-type-checking.quote-annotations`] is set to `true`, /// annotations will be wrapped in quotes if doing so would enable the /// corresponding import to remain in the type-checking block. /// -/// /// ## Example /// ```python /// from typing import TYPE_CHECKING @@ -49,7 +48,7 @@ use crate::rules::flake8_type_checking::imports::ImportBinding; /// ``` /// /// ## Options -/// - `flake8-type-checking.annotation-strategy` +/// - `flake8-type-checking.quote-annotations` /// /// ## References /// - [PEP 535](https://peps.python.org/pep-0563/#runtime-annotation-resolution-and-type-checking) @@ -145,18 +144,13 @@ pub(crate) fn runtime_import_in_type_checking_block( } else { // Determine whether the member should be fixed by moving the import out of the // type-checking block, or by quoting its references. - if checker - .settings - .flake8_type_checking - .annotation_strategy - .is_quote() + if checker.settings.flake8_type_checking.quote_annotations && binding.references().all(|reference_id| { let reference = checker.semantic().reference(reference_id); reference.context().is_typing() || reference.in_runtime_evaluated_annotation() }) { - println!("quote"); quotes_by_statement.entry(node_id).or_default().push(import); } else { moves_by_statement.entry(node_id).or_default().push(import); diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index b0184f4642d2b..5e854929e5b7b 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -26,7 +26,7 @@ use crate::rules::isort::{categorize, ImportSection, ImportType}; /// instead be imported conditionally under an `if TYPE_CHECKING:` block to /// minimize runtime overhead. /// -/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// If [`flake8-type-checking.quote-annotations`] is set to `true`, /// annotations will be wrapped in quotes if doing so would enable the /// corresponding import to be moved into an `if TYPE_CHECKING:` block. /// @@ -62,7 +62,7 @@ use crate::rules::isort::{categorize, ImportSection, ImportType}; /// ``` /// /// ## Options -/// - `flake8-type-checking.annotation-strategy` +/// - `flake8-type-checking.quote-annotations` /// - `flake8-type-checking.runtime-evaluated-base-classes` /// - `flake8-type-checking.runtime-evaluated-decorators` /// @@ -99,7 +99,7 @@ impl Violation for TypingOnlyFirstPartyImport { /// instead be imported conditionally under an `if TYPE_CHECKING:` block to /// minimize runtime overhead. /// -/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// If [`flake8-type-checking.quote-annotations`] is set to `true`, /// annotations will be wrapped in quotes if doing so would enable the /// corresponding import to be moved into an `if TYPE_CHECKING:` block. /// @@ -135,7 +135,7 @@ impl Violation for TypingOnlyFirstPartyImport { /// ``` /// /// ## Options -/// - `flake8-type-checking.annotation-strategy` +/// - `flake8-type-checking.quote-annotations` /// - `flake8-type-checking.runtime-evaluated-base-classes` /// - `flake8-type-checking.runtime-evaluated-decorators` /// @@ -172,7 +172,7 @@ impl Violation for TypingOnlyThirdPartyImport { /// instead be imported conditionally under an `if TYPE_CHECKING:` block to /// minimize runtime overhead. /// -/// If [`flake8-type-checking.annotation-strategy`] is set to `"quote"`, +/// If [`flake8-type-checking.quote-annotations`] is set to `true`, /// annotations will be wrapped in quotes if doing so would enable the /// corresponding import to be moved into an `if TYPE_CHECKING:` block. /// @@ -208,7 +208,7 @@ impl Violation for TypingOnlyThirdPartyImport { /// ``` /// /// ## Options -/// - `flake8-type-checking.annotation-strategy` +/// - `flake8-type-checking.quote-annotations` /// - `flake8-type-checking.runtime-evaluated-base-classes` /// - `flake8-type-checking.runtime-evaluated-decorators` /// @@ -281,11 +281,7 @@ pub(crate) fn typing_only_runtime_import( || reference.in_typing_only_annotation() || reference.in_complex_string_type_definition() || reference.in_simple_string_type_definition() - || (checker - .settings - .flake8_type_checking - .annotation_strategy - .is_quote() + || (checker.settings.flake8_type_checking.quote_annotations && reference.in_runtime_evaluated_annotation()) }) { diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs index 5380d16babb58..26730af895d08 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs @@ -1,7 +1,6 @@ //! Settings for the `flake8-type-checking` plugin. use ruff_macros::CacheKey; -use serde::{Deserialize, Serialize}; #[derive(Debug, CacheKey)] pub struct Settings { @@ -9,7 +8,7 @@ pub struct Settings { pub exempt_modules: Vec, pub runtime_evaluated_base_classes: Vec, pub runtime_evaluated_decorators: Vec, - pub annotation_strategy: AnnotationStrategy, + pub quote_annotations: bool, } impl Default for Settings { @@ -19,26 +18,7 @@ impl Default for Settings { exempt_modules: vec!["typing".to_string(), "typing_extensions".to_string()], runtime_evaluated_base_classes: vec![], runtime_evaluated_decorators: vec![], - annotation_strategy: AnnotationStrategy::Preserve, + quote_annotations: false, } } } - -#[derive( - Debug, Default, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, CacheKey, is_macro::Is, -)] -#[serde(deny_unknown_fields, rename_all = "kebab-case")] -#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] -pub enum AnnotationStrategy { - /// Avoid changing the semantics of runtime-evaluated annotations (e.g., by quoting them, or - /// inserting `from __future__ import annotations`). Imports will be classified as typing-only - /// or runtime-required based exclusively on the existing type annotations. - #[default] - Preserve, - /// Quote runtime-evaluated annotations, if doing so would enable the corresponding import to - /// be moved into an `if TYPE_CHECKING:` block. - Quote, - /// Insert `from __future__ import annotations` at the top of the file, if doing so would enable - /// an import to be moved into an `if TYPE_CHECKING:` block. - Future, -} diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap index 7d935c57a185e..5fd78ec8a0497 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap @@ -1,22 +1,22 @@ --- source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs --- -quote.py:47:28: TCH004 [*] Quote references to `pandas.DataFrame`. Import is not available at runtime. +quote.py:64:28: TCH004 [*] Quote references to `pandas.DataFrame`. Import is not available at runtime. | -46 | if TYPE_CHECKING: -47 | from pandas import DataFrame +63 | if TYPE_CHECKING: +64 | from pandas import DataFrame | ^^^^^^^^^ TCH004 -48 | -49 | def func(value: DataFrame): +65 | +66 | def func(value: DataFrame): | = help: Quote references ℹ Unsafe fix -46 46 | if TYPE_CHECKING: -47 47 | from pandas import DataFrame -48 48 | -49 |- def func(value: DataFrame): - 49 |+ def func(value: "DataFrame"): -50 50 | ... +63 63 | if TYPE_CHECKING: +64 64 | from pandas import DataFrame +65 65 | +66 |- def func(value: DataFrame): + 66 |+ def func(value: "DataFrame"): +67 67 | ... diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap index 14df699c48d49..9c43070c0f5b8 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_typing-only-third-party-import_quote.py.snap @@ -155,4 +155,45 @@ quote.py:37:22: TCH002 [*] Move third-party import `pandas` into a type-checking 41 44 | 42 45 | +quote.py:45:24: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block + | +44 | def f(): +45 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +46 | +47 | def baz() -> DataFrame(): + | + = help: Move into type-checking block + +ℹ Unsafe fix + 1 |+from typing import TYPE_CHECKING + 2 |+ + 3 |+if TYPE_CHECKING: + 4 |+ from pandas import DataFrame +1 5 | def f(): +2 6 | from pandas import DataFrame +3 7 | +-------------------------------------------------------------------------------- +42 46 | +43 47 | +44 48 | def f(): +45 |- from pandas import DataFrame +46 49 | +47 |- def baz() -> DataFrame(): + 50 |+ def baz() -> "DataFrame()": +48 51 | ... +49 52 | +50 53 | + +quote.py:54:24: TCH002 Move third-party import `pandas.DataFrame` into a type-checking block + | +52 | from typing import Literal +53 | +54 | from pandas import DataFrame + | ^^^^^^^^^ TCH002 +55 | +56 | def baz() -> DataFrame[Literal["int"]]: + | + = help: Move into type-checking block + diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap index 73d0d948de4a0..c09777853b97a 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_quote.py.snap @@ -1,13 +1,13 @@ --- source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs --- -quote.py:47:28: TCH004 [*] Move import `pandas.DataFrame` out of type-checking block. Import is used for more than type hinting. +quote.py:64:28: TCH004 [*] Move import `pandas.DataFrame` out of type-checking block. Import is used for more than type hinting. | -46 | if TYPE_CHECKING: -47 | from pandas import DataFrame +63 | if TYPE_CHECKING: +64 | from pandas import DataFrame | ^^^^^^^^^ TCH004 -48 | -49 | def func(value: DataFrame): +65 | +66 | def func(value: DataFrame): | = help: Move out of type-checking block @@ -17,13 +17,13 @@ quote.py:47:28: TCH004 [*] Move import `pandas.DataFrame` out of type-checking b 2 3 | from pandas import DataFrame 3 4 | -------------------------------------------------------------------------------- -44 45 | from typing import TYPE_CHECKING -45 46 | -46 47 | if TYPE_CHECKING: -47 |- from pandas import DataFrame - 48 |+ pass -48 49 | -49 50 | def func(value: DataFrame): -50 51 | ... +61 62 | from typing import TYPE_CHECKING +62 63 | +63 64 | if TYPE_CHECKING: +64 |- from pandas import DataFrame + 65 |+ pass +65 66 | +66 67 | def func(value: DataFrame): +67 68 | ... diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 4aae814dd273c..52d71613280f6 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -12,7 +12,6 @@ use ruff_linter::rules::flake8_pytest_style::settings::SettingsError; use ruff_linter::rules::flake8_pytest_style::types; use ruff_linter::rules::flake8_quotes::settings::Quote; use ruff_linter::rules::flake8_tidy_imports::settings::{ApiBan, Strictness}; -use ruff_linter::rules::flake8_type_checking::settings::AnnotationStrategy; use ruff_linter::rules::isort::settings::RelativeImportsOrder; use ruff_linter::rules::isort::{ImportSection, ImportType}; use ruff_linter::rules::pydocstyle::settings::Convention; @@ -1644,8 +1643,8 @@ pub struct Flake8TypeCheckingOptions { )] pub runtime_evaluated_decorators: Option>, - /// The strategy to use when analyzing type annotations that, by default, - /// Python would evaluate at runtime. + /// Whether to add quotes around type annotations, if doing so would allow + /// the corresponding import to be moved into a type-checking block. /// /// For example, in the following, Python requires that `Sequence` be /// available at runtime, despite the fact that it's only used in a type @@ -1660,31 +1659,27 @@ pub struct Flake8TypeCheckingOptions { /// In other words, moving `from collections.abc import Sequence` into an /// `if TYPE_CHECKING:` block above would cause a runtime error. /// - /// By default, Ruff will respect such runtime evaluations (`"preserve"`), - /// but supports two alternative strategies for handling them: + /// By default, Ruff will respect such runtime semantics, and avoid moving + /// the import. /// - /// - `"quote"`: Add quotes around the annotation (e.g., `"Sequence[int]"`), to - /// prevent Python from evaluating it at runtime. Adding quotes around - /// `Sequence` above will allow Ruff to move the import into an - /// `if TYPE_CHECKING:` block. - /// - `"future"`: Add a `from __future__ import annotations` import at the - /// top of the file, which will cause Python to treat all annotations as - /// strings, rather than evaluating them at runtime. + /// However, setting `quote-annotations` to `true` will instruct Ruff + /// to add quotes around the annotation (e.g., `"Sequence[int]"`). Adding + /// quotes around `Sequence`, above, will enable Ruff to move the import + /// into an `if TYPE_CHECKING:` block. /// - /// Setting `annotation-strategy` to `"quote"` or `"future"` will allow - /// Ruff to move more imports into type-checking blocks, but may cause - /// issues with other tools that expect annotations to be evaluated at - /// runtime. + /// Note that this setting has no effect when `from __future__ import annotations` + /// is present, as `__future__` annotations are always treated equivalently + /// to quoted annotations. #[option( - default = "[]", - value_type = r#""preserve" | "quote" | "future""#, + default = "false", + value_type = "bool", example = r#" # Add quotes around type annotations, if doing so would allow # an import to be moved into a type-checking block. - annotation-strategy = "quote" + quote-annotations = true "# )] - pub annotation_strategy: Option, + pub quote_annotations: Option, } impl Flake8TypeCheckingOptions { @@ -1696,7 +1691,7 @@ impl Flake8TypeCheckingOptions { .unwrap_or_else(|| vec!["typing".to_string()]), runtime_evaluated_base_classes: self.runtime_evaluated_base_classes.unwrap_or_default(), runtime_evaluated_decorators: self.runtime_evaluated_decorators.unwrap_or_default(), - annotation_strategy: self.annotation_strategy.unwrap_or_default(), + quote_annotations: self.quote_annotations.unwrap_or_default(), } } } diff --git a/pyproject.toml b/pyproject.toml index 2d2b49ebe0807..981dca4ca291b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -99,6 +99,3 @@ version_files = [ "crates/ruff_shrinking/Cargo.toml", "scripts/benchmarks/pyproject.toml", ] - -[tool.ruff.flake8-type-checking] -annotation-strategy = "quote" diff --git a/ruff.schema.json b/ruff.schema.json index afa8572f28e98..448f0a2cbb71b 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -699,31 +699,6 @@ }, "additionalProperties": false, "definitions": { - "AnnotationStrategy": { - "oneOf": [ - { - "description": "Avoid changing the semantics of runtime-evaluated annotations (e.g., by quoting them, or inserting `from __future__ import annotations`). Imports will be classified as typing-only or runtime-required based exclusively on the existing type annotations.", - "type": "string", - "enum": [ - "preserve" - ] - }, - { - "description": "Quote runtime-evaluated annotations, if doing so would enable the corresponding import to be moved into an `if TYPE_CHECKING:` block.", - "type": "string", - "enum": [ - "quote" - ] - }, - { - "description": "Insert `from __future__ import annotations` at the top of the file, if doing so would enable an import to be moved into an `if TYPE_CHECKING:` block.", - "type": "string", - "enum": [ - "future" - ] - } - ] - }, "ApiBan": { "type": "object", "required": [ @@ -1209,17 +1184,6 @@ "Flake8TypeCheckingOptions": { "type": "object", "properties": { - "annotation-strategy": { - "description": "The strategy to use when analyzing type annotations that, by default, Python would evaluate at runtime.\n\nFor example, in the following, Python requires that `Sequence` be available at runtime, despite the fact that it's only used in a type annotation: ```python from collections.abc import Sequence\n\ndef func(value: Sequence[int]) -> None: ... ```\n\nIn other words, moving `from collections.abc import Sequence` into an `if TYPE_CHECKING:` block above would cause a runtime error.\n\nBy default, Ruff will respect such runtime evaluations (`\"preserve\"`), but supports two alternative strategies for handling them:\n\n- `\"quote\"`: Add quotes around the annotation (e.g., `\"Sequence[int]\"`), to prevent Python from evaluating it at runtime. Adding quotes around `Sequence` above will allow Ruff to move the import into an `if TYPE_CHECKING:` block. - `\"future\"`: Add a `from __future__ import annotations` import at the top of the file, which will cause Python to treat all annotations as strings, rather than evaluating them at runtime.\n\nSetting `annotation-strategy` to `\"quote\"` or `\"future\"` will allow Ruff to move more imports into type-checking blocks, but may cause issues with other tools that expect annotations to be evaluated at runtime.", - "anyOf": [ - { - "$ref": "#/definitions/AnnotationStrategy" - }, - { - "type": "null" - } - ] - }, "exempt-modules": { "description": "Exempt certain modules from needing to be moved into type-checking blocks.", "type": [ @@ -1230,6 +1194,13 @@ "type": "string" } }, + "quote-annotations": { + "description": "Whether to add quotes around type annotations, if doing so would allow the corresponding import to be moved into a type-checking block.\n\nFor example, in the following, Python requires that `Sequence` be available at runtime, despite the fact that it's only used in a type annotation: ```python from collections.abc import Sequence\n\ndef func(value: Sequence[int]) -> None: ... ```\n\nIn other words, moving `from collections.abc import Sequence` into an `if TYPE_CHECKING:` block above would cause a runtime error.\n\nBy default, Ruff will respect such runtime semantics, and avoid moving the import.\n\nHowever, setting `quote-annotations` to `true` will instruct Ruff to add quotes around the annotation (e.g., `\"Sequence[int]\"`). Adding quotes around `Sequence`, above, will enable Ruff to move the import into an `if TYPE_CHECKING:` block.\n\nNote that this setting has no effect when `from __future__ import annotations` is present, as `__future__` annotations are always treated equivalently to quoted annotations.", + "type": [ + "boolean", + "null" + ] + }, "runtime-evaluated-base-classes": { "description": "Exempt classes that list any of the enumerated classes as a base class from needing to be moved into type-checking blocks.\n\nCommon examples include Pydantic's `pydantic.BaseModel` and SQLAlchemy's `sqlalchemy.orm.DeclarativeBase`, but can also support user-defined classes that inherit from those base classes. For example, if you define a common `DeclarativeBase` subclass that's used throughout your project (e.g., `class Base(DeclarativeBase) ...` in `base.py`), you can add it to this list (`runtime-evaluated-base-classes = [\"base.Base\"]`) to exempt models from being moved into type-checking blocks.", "type": [ From 5027f0d1817ca42a4ba036da7e0cc1a4dbe27f9e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 12 Dec 2023 21:46:34 -0500 Subject: [PATCH 5/5] Review feedback --- .../src/checkers/ast/annotation.rs | 66 +++++++++++++++++++ crates/ruff_linter/src/checkers/ast/mod.rs | 58 ++-------------- .../src/rules/flake8_type_checking/helpers.rs | 43 +++++++----- .../src/rules/flake8_type_checking/mod.rs | 6 +- .../runtime_import_in_type_checking_block.rs | 4 +- .../rules/typing_only_runtime_import.rs | 14 +--- .../rules/flake8_type_checking/settings.rs | 8 +-- ...mport-in-type-checking-block_quote.py.snap | 2 +- crates/ruff_python_semantic/src/model.rs | 3 +- crates/ruff_python_semantic/src/reference.rs | 2 +- crates/ruff_workspace/src/options.rs | 31 ++++++--- ruff.schema.json | 2 +- 12 files changed, 136 insertions(+), 103 deletions(-) create mode 100644 crates/ruff_linter/src/checkers/ast/annotation.rs diff --git a/crates/ruff_linter/src/checkers/ast/annotation.rs b/crates/ruff_linter/src/checkers/ast/annotation.rs new file mode 100644 index 0000000000000..aca5fc6c629ff --- /dev/null +++ b/crates/ruff_linter/src/checkers/ast/annotation.rs @@ -0,0 +1,66 @@ +use ruff_python_semantic::{ScopeKind, SemanticModel}; + +use crate::rules::flake8_type_checking; +use crate::settings::LinterSettings; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum AnnotationContext { + /// Python will evaluate the annotation at runtime, but it's not _required_ and, as such, could + /// be quoted to convert it into a typing-only annotation. + /// + /// For example: + /// ```python + /// from pandas import DataFrame + /// + /// def foo() -> DataFrame: + /// ... + /// ``` + /// + /// Above, Python will evaluate `DataFrame` at runtime in order to add it to `__annotations__`. + RuntimeEvaluated, + /// Python will evaluate the annotation at runtime, and it's required to be available at + /// runtime, as a library (like Pydantic) needs access to it. + RuntimeRequired, + /// The annotation is only evaluated at type-checking time. + TypingOnly, +} + +impl AnnotationContext { + pub(super) fn from_model(semantic: &SemanticModel, settings: &LinterSettings) -> Self { + // If the annotation is in a class scope (e.g., an annotated assignment for a + // class field), and that class is marked as annotation as runtime-required. + if semantic + .current_scope() + .kind + .as_class() + .is_some_and(|class_def| { + flake8_type_checking::helpers::runtime_required_class( + class_def, + &settings.flake8_type_checking.runtime_required_base_classes, + &settings.flake8_type_checking.runtime_required_decorators, + semantic, + ) + }) + { + return Self::RuntimeRequired; + } + + // If `__future__` annotations are enabled, then annotations are never evaluated + // at runtime, so we can treat them as typing-only. + if semantic.future_annotations() { + return Self::TypingOnly; + } + + // Otherwise, if we're in a class or module scope, then the annotation needs to + // be available at runtime. + // See: https://docs.python.org/3/reference/simple_stmts.html#annotated-assignment-statements + if matches!( + semantic.current_scope().kind, + ScopeKind::Class(_) | ScopeKind::Module + ) { + return Self::RuntimeEvaluated; + } + + Self::TypingOnly + } +} diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 41f5dcc17f43c..1c510f1cbb8d9 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -58,6 +58,7 @@ use ruff_python_semantic::{ use ruff_python_stdlib::builtins::{IPYTHON_BUILTINS, MAGIC_GLOBALS, PYTHON_BUILTINS}; use ruff_source_file::Locator; +use crate::checkers::ast::annotation::AnnotationContext; use crate::checkers::ast::deferred::Deferred; use crate::docstrings::extraction::ExtractionTarget; use crate::importer::Importer; @@ -68,6 +69,7 @@ use crate::settings::{flags, LinterSettings}; use crate::{docstrings, noqa}; mod analyze; +mod annotation; mod deferred; pub(crate) struct Checker<'a> { @@ -679,62 +681,14 @@ where value, .. }) => { - enum AnnotationKind { - RuntimeRequired, - RuntimeEvaluated, - TypingOnly, - } - - fn annotation_kind( - semantic: &SemanticModel, - settings: &LinterSettings, - ) -> AnnotationKind { - // If the annotation is in a class, and that class is marked as - // runtime-evaluated, treat the annotation as runtime-required. - // TODO(charlie): We could also include function calls here. - if semantic - .current_scope() - .kind - .as_class() - .is_some_and(|class_def| { - flake8_type_checking::helpers::runtime_required_class( - class_def, - &settings.flake8_type_checking.runtime_evaluated_base_classes, - &settings.flake8_type_checking.runtime_evaluated_decorators, - semantic, - ) - }) - { - return AnnotationKind::RuntimeRequired; - } - - // If `__future__` annotations are enabled, then annotations are never evaluated - // at runtime, so we can treat them as typing-only. - if semantic.future_annotations() { - return AnnotationKind::TypingOnly; - } - - // Otherwise, if we're in a class or module scope, then the annotation needs to - // be available at runtime. - // See: https://docs.python.org/3/reference/simple_stmts.html#annotated-assignment-statements - if matches!( - semantic.current_scope().kind, - ScopeKind::Class(_) | ScopeKind::Module - ) { - return AnnotationKind::RuntimeEvaluated; - } - - AnnotationKind::TypingOnly - } - - match annotation_kind(&self.semantic, self.settings) { - AnnotationKind::RuntimeRequired => { + match AnnotationContext::from_model(&self.semantic, self.settings) { + AnnotationContext::RuntimeRequired => { self.visit_runtime_required_annotation(annotation); } - AnnotationKind::RuntimeEvaluated => { + AnnotationContext::RuntimeEvaluated => { self.visit_runtime_evaluated_annotation(annotation); } - AnnotationKind::TypingOnly => self.visit_annotation(annotation), + AnnotationContext::TypingOnly => self.visit_annotation(annotation), } if let Some(expr) = value { diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs index 55b333d3f83e2..f238198ec1e88 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/helpers.rs @@ -6,12 +6,25 @@ use ruff_python_ast::call_path::from_qualified_name; use ruff_python_ast::helpers::{map_callable, map_subscript}; use ruff_python_ast::{self as ast, Expr}; use ruff_python_codegen::Stylist; -use ruff_python_semantic::{Binding, BindingId, BindingKind, NodeId, SemanticModel}; +use ruff_python_semantic::{ + Binding, BindingId, BindingKind, NodeId, ResolvedReference, SemanticModel, +}; use ruff_source_file::Locator; use ruff_text_size::Ranged; use crate::rules::flake8_type_checking::settings::Settings; +/// Returns `true` if the [`ResolvedReference`] is in a typing-only context _or_ a runtime-evaluated +/// context (with quoting enabled). +pub(crate) fn is_typing_reference(reference: &ResolvedReference, settings: &Settings) -> bool { + reference.in_type_checking_block() + || reference.in_typing_only_annotation() + || reference.in_complex_string_type_definition() + || reference.in_simple_string_type_definition() + || (settings.quote_annotations && reference.in_runtime_evaluated_annotation()) +} + +/// Returns `true` if the [`Binding`] represents a runtime-required import. pub(crate) fn is_valid_runtime_import( binding: &Binding, semantic: &SemanticModel, @@ -25,16 +38,7 @@ pub(crate) fn is_valid_runtime_import( && binding .references() .map(|reference_id| semantic.reference(reference_id)) - .any(|reference| { - // Are we in a typing-only context _or_ a runtime-evaluated context? We're - // willing to quote runtime-evaluated, but not runtime-required annotations. - !(reference.in_type_checking_block() - || reference.in_typing_only_annotation() - || reference.in_complex_string_type_definition() - || reference.in_simple_string_type_definition() - || (settings.quote_annotations - && reference.in_runtime_evaluated_annotation())) - }) + .any(|reference| !is_typing_reference(reference, settings)) } else { false } @@ -212,10 +216,10 @@ pub(crate) fn quote_annotation( locator: &Locator, stylist: &Stylist, ) -> Result { - let expr = semantic.expression(node_id); + 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) { - Expr::Subscript(parent) => { + Some(Expr::Subscript(parent)) => { if expr == parent.value.as_ref() { // 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 @@ -223,7 +227,7 @@ pub(crate) fn quote_annotation( return quote_annotation(parent_id, semantic, locator, stylist); } } - Expr::Attribute(parent) => { + Some(Expr::Attribute(parent)) => { if expr == parent.value.as_ref() { // 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 @@ -231,7 +235,7 @@ pub(crate) fn quote_annotation( return quote_annotation(parent_id, semantic, locator, stylist); } } - Expr::Call(parent) => { + Some(Expr::Call(parent)) => { if expr == parent.func.as_ref() { // If we're quoting the function of a call, we need to quote the entire // expression. For example, when quoting `DataFrame` in `DataFrame()`, we @@ -239,7 +243,7 @@ pub(crate) fn quote_annotation( return quote_annotation(parent_id, semantic, locator, stylist); } } - Expr::BinOp(parent) => { + Some(Expr::BinOp(parent)) => { if parent.op.is_bit_or() { // 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 @@ -253,7 +257,12 @@ pub(crate) fn quote_annotation( let annotation = locator.slice(expr); - // If the annotation already contains a quote, avoid attempting to re-quote it. + // If the annotation already contains a quote, avoid attempting to re-quote it. For example: + // ```python + // from typing import Literal + // + // Set[Literal["Foo"]] + // ``` if annotation.contains('\'') || annotation.contains('"') { return Err(anyhow::anyhow!("Annotation already contains a quote")); } 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 0f97bc708a446..5e5a05cb6ff0e 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -130,7 +130,7 @@ mod tests { Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { flake8_type_checking: super::settings::Settings { - runtime_evaluated_base_classes: vec![ + runtime_required_base_classes: vec![ "pydantic.BaseModel".to_string(), "sqlalchemy.orm.DeclarativeBase".to_string(), ], @@ -161,7 +161,7 @@ mod tests { Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { flake8_type_checking: super::settings::Settings { - runtime_evaluated_decorators: vec![ + runtime_required_decorators: vec![ "attrs.define".to_string(), "attrs.frozen".to_string(), ], @@ -186,7 +186,7 @@ mod tests { Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { flake8_type_checking: super::settings::Settings { - runtime_evaluated_base_classes: vec!["module.direct.MyBaseClass".to_string()], + runtime_required_base_classes: vec!["module.direct.MyBaseClass".to_string()], ..Default::default() }, ..settings::LinterSettings::for_rule(rule_code) diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index 08a639f3a427a..0bec7f317009c 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -72,7 +72,7 @@ impl Violation for RuntimeImportInTypeCheckingBlock { "Move import `{qualified_name}` out of type-checking block. Import is used for more than type hinting." ), Strategy::QuoteUsages => format!( - "Quote references to `{qualified_name}`. Import is not available at runtime." + "Quote references to `{qualified_name}`. Import is in a type-checking block." ), } } @@ -249,7 +249,7 @@ fn quote_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) let reference = checker.semantic().reference(*reference_id); if reference.context().is_runtime() { Some(quote_annotation( - reference.node_id()?, + reference.expression_id()?, checker.semantic(), checker.locator(), checker.stylist(), diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 5e854929e5b7b..cb3adaed08368 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -12,7 +12,7 @@ use crate::checkers::ast::Checker; use crate::codes::Rule; use crate::fix; use crate::importer::ImportedMembers; -use crate::rules::flake8_type_checking::helpers::quote_annotation; +use crate::rules::flake8_type_checking::helpers::{is_typing_reference, quote_annotation}; use crate::rules::flake8_type_checking::imports::ImportBinding; use crate::rules::isort::{categorize, ImportSection, ImportType}; @@ -274,15 +274,7 @@ pub(crate) fn typing_only_runtime_import( .references() .map(|reference_id| checker.semantic().reference(reference_id)) .all(|reference| { - // All references should be in a typing context _or_ a runtime-evaluated - // annotation (as opposed to a runtime-required annotation), which we can - // quote. - reference.in_type_checking_block() - || reference.in_typing_only_annotation() - || reference.in_complex_string_type_definition() - || reference.in_simple_string_type_definition() - || (checker.settings.flake8_type_checking.quote_annotations - && reference.in_runtime_evaluated_annotation()) + is_typing_reference(reference, &checker.settings.flake8_type_checking) }) { let qualified_name = import.qualified_name(); @@ -497,7 +489,7 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> let reference = checker.semantic().reference(*reference_id); if reference.context().is_runtime() { Some(quote_annotation( - reference.node_id()?, + reference.expression_id()?, checker.semantic(), checker.locator(), checker.stylist(), diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs index 26730af895d08..16baf1b91edbe 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/settings.rs @@ -6,8 +6,8 @@ use ruff_macros::CacheKey; pub struct Settings { pub strict: bool, pub exempt_modules: Vec, - pub runtime_evaluated_base_classes: Vec, - pub runtime_evaluated_decorators: Vec, + pub runtime_required_base_classes: Vec, + pub runtime_required_decorators: Vec, pub quote_annotations: bool, } @@ -16,8 +16,8 @@ impl Default for Settings { Self { strict: false, exempt_modules: vec!["typing".to_string(), "typing_extensions".to_string()], - runtime_evaluated_base_classes: vec![], - runtime_evaluated_decorators: vec![], + runtime_required_base_classes: vec![], + runtime_required_decorators: vec![], quote_annotations: false, } } diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap index 5fd78ec8a0497..0baeba9f62ec1 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap +++ b/crates/ruff_linter/src/rules/flake8_type_checking/snapshots/ruff_linter__rules__flake8_type_checking__tests__quote_runtime-import-in-type-checking-block_quote.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/flake8_type_checking/mod.rs --- -quote.py:64:28: TCH004 [*] Quote references to `pandas.DataFrame`. Import is not available at runtime. +quote.py:64:28: TCH004 [*] Quote references to `pandas.DataFrame`. Import is in a type-checking block. | 63 | if TYPE_CHECKING: 64 | from pandas import DataFrame diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index b1a63fd1f9d8c..dbcc021b71f22 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -995,11 +995,10 @@ impl<'a> SemanticModel<'a> { /// Return the [`Expr`] corresponding to the given [`NodeId`]. #[inline] - pub fn expression(&self, node_id: NodeId) -> &'a Expr { + pub fn expression(&self, node_id: NodeId) -> Option<&'a Expr> { self.nodes .ancestor_ids(node_id) .find_map(|id| self.nodes[id].as_expression()) - .expect("No expression found") } /// Given a [`Expr`], return its parent, if any. diff --git a/crates/ruff_python_semantic/src/reference.rs b/crates/ruff_python_semantic/src/reference.rs index ae6d2825d0504..6bb807e1c8523 100644 --- a/crates/ruff_python_semantic/src/reference.rs +++ b/crates/ruff_python_semantic/src/reference.rs @@ -26,7 +26,7 @@ pub struct ResolvedReference { impl ResolvedReference { /// The expression that the reference occurs in. - pub const fn node_id(&self) -> Option { + pub const fn expression_id(&self) -> Option { self.node_id } diff --git a/crates/ruff_workspace/src/options.rs b/crates/ruff_workspace/src/options.rs index 52d71613280f6..c0ce2c2e79f98 100644 --- a/crates/ruff_workspace/src/options.rs +++ b/crates/ruff_workspace/src/options.rs @@ -1649,23 +1649,36 @@ pub struct Flake8TypeCheckingOptions { /// For example, in the following, Python requires that `Sequence` be /// available at runtime, despite the fact that it's only used in a type /// annotation: + /// /// ```python /// from collections.abc import Sequence /// + /// /// def func(value: Sequence[int]) -> None: /// ... /// ``` /// /// In other words, moving `from collections.abc import Sequence` into an - /// `if TYPE_CHECKING:` block above would cause a runtime error. + /// `if TYPE_CHECKING:` block above would cause a runtime error, as the + /// type would no longer be available at runtime. + /// + /// By default, Ruff will respect such runtime semantics and avoid moving + /// the import to prevent such runtime errors. /// - /// By default, Ruff will respect such runtime semantics, and avoid moving - /// the import. + /// Setting `quote-annotations` to `true` will instruct Ruff to add quotes + /// around the annotation (e.g., `"Sequence[int]"`), which in turn enables + /// Ruff to move the import into an `if TYPE_CHECKING:` block, like so: + /// + /// ```python + /// from typing import TYPE_CHECKING /// - /// However, setting `quote-annotations` to `true` will instruct Ruff - /// to add quotes around the annotation (e.g., `"Sequence[int]"`). Adding - /// quotes around `Sequence`, above, will enable Ruff to move the import - /// into an `if TYPE_CHECKING:` block. + /// if TYPE_CHECKING: + /// from collections.abc import Sequence + /// + /// + /// def func(value: "Sequence[int]") -> None: + /// ... + /// ``` /// /// Note that this setting has no effect when `from __future__ import annotations` /// is present, as `__future__` annotations are always treated equivalently @@ -1689,8 +1702,8 @@ impl Flake8TypeCheckingOptions { exempt_modules: self .exempt_modules .unwrap_or_else(|| vec!["typing".to_string()]), - runtime_evaluated_base_classes: self.runtime_evaluated_base_classes.unwrap_or_default(), - runtime_evaluated_decorators: self.runtime_evaluated_decorators.unwrap_or_default(), + runtime_required_base_classes: self.runtime_evaluated_base_classes.unwrap_or_default(), + runtime_required_decorators: self.runtime_evaluated_decorators.unwrap_or_default(), quote_annotations: self.quote_annotations.unwrap_or_default(), } } diff --git a/ruff.schema.json b/ruff.schema.json index 448f0a2cbb71b..8f67f9a12d787 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1195,7 +1195,7 @@ } }, "quote-annotations": { - "description": "Whether to add quotes around type annotations, if doing so would allow the corresponding import to be moved into a type-checking block.\n\nFor example, in the following, Python requires that `Sequence` be available at runtime, despite the fact that it's only used in a type annotation: ```python from collections.abc import Sequence\n\ndef func(value: Sequence[int]) -> None: ... ```\n\nIn other words, moving `from collections.abc import Sequence` into an `if TYPE_CHECKING:` block above would cause a runtime error.\n\nBy default, Ruff will respect such runtime semantics, and avoid moving the import.\n\nHowever, setting `quote-annotations` to `true` will instruct Ruff to add quotes around the annotation (e.g., `\"Sequence[int]\"`). Adding quotes around `Sequence`, above, will enable Ruff to move the import into an `if TYPE_CHECKING:` block.\n\nNote that this setting has no effect when `from __future__ import annotations` is present, as `__future__` annotations are always treated equivalently to quoted annotations.", + "description": "Whether to add quotes around type annotations, if doing so would allow the corresponding import to be moved into a type-checking block.\n\nFor example, in the following, Python requires that `Sequence` be available at runtime, despite the fact that it's only used in a type annotation:\n\n```python from collections.abc import Sequence\n\ndef func(value: Sequence[int]) -> None: ... ```\n\nIn other words, moving `from collections.abc import Sequence` into an `if TYPE_CHECKING:` block above would cause a runtime error, as the type would no longer be available at runtime.\n\nBy default, Ruff will respect such runtime semantics and avoid moving the import to prevent such runtime errors.\n\nSetting `quote-annotations` to `true` will instruct Ruff to add quotes around the annotation (e.g., `\"Sequence[int]\"`), which in turn enables Ruff to move the import into an `if TYPE_CHECKING:` block, like so:\n\n```python from typing import TYPE_CHECKING\n\nif TYPE_CHECKING: from collections.abc import Sequence\n\ndef func(value: \"Sequence[int]\") -> None: ... ```\n\nNote that this setting has no effect when `from __future__ import annotations` is present, as `__future__` annotations are always treated equivalently to quoted annotations.", "type": [ "boolean", "null"