Avoid marking required imports as unused (#12537)

## Summary

If an import is marked as "required", we should never flag it as unused.
In practice, this is rare, since required imports are typically used for
`__future__` annotations, which are always considered "used".

Closes https://github.com/astral-sh/ruff/issues/12458.
This commit is contained in:
Charlie Marsh 2024-07-26 14:23:43 -04:00 committed by GitHub
parent d930052de8
commit e18c45c310
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 157 additions and 1 deletions

View file

@ -0,0 +1,20 @@
# Unused, but marked as required.
import os
# Unused, _not_ marked as required.
import sys
# Unused, _not_ marked as required (due to the alias).
import pathlib as non_alias
# Unused, marked as required.
import shelve as alias
# Unused, but marked as required.
from typing import List
# Unused, but marked as required.
from typing import Set as SetAlias
# Unused, but marked as required.
import urllib.parse

View file

@ -906,6 +906,40 @@ mod tests {
Ok(()) Ok(())
} }
#[test_case(Path::new("unused.py"))]
fn required_import_unused(path: &Path) -> Result<()> {
let snapshot = format!("required_import_{}", path.to_string_lossy());
let diagnostics = test_path(
Path::new("isort/required_imports").join(path).as_path(),
&LinterSettings {
src: vec![test_resource_path("fixtures/isort")],
isort: super::settings::Settings {
required_imports: BTreeSet::from_iter([
NameImport::Import(ModuleNameImport::module("os".to_string())),
NameImport::Import(ModuleNameImport::alias(
"shelve".to_string(),
"alias".to_string(),
)),
NameImport::ImportFrom(MemberNameImport::member(
"typing".to_string(),
"List".to_string(),
)),
NameImport::ImportFrom(MemberNameImport::alias(
"typing".to_string(),
"Set".to_string(),
"SetAlias".to_string(),
)),
NameImport::Import(ModuleNameImport::module("urllib.parse".to_string())),
]),
..super::settings::Settings::default()
},
..LinterSettings::for_rules([Rule::MissingRequiredImport, Rule::UnusedImport])
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
#[test_case(Path::new("from_first.py"))] #[test_case(Path::new("from_first.py"))]
fn from_first(path: &Path) -> Result<()> { fn from_first(path: &Path) -> Result<()> {
let snapshot = format!("from_first_{}", path.to_string_lossy()); let snapshot = format!("from_first_{}", path.to_string_lossy());

View file

@ -0,0 +1,40 @@
---
source: crates/ruff_linter/src/rules/isort/mod.rs
---
unused.py:5:8: F401 [*] `sys` imported but unused
|
4 | # Unused, _not_ marked as required.
5 | import sys
| ^^^ F401
6 |
7 | # Unused, _not_ marked as required (due to the alias).
|
= help: Remove unused import: `sys`
Safe fix
2 2 | import os
3 3 |
4 4 | # Unused, _not_ marked as required.
5 |-import sys
6 5 |
7 6 | # Unused, _not_ marked as required (due to the alias).
8 7 | import pathlib as non_alias
unused.py:8:19: F401 [*] `pathlib` imported but unused
|
7 | # Unused, _not_ marked as required (due to the alias).
8 | import pathlib as non_alias
| ^^^^^^^^^ F401
9 |
10 | # Unused, marked as required.
|
= help: Remove unused import: `pathlib`
Safe fix
5 5 | import sys
6 6 |
7 7 | # Unused, _not_ marked as required (due to the alias).
8 |-import pathlib as non_alias
9 8 |
10 9 | # Unused, marked as required.
11 10 | import shelve as alias

View file

@ -242,8 +242,22 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
continue; continue;
}; };
let name = binding.name(checker.locator());
// If an import is marked as required, avoid treating it as unused, regardless of whether
// it was _actually_ used.
if checker
.settings
.isort
.required_imports
.iter()
.any(|required_import| required_import.matches(name, &import))
{
continue;
}
let import = ImportBinding { let import = ImportBinding {
name: binding.name(checker.locator()), name,
import, import,
range: binding.range(), range: binding.range(),
parent_range: binding.parent_range(checker.semantic()), parent_range: binding.parent_range(checker.semantic()),

View file

@ -1,4 +1,8 @@
use ruff_macros::CacheKey; use ruff_macros::CacheKey;
use ruff_python_ast::helpers::collect_import_from_member;
use ruff_python_ast::name::QualifiedName;
use crate::{AnyImport, Imported};
/// A list of names imported via any import statement. /// A list of names imported via any import statement.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, CacheKey)] #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, CacheKey)]
@ -37,6 +41,41 @@ impl NameImports {
} }
} }
impl NameImport {
/// Returns the name under which the member is bound (e.g., given `from foo import bar as baz`, returns `baz`).
fn bound_name(&self) -> &str {
match self {
NameImport::Import(import) => {
import.name.as_name.as_deref().unwrap_or(&import.name.name)
}
NameImport::ImportFrom(import_from) => import_from
.name
.as_name
.as_deref()
.unwrap_or(&import_from.name.name),
}
}
/// Returns the [`QualifiedName`] of the imported name (e.g., given `import foo import bar as baz`, returns `["foo", "bar"]`).
fn qualified_name(&self) -> QualifiedName {
match self {
NameImport::Import(import) => QualifiedName::user_defined(&import.name.name),
NameImport::ImportFrom(import_from) => collect_import_from_member(
import_from.level,
import_from.module.as_deref(),
import_from.name.name.as_str(),
),
}
}
}
impl NameImport {
/// Returns `true` if the [`NameImport`] matches the specified name and binding.
pub fn matches(&self, name: &str, binding: &AnyImport) -> bool {
name == self.bound_name() && self.qualified_name() == *binding.qualified_name()
}
}
impl ModuleNameImport { impl ModuleNameImport {
/// Creates a new `Import` to import the specified module. /// Creates a new `Import` to import the specified module.
pub fn module(name: String) -> Self { pub fn module(name: String) -> Self {
@ -47,6 +86,15 @@ impl ModuleNameImport {
}, },
} }
} }
pub fn alias(name: String, as_name: String) -> Self {
Self {
name: Alias {
name,
as_name: Some(as_name),
},
}
}
} }
impl MemberNameImport { impl MemberNameImport {