Automatically remove unused imports (#298)

This commit is contained in:
Charlie Marsh 2022-10-03 14:08:16 -04:00 committed by GitHub
parent bc335f839e
commit b049cced04
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 467 additions and 65 deletions

View file

@ -30,6 +30,17 @@ if TYPE_CHECKING:
from models import Fruit, Nut, Vegetable
if TYPE_CHECKING:
import shelve
import importlib
if TYPE_CHECKING:
"""Hello, world!"""
import pathlib
z = 1
class X:
datetime: datetime
foo: Type["NamedTuple"]
@ -39,6 +50,9 @@ class X:
y = Counter()
z = multiprocessing.pool.ThreadPool()
def b(self) -> None:
import pickle
__all__ = ["ClassA"] + ["ClassB"]
__all__ += ["ClassC"]

View file

@ -2,7 +2,10 @@ from __future__ import annotations
from dataclasses import dataclass
from models import Fruit, Nut
from models import (
Fruit,
Nut,
)
@dataclass

View file

@ -99,7 +99,7 @@ pub fn check_unused_variables(
{
checks.push(Check::new(
CheckKind::UnusedVariable(name.to_string()),
locator.locate_check(binding.location),
locator.locate_check(binding.range),
));
}
}

View file

@ -1,6 +1,6 @@
use rustpython_parser::ast::{Constant, Expr, ExprKind, Location, Stmt, StmtKind};
use crate::ast::types::{BindingKind, Scope};
use crate::ast::types::{BindingKind, Range, Scope};
/// Extract the names bound to a given __all__ assignment.
pub fn extract_all_names(stmt: &Stmt, scope: &Scope) -> Vec<String> {
@ -148,7 +148,7 @@ impl<'a> SourceCodeLocator<'a> {
&self.content[offset..]
}
pub fn slice_source_code_range(&mut self, start: &Location, end: &Location) -> &'a str {
pub fn slice_source_code_range(&mut self, range: &Range) -> &'a str {
if !self.initialized {
let mut offset = 0;
for i in self.content.lines() {
@ -158,8 +158,8 @@ impl<'a> SourceCodeLocator<'a> {
}
self.initialized = true;
}
let start = self.offsets[start.row() - 1] + start.column() - 1;
let end = self.offsets[end.row() - 1] + end.column() - 1;
let start = self.offsets[range.location.row() - 1] + range.location.column() - 1;
let end = self.offsets[range.end_location.row() - 1] + range.end_location.column() - 1;
&self.content[start..end]
}
}

View file

@ -8,7 +8,7 @@ fn id() -> usize {
COUNTER.fetch_add(1, Ordering::Relaxed)
}
#[derive(Clone, Copy, Debug, Default)]
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord)]
pub struct Range {
pub location: Location,
pub end_location: Location,
@ -55,6 +55,12 @@ impl Scope {
}
}
#[derive(Clone, Debug)]
pub struct BindingContext {
pub defined_by: usize,
pub defined_in: Option<usize>,
}
#[derive(Clone, Debug)]
pub enum BindingKind {
Annotation,
@ -67,15 +73,16 @@ pub enum BindingKind {
Definition,
Export(Vec<String>),
FutureImportation,
Importation(String),
StarImportation,
SubmoduleImportation(String),
Importation(String, BindingContext),
FromImportation(String, BindingContext),
SubmoduleImportation(String, BindingContext),
}
#[derive(Clone, Debug)]
pub struct Binding {
pub kind: BindingKind,
pub location: Range,
pub range: Range,
/// Tuple of (scope index, range) indicating the scope and range at which the binding was
/// last used.
pub used: Option<(usize, Range)>,
@ -84,3 +91,9 @@ pub struct Binding {
pub trait CheckLocator {
fn locate_check(&self, default: Range) -> Range;
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum ImportKind {
Import,
ImportFrom,
}

View file

@ -2,6 +2,7 @@ use std::fs;
use std::path::Path;
use anyhow::Result;
use itertools::Itertools;
use rustpython_parser::ast::Location;
use crate::checks::{Check, Fix};
@ -43,7 +44,7 @@ fn apply_fixes<'a>(fixes: impl Iterator<Item = &'a mut Fix>, contents: &str) ->
let mut output = "".to_string();
let mut last_pos: Location = Location::new(0, 0);
for fix in fixes {
for fix in fixes.sorted_by_key(|fix| fix.location) {
// Best-effort approach: if this fix overlaps with a fix we've already applied, skip it.
if last_pos > fix.location {
continue;
@ -71,14 +72,19 @@ fn apply_fixes<'a>(fixes: impl Iterator<Item = &'a mut Fix>, contents: &str) ->
fix.applied = true;
}
if last_pos.row() > 0 || last_pos.column() > 0 {
if last_pos.row() > 0
&& (last_pos.row() - 1) < lines.len()
&& (last_pos.row() > 0 || last_pos.column() > 0)
{
output.push_str(&lines[last_pos.row() - 1][last_pos.column() - 1..]);
output.push('\n');
}
if last_pos.row() < lines.len() {
for line in &lines[last_pos.row()..] {
output.push_str(line);
output.push('\n');
}
}
output
}

View file

@ -1,9 +1,14 @@
use anyhow::Result;
use itertools::Itertools;
use libcst_native::ImportNames::Aliases;
use libcst_native::NameOrAttribute::N;
use libcst_native::{Codegen, Expression, SmallStatement, Statement};
use rustpython_parser::ast::{Expr, Keyword, Location};
use rustpython_parser::ast::{ExcepthandlerKind, Expr, Keyword, Location, Stmt, StmtKind};
use rustpython_parser::lexer;
use rustpython_parser::token::Tok;
use crate::ast::operations::SourceCodeLocator;
use crate::ast::types::Range;
use crate::checks::Fix;
/// Convert a location within a file (relative to `base`) to an absolute position.
@ -127,7 +132,8 @@ pub fn remove_class_def_base(
}
pub fn remove_super_arguments(locator: &mut SourceCodeLocator, expr: &Expr) -> Option<Fix> {
let contents = locator.slice_source_code_range(&expr.location, &expr.end_location);
let range = Range::from_located(expr);
let contents = locator.slice_source_code_range(&range);
let mut tree = match libcst_native::parse_module(contents, None) {
Ok(m) => m,
@ -146,8 +152,8 @@ pub fn remove_super_arguments(locator: &mut SourceCodeLocator, expr: &Expr) -> O
return Some(Fix {
content: state.to_string(),
location: expr.location,
end_location: expr.end_location,
location: range.location,
end_location: range.end_location,
applied: false,
});
}
@ -156,3 +162,170 @@ pub fn remove_super_arguments(locator: &mut SourceCodeLocator, expr: &Expr) -> O
None
}
/// Determine if a body contains only a single statement, taking into account deleted.
fn has_single_child(body: &[Stmt], deleted: &[&Stmt]) -> bool {
body.iter().filter(|child| !deleted.contains(child)).count() == 1
}
/// Determine if a child is the only statement in its body.
fn is_lone_child(child: &Stmt, parent: &Stmt, deleted: &[&Stmt]) -> Result<bool> {
match &parent.node {
StmtKind::FunctionDef { body, .. }
| StmtKind::AsyncFunctionDef { body, .. }
| StmtKind::ClassDef { body, .. }
| StmtKind::With { body, .. }
| StmtKind::AsyncWith { body, .. } => {
if body.iter().contains(child) {
Ok(has_single_child(body, deleted))
} else {
Err(anyhow::anyhow!("Unable to find child in parent body."))
}
}
StmtKind::For { body, orelse, .. }
| StmtKind::AsyncFor { body, orelse, .. }
| StmtKind::While { body, orelse, .. }
| StmtKind::If { body, orelse, .. } => {
if body.iter().contains(child) {
Ok(has_single_child(body, deleted))
} else if orelse.iter().contains(child) {
Ok(has_single_child(orelse, deleted))
} else {
Err(anyhow::anyhow!("Unable to find child in parent body."))
}
}
StmtKind::Try {
body,
handlers,
orelse,
finalbody,
} => {
if body.iter().contains(child) {
Ok(has_single_child(body, deleted))
} else if orelse.iter().contains(child) {
Ok(has_single_child(orelse, deleted))
} else if finalbody.iter().contains(child) {
Ok(has_single_child(finalbody, deleted))
} else if let Some(body) = handlers.iter().find_map(|handler| match &handler.node {
ExcepthandlerKind::ExceptHandler { body, .. } => {
if body.iter().contains(child) {
Some(body)
} else {
None
}
}
}) {
Ok(has_single_child(body, deleted))
} else {
Err(anyhow::anyhow!("Unable to find child in parent body."))
}
}
_ => Err(anyhow::anyhow!("Unable to find child in parent body.")),
}
}
fn remove_stmt(stmt: &Stmt, parent: Option<&Stmt>, deleted: &[&Stmt]) -> Result<Fix> {
if parent
.map(|parent| is_lone_child(stmt, parent, deleted))
.map_or(Ok(None), |v| v.map(Some))?
.unwrap_or_default()
{
// If removing this node would lead to an invalid syntax tree, replace
// it with a `pass`.
Ok(Fix {
location: stmt.location,
end_location: stmt.end_location,
content: "pass".to_string(),
applied: false,
})
} else {
// Otherwise, nuke the entire line.
// TODO(charlie): This logic assumes that there are no multi-statement physical lines.
Ok(Fix {
location: Location::new(stmt.location.row(), 1),
end_location: Location::new(stmt.end_location.row() + 1, 1),
content: "".to_string(),
applied: false,
})
}
}
/// Generate a Fix to remove any unused imports from an `import` statement.
pub fn remove_unused_imports(stmt: &Stmt, parent: Option<&Stmt>, deleted: &[&Stmt]) -> Result<Fix> {
remove_stmt(stmt, parent, deleted)
}
/// Generate a Fix to remove any unused imports from an `import from` statement.
pub fn remove_unused_import_froms(
locator: &mut SourceCodeLocator,
full_names: &[String],
stmt: &Stmt,
parent: Option<&Stmt>,
deleted: &[&Stmt],
) -> Result<Fix> {
let mut tree = match libcst_native::parse_module(
locator.slice_source_code_range(&Range::from_located(stmt)),
None,
) {
Ok(m) => m,
Err(_) => return Err(anyhow::anyhow!("Failed to extract CST from source.")),
};
let body = if let Some(Statement::Simple(body)) = tree.body.first_mut() {
body
} else {
return Err(anyhow::anyhow!("Expected node to be: Statement::Simple."));
};
let body = if let Some(SmallStatement::ImportFrom(body)) = body.body.first_mut() {
body
} else {
return Err(anyhow::anyhow!(
"Expected node to be: SmallStatement::ImportFrom."
));
};
let aliases = if let Aliases(aliases) = &mut body.names {
aliases
} else {
return Err(anyhow::anyhow!("Expected node to be: Aliases."));
};
// Preserve the trailing comma (or not) from the last entry.
let trailing_comma = aliases.last().and_then(|alias| alias.comma.clone());
// Identify unused imports from within the `import from`.
let mut removable = vec![];
for (index, alias) in aliases.iter().enumerate() {
if let N(name) = &alias.name {
let import_name = if let Some(N(module_name)) = &body.module {
format!("{}.{}", module_name.value, name.value)
} else {
name.value.to_string()
};
if full_names.contains(&import_name) {
removable.push(index);
}
}
}
// TODO(charlie): This is quadratic.
for index in removable.iter().rev() {
aliases.remove(*index);
}
if let Some(alias) = aliases.last_mut() {
alias.comma = trailing_comma;
}
if aliases.is_empty() {
remove_stmt(stmt, parent, deleted)
} else {
let mut state = Default::default();
tree.codegen(&mut state);
Ok(Fix {
content: state.to_string(),
location: stmt.location,
end_location: stmt.end_location,
applied: false,
})
}
}

View file

@ -1,6 +1,8 @@
use std::collections::{BTreeMap, BTreeSet};
use std::ops::Deref;
use std::path::Path;
use log::error;
use once_cell::sync::Lazy;
use regex::Regex;
use rustpython_parser::ast::{
@ -12,11 +14,12 @@ use rustpython_parser::parser;
use crate::ast::operations::{extract_all_names, SourceCodeLocator};
use crate::ast::relocate::relocate_expr;
use crate::ast::types::{
Binding, BindingKind, CheckLocator, FunctionScope, Range, Scope, ScopeKind,
Binding, BindingContext, BindingKind, CheckLocator, FunctionScope, ImportKind, Range, Scope,
ScopeKind,
};
use crate::ast::visitor::{walk_excepthandler, Visitor};
use crate::ast::{checks, operations, visitor};
use crate::autofix::fixer;
use crate::autofix::{fixer, fixes};
use crate::checks::{Check, CheckCode, CheckKind};
use crate::python::builtins::{BUILTINS, MAGIC_GLOBALS};
use crate::python::future::ALL_FEATURE_NAMES;
@ -55,6 +58,8 @@ struct Checker<'a> {
seen_docstring: bool,
futures_allowed: bool,
annotations_future_enabled: bool,
// Edit tracking.
deletions: BTreeSet<usize>,
}
impl<'a> Checker<'a> {
@ -87,6 +92,7 @@ impl<'a> Checker<'a> {
seen_docstring: false,
futures_allowed: true,
annotations_future_enabled: false,
deletions: Default::default(),
}
}
}
@ -221,7 +227,7 @@ where
Binding {
kind: BindingKind::Assignment,
used: Some((global_scope_id, Range::from_located(stmt))),
location: Range::from_located(stmt),
range: Range::from_located(stmt),
},
);
}
@ -329,7 +335,7 @@ where
Binding {
kind: BindingKind::Definition,
used: None,
location: Range::from_located(stmt),
range: Range::from_located(stmt),
},
);
}
@ -424,9 +430,10 @@ where
Binding {
kind: BindingKind::SubmoduleImportation(
alias.node.name.to_string(),
self.binding_context(),
),
used: None,
location: Range::from_located(stmt),
range: Range::from_located(stmt),
},
)
} else {
@ -447,9 +454,10 @@ where
.asname
.clone()
.unwrap_or_else(|| alias.node.name.clone()),
self.binding_context(),
),
used: None,
location: Range::from_located(stmt),
range: Range::from_located(stmt),
},
)
}
@ -492,7 +500,7 @@ where
.id,
Range::from_located(stmt),
)),
location: Range::from_located(stmt),
range: Range::from_located(stmt),
},
);
@ -528,7 +536,7 @@ where
Binding {
kind: BindingKind::StarImportation,
used: None,
location: Range::from_located(stmt),
range: Range::from_located(stmt),
},
);
@ -561,12 +569,15 @@ where
}
let binding = Binding {
kind: BindingKind::Importation(match module {
kind: BindingKind::FromImportation(
match module {
None => name.clone(),
Some(parent) => format!("{}.{}", parent, name),
}),
},
self.binding_context(),
),
used: None,
location: Range::from_located(stmt),
range: Range::from_located(stmt),
};
self.add_binding(name, binding)
}
@ -661,7 +672,7 @@ where
Binding {
kind: BindingKind::ClassDefinition,
used: None,
location: Range::from_located(stmt),
range: Range::from_located(stmt),
},
);
};
@ -1106,7 +1117,6 @@ where
),
parent,
);
self.parents.push(parent);
}
let parent =
@ -1125,7 +1135,6 @@ where
),
parent,
);
self.parents.push(parent);
walk_excepthandler(self, excepthandler);
@ -1183,7 +1192,7 @@ where
Binding {
kind: BindingKind::Argument,
used: None,
location: Range::from_located(arg),
range: Range::from_located(arg),
},
);
@ -1239,7 +1248,7 @@ impl<'a> Checker<'a> {
(*builtin).to_string(),
Binding {
kind: BindingKind::Builtin,
location: Default::default(),
range: Default::default(),
used: None,
},
);
@ -1249,13 +1258,23 @@ impl<'a> Checker<'a> {
(*builtin).to_string(),
Binding {
kind: BindingKind::Builtin,
location: Default::default(),
range: Default::default(),
used: None,
},
);
}
}
fn binding_context(&self) -> BindingContext {
let mut rev = self.parent_stack.iter().rev().fuse();
let defined_by = *rev.next().expect("Expected to bind within a statement.");
let defined_in = rev.next().cloned();
BindingContext {
defined_by,
defined_in,
}
}
fn add_binding(&mut self, name: String, binding: Binding) {
let scope = &mut self.scopes[*(self.scope_stack.last().expect("No current scope found."))];
@ -1264,20 +1283,23 @@ impl<'a> Checker<'a> {
None => binding,
Some(existing) => {
if self.settings.select.contains(&CheckCode::F402)
&& matches!(existing.kind, BindingKind::Importation(_))
&& matches!(binding.kind, BindingKind::LoopVar)
&& matches!(
existing.kind,
BindingKind::Importation(_, _) | BindingKind::FromImportation(_, _)
)
{
self.checks.push(Check::new(
CheckKind::ImportShadowedByLoopVar(
name.clone(),
existing.location.location.row(),
existing.range.location.row(),
),
binding.location,
binding.range,
));
}
Binding {
kind: binding.kind,
location: binding.location,
range: binding.range,
used: existing.used,
}
}
@ -1378,7 +1400,7 @@ impl<'a> Checker<'a> {
Binding {
kind: BindingKind::Annotation,
used: None,
location: Range::from_located(expr),
range: Range::from_located(expr),
},
);
return;
@ -1394,7 +1416,7 @@ impl<'a> Checker<'a> {
Binding {
kind: BindingKind::LoopVar,
used: None,
location: Range::from_located(expr),
range: Range::from_located(expr),
},
);
return;
@ -1406,7 +1428,7 @@ impl<'a> Checker<'a> {
Binding {
kind: BindingKind::Binding,
used: None,
location: Range::from_located(expr),
range: Range::from_located(expr),
},
);
return;
@ -1426,7 +1448,7 @@ impl<'a> Checker<'a> {
Binding {
kind: BindingKind::Export(extract_all_names(parent, current)),
used: None,
location: Range::from_located(expr),
range: Range::from_located(expr),
},
);
return;
@ -1437,7 +1459,7 @@ impl<'a> Checker<'a> {
Binding {
kind: BindingKind::Assignment,
used: None,
location: Range::from_located(expr),
range: Range::from_located(expr),
},
);
}
@ -1571,7 +1593,7 @@ impl<'a> Checker<'a> {
if !scope.values.contains_key(name) {
self.checks.push(Check::new(
CheckKind::UndefinedExport(name.to_string()),
self.locate_check(all_binding.location),
self.locate_check(all_binding.range),
));
}
}
@ -1594,7 +1616,7 @@ impl<'a> Checker<'a> {
if !scope.values.contains_key(name) {
self.checks.push(Check::new(
CheckKind::ImportStarUsage(name.clone(), from_list.join(", ")),
self.locate_check(all_binding.location),
self.locate_check(all_binding.range),
));
}
}
@ -1603,6 +1625,11 @@ impl<'a> Checker<'a> {
}
if self.settings.select.contains(&CheckCode::F401) {
// Collect all unused imports by location. (Multiple unused imports at the same
// location indicates an `import from`.)
let mut unused: BTreeMap<(ImportKind, usize, Option<usize>), Vec<String>> =
BTreeMap::new();
for (name, binding) in scope.values.iter().rev() {
let used = binding.used.is_some()
|| all_names
@ -1611,17 +1638,82 @@ impl<'a> Checker<'a> {
if !used {
match &binding.kind {
BindingKind::Importation(full_name)
| BindingKind::SubmoduleImportation(full_name) => {
self.checks.push(Check::new(
CheckKind::UnusedImport(full_name.to_string()),
self.locate_check(binding.location),
));
BindingKind::FromImportation(full_name, context) => {
let full_names = unused
.entry((
ImportKind::ImportFrom,
context.defined_by,
context.defined_in,
))
.or_insert(vec![]);
full_names.push(full_name.to_string());
}
BindingKind::Importation(full_name, context)
| BindingKind::SubmoduleImportation(full_name, context) => {
let full_names = unused
.entry((
ImportKind::Import,
context.defined_by,
context.defined_in,
))
.or_insert(vec![]);
full_names.push(full_name.to_string());
}
_ => {}
}
}
}
for ((kind, defined_by, defined_in), full_names) in unused {
let child = self.parents[defined_by];
let parent = defined_in.map(|defined_in| self.parents[defined_in]);
let mut check = Check::new(
CheckKind::UnusedImport(full_names.join(", ")),
self.locate_check(Range::from_located(child)),
);
if matches!(self.autofix, fixer::Mode::Generate | fixer::Mode::Apply) {
let deleted: Vec<&Stmt> = self
.deletions
.iter()
.map(|index| self.parents[*index])
.collect();
match kind {
ImportKind::Import => {
match fixes::remove_unused_imports(child, parent, &deleted) {
Ok(fix) => {
if fix.content.is_empty() || fix.content == "pass" {
self.deletions.insert(defined_by);
}
check.amend(fix)
}
Err(e) => error!("Failed to fix unused imports: {}", e),
}
}
ImportKind::ImportFrom => {
match fixes::remove_unused_import_froms(
&mut self.locator,
&full_names,
child,
parent,
&deleted,
) {
Ok(fix) => {
if fix.content.is_empty() || fix.content == "pass" {
self.deletions.insert(defined_by);
}
check.amend(fix)
}
Err(e) => error!("Failed to fix unused imports: {}", e),
}
}
}
}
self.checks.push(check);
}
}
}
}
@ -1666,12 +1758,12 @@ impl<'a> Checker<'a> {
pub fn check_ast(
python_ast: &Suite,
content: &str,
contents: &str,
settings: &Settings,
autofix: &fixer::Mode,
path: &Path,
) -> Vec<Check> {
let mut checker = Checker::new(settings, autofix, path, content);
let mut checker = Checker::new(settings, autofix, path, contents);
checker.push_scope(Scope::new(ScopeKind::Module));
checker.bind_builtins();

View file

@ -713,6 +713,7 @@ impl CheckKind {
| CheckKind::UselessObjectInheritance(_)
| CheckKind::UnusedNOQA(_)
| CheckKind::SuperCallWithParameters
| CheckKind::UnusedImport(_)
)
}
}

View file

@ -10,7 +10,15 @@ expression: checks
end_location:
row: 3
column: 17
fix: ~
fix:
content: ""
location:
row: 3
column: 1
end_location:
row: 4
column: 1
applied: false
- kind:
UnusedImport: collections.OrderedDict
location:
@ -19,7 +27,15 @@ expression: checks
end_location:
row: 9
column: 2
fix: ~
fix:
content: "from collections import (\n Counter,\n namedtuple,\n)"
location:
row: 5
column: 1
end_location:
row: 9
column: 2
applied: false
- kind:
UnusedImport: logging.handlers
location:
@ -28,5 +44,81 @@ expression: checks
end_location:
row: 13
column: 24
fix: ~
fix:
content: ""
location:
row: 13
column: 1
end_location:
row: 14
column: 1
applied: false
- kind:
UnusedImport: shelve
location:
row: 34
column: 5
end_location:
row: 34
column: 18
fix:
content: ""
location:
row: 34
column: 1
end_location:
row: 35
column: 1
applied: false
- kind:
UnusedImport: importlib
location:
row: 35
column: 5
end_location:
row: 35
column: 21
fix:
content: pass
location:
row: 35
column: 5
end_location:
row: 35
column: 21
applied: false
- kind:
UnusedImport: pathlib
location:
row: 39
column: 5
end_location:
row: 39
column: 19
fix:
content: ""
location:
row: 39
column: 1
end_location:
row: 40
column: 1
applied: false
- kind:
UnusedImport: pickle
location:
row: 54
column: 9
end_location:
row: 54
column: 22
fix:
content: pass
location:
row: 54
column: 9
end_location:
row: 54
column: 22
applied: false

View file

@ -8,16 +8,24 @@ expression: checks
row: 5
column: 1
end_location:
row: 8
column: 2
fix:
content: "from models import (\n Fruit,\n)"
location:
row: 5
column: 30
fix: ~
column: 1
end_location:
row: 8
column: 2
applied: false
- kind:
UndefinedName: Bar
location:
row: 22
row: 25
column: 19
end_location:
row: 22
row: 25
column: 22
fix: ~