Skip to content

Commit

Permalink
Fix isolation groups for unused imports
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Aug 22, 2023
1 parent fa32cd9 commit 649f421
Show file tree
Hide file tree
Showing 18 changed files with 219 additions and 27 deletions.
13 changes: 13 additions & 0 deletions crates/ruff/resources/test/fixtures/pyflakes/F401_0.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,16 @@ def b(self) -> None:
import foo.bar.baz

print(bop.baz.read_csv("test.csv"))

# Test: isolated deletions.
if TYPE_CHECKING:
import a1

import a2


match *0, 1, *2:
case 0,:
import b1

import b2
11 changes: 11 additions & 0 deletions crates/ruff/resources/test/fixtures/pyflakes/F841_3.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,14 @@ def f() -> None:
print("hello")
except A as e :
print("oh no!")


def f():
x = 1
y = 2


def f():
x = 1

y = 2
13 changes: 4 additions & 9 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ use ruff_python_parser::typing::{parse_type_annotation, AnnotationKind};
use ruff_python_semantic::analyze::{typing, visibility};
use ruff_python_semantic::{
BindingFlags, BindingId, BindingKind, Exceptions, Export, FromImport, Globals, Import, Module,
ModuleKind, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, StarImport, SubmoduleImport,
ModuleKind, NodeId, ScopeId, ScopeKind, SemanticModel, SemanticModelFlags, StarImport,
SubmoduleImport,
};
use ruff_python_stdlib::builtins::{BUILTINS, MAGIC_GLOBALS};
use ruff_source_file::Locator;
Expand Down Expand Up @@ -199,14 +200,8 @@ impl<'a> Checker<'a> {
/// in a given indented block, which would cause a syntax error. We therefore need to ensure
/// that we delete at most one statement per indented block per fixer pass. Fix isolation should
/// thus be applied whenever we delete a statement, but can otherwise be omitted.
pub(crate) fn statement_isolation(&self) -> IsolationLevel {
IsolationLevel::Group(self.semantic.current_statement_id().into())
}

/// Returns the [`IsolationLevel`] to isolate fixes in the current statement's parent.
pub(crate) fn parent_isolation(&self) -> IsolationLevel {
self.semantic
.current_statement_parent_id()
pub(crate) fn isolation(&self, node_id: Option<NodeId>) -> IsolationLevel {
node_id
.map(|node_id| IsolationLevel::Group(node_id.into()))
.unwrap_or_default()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ pub(crate) fn duplicate_class_field_definition(checker: &mut Checker, body: &[St
checker.locator(),
checker.indexer(),
);
diagnostic.set_fix(Fix::suggested(edit).isolate(checker.statement_isolation()));
diagnostic.set_fix(
Fix::suggested(edit).isolate(
checker.isolation(Some(checker.semantic().current_statement_id())),
),
);
}
checker.diagnostics.push(diagnostic);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ pub(crate) fn ellipsis_in_non_empty_class_body(checker: &mut Checker, body: &[St
checker.locator(),
checker.indexer(),
);
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.statement_isolation()));
diagnostic.set_fix(
Fix::automatic(edit).isolate(
checker.isolation(Some(checker.semantic().current_statement_id())),
),
);
}
checker.diagnostics.push(diagnostic);
}
Expand Down
5 changes: 4 additions & 1 deletion crates/ruff/src/rules/flake8_pyi/rules/pass_in_class_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ pub(crate) fn pass_in_class_body(checker: &mut Checker, class_def: &ast::StmtCla
if checker.patch(diagnostic.kind.rule()) {
let edit =
autofix::edits::delete_stmt(stmt, Some(stmt), checker.locator(), checker.indexer());
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.statement_isolation()));
diagnostic.set_fix(
Fix::automatic(edit)
.isolate(checker.isolation(Some(checker.semantic().current_statement_id()))),
);
}
checker.diagnostics.push(diagnostic);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,10 @@ pub(crate) fn str_or_repr_defined_in_stub(checker: &mut Checker, stmt: &Stmt) {
let stmt = checker.semantic().current_statement();
let parent = checker.semantic().current_statement_parent();
let edit = delete_stmt(stmt, parent, checker.locator(), checker.indexer());
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.parent_isolation()));
diagnostic.set_fix(
Fix::automatic(edit)
.isolate(checker.isolation(checker.semantic().current_statement_parent_id())),
);
}
checker.diagnostics.push(diagnostic);
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ pub(crate) fn empty_type_checking_block(checker: &mut Checker, stmt: &ast::StmtI
let stmt = checker.semantic().current_statement();
let parent = checker.semantic().current_statement_parent();
let edit = autofix::edits::delete_stmt(stmt, parent, checker.locator(), checker.indexer());
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.parent_isolation()));
diagnostic.set_fix(
Fix::automatic(edit)
.isolate(checker.isolation(checker.semantic().current_statement_parent_id())),
);
}
checker.diagnostics.push(diagnostic);
}
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,6 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->

Ok(
Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits())
.isolate(checker.parent_isolation()),
.isolate(checker.isolation(checker.semantic().parent_statement_id(node_id))),
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,6 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->

Ok(
Fix::suggested_edits(remove_import_edit, add_import_edit.into_edits())
.isolate(checker.parent_isolation()),
.isolate(checker.isolation(checker.semantic().parent_statement_id(node_id))),
)
}
10 changes: 9 additions & 1 deletion crates/ruff/src/rules/pyflakes/rules/unused_import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope, diagnostics: &mut
}

/// An unused import with its surrounding context.
#[derive(Debug)]
struct ImportBinding<'a> {
/// The qualified name of the import (e.g., `typing.List` for `from typing import List`).
import: AnyImport<'a>,
Expand All @@ -234,6 +235,12 @@ impl Ranged for ImportBinding<'_> {

/// Generate a [`Fix`] to remove unused imports from a statement.
fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) -> Result<Fix> {
println!("import: {:?}", imports);
println!(
"curr: {:?}",
checker.semantic().current_statement_parent_id()
);

let statement = checker.semantic().statement(node_id);
let parent = checker.semantic().parent_statement(node_id);

Expand All @@ -251,5 +258,6 @@ fn fix_imports(checker: &Checker, node_id: NodeId, imports: &[ImportBinding]) ->
checker.stylist(),
checker.indexer(),
)?;
Ok(Fix::automatic(edit).isolate(checker.parent_isolation()))
Ok(Fix::automatic(edit)
.isolate(checker.isolation(checker.semantic().parent_statement_id(node_id))))
}
8 changes: 2 additions & 6 deletions crates/ruff/src/rules/pyflakes/rules/unused_variable.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use itertools::Itertools;

use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, IsolationLevel, Violation};
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::contains_effect;
use ruff_python_ast::{self as ast, PySourceType, Ranged, Stmt};
Expand Down Expand Up @@ -206,11 +206,7 @@ fn remove_unused_variable(binding: &Binding, checker: &Checker) -> Option<Fix> {
let node_id = binding.source?;
let statement = checker.semantic().statement(node_id);
let parent = checker.semantic().parent_statement(node_id);
let isolation = checker
.semantic()
.parent_statement_id(node_id)
.map(|node_id| IsolationLevel::Group(node_id.into()))
.unwrap_or_default();
let isolation = checker.isolation(checker.semantic().parent_statement_id(node_id));

// First case: simple assignment (`x = 1`)
if let Stmt::Assign(ast::StmtAssign { targets, value, .. }) = statement {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,5 +190,78 @@ F401_0.py:99:8: F401 [*] `foo.bar.baz` imported but unused
99 |-import foo.bar.baz
100 99 |
101 100 | print(bop.baz.read_csv("test.csv"))
102 101 |

F401_0.py:105:12: F401 [*] `a1` imported but unused
|
103 | # Test: isolated deletions.
104 | if TYPE_CHECKING:
105 | import a1
| ^^ F401
106 |
107 | import a2
|
= help: Remove unused import: `a1`

Fix
102 102 |
103 103 | # Test: isolated deletions.
104 104 | if TYPE_CHECKING:
105 |- import a1
106 105 |
107 106 | import a2
108 107 |

F401_0.py:107:12: F401 [*] `a2` imported but unused
|
105 | import a1
106 |
107 | import a2
| ^^ F401
|
= help: Remove unused import: `a2`

Fix
104 104 | if TYPE_CHECKING:
105 105 | import a1
106 106 |
107 |- import a2
108 107 |
109 108 |
110 109 | match *0, 1, *2:

F401_0.py:112:16: F401 [*] `b1` imported but unused
|
110 | match *0, 1, *2:
111 | case 0,:
112 | import b1
| ^^ F401
113 |
114 | import b2
|
= help: Remove unused import: `b1`

Fix
109 109 |
110 110 | match *0, 1, *2:
111 111 | case 0,:
112 |- import b1
113 112 |
114 113 | import b2

F401_0.py:114:16: F401 [*] `b2` imported but unused
|
112 | import b1
113 |
114 | import b2
| ^^ F401
|
= help: Remove unused import: `b2`

Fix
111 111 | case 0,:
112 112 | import b1
113 113 |
114 |- import b2


Original file line number Diff line number Diff line change
Expand Up @@ -601,5 +601,76 @@ F841_3.py:155:17: F841 [*] Local variable `e` is assigned to but never used
155 |- except A as e :
155 |+ except A:
156 156 | print("oh no!")
157 157 |
158 158 |

F841_3.py:160:5: F841 [*] Local variable `x` is assigned to but never used
|
159 | def f():
160 | x = 1
| ^ F841
161 | y = 2
|
= help: Remove assignment to unused variable `x`

Suggested fix
157 157 |
158 158 |
159 159 | def f():
160 |- x = 1
161 160 | y = 2
162 161 |
163 162 |

F841_3.py:161:5: F841 [*] Local variable `y` is assigned to but never used
|
159 | def f():
160 | x = 1
161 | y = 2
| ^ F841
|
= help: Remove assignment to unused variable `y`

Suggested fix
158 158 |
159 159 | def f():
160 160 | x = 1
161 |- y = 2
162 161 |
163 162 |
164 163 | def f():

F841_3.py:165:5: F841 [*] Local variable `x` is assigned to but never used
|
164 | def f():
165 | x = 1
| ^ F841
166 |
167 | y = 2
|
= help: Remove assignment to unused variable `x`

Suggested fix
162 162 |
163 163 |
164 164 | def f():
165 |- x = 1
166 165 |
167 166 | y = 2

F841_3.py:167:5: F841 [*] Local variable `y` is assigned to but never used
|
165 | x = 1
166 |
167 | y = 2
| ^ F841
|
= help: Remove assignment to unused variable `y`

Suggested fix
164 164 | def f():
165 165 | x = 1
166 166 |
167 |- y = 2


5 changes: 4 additions & 1 deletion crates/ruff/src/rules/pylint/rules/useless_return.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,10 @@ pub(crate) fn useless_return(
checker.locator(),
checker.indexer(),
);
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.statement_isolation()));
diagnostic.set_fix(
Fix::automatic(edit)
.isolate(checker.isolation(Some(checker.semantic().current_statement_id()))),
);
}
checker.diagnostics.push(diagnostic);
}
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ pub(crate) fn unnecessary_builtin_import(
checker.stylist(),
checker.indexer(),
)?;
Ok(Fix::suggested(edit).isolate(checker.parent_isolation()))
Ok(Fix::suggested(edit)
.isolate(checker.isolation(checker.semantic().current_statement_parent_id())))
});
}
checker.diagnostics.push(diagnostic);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ pub(crate) fn unnecessary_future_import(checker: &mut Checker, stmt: &Stmt, name
checker.stylist(),
checker.indexer(),
)?;
Ok(Fix::suggested(edit).isolate(checker.parent_isolation()))
Ok(Fix::suggested(edit)
.isolate(checker.isolation(checker.semantic().current_statement_parent_id())))
});
}
checker.diagnostics.push(diagnostic);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ pub(crate) fn useless_metaclass_type(
let stmt = checker.semantic().current_statement();
let parent = checker.semantic().current_statement_parent();
let edit = autofix::edits::delete_stmt(stmt, parent, checker.locator(), checker.indexer());
diagnostic.set_fix(Fix::automatic(edit).isolate(checker.parent_isolation()));
diagnostic.set_fix(
Fix::automatic(edit)
.isolate(checker.isolation(checker.semantic().current_statement_parent_id())),
);
}
checker.diagnostics.push(diagnostic);
}

0 comments on commit 649f421

Please sign in to comment.