diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 25b2e30af02cc4..95c78407ae6c17 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -5064,22 +5064,19 @@ impl<'a> Checker<'a> { .copied() .collect() }; - for binding_id in scope.binding_ids() { - let binding = &self.semantic_model.bindings[binding_id]; - flake8_type_checking::rules::runtime_import_in_type_checking_block( - self, - binding, - &mut diagnostics, - ); + flake8_type_checking::rules::runtime_import_in_type_checking_block( + self, + scope, + &mut diagnostics, + ); - flake8_type_checking::rules::typing_only_runtime_import( - self, - binding, - &runtime_imports, - &mut diagnostics, - ); - } + flake8_type_checking::rules::typing_only_runtime_import( + self, + scope, + &runtime_imports, + &mut diagnostics, + ); } if self.enabled(Rule::UnusedImport) { diff --git a/crates/ruff/src/importer/mod.rs b/crates/ruff/src/importer/mod.rs index ba923aed3f9be6..aca15cd632d2ac 100644 --- a/crates/ruff/src/importer/mod.rs +++ b/crates/ruff/src/importer/mod.rs @@ -83,12 +83,12 @@ impl<'a> Importer<'a> { /// import statement. pub(crate) fn runtime_import_edit( &self, - import: &StmtImport, + import: &StmtImports, at: TextSize, ) -> Result { // Generate the modified import statement. let content = autofix::codemods::retain_imports( - &[import.qualified_name], + &import.qualified_names, import.stmt, self.locator, self.stylist, @@ -114,13 +114,13 @@ impl<'a> Importer<'a> { /// `TYPE_CHECKING` block. pub(crate) fn typing_import_edit( &self, - import: &StmtImport, + import: &StmtImports, at: TextSize, semantic_model: &SemanticModel, ) -> Result { // Generate the modified import statement. let content = autofix::codemods::retain_imports( - &[import.qualified_name], + &import.qualified_names, import.stmt, self.locator, self.stylist, @@ -442,12 +442,12 @@ impl<'a> ImportRequest<'a> { } } -/// An existing module or member import, located within an import statement. -pub(crate) struct StmtImport<'a> { +/// An existing list of module or member imports, located within an import statement. +pub(crate) struct StmtImports<'a> { /// The import statement. pub(crate) stmt: &'a Stmt, - /// The "full name" of the imported module or member. - pub(crate) qualified_name: &'a str, + /// The "qualified names" of the imported modules or members. + pub(crate) qualified_names: Vec<&'a str>, } /// The result of an [`Importer::get_or_import_symbol`] call. diff --git a/crates/ruff/src/rules/flake8_type_checking/mod.rs b/crates/ruff/src/rules/flake8_type_checking/mod.rs index 864385a306a3ed..06ea6ab839a0cd 100644 --- a/crates/ruff/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff/src/rules/flake8_type_checking/mod.rs @@ -282,6 +282,48 @@ mod tests { "#, "import_from_type_checking_block" )] + #[test_case( + r#" + from __future__ import annotations + + from typing import TYPE_CHECKING + + from pandas import ( + DataFrame, # DataFrame + Series, # Series + ) + + def f(x: DataFrame, y: Series): + pass + "#, + "multiple_members" + )] + #[test_case( + r#" + from __future__ import annotations + + from typing import TYPE_CHECKING + + import os, sys + + def f(x: os, y: sys): + pass + "#, + "multiple_modules_same_type" + )] + #[test_case( + r#" + from __future__ import annotations + + from typing import TYPE_CHECKING + + import os, pandas + + def f(x: os, y: pandas): + pass + "#, + "multiple_modules_different_types" + )] fn contents(contents: &str, snapshot: &str) { let diagnostics = test_snippet( contents, diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs b/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs index a2da8d1c988f34..1fa1b471523e34 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs @@ -1,11 +1,17 @@ +use anyhow::Result; +use ruff_text_size::TextRange; +use rustc_hash::FxHashMap; + use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::binding::Binding; +use ruff_python_semantic::node::NodeId; +use ruff_python_semantic::reference::ReferenceId; +use ruff_python_semantic::scope::Scope; use crate::autofix; use crate::checkers::ast::Checker; -use crate::importer::StmtImport; -use crate::registry::AsRule; +use crate::codes::Rule; +use crate::importer::StmtImports; /// ## What it does /// Checks for runtime imports defined in a type-checking block. @@ -61,72 +67,172 @@ impl Violation for RuntimeImportInTypeCheckingBlock { /// TCH004 pub(crate) fn runtime_import_in_type_checking_block( checker: &Checker, - binding: &Binding, + scope: &Scope, diagnostics: &mut Vec, ) { - let Some(qualified_name) = binding.qualified_name() else { - return; - }; + // Collect all runtime imports by statement. + let mut errors_by_statement: FxHashMap> = FxHashMap::default(); + let mut ignores_by_statement: FxHashMap> = FxHashMap::default(); - let Some(reference_id) = binding.references.first() else { - return; - }; + for binding_id in scope.binding_ids() { + let binding = &checker.semantic_model().bindings[binding_id]; - if binding.context.is_typing() - && binding.references().any(|reference_id| { - checker - .semantic_model() - .references - .resolve(reference_id) - .context() - .is_runtime() - }) + let Some(qualified_name) = binding.qualified_name() else { + continue; + }; + + let Some(reference_id) = binding.references.first().copied() else { + continue; + }; + + if binding.context.is_typing() + && binding.references().any(|reference_id| { + checker + .semantic_model() + .references + .resolve(reference_id) + .context() + .is_runtime() + }) + { + let Some(stmt_id) = binding.source else { + continue; + }; + + let import = Import { + qualified_name, + reference_id, + trimmed_range: binding.trimmed_range(checker.semantic_model(), checker.locator), + parent_range: binding.parent_range(checker.semantic_model()), + }; + + if checker.rule_is_ignored( + Rule::RuntimeImportInTypeCheckingBlock, + import.trimmed_range.start(), + ) || import.parent_range.map_or(false, |parent_range| { + checker + .rule_is_ignored(Rule::RuntimeImportInTypeCheckingBlock, parent_range.start()) + }) { + ignores_by_statement + .entry(stmt_id) + .or_default() + .push(import); + } else { + errors_by_statement.entry(stmt_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 (stmt_id, imports) in errors_by_statement { + let fix = if checker.patch(Rule::RuntimeImportInTypeCheckingBlock) { + fix_imports(checker, stmt_id, &imports).ok() + } else { + None + }; + + for Import { + qualified_name, + trimmed_range, + parent_range, + .. + } in imports + { + let mut diagnostic = Diagnostic::new( + RuntimeImportInTypeCheckingBlock { + qualified_name: qualified_name.to_string(), + }, + trimmed_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); + } + } + + // Separately, generate a diagnostic for every _ignored_ import, to ensure that the + // suppression comments aren't marked as unused. + for Import { + qualified_name, + trimmed_range, + parent_range, + .. + } in ignores_by_statement.into_values().flatten() { let mut diagnostic = Diagnostic::new( RuntimeImportInTypeCheckingBlock { qualified_name: qualified_name.to_string(), }, - binding.trimmed_range(checker.semantic_model(), checker.locator), + trimmed_range, ); - if let Some(range) = binding.parent_range(checker.semantic_model()) { + if let Some(range) = parent_range { diagnostic.set_parent(range.start()); } + diagnostics.push(diagnostic); + } +} - if checker.patch(diagnostic.kind.rule()) { - diagnostic.try_set_fix(|| { - // Step 1) Remove the import. - // SAFETY: All non-builtin bindings have a source. - let source = binding.source.unwrap(); - let stmt = checker.semantic_model().stmts[source]; - let parent = checker.semantic_model().stmts.parent(stmt); - let remove_import_edit = autofix::edits::remove_unused_imports( - std::iter::once(qualified_name), - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - )?; - - // Step 2) Add the import to the top-level. - let reference = checker.semantic_model().references.resolve(*reference_id); - let add_import_edit = checker.importer.runtime_import_edit( - &StmtImport { - stmt, - qualified_name, - }, - reference.range().start(), - )?; - - Ok( - Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()) - .isolate(checker.isolation(parent)), - ) - }); - } +/// A runtime-required import with its surrounding context. +struct Import<'a> { + /// The qualified name of the import. + qualified_name: &'a str, + /// The first reference to the imported symbol. + reference_id: ReferenceId, + /// The trimmed range of the import. + trimmed_range: TextRange, + /// The range of the import's parent statement. + parent_range: Option, +} - if checker.enabled(diagnostic.kind.rule()) { - diagnostics.push(diagnostic); - } - } +/// Generate a [`Fix`] to remove runtime imports from a type-checking block. +fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result { + let stmt = checker.semantic_model().stmts[stmt_id]; + let parent = checker.semantic_model().stmts.parent(stmt); + let qualified_names: Vec<&str> = imports + .iter() + .map(|Import { qualified_name, .. }| *qualified_name) + .collect(); + + // Find the first reference across all imports. + let at = imports + .iter() + .map(|Import { reference_id, .. }| { + checker + .semantic_model() + .references + .resolve(*reference_id) + .range() + .start() + }) + .min() + .expect("Expected at least one import"); + + // Step 1) Remove the import. + let remove_import_edit = autofix::edits::remove_unused_imports( + qualified_names.iter().copied(), + stmt, + parent, + checker.locator, + checker.indexer, + checker.stylist, + )?; + + // Step 2) Add the import to the top-level. + let add_import_edit = checker.importer.runtime_import_edit( + &StmtImports { + stmt, + qualified_names, + }, + at, + )?; + + Ok( + Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()) + .isolate(checker.isolation(parent)), + ) } diff --git a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs index 085c565271973c..93b5d2d93057f8 100644 --- a/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs +++ b/crates/ruff/src/rules/flake8_type_checking/rules/typing_only_runtime_import.rs @@ -1,11 +1,18 @@ +use anyhow::Result; +use ruff_text_size::TextRange; +use rustc_hash::FxHashMap; + use ruff_diagnostics::{AutofixKind, Diagnostic, DiagnosticKind, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_semantic::binding::Binding; +use ruff_python_semantic::node::NodeId; +use ruff_python_semantic::reference::ReferenceId; +use ruff_python_semantic::scope::Scope; use crate::autofix; use crate::checkers::ast::Checker; -use crate::importer::StmtImport; -use crate::registry::AsRule; +use crate::codes::Rule; +use crate::importer::StmtImports; use crate::rules::isort::{categorize, ImportSection, ImportType}; /// ## What it does @@ -176,6 +183,211 @@ impl Violation for TypingOnlyStandardLibraryImport { } } +/// TCH001, TCH002, TCH003 +pub(crate) fn typing_only_runtime_import( + checker: &Checker, + scope: &Scope, + runtime_imports: &[&Binding], + diagnostics: &mut Vec, +) { + // Collect all typing-only imports by statement and import type. + let mut errors_by_statement: FxHashMap<(NodeId, ImportType), Vec> = + FxHashMap::default(); + let mut ignores_by_statement: FxHashMap<(NodeId, ImportType), Vec> = + FxHashMap::default(); + + for binding_id in scope.binding_ids() { + let binding = &checker.semantic_model().bindings[binding_id]; + + // If we're in un-strict mode, don't flag typing-only imports that are + // implicitly loaded by way of a valid runtime import. + if !checker.settings.flake8_type_checking.strict + && runtime_imports + .iter() + .any(|import| is_implicit_import(binding, import)) + { + continue; + } + + let Some(qualified_name) = binding.qualified_name() else { + continue; + }; + + if is_exempt( + qualified_name, + &checker + .settings + .flake8_type_checking + .exempt_modules + .iter() + .map(String::as_str) + .collect::>(), + ) { + continue; + } + + let Some(reference_id) = binding.references.first().copied() else { + continue; + }; + + if binding.context.is_runtime() + && binding.references().all(|reference_id| { + checker + .semantic_model() + .references + .resolve(reference_id) + .context() + .is_typing() + }) + { + // Extract the module base and level from the full name. + // Ex) `foo.bar.baz` -> `foo`, `0` + // Ex) `.foo.bar.baz` -> `foo`, `1` + let level = qualified_name + .chars() + .take_while(|c| *c == '.') + .count() + .try_into() + .unwrap(); + + // Categorize the import, using coarse-grained categorization. + let import_type = match categorize( + qualified_name, + Some(level), + &checker.settings.src, + checker.package(), + &checker.settings.isort.known_modules, + checker.settings.target_version, + ) { + ImportSection::Known(ImportType::LocalFolder | ImportType::FirstParty) => { + ImportType::FirstParty + } + ImportSection::Known(ImportType::ThirdParty) | ImportSection::UserDefined(_) => { + ImportType::ThirdParty + } + ImportSection::Known(ImportType::StandardLibrary) => ImportType::StandardLibrary, + ImportSection::Known(ImportType::Future) => { + continue; + } + }; + + if !checker.enabled(rule_for(import_type)) { + continue; + } + + let Some(stmt_id) = binding.source else { + continue; + }; + + let import = Import { + qualified_name, + reference_id, + trimmed_range: binding.trimmed_range(checker.semantic_model(), checker.locator), + parent_range: binding.parent_range(checker.semantic_model()), + }; + + if checker.rule_is_ignored(rule_for(import_type), import.trimmed_range.start()) + || import.parent_range.map_or(false, |parent_range| { + checker.rule_is_ignored(rule_for(import_type), parent_range.start()) + }) + { + ignores_by_statement + .entry((stmt_id, import_type)) + .or_default() + .push(import); + } else { + errors_by_statement + .entry((stmt_id, import_type)) + .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 ((stmt_id, import_type), imports) in errors_by_statement { + let fix = if checker.patch(rule_for(import_type)) { + fix_imports(checker, stmt_id, &imports).ok() + } else { + None + }; + + for Import { + qualified_name, + trimmed_range, + parent_range, + .. + } in imports + { + let mut diagnostic = Diagnostic::new( + diagnostic_for(import_type, qualified_name.to_string()), + trimmed_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); + } + } + + // Separately, generate a diagnostic for every _ignored_ import, to ensure that the + // suppression comments aren't marked as unused. + for ((_, import_type), imports) in ignores_by_statement { + for Import { + qualified_name, + trimmed_range, + parent_range, + .. + } in imports + { + let mut diagnostic = Diagnostic::new( + diagnostic_for(import_type, qualified_name.to_string()), + trimmed_range, + ); + if let Some(range) = parent_range { + diagnostic.set_parent(range.start()); + } + diagnostics.push(diagnostic); + } + } +} + +/// A runtime-required import with its surrounding context. +struct Import<'a> { + /// The qualified name of the import. + qualified_name: &'a str, + /// The first reference to the imported symbol. + reference_id: ReferenceId, + /// The trimmed range of the import. + trimmed_range: TextRange, + /// The range of the import's parent statement. + parent_range: Option, +} + +/// Return the [`Rule`] for the given import type. +fn rule_for(import_type: ImportType) -> Rule { + match import_type { + ImportType::StandardLibrary => Rule::TypingOnlyStandardLibraryImport, + ImportType::ThirdParty => Rule::TypingOnlyThirdPartyImport, + ImportType::FirstParty => Rule::TypingOnlyFirstPartyImport, + _ => unreachable!("Unexpected import type"), + } +} + +/// Return the [`Diagnostic`] for the given import type. +fn diagnostic_for(import_type: ImportType, qualified_name: String) -> DiagnosticKind { + match import_type { + ImportType::StandardLibrary => TypingOnlyStandardLibraryImport { qualified_name }.into(), + ImportType::ThirdParty => TypingOnlyThirdPartyImport { qualified_name }.into(), + ImportType::FirstParty => TypingOnlyFirstPartyImport { qualified_name }.into(), + _ => unreachable!("Unexpected import type"), + } +} + /// Return `true` if `this` is implicitly loaded via importing `that`. fn is_implicit_import(this: &Binding, that: &Binding) -> bool { let Some(this_module) = this.module_name() else { @@ -203,140 +415,51 @@ fn is_exempt(name: &str, exempt_modules: &[&str]) -> bool { } } -/// TCH001, TCH002, TCH003 -pub(crate) fn typing_only_runtime_import( - checker: &Checker, - binding: &Binding, - runtime_imports: &[&Binding], - diagnostics: &mut Vec, -) { - // If we're in un-strict mode, don't flag typing-only imports that are - // implicitly loaded by way of a valid runtime import. - if !checker.settings.flake8_type_checking.strict - && runtime_imports - .iter() - .any(|import| is_implicit_import(binding, import)) - { - return; - } - - let Some(qualified_name) = binding.qualified_name() else { - return; - }; - - if is_exempt( - qualified_name, - &checker - .settings - .flake8_type_checking - .exempt_modules - .iter() - .map(String::as_str) - .collect::>(), - ) { - return; - } - - let Some(reference_id) = binding.references.first() else { - return; - }; +/// Generate a [`Fix`] to remove typing-only imports from a runtime context. +fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result { + let stmt = checker.semantic_model().stmts[stmt_id]; + let parent = checker.semantic_model().stmts.parent(stmt); + let qualified_names: Vec<&str> = imports + .iter() + .map(|Import { qualified_name, .. }| *qualified_name) + .collect(); - if binding.context.is_runtime() - && binding.is_used() - && binding.references().all(|reference_id| { + // Find the first reference across all imports. + let at = imports + .iter() + .map(|Import { reference_id, .. }| { checker .semantic_model() .references - .resolve(reference_id) - .context() - .is_typing() + .resolve(*reference_id) + .range() + .start() }) - { - // Extract the module base and level from the full name. - // Ex) `foo.bar.baz` -> `foo`, `0` - // Ex) `.foo.bar.baz` -> `foo`, `1` - let level = qualified_name - .chars() - .take_while(|c| *c == '.') - .count() - .try_into() - .unwrap(); - - // Categorize the import. - let kind: DiagnosticKind = match categorize( - qualified_name, - Some(level), - &checker.settings.src, - checker.package(), - &checker.settings.isort.known_modules, - checker.settings.target_version, - ) { - ImportSection::Known(ImportType::LocalFolder | ImportType::FirstParty) => { - TypingOnlyFirstPartyImport { - qualified_name: qualified_name.to_string(), - } - .into() - } - ImportSection::Known(ImportType::ThirdParty) | ImportSection::UserDefined(_) => { - TypingOnlyThirdPartyImport { - qualified_name: qualified_name.to_string(), - } - .into() - } - ImportSection::Known(ImportType::StandardLibrary) => TypingOnlyStandardLibraryImport { - qualified_name: qualified_name.to_string(), - } - .into(), - - ImportSection::Known(ImportType::Future) => { - unreachable!("`__future__` imports should be marked as used") - } - }; + .min() + .expect("Expected at least one import"); - let mut diagnostic = Diagnostic::new( - kind, - binding.trimmed_range(checker.semantic_model(), checker.locator), - ); - if let Some(range) = binding.parent_range(checker.semantic_model()) { - diagnostic.set_parent(range.start()); - } + // Step 1) Remove the import. + let remove_import_edit = autofix::edits::remove_unused_imports( + qualified_names.iter().copied(), + stmt, + parent, + checker.locator, + checker.indexer, + checker.stylist, + )?; - if checker.patch(diagnostic.kind.rule()) { - diagnostic.try_set_fix(|| { - // Step 1) Remove the import. - // SAFETY: All non-builtin bindings have a source. - let source = binding.source.unwrap(); - let stmt = checker.semantic_model().stmts[source]; - let parent = checker.semantic_model().stmts.parent(stmt); - let remove_import_edit = autofix::edits::remove_unused_imports( - std::iter::once(qualified_name), - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - )?; - - // Step 2) Add the import to a `TYPE_CHECKING` block. - let reference = checker.semantic_model().references.resolve(*reference_id); - let add_import_edit = checker.importer.typing_import_edit( - &StmtImport { - stmt, - qualified_name, - }, - reference.range().start(), - checker.semantic_model(), - )?; - - Ok( - Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()) - .isolate(checker.isolation(parent)), - ) - }); - } + // Step 2) Add the import to a `TYPE_CHECKING` block. + let add_import_edit = checker.importer.typing_import_edit( + &StmtImports { + stmt, + qualified_names, + }, + at, + checker.semantic_model(), + )?; - if checker.enabled(diagnostic.kind.rule()) { - diagnostics.push(diagnostic); - } - } + Ok( + Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits()) + .isolate(checker.isolation(parent)), + ) } diff --git a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_members.snap b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_members.snap new file mode 100644 index 00000000000000..08a13a21f56bdc --- /dev/null +++ b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_members.snap @@ -0,0 +1,60 @@ +--- +source: crates/ruff/src/rules/flake8_type_checking/mod.rs +--- +:7:5: TCH002 [*] Move third-party import `pandas.DataFrame` into a type-checking block + | + 7 | from pandas import ( + 8 | DataFrame, # DataFrame + | ^^^^^^^^^ TCH002 + 9 | Series, # Series +10 | ) + | + = help: Move into type-checking block + +ℹ Suggested fix +3 3 | +4 4 | from typing import TYPE_CHECKING +5 5 | +6 |-from pandas import ( +7 |- DataFrame, # DataFrame +8 |- Series, # Series +9 |-) +10 6 | + 7 |+if TYPE_CHECKING: + 8 |+ from pandas import ( + 9 |+ DataFrame, # DataFrame + 10 |+ Series, # Series + 11 |+ ) + 12 |+ +11 13 | def f(x: DataFrame, y: Series): +12 14 | pass + +:8:5: TCH002 [*] Move third-party import `pandas.Series` into a type-checking block + | + 8 | from pandas import ( + 9 | DataFrame, # DataFrame +10 | Series, # Series + | ^^^^^^ TCH002 +11 | ) + | + = help: Move into type-checking block + +ℹ Suggested fix +3 3 | +4 4 | from typing import TYPE_CHECKING +5 5 | +6 |-from pandas import ( +7 |- DataFrame, # DataFrame +8 |- Series, # Series +9 |-) +10 6 | + 7 |+if TYPE_CHECKING: + 8 |+ from pandas import ( + 9 |+ DataFrame, # DataFrame + 10 |+ Series, # Series + 11 |+ ) + 12 |+ +11 13 | def f(x: DataFrame, y: Series): +12 14 | pass + + diff --git a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_modules_different_types.snap b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_modules_different_types.snap new file mode 100644 index 00000000000000..916df04b90a761 --- /dev/null +++ b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_modules_different_types.snap @@ -0,0 +1,52 @@ +--- +source: crates/ruff/src/rules/flake8_type_checking/mod.rs +--- +:6:8: TCH003 [*] Move standard library import `os` into a type-checking block + | + 6 | from typing import TYPE_CHECKING + 7 | + 8 | import os, pandas + | ^^ TCH003 + 9 | +10 | def f(x: os, y: pandas): + | + = help: Move into type-checking block + +ℹ Suggested fix +3 3 | +4 4 | from typing import TYPE_CHECKING +5 5 | +6 |-import os, pandas + 6 |+import pandas + 7 |+ + 8 |+if TYPE_CHECKING: + 9 |+ import os +7 10 | +8 11 | def f(x: os, y: pandas): +9 12 | pass + +:6:12: TCH002 [*] Move third-party import `pandas` into a type-checking block + | + 6 | from typing import TYPE_CHECKING + 7 | + 8 | import os, pandas + | ^^^^^^ TCH002 + 9 | +10 | def f(x: os, y: pandas): + | + = help: Move into type-checking block + +ℹ Suggested fix +3 3 | +4 4 | from typing import TYPE_CHECKING +5 5 | +6 |-import os, pandas + 6 |+import os + 7 |+ + 8 |+if TYPE_CHECKING: + 9 |+ import pandas +7 10 | +8 11 | def f(x: os, y: pandas): +9 12 | pass + + diff --git a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_modules_same_type.snap b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_modules_same_type.snap new file mode 100644 index 00000000000000..8c792b8dd66cf8 --- /dev/null +++ b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__multiple_modules_same_type.snap @@ -0,0 +1,50 @@ +--- +source: crates/ruff/src/rules/flake8_type_checking/mod.rs +--- +:6:8: TCH003 [*] Move standard library import `os` into a type-checking block + | + 6 | from typing import TYPE_CHECKING + 7 | + 8 | import os, sys + | ^^ TCH003 + 9 | +10 | def f(x: os, y: sys): + | + = help: Move into type-checking block + +ℹ Suggested fix +3 3 | +4 4 | from typing import TYPE_CHECKING +5 5 | +6 |-import os, sys +7 6 | + 7 |+if TYPE_CHECKING: + 8 |+ import os, sys + 9 |+ +8 10 | def f(x: os, y: sys): +9 11 | pass + +:6:12: TCH003 [*] Move standard library import `sys` into a type-checking block + | + 6 | from typing import TYPE_CHECKING + 7 | + 8 | import os, sys + | ^^^ TCH003 + 9 | +10 | def f(x: os, y: sys): + | + = help: Move into type-checking block + +ℹ Suggested fix +3 3 | +4 4 | from typing import TYPE_CHECKING +5 5 | +6 |-import os, sys +7 6 | + 7 |+if TYPE_CHECKING: + 8 |+ import os, sys + 9 |+ +8 10 | def f(x: os, y: sys): +9 11 | pass + + diff --git a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_5.py.snap b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_5.py.snap index 2f5826dabfdbde..454256ed7fc47a 100644 --- a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_5.py.snap +++ b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_5.py.snap @@ -11,11 +11,11 @@ TCH004_5.py:4:24: TCH004 [*] Move import `typing.List` out of type-checking bloc ℹ Suggested fix 1 1 | from typing import TYPE_CHECKING - 2 |+from typing import List + 2 |+from typing import List, Sequence, Set 2 3 | 3 4 | if TYPE_CHECKING: 4 |- from typing import List, Sequence, Set - 5 |+ from typing import Sequence, Set + 5 |+ pass 5 6 | 6 7 | 7 8 | def example(a: List[int], /, b: Sequence[int], *, c: Set[int]): @@ -30,11 +30,11 @@ TCH004_5.py:4:30: TCH004 [*] Move import `typing.Sequence` out of type-checking ℹ Suggested fix 1 1 | from typing import TYPE_CHECKING - 2 |+from typing import Sequence + 2 |+from typing import List, Sequence, Set 2 3 | 3 4 | if TYPE_CHECKING: 4 |- from typing import List, Sequence, Set - 5 |+ from typing import List, Set + 5 |+ pass 5 6 | 6 7 | 7 8 | def example(a: List[int], /, b: Sequence[int], *, c: Set[int]): @@ -49,11 +49,11 @@ TCH004_5.py:4:40: TCH004 [*] Move import `typing.Set` out of type-checking block ℹ Suggested fix 1 1 | from typing import TYPE_CHECKING - 2 |+from typing import Set + 2 |+from typing import List, Sequence, Set 2 3 | 3 4 | if TYPE_CHECKING: 4 |- from typing import List, Sequence, Set - 5 |+ from typing import List, Sequence + 5 |+ pass 5 6 | 6 7 | 7 8 | def example(a: List[int], /, b: Sequence[int], *, c: Set[int]): diff --git a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_9.py.snap b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_9.py.snap index 2acf995e2e79f9..a55aacd56f3774 100644 --- a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_9.py.snap +++ b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__runtime-import-in-type-checking-block_TCH004_9.py.snap @@ -13,11 +13,11 @@ TCH004_9.py:4:24: TCH004 [*] Move import `typing.Tuple` out of type-checking blo ℹ Suggested fix 1 1 | from typing import TYPE_CHECKING - 2 |+from typing import Tuple + 2 |+from typing import Tuple, List 2 3 | 3 4 | if TYPE_CHECKING: 4 |- from typing import Tuple, List, Dict - 5 |+ from typing import List, Dict + 5 |+ from typing import Dict 5 6 | 6 7 | x: Tuple 7 8 | @@ -34,11 +34,11 @@ TCH004_9.py:4:31: TCH004 [*] Move import `typing.List` out of type-checking bloc ℹ Suggested fix 1 1 | from typing import TYPE_CHECKING - 2 |+from typing import List + 2 |+from typing import Tuple, List 2 3 | 3 4 | if TYPE_CHECKING: 4 |- from typing import Tuple, List, Dict - 5 |+ from typing import Tuple, Dict + 5 |+ from typing import Dict 5 6 | 6 7 | x: Tuple 7 8 | diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs index a8f25b8809b3cf..ebed4bf166ebcd 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs @@ -1,9 +1,8 @@ -use itertools::Itertools; +use anyhow::Result; use ruff_text_size::TextRange; use rustc_hash::FxHashMap; -use rustpython_parser::ast::Ranged; -use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, IsolationLevel, Violation}; +use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_semantic::binding::Exceptions; use ruff_python_semantic::node::NodeId; @@ -82,8 +81,7 @@ impl Violation for UnusedImport { } Some(UnusedImportContext::Init) => { format!( - "`{name}` imported but unused; consider adding to `__all__` or using a redundant \ - alias" + "`{name}` imported but unused; consider adding to `__all__` or using a redundant alias" ) } None => format!("`{name}` imported but unused"), @@ -100,13 +98,10 @@ impl Violation for UnusedImport { } } -type SpannedName<'a> = (&'a str, TextRange); -type BindingContext = (NodeId, Option, Exceptions); - pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut Vec) { // Collect all unused imports by statement. - let mut unused: FxHashMap> = FxHashMap::default(); - let mut ignored: FxHashMap> = FxHashMap::default(); + let mut unused: FxHashMap<(NodeId, Exceptions), Vec> = FxHashMap::default(); + let mut ignored: FxHashMap<(NodeId, Exceptions), Vec> = FxHashMap::default(); for binding_id in scope.binding_ids() { let binding = &checker.semantic_model().bindings[binding_id]; @@ -119,67 +114,55 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut continue; }; - let stmt_id = binding.source.unwrap(); - let parent_id = checker.semantic_model().stmts.parent_id(stmt_id); + let Some(stmt_id) = binding.source else { + continue; + }; - let exceptions = binding.exceptions; - let diagnostic_offset = binding.range.start(); - let stmt = &checker.semantic_model().stmts[stmt_id]; - let parent_offset = if stmt.is_import_from_stmt() { - Some(stmt.start()) - } else { - None + let import = Import { + qualified_name, + trimmed_range: binding.trimmed_range(checker.semantic_model(), checker.locator), + parent_range: binding.parent_range(checker.semantic_model()), }; - if checker.rule_is_ignored(Rule::UnusedImport, diagnostic_offset) - || parent_offset.map_or(false, |parent_offset| { - checker.rule_is_ignored(Rule::UnusedImport, parent_offset) + if checker.rule_is_ignored(Rule::UnusedImport, import.trimmed_range.start()) + || import.parent_range.map_or(false, |parent_range| { + checker.rule_is_ignored(Rule::UnusedImport, parent_range.start()) }) { ignored - .entry((stmt_id, parent_id, exceptions)) + .entry((stmt_id, binding.exceptions)) .or_default() - .push((qualified_name, binding.range)); + .push(import); } else { unused - .entry((stmt_id, parent_id, exceptions)) + .entry((stmt_id, binding.exceptions)) .or_default() - .push((qualified_name, binding.range)); + .push(import); } } let in_init = checker.settings.ignore_init_module_imports && checker.path().ends_with("__init__.py"); - // Generate a diagnostic for every unused import, but share a fix across all unused imports - // within the same statement (excluding those that are ignored). - for ((stmt_id, parent_id, exceptions), unused_imports) in unused - .into_iter() - .sorted_by_key(|((defined_by, ..), ..)| *defined_by) - { - let stmt = checker.semantic_model().stmts[stmt_id]; - let parent = parent_id.map(|parent_id| checker.semantic_model().stmts[parent_id]); - let multiple = unused_imports.len() > 1; + // Generate a diagnostic for every import, but share a fix across all imports within the same + // statement (excluding those that are ignored). + for ((stmt_id, exceptions), imports) in unused { let in_except_handler = exceptions.intersects(Exceptions::MODULE_NOT_FOUND_ERROR | Exceptions::IMPORT_ERROR); + let multiple = imports.len() > 1; let fix = if !in_init && !in_except_handler && checker.patch(Rule::UnusedImport) { - autofix::edits::remove_unused_imports( - unused_imports - .iter() - .map(|(qualified_name, _)| *qualified_name), - stmt, - parent, - checker.locator, - checker.indexer, - checker.stylist, - ) - .ok() + fix_imports(checker, stmt_id, &imports).ok() } else { None }; - for (qualified_name, range) in unused_imports { + for Import { + qualified_name, + trimmed_range, + parent_range, + } in imports + { let mut diagnostic = Diagnostic::new( UnusedImport { name: qualified_name.to_string(), @@ -192,52 +175,66 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut }, multiple, }, - range, + trimmed_range, ); - if stmt.is_import_from_stmt() { - diagnostic.set_parent(stmt.start()); + if let Some(range) = parent_range { + diagnostic.set_parent(range.start()); } - if let Some(edit) = fix.as_ref() { - diagnostic.set_fix(Fix::automatic(edit.clone()).isolate( - parent_id.map_or(IsolationLevel::default(), |node_id| { - IsolationLevel::Group(node_id.into()) - }), - )); + if !in_except_handler { + if let Some(fix) = fix.as_ref() { + diagnostic.set_fix(fix.clone()); + } } diagnostics.push(diagnostic); } } - // Separately, generate a diagnostic for every _ignored_ unused import, but don't bother - // creating a fix. We have to generate these diagnostics, even though they'll be ignored later - // on, so that the suppression comments themselves aren't marked as unnecessary. - for ((stmt_id, .., exceptions), unused_imports) in ignored - .into_iter() - .sorted_by_key(|((stmt_id, ..), ..)| *stmt_id) + // Separately, generate a diagnostic for every _ignored_ import, to ensure that the + // suppression comments aren't marked as unused. + for Import { + qualified_name, + trimmed_range, + parent_range, + } in ignored.into_values().flatten() { - let stmt = checker.semantic_model().stmts[stmt_id]; - let multiple = unused_imports.len() > 1; - let in_except_handler = - exceptions.intersects(Exceptions::MODULE_NOT_FOUND_ERROR | Exceptions::IMPORT_ERROR); - for (qualified_name, range) in unused_imports { - let mut diagnostic = Diagnostic::new( - UnusedImport { - name: qualified_name.to_string(), - context: if in_except_handler { - Some(UnusedImportContext::ExceptHandler) - } else if in_init { - Some(UnusedImportContext::Init) - } else { - None - }, - multiple, - }, - range, - ); - if stmt.is_import_from_stmt() { - diagnostic.set_parent(stmt.start()); - } - diagnostics.push(diagnostic); + let mut diagnostic = Diagnostic::new( + UnusedImport { + name: qualified_name.to_string(), + context: None, + multiple: false, + }, + trimmed_range, + ); + if let Some(range) = parent_range { + diagnostic.set_parent(range.start()); } + diagnostics.push(diagnostic); } } + +/// An unused import with its surrounding context. +struct Import<'a> { + /// The qualified name of the import. + qualified_name: &'a str, + /// The trimmed range of the import. + trimmed_range: TextRange, + /// The range of the import's parent statement. + parent_range: Option, +} + +/// Generate a [`Fix`] to remove unused imports from a statement. +fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result { + let stmt = checker.semantic_model().stmts[stmt_id]; + let parent = checker.semantic_model().stmts.parent(stmt); + let edit = autofix::edits::remove_unused_imports( + imports + .iter() + .map(|Import { qualified_name, .. }| *qualified_name), + stmt, + parent, + checker.locator, + checker.indexer, + checker.stylist, + )?; + Ok(Fix::automatic(edit).isolate(checker.isolation(parent))) +}