Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid marking required imports as unused #12537

Merged
merged 1 commit into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Ignore
  • Loading branch information
charliermarsh committed Jul 26, 2024
commit 754672193e3f6f8206466c9e4f727da944a8fb4b
Original file line number Diff line number Diff line change
@@ -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
34 changes: 34 additions & 0 deletions crates/ruff_linter/src/rules/isort/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -906,6 +906,40 @@ mod tests {
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"))]
fn from_first(path: &Path) -> Result<()> {
let snapshot = format!("from_first_{}", path.to_string_lossy());
Expand Down
Original file line number Diff line number Diff line change
@@ -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
16 changes: 15 additions & 1 deletion crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -242,8 +242,22 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
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 {
name: binding.name(checker.locator()),
name,
import,
range: binding.range(),
parent_range: binding.parent_range(checker.semantic()),
Expand Down
48 changes: 48 additions & 0 deletions crates/ruff_python_semantic/src/imports.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
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.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, CacheKey)]
Expand Down Expand Up @@ -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 {
/// Creates a new `Import` to import the specified module.
pub fn module(name: String) -> Self {
Expand All @@ -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 {
Expand Down
Loading