From 3eec7de6e4b252d26e7cb7c3966dfa4229d70052 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 27 Jul 2023 16:59:56 -0400 Subject: [PATCH] Clean up --- crates/ruff/src/autofix/codemods.rs | 1 + crates/ruff/src/checkers/ast/mod.rs | 33 ++- .../runtime_import_in_type_checking_block.rs | 3 + .../src/rules/pyflakes/rules/unused_import.rs | 16 +- .../rules/pyupgrade/rules/os_error_alias.rs | 2 + .../rules/unnecessary_builtin_import.rs | 1 + ...les__pyupgrade__tests__UP024_0.py.snap.new | 268 ------------------ crates/ruff_python_ast/src/helpers.rs | 35 ++- crates/ruff_python_semantic/src/binding.rs | 17 ++ foo.py | 2 + 10 files changed, 86 insertions(+), 292 deletions(-) delete mode 100644 crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap.new create mode 100644 foo.py diff --git a/crates/ruff/src/autofix/codemods.rs b/crates/ruff/src/autofix/codemods.rs index 18f7723b3c..eae3e61182 100644 --- a/crates/ruff/src/autofix/codemods.rs +++ b/crates/ruff/src/autofix/codemods.rs @@ -83,6 +83,7 @@ pub(crate) fn remove_imports<'a>( 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)) => { diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index def709f656..b6205386b0 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -39,7 +39,7 @@ use ruff_text_size::{TextRange, TextSize}; use ruff_diagnostics::{Diagnostic, IsolationLevel}; use ruff_python_ast::all::{extract_all_names, DunderAllFlags}; use ruff_python_ast::helpers::{ - extract_handled_exceptions, from_relative_import_parts, to_module_path, + extract_handled_exceptions, from_relative_import_parts, literal_path, to_module_path, }; use ruff_python_ast::identifier::Identifier; use ruff_python_ast::str::trailing_quote; @@ -390,20 +390,23 @@ where // be "foo.bar". Given `from foo import bar as baz`, `name` would be "baz" // and `qualified_name` would be "foo.bar". let name = alias.asname.as_ref().unwrap_or(&alias.name); - if let Some(call_path) = from_relative_import_parts( - self.module_path.unwrap_or_default(), - level, - module, - &alias.name, - ) { - let call_path: Box<[&str]> = call_path.into_boxed_slice(); - self.add_binding( - name, - alias.identifier(), - BindingKind::FromImport(FromImport { call_path }), - flags, - ); - } + + // Attempt to resolve any relative imports; but if we don't know the current + // module path, or the relative import extends beyond the package root, + // fallback to a literal representation (e.g., `[".", "foo"]`). + let call_path = self + .module_path + .and_then(|module_path| { + from_relative_import_parts(module_path, level, module, &alias.name) + }) + .unwrap_or_else(|| literal_path(level, module, &alias.name)); + let call_path: Box<[&str]> = call_path.into_boxed_slice(); + self.add_binding( + name, + alias.identifier(), + BindingKind::FromImport(FromImport { call_path }), + flags, + ); } } } 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 260c6989b3..c51df861c1 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 @@ -206,6 +206,9 @@ fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result .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()), stmt, diff --git a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs index 0b7c7f144b..245626e7da 100644 --- a/crates/ruff/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff/src/rules/pyflakes/rules/unused_import.rs @@ -111,7 +111,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut continue; } - let Some(qualified_name) = binding.qualified_name() else { + let Some(member_name) = binding.member_name() else { continue; }; @@ -120,7 +120,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut }; let import = Import { - qualified_name, + member_name, range: binding.range, parent_range: binding.parent_range(checker.semantic()), }; @@ -159,14 +159,14 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut }; for Import { - qualified_name, + member_name, range, parent_range, } in imports { let mut diagnostic = Diagnostic::new( UnusedImport { - name: qualified_name.to_string(), + name: member_name.to_string(), context: if in_except_handler { Some(UnusedImportContext::ExceptHandler) } else if in_init { @@ -193,14 +193,14 @@ 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 { - qualified_name, + member_name, range, parent_range, } in ignored.into_values().flatten() { let mut diagnostic = Diagnostic::new( UnusedImport { - name: qualified_name.to_string(), + name: member_name.to_string(), context: None, multiple: false, }, @@ -216,7 +216,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut /// An unused import with its surrounding context. struct Import { /// The qualified name of the import (e.g., `typing.List` for `from typing import List`). - qualified_name: String, + member_name: String, /// The trimmed range of the import (e.g., `List` in `from typing import List`). range: TextRange, /// The range of the import's parent statement. @@ -230,7 +230,7 @@ fn fix_imports(checker: &Checker, stmt_id: NodeId, imports: &[Import]) -> Result let edit = autofix::edits::remove_unused_imports( imports .iter() - .map(|Import { qualified_name, .. }| qualified_name.as_str()), + .map(|Import { member_name, .. }| member_name.as_str()), stmt, parent, checker.locator(), 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 4fdd9c9941..faa36f5de9 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/os_error_alias.rs @@ -57,6 +57,8 @@ 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 01e12717a8..208b52f741 100644 --- a/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs +++ b/crates/ruff/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs @@ -126,6 +126,7 @@ pub(crate) fn unnecessary_builtin_import( 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( diff --git a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap.new b/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap.new deleted file mode 100644 index 5893fcba3e..0000000000 --- a/crates/ruff/src/rules/pyupgrade/snapshots/ruff__rules__pyupgrade__tests__UP024_0.py.snap.new +++ /dev/null @@ -1,268 +0,0 @@ ---- -source: crates/ruff/src/rules/pyupgrade/mod.rs -assertion_line: 88 ---- -UP024_0.py:6:8: UP024 [*] Replace aliased errors with `OSError` - | -4 | try: -5 | pass -6 | except EnvironmentError: - | ^^^^^^^^^^^^^^^^ UP024 -7 | pass - | - = help: Replace `EnvironmentError` with builtin `OSError` - -ℹ Fix -3 3 | # These should be fixed -4 4 | try: -5 5 | pass -6 |-except EnvironmentError: - 6 |+except OSError: -7 7 | pass -8 8 | -9 9 | try: - -UP024_0.py:11:8: UP024 [*] Replace aliased errors with `OSError` - | - 9 | try: -10 | pass -11 | except IOError: - | ^^^^^^^ UP024 -12 | pass - | - = help: Replace `IOError` with builtin `OSError` - -ℹ Fix -8 8 | -9 9 | try: -10 10 | pass -11 |-except IOError: - 11 |+except OSError: -12 12 | pass -13 13 | -14 14 | try: - -UP024_0.py:16:8: UP024 [*] Replace aliased errors with `OSError` - | -14 | try: -15 | pass -16 | except WindowsError: - | ^^^^^^^^^^^^ UP024 -17 | pass - | - = help: Replace `WindowsError` with builtin `OSError` - -ℹ Fix -13 13 | -14 14 | try: -15 15 | pass -16 |-except WindowsError: - 16 |+except OSError: -17 17 | pass -18 18 | -19 19 | try: - -UP024_0.py:21:8: UP024 [*] Replace aliased errors with `OSError` - | -19 | try: -20 | pass -21 | except mmap.error: - | ^^^^^^^^^^ UP024 -22 | pass - | - = help: Replace `mmap.error` with builtin `OSError` - -ℹ Fix -18 18 | -19 19 | try: -20 20 | pass -21 |-except mmap.error: - 21 |+except OSError: -22 22 | pass -23 23 | -24 24 | try: - -UP024_0.py:26:8: UP024 [*] Replace aliased errors with `OSError` - | -24 | try: -25 | pass -26 | except select.error: - | ^^^^^^^^^^^^ UP024 -27 | pass - | - = help: Replace `select.error` with builtin `OSError` - -ℹ Fix -23 23 | -24 24 | try: -25 25 | pass -26 |-except select.error: - 26 |+except OSError: -27 27 | pass -28 28 | -29 29 | try: - -UP024_0.py:31:8: UP024 [*] Replace aliased errors with `OSError` - | -29 | try: -30 | pass -31 | except socket.error: - | ^^^^^^^^^^^^ UP024 -32 | pass - | - = help: Replace `socket.error` with builtin `OSError` - -ℹ Fix -28 28 | -29 29 | try: -30 30 | pass -31 |-except socket.error: - 31 |+except OSError: -32 32 | pass -33 33 | -34 34 | try: - -UP024_0.py:36:8: UP024 [*] Replace aliased errors with `OSError` - | -34 | try: -35 | pass -36 | except error: - | ^^^^^ UP024 -37 | pass - | - = help: Replace `error` with builtin `OSError` - -ℹ Fix -33 33 | -34 34 | try: -35 35 | pass -36 |-except error: - 36 |+except OSError: -37 37 | pass -38 38 | -39 39 | # Should NOT be in parentheses when replaced - -UP024_0.py:43:8: UP024 [*] Replace aliased errors with `OSError` - | -41 | try: -42 | pass -43 | except (IOError,): - | ^^^^^^^^^^ UP024 -44 | pass -45 | try: - | - = help: Replace with builtin `OSError` - -ℹ Fix -40 40 | -41 41 | try: -42 42 | pass -43 |-except (IOError,): - 43 |+except OSError: -44 44 | pass -45 45 | try: -46 46 | pass - -UP024_0.py:47:8: UP024 [*] Replace aliased errors with `OSError` - | -45 | try: -46 | pass -47 | except (mmap.error,): - | ^^^^^^^^^^^^^ UP024 -48 | pass -49 | try: - | - = help: Replace with builtin `OSError` - -ℹ Fix -44 44 | pass -45 45 | try: -46 46 | pass -47 |-except (mmap.error,): - 47 |+except OSError: -48 48 | pass -49 49 | try: -50 50 | pass - -UP024_0.py:51:8: UP024 [*] Replace aliased errors with `OSError` - | -49 | try: -50 | pass -51 | except (EnvironmentError, IOError, OSError, select.error): - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP024 -52 | pass - | - = help: Replace with builtin `OSError` - -ℹ Fix -48 48 | pass -49 49 | try: -50 50 | pass -51 |-except (EnvironmentError, IOError, OSError, select.error): - 51 |+except OSError: -52 52 | pass -53 53 | -54 54 | # Should be kept in parentheses (because multiple) - -UP024_0.py:58:8: UP024 [*] Replace aliased errors with `OSError` - | -56 | try: -57 | pass -58 | except (IOError, KeyError, OSError): - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP024 -59 | pass - | - = help: Replace with builtin `OSError` - -ℹ Fix -55 55 | -56 56 | try: -57 57 | pass -58 |-except (IOError, KeyError, OSError): - 58 |+except (KeyError, OSError): -59 59 | pass -60 60 | -61 61 | # First should change, second should not - -UP024_0.py:65:8: UP024 [*] Replace aliased errors with `OSError` - | -63 | try: -64 | pass -65 | except (IOError, error): - | ^^^^^^^^^^^^^^^^ UP024 -66 | pass -67 | # These should not change - | - = help: Replace with builtin `OSError` - -ℹ Fix -62 62 | from .mmap import error -63 63 | try: -64 64 | pass -65 |-except (IOError, error): - 65 |+except OSError: -66 66 | pass -67 67 | # These should not change -68 68 | - -UP024_0.py:87:8: UP024 [*] Replace aliased errors with `OSError` - | -85 | try: -86 | pass -87 | except (mmap).error: - | ^^^^^^^^^^^^ UP024 -88 | pass - | - = help: Replace `mmap.error` with builtin `OSError` - -ℹ Fix -84 84 | pass -85 85 | try: -86 86 | pass -87 |-except (mmap).error: - 87 |+except OSError: -88 88 | pass -89 89 | -90 90 | try: - - diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 1d798b3586..6447d843b6 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -909,7 +909,7 @@ pub fn from_relative_import_parts<'a>( for _ in 0..level { if call_path.is_empty() { - return None; + break; } call_path.pop(); } @@ -927,6 +927,39 @@ pub fn from_relative_import_parts<'a>( Some(call_path) } +pub fn literal_path<'a>( + level: Option, + module: Option<&'a str>, + member: &'a str, +) -> CallPath<'a> { + let mut call_path: CallPath = SmallVec::with_capacity( + level.unwrap_or_default() as usize + + module + .map(|module| module.split('.').count()) + .unwrap_or_default() + + 1, + ); + + // Remove segments based on the number of dots. + if let Some(level) = level { + if level > 0 { + for _ in 0..level { + call_path.push("."); + } + } + } + + // Add the remaining segments. + if let Some(module) = module { + call_path.extend(module.split('.')); + } + + // Add the member. + call_path.push(member); + + call_path +} + /// Given an imported module (based on its relative import level and module name), return the /// fully-qualified module path. pub fn resolve_imported_module_path<'a>( diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index 4c20732af0..6a8c08706d 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::ops::{Deref, DerefMut}; use bitflags::bitflags; @@ -204,6 +205,22 @@ impl<'a> Binding<'a> { } } + /// Returns the fully-qualified symbol name, if this symbol was imported from another module. + pub fn member_name(&self) -> Option> { + match &self.kind { + BindingKind::Import(Import { call_path }) => { + Some(Cow::Owned(format_call_path(call_path))) + } + BindingKind::FromImport(FromImport { call_path }) => { + call_path.last().map(|member| Cow::Borrowed(*member)) + } + BindingKind::SubmoduleImport(SubmoduleImport { call_path }) => { + Some(Cow::Owned(format_call_path(call_path))) + } + _ => None, + } + } + /// Returns the name of the binding (e.g., `x` in `x = 1`). pub fn name<'b>(&self, locator: &'b Locator) -> &'b str { locator.slice(self.range) diff --git a/foo.py b/foo.py new file mode 100644 index 0000000000..f3449fe44c --- /dev/null +++ b/foo.py @@ -0,0 +1,2 @@ +# F401 `datastructures.UploadFile` imported but unused +from .datastructures import UploadFile as FileUpload