Skip to content

Commit

Permalink
Group redefinition fixes by source statement (#15574)
Browse files Browse the repository at this point in the history
## Summary

Like unused imports, we should create a single fix for all redefined
members in a single statement.

Closes #15182.
  • Loading branch information
charliermarsh authored Jan 18, 2025
1 parent 8a50f3f commit 1344c8a
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 32 deletions.
85 changes: 60 additions & 25 deletions crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use ruff_diagnostics::{Diagnostic, Fix};
use ruff_python_semantic::analyze::visibility;
use ruff_python_semantic::{Binding, BindingKind, Imported, ResolvedReference, ScopeKind};
use ruff_text_size::Ranged;
use rustc_hash::FxHashMap;

use crate::checkers::ast::Checker;
use crate::codes::Rule;
Expand Down Expand Up @@ -194,6 +195,9 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
}

if checker.enabled(Rule::RedefinedWhileUnused) {
// Index the redefined bindings by statement.
let mut redefinitions = FxHashMap::default();

for (name, binding_id) in scope.bindings() {
for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) {
// If the shadowing binding is a loop variable, abort, to avoid overlap
Expand Down Expand Up @@ -271,9 +275,62 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
continue;
}

redefinitions
.entry(binding.source)
.or_insert_with(Vec::new)
.push((shadowed, binding));
}
}

// Create a fix for each source statement.
let mut fixes = FxHashMap::default();
for (source, entries) in &redefinitions {
let Some(source) = source else {
continue;
};

let member_names = entries
.iter()
.filter_map(|(shadowed, binding)| {
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() {
return Some(import.member_name());
}
}
}
None
})
.collect::<Vec<_>>();

if !member_names.is_empty() {
let statement = checker.semantic.statement(*source);
let parent = checker.semantic.parent_statement(*source);
let Ok(edit) = fix::edits::remove_unused_imports(
member_names.iter().map(std::convert::AsRef::as_ref),
statement,
parent,
checker.locator(),
checker.stylist(),
checker.indexer(),
) else {
continue;
};
fixes.insert(
*source,
Fix::safe_edit(edit).isolate(Checker::isolation(
checker.semantic().parent_statement_id(*source),
)),
);
}
}

// Create diagnostics for each statement.
for (source, entries) in &redefinitions {
for (shadowed, binding) in entries {
let mut diagnostic = Diagnostic::new(
pyflakes::rules::RedefinedWhileUnused {
name: (*name).to_string(),
name: binding.name(checker.source()).to_string(),
row: checker.compute_source_row(shadowed.start()),
},
binding.range(),
Expand All @@ -283,30 +340,8 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
diagnostic.set_parent(range.start());
}

// 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),
)))
});
}
}
}
if let Some(fix) = source.as_ref().and_then(|source| fixes.get(source)) {
diagnostic.set_fix(fix.clone());
}

diagnostics.push(diagnostic);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
snapshot_kind: text
---
F811_21.py:32:5: F811 [*] Redefinition of unused `Sequence` from line 26
|
Expand All @@ -13,12 +12,13 @@ F811_21.py:32:5: F811 [*] Redefinition of unused `Sequence` from line 26
= help: Remove definition: `Sequence`

Safe fix
27 27 | )
28 28 |
29 29 | # This should ignore the first error.
30 30 | from typing import (
31 31 | List, # noqa: F811
30 |-from typing import (
31 |- List, # noqa: F811
32 |- Sequence,
33 |-)
32 |+ )
34 33 |
35 34 | # This should ignore both errors.
36 35 | from typing import ( # noqa
34 30 |
35 31 | # This should ignore both errors.
36 32 | from typing import ( # noqa

0 comments on commit 1344c8a

Please sign in to comment.