Skip to content

Commit

Permalink
[pyflakes] Fix check of unused imports
Browse files Browse the repository at this point in the history 
Fix error when importing modules without submodules.
Add attribute check for similar imports with submodule.
  • Loading branch information
@gpilikin
gpilikin committed Oct 29, 2024
1 parent 2f88f84 commit 1dea0e6
Show file tree
Hide file tree
Showing 5 changed files with 445 additions and 208 deletions.
16 changes: 16 additions & 0 deletions crates/ruff_linter/src/checkers/ast/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1114,6 +1114,15 @@ impl<'a> Visitor<'a> for Checker<'a> {
}
}
}
Expr::Attribute(ast::ExprAttribute {
value: _,
range: _,
ctx,
attr: _,
}) => match ctx {
ExprContext::Load => self.handle_attribute_load(expr),
_ => {}
},
Expr::Name(ast::ExprName { id, ctx, range: _ }) => match ctx {
ExprContext::Load => self.handle_node_load(expr),
ExprContext::Store => self.handle_node_store(id, expr),
Expand Down Expand Up @@ -1980,6 +1989,13 @@ impl<'a> Checker<'a> {
self.semantic.resolve_load(expr);
}

fn handle_attribute_load(&mut self, expr: &Expr) {
let Expr::Attribute(expr) = expr else {
return;
};
self.semantic.resolve_attribute_load(expr);
}

fn handle_node_store(&mut self, id: &'a str, expr: &Expr) {
let parent = self.semantic.current_statement();

Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2654,7 +2654,7 @@ lambda: fu
import foo.baz as foo
foo
",
&[Rule::RedefinedWhileUnused],
&[Rule::UnusedImport, Rule::RedefinedWhileUnused],
);
}

Expand Down Expand Up @@ -2704,13 +2704,13 @@ lambda: fu

#[test]
fn unused_package_with_submodule_import() {
// When a package and its submodule are imported, only report once.
// When a package and its submodule are imported, report both.
flakes(
r"
import fu
import fu.bar
",
&[Rule::UnusedImport],
&[Rule::UnusedImport, Rule::UnusedImport],
);
}

Expand Down
139 changes: 80 additions & 59 deletions crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use std::borrow::Cow;
use std::iter;
use unicode_normalization::UnicodeNormalization;

use anyhow::{anyhow, bail, Result};
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashSet};

use ruff_diagnostics::{Applicability, Diagnostic, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
Expand Down Expand Up @@ -275,75 +276,95 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
// Collect all unused imports by statement.
let mut unused: BTreeMap<(NodeId, Exceptions), Vec<ImportBinding>> = BTreeMap::default();
let mut ignored: BTreeMap<(NodeId, Exceptions), Vec<ImportBinding>> = BTreeMap::default();
let mut already_checked_imports: HashSet<String> = HashSet::new();

for binding_id in scope.binding_ids() {
let binding = checker.semantic().binding(binding_id);
let top_binding = checker.semantic().binding(binding_id);

if binding.is_used()
|| binding.is_explicit_export()
|| binding.is_nonlocal()
|| binding.is_global()
{
continue;
}

let Some(import) = binding.as_any_import() else {
let Some(_) = top_binding.as_any_import() else {
continue;
};

let Some(node_id) = binding.source else {
continue;
};

let name = binding.name(checker.locator());
let binding_name = top_binding.name(checker.locator()).split(".").next().unwrap_or("");

// 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))
for binding in scope
.get_all(&binding_name.nfkc().collect::<String>())
.map(|binding_id| checker.semantic().binding(binding_id))
{
continue;
}
let name = binding.name(checker.locator());

// If an import was marked as allowed, avoid treating it as unused.
if checker
.settings
.pyflakes
.allowed_unused_imports
.iter()
.any(|allowed_unused_import| {
let allowed_unused_import = QualifiedName::from_dotted_name(allowed_unused_import);
import.qualified_name().starts_with(&allowed_unused_import)
})
{
continue;
}
if already_checked_imports.contains(name)
{
continue;
}
{
already_checked_imports.insert(name.to_string());
}

let import = ImportBinding {
name,
import,
range: binding.range(),
parent_range: binding.parent_range(checker.semantic()),
};
if binding.is_used()
|| binding.is_explicit_export()
|| binding.is_nonlocal()
|| binding.is_global()
{
continue;
}

if checker.rule_is_ignored(Rule::UnusedImport, import.start())
|| import.parent_range.is_some_and(|parent_range| {
checker.rule_is_ignored(Rule::UnusedImport, parent_range.start())
})
{
ignored
.entry((node_id, binding.exceptions))
.or_default()
.push(import);
} else {
unused
.entry((node_id, binding.exceptions))
.or_default()
.push(import);
let Some(import) = binding.as_any_import() else {
continue;
};

let Some(stmt_id) = binding.source else {
continue;
};

// 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;
}

// If an import was marked as allowed, avoid treating it as unused.
if checker
.settings
.pyflakes
.allowed_unused_imports
.iter()
.any(|allowed_unused_import| {
let allowed_unused_import = QualifiedName::from_dotted_name(allowed_unused_import);
import.qualified_name().starts_with(&allowed_unused_import)
})
{
continue;
}

let import = ImportBinding {
name,
import,
range: binding.range(),
parent_range: binding.parent_range(checker.semantic()),
};

if checker.rule_is_ignored(Rule::UnusedImport, import.start())
|| import.parent_range.is_some_and(|parent_range| {
checker.rule_is_ignored(Rule::UnusedImport, parent_range.start())
})
{
ignored
.entry((stmt_id, binding.exceptions))
.or_default()
.push(import);
} else {
unused
.entry((stmt_id, binding.exceptions))
.or_default()
.push(import);
}
}
}

Expand Down
42 changes: 40 additions & 2 deletions ...ter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F401_F401_0.py.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,46 @@ F401_0.py:6:5: F401 [*] `collections.OrderedDict` imported but unused
8 7 | )
9 8 | import multiprocessing.pool

F401_0.py:10:8: F401 [*] `multiprocessing.process` imported but unused
|
8 | )
9 | import multiprocessing.pool
10 | import multiprocessing.process
| ^^^^^^^^^^^^^^^^^^^^^^^ F401
11 | import logging.config
12 | import logging.handlers
|
= help: Remove unused import: `multiprocessing.process`

ℹ Safe fix
7 7 | namedtuple,
8 8 | )
9 9 | import multiprocessing.pool
10 |-import multiprocessing.process
11 10 | import logging.config
12 11 | import logging.handlers
13 12 | from typing import (

F401_0.py:11:8: F401 [*] `logging.config` imported but unused
|
9 | import multiprocessing.pool
10 | import multiprocessing.process
11 | import logging.config
| ^^^^^^^^^^^^^^ F401
12 | import logging.handlers
13 | from typing import (
|
= help: Remove unused import: `logging.config`

ℹ Safe fix
8 8 | )
9 9 | import multiprocessing.pool
10 10 | import multiprocessing.process
11 |-import logging.config
12 11 | import logging.handlers
13 12 | from typing import (
14 13 | TYPE_CHECKING,

F401_0.py:12:8: F401 [*] `logging.handlers` imported but unused
|
10 | import multiprocessing.process
Expand Down Expand Up @@ -282,5 +322,3 @@ F401_0.py:122:1: F401 [*] `datameta_client_lib.model_helpers.noqa` imported but
120 120 |
121 |-from datameta_client_lib.model_helpers import (
122 |-noqa )


Loading

0 comments on commit 1dea0e6

Please sign in to comment.