Skip to content

Commit

Permalink
Avoid removing shadowed imports that point to different symbols (#10387)
Browse files Browse the repository at this point in the history
This ensures that we don't have incorrect, automated fixes for shadowed
names that actually point to different imports.

See: #10384.
  • Loading branch information
charliermarsh authored Mar 13, 2024
1 parent 2bf1882 commit d59433b
Show file tree
Hide file tree
Showing 8 changed files with 46 additions and 51 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
"""Regression test for: https://github.com/astral-sh/ruff/issues/10384"""

import datetime
from datetime import datetime

datetime(1, 2, 3)
40 changes: 23 additions & 17 deletions crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -259,23 +259,29 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
diagnostic.set_parent(range.start());
}

if let Some(import) = binding.as_any_import() {
if let Some(source) = binding.source {
diagnostic.try_set_fix(|| {
let statement = checker.semantic().statement(source);
let parent = checker.semantic().parent_statement(source);
let edit = fix::edits::remove_unused_imports(
std::iter::once(import.member_name().as_ref()),
statement,
parent,
checker.locator(),
checker.stylist(),
checker.indexer(),
)?;
Ok(Fix::safe_edit(edit).isolate(Checker::isolation(
checker.semantic().parent_statement_id(source),
)))
});
// Remove the import if the binding and the shadowed binding are both imports,
// and both point to the same qualified name.
if let Some(shadowed_import) = shadowed.as_any_import() {
if let Some(import) = binding.as_any_import() {
if shadowed_import.qualified_name() == import.qualified_name() {
if let Some(source) = binding.source {
diagnostic.try_set_fix(|| {
let statement = checker.semantic().statement(source);
let parent = checker.semantic().parent_statement(source);
let edit = fix::edits::remove_unused_imports(
std::iter::once(import.member_name().as_ref()),
statement,
parent,
checker.locator(),
checker.stylist(),
checker.indexer(),
)?;
Ok(Fix::safe_edit(edit).isolate(Checker::isolation(
checker.semantic().parent_statement_id(source),
)))
});
}
}
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ mod tests {
#[test_case(Rule::RedefinedWhileUnused, Path::new("F811_25.py"))]
#[test_case(Rule::RedefinedWhileUnused, Path::new("F811_26.py"))]
#[test_case(Rule::RedefinedWhileUnused, Path::new("F811_27.py"))]
#[test_case(Rule::RedefinedWhileUnused, Path::new("F811_28.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_0.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_1.py"))]
#[test_case(Rule::UndefinedName, Path::new("F821_2.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,9 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811_1.py:1:25: F811 [*] Redefinition of unused `FU` from line 1
F811_1.py:1:25: F811 Redefinition of unused `FU` from line 1
|
1 | import fu as FU, bar as FU
| ^^ F811
|
= help: Remove definition: `FU`

Safe fix
1 |-import fu as FU, bar as FU
1 |+import fu as FU


Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811_12.py:6:20: F811 [*] Redefinition of unused `mixer` from line 2
F811_12.py:6:20: F811 Redefinition of unused `mixer` from line 2
|
4 | pass
5 | else:
Expand All @@ -10,13 +10,3 @@ F811_12.py:6:20: F811 [*] Redefinition of unused `mixer` from line 2
7 | mixer(123)
|
= help: Remove definition: `mixer`

Safe fix
3 3 | except ImportError:
4 4 | pass
5 5 | else:
6 |- from bb import mixer
6 |+ pass
7 7 | mixer(123)


Original file line number Diff line number Diff line change
@@ -1,15 +1,9 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811_2.py:1:34: F811 [*] Redefinition of unused `FU` from line 1
F811_2.py:1:34: F811 Redefinition of unused `FU` from line 1
|
1 | from moo import fu as FU, bar as FU
| ^^ F811
|
= help: Remove definition: `FU`

Safe fix
1 |-from moo import fu as FU, bar as FU
1 |+from moo import fu as FU


Original file line number Diff line number Diff line change
@@ -1,18 +1,10 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811_23.py:4:15: F811 [*] Redefinition of unused `foo` from line 3
F811_23.py:4:15: F811 Redefinition of unused `foo` from line 3
|
3 | import foo as foo
4 | import bar as foo
| ^^^ F811
|
= help: Remove definition: `foo`

Safe fix
1 1 | """Test that shadowing an explicit re-export produces a warning."""
2 2 |
3 3 | import foo as foo
4 |-import bar as foo


Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F811_28.py:4:22: F811 Redefinition of unused `datetime` from line 3
|
3 | import datetime
4 | from datetime import datetime
| ^^^^^^^^ F811
5 |
6 | datetime(1, 2, 3)
|
= help: Remove definition: `datetime`

0 comments on commit d59433b

Please sign in to comment.