From ac0f51ef4f3cf04f18d23ae2faad181ff33ae7c9 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 27 Jul 2023 20:19:39 -0400 Subject: [PATCH] Some abstractions --- crates/ruff/src/autofix/codemods.rs | 86 ++++------------- crates/ruff/src/autofix/edits.rs | 4 +- crates/ruff/src/importer/mod.rs | 20 ++-- .../runtime_import_in_type_checking_block.rs | 52 +++++----- .../rules/typing_only_runtime_import.rs | 96 +++++++++---------- ...only-first-party-import_TCH001.py.snap.new | 5 - .../pandas_vet/rules/inplace_argument.rs | 2 +- .../src/rules/pyflakes/rules/unused_import.rs | 43 +++++---- ...ules__pyflakes__tests__F401_F401_6.py.snap | 8 +- ...__pyflakes__tests__F401_F401_6.py.snap.new | 62 ------------ .../rules/pyupgrade/rules/os_error_alias.rs | 2 - .../rules/unnecessary_builtin_import.rs | 10 +- .../rules/unnecessary_future_import.rs | 9 +- crates/ruff_python_ast/src/call_path.rs | 29 +++++- crates/ruff_python_ast/src/helpers.rs | 2 +- crates/ruff_python_semantic/src/binding.rs | 54 +++++++++++ crates/ruff_python_semantic/src/model.rs | 1 + foo.py | 2 - 18 files changed, 215 insertions(+), 272 deletions(-) delete mode 100644 crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-first-party-import_TCH001.py.snap.new delete mode 100644 crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap.new delete mode 100644 foo.py diff --git a/crates/ruff/src/autofix/codemods.rs b/crates/ruff/src/autofix/codemods.rs index eae3e61182..be7028d5f3 100644 --- a/crates/ruff/src/autofix/codemods.rs +++ b/crates/ruff/src/autofix/codemods.rs @@ -33,7 +33,7 @@ impl<'a, T: Codegen<'a>> CodegenStylist<'a> for T { /// /// Returns `Ok(None)` if the statement is empty after removing the imports. pub(crate) fn remove_imports<'a>( - imports: impl Iterator, + member_names: impl Iterator, stmt: &Stmt, locator: &Locator, stylist: &Stylist, @@ -45,27 +45,20 @@ pub(crate) fn remove_imports<'a>( bail!("Expected Statement::Simple"); }; - let (aliases, import_module) = match body.body.first_mut() { - Some(SmallStatement::Import(import_body)) => (&mut import_body.names, None), + let aliases = match body.body.first_mut() { + Some(SmallStatement::Import(import_body)) => &mut import_body.names, Some(SmallStatement::ImportFrom(import_body)) => { if let ImportNames::Aliases(names) = &mut import_body.names { - ( - names, - Some((&import_body.relative, import_body.module.as_ref())), - ) + names } else if let ImportNames::Star(..) = &import_body.names { // Special-case: if the import is a `from ... import *`, then we delete the // entire statement. let mut found_star = false; - for import in imports { - let qualified_name = match import_body.module.as_ref() { - Some(module_name) => format!("{}.*", compose_module_path(module_name)), - None => "*".to_string(), - }; - if import == qualified_name { + for member in member_names { + if member == "*" { found_star = true; } else { - bail!("Expected \"*\" for unused import (got: \"{}\")", import); + bail!("Expected \"*\" for unused import (got: \"{}\")", member); } } if !found_star { @@ -82,31 +75,10 @@ pub(crate) fn remove_imports<'a>( // Preserve the trailing comma (or not) from the last entry. let trailing_comma = aliases.last().and_then(|alias| alias.comma.clone()); - for import in imports { - // If it's an import-from, we only care about members. - let alias_index = aliases.iter().position(|alias| { - let qualified_name = match import_module { - Some((relative, module)) => { - let module = module.map(compose_module_path); - let member = compose_module_path(&alias.name); - let mut qualified_name = String::with_capacity( - relative.len() + module.as_ref().map_or(0, String::len) + member.len() + 1, - ); - for _ in 0..relative.len() { - qualified_name.push('.'); - } - if let Some(module) = module { - qualified_name.push_str(&module); - qualified_name.push('.'); - } - qualified_name.push_str(&member); - qualified_name - } - None => compose_module_path(&alias.name), - }; - qualified_name == import - }); - + for member in member_names { + let alias_index = aliases + .iter() + .position(|alias| member == compose_module_path(&alias.name)); if let Some(index) = alias_index { aliases.remove(index); } @@ -140,7 +112,7 @@ pub(crate) fn remove_imports<'a>( /// /// Returns the modified import statement. pub(crate) fn retain_imports( - imports: &[&str], + member_names: &[&str], stmt: &Stmt, locator: &Locator, stylist: &Stylist, @@ -152,14 +124,11 @@ pub(crate) fn retain_imports( bail!("Expected Statement::Simple"); }; - let (aliases, import_module) = match body.body.first_mut() { - Some(SmallStatement::Import(import_body)) => (&mut import_body.names, None), + let aliases = match body.body.first_mut() { + Some(SmallStatement::Import(import_body)) => &mut import_body.names, Some(SmallStatement::ImportFrom(import_body)) => { if let ImportNames::Aliases(names) = &mut import_body.names { - ( - names, - Some((&import_body.relative, import_body.module.as_ref())), - ) + names } else { bail!("Expected: ImportNames::Aliases"); } @@ -171,28 +140,9 @@ pub(crate) fn retain_imports( let trailing_comma = aliases.last().and_then(|alias| alias.comma.clone()); aliases.retain(|alias| { - imports.iter().any(|import| { - let qualified_name = match import_module { - Some((relative, module)) => { - let module = module.map(compose_module_path); - let member = compose_module_path(&alias.name); - let mut qualified_name = String::with_capacity( - relative.len() + module.as_ref().map_or(0, String::len) + member.len() + 1, - ); - for _ in 0..relative.len() { - qualified_name.push('.'); - } - if let Some(module) = module { - qualified_name.push_str(&module); - qualified_name.push('.'); - } - qualified_name.push_str(&member); - qualified_name - } - None => compose_module_path(&alias.name), - }; - qualified_name == *import - }) + member_names + .iter() + .any(|member| *member == compose_module_path(&alias.name)) }); // But avoid destroying any trailing comments. diff --git a/crates/ruff/src/autofix/edits.rs b/crates/ruff/src/autofix/edits.rs index c2c02bdf30..005127d56d 100644 --- a/crates/ruff/src/autofix/edits.rs +++ b/crates/ruff/src/autofix/edits.rs @@ -55,14 +55,14 @@ pub(crate) fn delete_stmt( /// Generate a `Fix` to remove the specified imports from an `import` statement. pub(crate) fn remove_unused_imports<'a>( - unused_imports: impl Iterator, + member_names: impl Iterator, stmt: &Stmt, parent: Option<&Stmt>, locator: &Locator, stylist: &Stylist, indexer: &Indexer, ) -> Result { - match codemods::remove_imports(unused_imports, stmt, locator, stylist)? { + match codemods::remove_imports(member_names, stmt, locator, stylist)? { None => Ok(delete_stmt(stmt, parent, locator, indexer)), Some(content) => Ok(Edit::range_replacement(content, stmt.range())), } diff --git a/crates/ruff/src/importer/mod.rs b/crates/ruff/src/importer/mod.rs index 12de3f2702..c9ba35e66f 100644 --- a/crates/ruff/src/importer/mod.rs +++ b/crates/ruff/src/importer/mod.rs @@ -87,13 +87,13 @@ impl<'a> Importer<'a> { /// import statement. pub(crate) fn runtime_import_edit( &self, - import: &StmtImports, + import: &ImportedMembers, at: TextSize, ) -> Result { // Generate the modified import statement. let content = autofix::codemods::retain_imports( - &import.qualified_names, - import.stmt, + &import.names, + import.statement, self.locator, self.stylist, )?; @@ -118,14 +118,14 @@ impl<'a> Importer<'a> { /// `TYPE_CHECKING` block. pub(crate) fn typing_import_edit( &self, - import: &StmtImports, + import: &ImportedMembers, at: TextSize, semantic: &SemanticModel, ) -> Result { // Generate the modified import statement. let content = autofix::codemods::retain_imports( - &import.qualified_names, - import.stmt, + &import.names, + import.statement, self.locator, self.stylist, )?; @@ -446,11 +446,11 @@ impl<'a> ImportRequest<'a> { } /// An existing list of module or member imports, located within an import statement. -pub(crate) struct StmtImports<'a> { +pub(crate) struct ImportedMembers<'a> { /// The import statement. - pub(crate) stmt: &'a Stmt, - /// The "qualified names" of the imported modules or members. - pub(crate) qualified_names: Vec<&'a str>, + pub(crate) statement: &'a Stmt, + /// The "names" of the imported members. + pub(crate) names: Vec<&'a str>, } /// The result of an [`Importer::get_or_import_symbol`] call. 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 c51df861c1..ccbb3674e1 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,15 +1,16 @@ use anyhow::Result; use ruff_text_size::TextRange; use rustc_hash::FxHashMap; +use std::borrow::Cow; use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::{NodeId, ResolvedReferenceId, Scope}; +use ruff_python_semantic::{AnyImport, NodeId, ResolvedReferenceId, Scope}; use crate::autofix; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::importer::StmtImports; +use crate::importer::ImportedMembers; /// ## What it does /// Checks for runtime imports defined in a type-checking block. @@ -51,7 +52,7 @@ impl Violation for RuntimeImportInTypeCheckingBlock { #[derive_message_formats] fn message(&self) -> String { - let RuntimeImportInTypeCheckingBlock { qualified_name, .. } = self; + let RuntimeImportInTypeCheckingBlock { qualified_name } = self; format!( "Move import `{qualified_name}` out of type-checking block. Import is used for more than type hinting." ) @@ -69,13 +70,13 @@ 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 ignores_by_statement: FxHashMap> = FxHashMap::default(); + let mut errors_by_statement: FxHashMap> = FxHashMap::default(); + let mut ignores_by_statement: FxHashMap> = FxHashMap::default(); for binding_id in scope.binding_ids() { let binding = checker.semantic().binding(binding_id); - let Some(call_path) = binding.call_path() else { + let Some(import) = binding.as_any_import() else { continue; }; @@ -96,8 +97,8 @@ pub(crate) fn runtime_import_in_type_checking_block( continue; }; - let import = Import { - call_path, + let import = ImportBinding { + import, reference_id, range: binding.range, parent_range: binding.parent_range(checker.semantic()), @@ -130,8 +131,8 @@ pub(crate) fn runtime_import_in_type_checking_block( None }; - for Import { - call_path, + for ImportBinding { + import, range, parent_range, .. @@ -139,7 +140,7 @@ pub(crate) fn runtime_import_in_type_checking_block( { let mut diagnostic = Diagnostic::new( RuntimeImportInTypeCheckingBlock { - qualified_name: call_path.join("."), + qualified_name: import.qualified_name(), }, range, ); @@ -155,8 +156,8 @@ pub(crate) fn runtime_import_in_type_checking_block( // Separately, generate a diagnostic for every _ignored_ import, to ensure that the // suppression comments aren't marked as unused. - for Import { - call_path, + for ImportBinding { + import, range, parent_range, .. @@ -164,7 +165,7 @@ pub(crate) fn runtime_import_in_type_checking_block( { let mut diagnostic = Diagnostic::new( RuntimeImportInTypeCheckingBlock { - qualified_name: call_path.join("."), + qualified_name: import.qualified_name(), }, range, ); @@ -176,9 +177,9 @@ pub(crate) fn runtime_import_in_type_checking_block( } /// A runtime-required import with its surrounding context. -struct Import<'a> { +struct ImportBinding<'a> { /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). - call_path: &'a [&'a str], + 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`). @@ -188,29 +189,26 @@ struct Import<'a> { } /// Generate a [`Fix`] to remove runtime imports from a type-checking block. -fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result { +fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[ImportBinding]) -> Result { let stmt = checker.semantic().stmts[stmt_id]; let parent = checker.semantic().stmts.parent(stmt); - let qualified_names: Vec = imports + let member_names: Vec> = imports .iter() - .map(|Import { call_path, .. }| call_path.join(".")) + .map(|ImportBinding { import, .. }| import.member_name()) .collect(); // Find the first reference across all imports. let at = imports .iter() - .map(|Import { reference_id, .. }| { + .map(|ImportBinding { reference_id, .. }| { checker.semantic().reference(*reference_id).range().start() }) .min() .expect("Expected at least one import"); // Step 1) Remove the import. - // This could be a mix of imports... - // One framing could be: what's the name of the symbol being imported (e.g., `List` in `from typing import List`, - // or `foo` in `import foo`, or `foo.bar` in `import foo.bar`). let remove_import_edit = autofix::edits::remove_unused_imports( - qualified_names.iter().map(|name| name.as_str()), + member_names.iter().map(AsRef::as_ref), stmt, parent, checker.locator(), @@ -220,9 +218,9 @@ fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result // Step 2) Add the import to the top-level. let add_import_edit = checker.importer().runtime_import_edit( - &StmtImports { - stmt, - qualified_names: qualified_names.iter().map(|name| name.as_str()).collect(), + &ImportedMembers { + statement: stmt, + names: member_names.iter().map(AsRef::as_ref).collect(), }, at, )?; 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 b38e0e0635..19eb915271 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,15 +1,17 @@ +use std::borrow::Cow; + 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, NodeId, ResolvedReferenceId, Scope}; +use ruff_python_semantic::{AnyImport, Binding, NodeId, ResolvedReferenceId, Scope}; +use ruff_text_size::TextRange; use crate::autofix; use crate::checkers::ast::Checker; use crate::codes::Rule; -use crate::importer::StmtImports; +use crate::importer::ImportedMembers; use crate::rules::isort::{categorize, ImportSection, ImportType}; /// ## What it does @@ -188,9 +190,9 @@ pub(crate) fn typing_only_runtime_import( diagnostics: &mut Vec, ) { // Collect all typing-only imports by statement and import type. - let mut errors_by_statement: FxHashMap<(NodeId, ImportType), Vec> = + let mut errors_by_statement: FxHashMap<(NodeId, ImportType), Vec> = FxHashMap::default(); - let mut ignores_by_statement: FxHashMap<(NodeId, ImportType), Vec> = + let mut ignores_by_statement: FxHashMap<(NodeId, ImportType), Vec> = FxHashMap::default(); for binding_id in scope.binding_ids() { @@ -206,23 +208,10 @@ pub(crate) fn typing_only_runtime_import( continue; } - let Some(qualified_name) = binding.qualified_name() else { + let Some(import) = binding.as_any_import() else { continue; }; - if is_exempt( - qualified_name.as_str(), - &checker - .settings - .flake8_type_checking - .exempt_modules - .iter() - .map(String::as_str) - .collect::>(), - ) { - continue; - } - let Some(reference_id) = binding.references.first().copied() else { continue; }; @@ -236,20 +225,25 @@ pub(crate) fn typing_only_runtime_import( .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(); + let qualified_name = import.qualified_name(); + + if is_exempt( + qualified_name.as_str(), + &checker + .settings + .flake8_type_checking + .exempt_modules + .iter() + .map(String::as_str) + .collect::>(), + ) { + continue; + } // Categorize the import, using coarse-grained categorization. let import_type = match categorize( qualified_name.as_str(), - Some(level), + None, &checker.settings.src, checker.package(), &checker.settings.isort.known_modules, @@ -275,8 +269,8 @@ pub(crate) fn typing_only_runtime_import( continue; }; - let import = Import { - qualified_name, + let import = ImportBinding { + import, reference_id, range: binding.range, parent_range: binding.parent_range(checker.semantic()), @@ -309,17 +303,15 @@ pub(crate) fn typing_only_runtime_import( None }; - for Import { - qualified_name, + for ImportBinding { + import, range, parent_range, .. } in imports { - let mut diagnostic = Diagnostic::new( - diagnostic_for(import_type, qualified_name.to_string()), - range, - ); + let mut diagnostic = + Diagnostic::new(diagnostic_for(import_type, import.qualified_name()), range); if let Some(range) = parent_range { diagnostic.set_parent(range.start()); } @@ -333,17 +325,15 @@ pub(crate) fn typing_only_runtime_import( // 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, + for ImportBinding { + import, range, parent_range, .. } in imports { - let mut diagnostic = Diagnostic::new( - diagnostic_for(import_type, qualified_name.to_string()), - range, - ); + let mut diagnostic = + Diagnostic::new(diagnostic_for(import_type, import.qualified_name()), range); if let Some(range) = parent_range { diagnostic.set_parent(range.start()); } @@ -353,9 +343,9 @@ pub(crate) fn typing_only_runtime_import( } /// A runtime-required import with its surrounding context. -struct Import { +struct ImportBinding<'a> { /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). - qualified_name: String, + 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`). @@ -412,18 +402,18 @@ fn is_exempt(name: &str, exempt_modules: &[&str]) -> bool { } /// Generate a [`Fix`] to remove typing-only imports from a runtime context. -fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result { +fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[ImportBinding]) -> Result { let stmt = checker.semantic().stmts[stmt_id]; let parent = checker.semantic().stmts.parent(stmt); - let qualified_names: Vec<&str> = imports + let member_names: Vec> = imports .iter() - .map(|Import { qualified_name, .. }| qualified_name.as_str()) + .map(|ImportBinding { import, .. }| import.member_name()) .collect(); // Find the first reference across all imports. let at = imports .iter() - .map(|Import { reference_id, .. }| { + .map(|ImportBinding { reference_id, .. }| { checker.semantic().reference(*reference_id).range().start() }) .min() @@ -431,7 +421,7 @@ fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result // Step 1) Remove the import. let remove_import_edit = autofix::edits::remove_unused_imports( - qualified_names.iter().copied(), + member_names.iter().map(AsRef::as_ref), stmt, parent, checker.locator(), @@ -441,9 +431,9 @@ fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result // Step 2) Add the import to a `TYPE_CHECKING` block. let add_import_edit = checker.importer().typing_import_edit( - &StmtImports { - stmt, - qualified_names, + &ImportedMembers { + statement: stmt, + names: member_names.iter().map(AsRef::as_ref).collect(), }, at, checker.semantic(), diff --git a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-first-party-import_TCH001.py.snap.new b/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-first-party-import_TCH001.py.snap.new deleted file mode 100644 index e5d8c04739..0000000000 --- a/crates/ruff/src/rules/flake8_type_checking/snapshots/ruff__rules__flake8_type_checking__tests__typing-only-first-party-import_TCH001.py.snap.new +++ /dev/null @@ -1,5 +0,0 @@ ---- -source: crates/ruff/src/rules/flake8_type_checking/mod.rs -assertion_line: 43 ---- - diff --git a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs index af3814e1db..ab1bf3d870 100644 --- a/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs +++ b/crates/ruff/src/rules/pandas_vet/rules/inplace_argument.rs @@ -65,7 +65,7 @@ pub(crate) fn inplace_argument( .first() .and_then(|module| checker.semantic().find_binding(module)) .map_or(false, |binding| { - if let BindingKind::Import(Import { call_path, .. }) = &binding.kind { + if let BindingKind::Import(Import { call_path }) = &binding.kind { matches!(call_path.as_ref(), ["pandas"]) } else { false diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs index 245626e7da..f03707c5bd 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs @@ -1,10 +1,11 @@ use anyhow::Result; -use ruff_text_size::TextRange; use rustc_hash::FxHashMap; +use std::borrow::Cow; use ruff_diagnostics::{AutofixKind, Diagnostic, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_semantic::{Exceptions, NodeId, Scope}; +use ruff_python_semantic::{AnyImport, Exceptions, NodeId, Scope}; +use ruff_text_size::TextRange; use crate::autofix; use crate::checkers::ast::Checker; @@ -97,8 +98,8 @@ impl Violation for UnusedImport { pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut Vec) { // Collect all unused imports by statement. - let mut unused: FxHashMap<(NodeId, Exceptions), Vec> = FxHashMap::default(); - let mut ignored: FxHashMap<(NodeId, Exceptions), Vec> = 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().binding(binding_id); @@ -111,7 +112,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut continue; } - let Some(member_name) = binding.member_name() else { + let Some(import) = binding.as_any_import() else { continue; }; @@ -119,8 +120,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut continue; }; - let import = Import { - member_name, + let import = ImportBinding { + import, range: binding.range, parent_range: binding.parent_range(checker.semantic()), }; @@ -158,15 +159,15 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut None }; - for Import { - member_name, + for ImportBinding { + import, range, parent_range, } in imports { let mut diagnostic = Diagnostic::new( UnusedImport { - name: member_name.to_string(), + name: import.qualified_name(), context: if in_except_handler { Some(UnusedImportContext::ExceptHandler) } else if in_init { @@ -192,15 +193,15 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut // Separately, generate a diagnostic for every _ignored_ import, to ensure that the // suppression comments aren't marked as unused. - for Import { - member_name, + for ImportBinding { + import, range, parent_range, } in ignored.into_values().flatten() { let mut diagnostic = Diagnostic::new( UnusedImport { - name: member_name.to_string(), + name: import.qualified_name(), context: None, multiple: false, }, @@ -214,9 +215,9 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut } /// An unused import with its surrounding context. -struct Import { +struct ImportBinding<'a> { /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). - member_name: String, + import: AnyImport<'a>, /// The trimmed range of the import (e.g., `List` in `from typing import List`). range: TextRange, /// The range of the import's parent statement. @@ -224,13 +225,17 @@ struct Import { } /// Generate a [`Fix`] to remove unused imports from a statement. -fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result { +fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[ImportBinding]) -> Result { let stmt = checker.semantic().stmts[stmt_id]; let parent = checker.semantic().stmts.parent(stmt); + + let member_names: Vec> = imports + .iter() + .map(|ImportBinding { import, .. }| import.member_name()) + .collect(); + let edit = autofix::edits::remove_unused_imports( - imports - .iter() - .map(|Import { member_name, .. }| member_name.as_str()), + member_names.iter().map(AsRef::as_ref), stmt, parent, checker.locator(), diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap index a6b21228c3..c5b397feea 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap +++ b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff/src/rules/pyflakes/mod.rs --- -F401_6.py:7:25: F401 [*] `.background.BackgroundTasks` imported but unused +F401_6.py:7:25: F401 [*] `pyflakes.background.BackgroundTasks` imported but unused | 6 | # F401 `background.BackgroundTasks` imported but unused 7 | from .background import BackgroundTasks @@ -9,7 +9,7 @@ F401_6.py:7:25: F401 [*] `.background.BackgroundTasks` imported but unused 8 | 9 | # F401 `datastructures.UploadFile` imported but unused | - = help: Remove unused import: `.background.BackgroundTasks` + = help: Remove unused import: `pyflakes.background.BackgroundTasks` ℹ Fix 4 4 | from .applications import FastAPI as FastAPI @@ -20,7 +20,7 @@ F401_6.py:7:25: F401 [*] `.background.BackgroundTasks` imported but unused 9 8 | # F401 `datastructures.UploadFile` imported but unused 10 9 | from .datastructures import UploadFile as FileUpload -F401_6.py:10:43: F401 [*] `.datastructures.UploadFile` imported but unused +F401_6.py:10:43: F401 [*] `pyflakes.datastructures.UploadFile` imported but unused | 9 | # F401 `datastructures.UploadFile` imported but unused 10 | from .datastructures import UploadFile as FileUpload @@ -28,7 +28,7 @@ F401_6.py:10:43: F401 [*] `.datastructures.UploadFile` imported but unused 11 | 12 | # OK | - = help: Remove unused import: `.datastructures.UploadFile` + = help: Remove unused import: `pyflakes.datastructures.UploadFile` ℹ Fix 7 7 | from .background import BackgroundTasks diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap.new b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap.new deleted file mode 100644 index 4c578d39a8..0000000000 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F401_F401_6.py.snap.new +++ /dev/null @@ -1,62 +0,0 @@ ---- -source: crates/ruff/src/rules/pyflakes/mod.rs -assertion_line: 147 ---- -F401_6.py:7:25: F401 [*] `pyflakes.background.BackgroundTasks` imported but unused - | -6 | # F401 `background.BackgroundTasks` imported but unused -7 | from .background import BackgroundTasks - | ^^^^^^^^^^^^^^^ F401 -8 | -9 | # F401 `datastructures.UploadFile` imported but unused - | - = help: Remove unused import: `pyflakes.background.BackgroundTasks` - -ℹ Fix - -F401_6.py:10:43: F401 [*] `pyflakes.datastructures.UploadFile` imported but unused - | - 9 | # F401 `datastructures.UploadFile` imported but unused -10 | from .datastructures import UploadFile as FileUpload - | ^^^^^^^^^^ F401 -11 | -12 | # OK - | - = help: Remove unused import: `pyflakes.datastructures.UploadFile` - -ℹ Fix - -F401_6.py:16:8: F401 [*] `background` imported but unused - | -15 | # F401 `background` imported but unused -16 | import background - | ^^^^^^^^^^ F401 -17 | -18 | # F401 `datastructures` imported but unused - | - = help: Remove unused import: `background` - -ℹ Fix -13 13 | import applications as applications -14 14 | -15 15 | # F401 `background` imported but unused -16 |-import background -17 16 | -18 17 | # F401 `datastructures` imported but unused -19 18 | import datastructures as structures - -F401_6.py:19:26: F401 [*] `datastructures` imported but unused - | -18 | # F401 `datastructures` imported but unused -19 | import datastructures as structures - | ^^^^^^^^^^ F401 - | - = help: Remove unused import: `datastructures` - -ℹ Fix -16 16 | import background -17 17 | -18 18 | # F401 `datastructures` imported but unused -19 |-import datastructures as structures - - diff --git a/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs b/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs index faa36f5de9..4fdd9c9941 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs @@ -57,8 +57,6 @@ impl AlwaysAutofixableViolation for OSErrorAlias { /// Return `true` if an [`Expr`] is an alias of `OSError`. fn is_alias(expr: &Expr, semantic: &SemanticModel) -> bool { semantic.resolve_call_path(expr).map_or(false, |call_path| { - println!("expr: {:?}", expr); - println!("call_path: {:?}", call_path); matches!( call_path.as_slice(), ["", "EnvironmentError" | "IOError" | "WindowsError"] diff --git a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs index 208b52f741..17fbcbe284 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs @@ -124,13 +124,11 @@ pub(crate) fn unnecessary_builtin_import( diagnostic.try_set_fix(|| { let stmt = checker.semantic().stmt(); let parent = checker.semantic().stmt_parent(); - let unused_imports: Vec = unused_imports - .iter() - // These are always ImportFrom. - .map(|alias| format!("{module}.{}", alias.name)) - .collect(); let edit = autofix::edits::remove_unused_imports( - unused_imports.iter().map(String::as_str), + unused_imports + .iter() + .map(|alias| &alias.name) + .map(ruff_python_ast::Identifier::as_str), stmt, parent, checker.locator(), diff --git a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs index 52c5b543c8..ec4e4d7ba8 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_future_import.rs @@ -111,14 +111,13 @@ pub(crate) fn unnecessary_future_import(checker: &mut Checker, stmt: &Stmt, name if checker.patch(diagnostic.kind.rule()) { diagnostic.try_set_fix(|| { - let unused_imports: Vec = unused_imports - .iter() - .map(|alias| format!("__future__.{}", alias.name)) - .collect(); let stmt = checker.semantic().stmt(); let parent = checker.semantic().stmt_parent(); let edit = autofix::edits::remove_unused_imports( - unused_imports.iter().map(String::as_str), + unused_imports + .iter() + .map(|alias| &alias.name) + .map(ruff_python_ast::Identifier::as_str), stmt, parent, checker.locator(), diff --git a/crates/ruff_python_ast/src/call_path.rs b/crates/ruff_python_ast/src/call_path.rs index 90f6362152..72fa702c98 100644 --- a/crates/ruff_python_ast/src/call_path.rs +++ b/crates/ruff_python_ast/src/call_path.rs @@ -278,12 +278,31 @@ pub fn compose_call_path(expr: &Expr) -> Option { /// Format a call path for display. pub fn format_call_path(call_path: &[&str]) -> String { - if call_path - .first() - .expect("Unable to format empty call path") - .is_empty() - { + if call_path.first().map_or(false, |first| first.is_empty()) { + // If the first segment is empty, the `CallPath` is that of a builtin. + // Ex) `["", "bool"]` -> `"bool"` call_path[1..].join(".") + } else if call_path + .first() + .map_or(false, |first| matches!(*first, ".")) + { + // If the call path is dot-prefixed, it's an unresolved relative import. + // Ex) `[".foo", "bar"]` -> `".foo.bar"` + let mut formatted = String::new(); + let mut iter = call_path.iter(); + for segment in iter.by_ref() { + if matches!(*segment, ".") { + formatted.push('.'); + } else { + formatted.push_str(segment); + break; + } + } + for segment in iter { + formatted.push('.'); + formatted.push_str(segment); + } + formatted } else { call_path.join(".") } diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 6447d843b6..5c6683a900 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -940,7 +940,7 @@ pub fn literal_path<'a>( + 1, ); - // Remove segments based on the number of dots. + // Include the dots if let Some(level) = level { if level > 0 { for _ in 0..level { diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 6a8c08706d..23c6c71221 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -238,6 +238,60 @@ impl<'a> Binding<'a> { } }) } + + pub fn as_any_import(&'a self) -> Option> { + match &self.kind { + BindingKind::Import(import) => Some(AnyImport::Import(import)), + BindingKind::SubmoduleImport(import) => Some(AnyImport::SubmoduleImport(import)), + BindingKind::FromImport(import) => Some(AnyImport::FromImport(import)), + _ => None, + } + } +} + +pub enum AnyImport<'a> { + Import(&'a Import<'a>), + SubmoduleImport(&'a SubmoduleImport<'a>), + FromImport(&'a FromImport<'a>), +} + +impl<'a> AnyImport<'a> { + /// Returns the fully-qualified symbol name, if this symbol was imported from another module. + pub fn call_path(&self) -> &[&str] { + match self { + Self::Import(Import { call_path }) => call_path.as_ref(), + Self::SubmoduleImport(SubmoduleImport { call_path }) => call_path.as_ref(), + Self::FromImport(FromImport { call_path }) => call_path.as_ref(), + } + } + + /// Returns the fully-qualified symbol name, if this symbol was imported from another module. + pub fn qualified_name(&self) -> String { + format_call_path(self.call_path()) + } + + /// Returns the fully-qualified name of the module from which this symbol was imported, if this + /// symbol was imported from another module. + pub fn module_name(&self) -> &[&str] { + match self { + Self::Import(Import { call_path }) => &call_path[..1], + Self::SubmoduleImport(SubmoduleImport { call_path }) => &call_path[..1], + Self::FromImport(FromImport { call_path }) => &call_path[..call_path.len() - 1], + } + } + + /// Returns the fully-qualified symbol name, if this symbol was imported from another module. + pub fn member_name(&self) -> Cow<'a, str> { + match self { + Self::Import(Import { call_path }) => Cow::Owned(format_call_path(call_path)), + Self::SubmoduleImport(SubmoduleImport { call_path }) => { + Cow::Owned(format_call_path(call_path)) + } + Self::FromImport(FromImport { call_path }) => { + Cow::Borrowed(call_path[call_path.len() - 1]) + } + } + } } bitflags! { diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index e006a63927..39685c5e56 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -664,6 +664,7 @@ impl<'a> SemanticModel<'a> { module: &str, member: &str, ) -> Option { + // TODO(charlie): Pass in a slice. let module_path: Vec<&str> = module.split('.').collect(); self.scopes().enumerate().find_map(|(scope_index, scope)| { scope.bindings().find_map(|(name, binding_id)| { diff --git a/foo.py b/foo.py deleted file mode 100644 index f3449fe44c..0000000000 --- a/foo.py +++ /dev/null @@ -1,2 +0,0 @@ -# F401 `datastructures.UploadFile` imported but unused -from .datastructures import UploadFile as FileUpload