F401 + F811 - auto-fix removes both top the level import and any inside-function import

[Adamasterr]

Hello!

Here's a minimal example, applying auto-fix to the following:

import os  # F401


def function():
    import os  # F811

    print(os.name)

Results in the following:

def function():

    print(os.name)  # F821, here, "os" is not defined

This makes for a fix that breaks the code.

[charliermarsh]

Thanks, that makes sense.

[AlexWaygood]

This patch fixes things, in that the two fixes don't end up creating invalid code at the end of the day when they're combined:

--- a/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
+++ b/crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
@@ -1,4 +1,4 @@
-use ruff_diagnostics::{Diagnostic, Fix};
+use ruff_diagnostics::{Diagnostic, Edit, Fix};
 use ruff_python_semantic::analyze::visibility;
 use ruff_python_semantic::{Binding, BindingKind, Imported, ResolvedReference, ScopeKind};
 use ruff_text_size::Ranged;
@@ -294,9 +294,33 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
                                             checker.stylist(),
                                             checker.indexer(),
                                         )?;
-                                        Ok(Fix::safe_edit(edit).isolate(Checker::isolation(
-                                            checker.semantic().parent_statement_id(source),
-                                        )))
+                                        // We also add a no-op edit to force conflicts with the fix for `unused-imports`.
+                                        // Consider:
+                                        // ```python
+                                        // import sys
+                                        //
+                                        // def foo():
+                                        //     import sys
+                                        //     return sys.version_info
+                                        // ```
+                                        //
+                                        // Assume you omit this no-op edit. If you run Ruff with `unused-imports` and
+                                        // `redefined-while-unused` over this snippet, it will generate two fixes:
+                                        // (1) remove the unused `sys` import; and
+                                        // (2) remove the "redundant redefinition" of `sys`, under the assumption that
+                                        //     `sys` is already imported and available.
+                                        //
+                                        // By adding this no-op edit, we force the `unused-imports` fix to conflict
+                                        // with the `redefined-while-unused` fix, and thus will avoid applying both
+                                        // fixes in the same pass.
+                                        let noop_edit = Edit::range_replacement(
+                                            checker.locator().slice(shadowed).to_string(),
+                                            shadowed.range(),
+                                        );
+                                        Ok(Fix::safe_edits(edit, std::iter::once(noop_edit))
+                                            .isolate(Checker::isolation(
+                                                checker.semantic().parent_statement_id(source),
+                                            )))
                                     });

(Logic inspired by

ruff/crates/ruff_linter/src/importer/mod.rs

Lines 277 to 296 in 812b097

	// We also add a no-op edit to force conflicts with any other fixes that might try to
	// remove the import. Consider:
	//
	// ```python
	// import sys
	//
	// quit()
	// ```
	//
	// Assume you omit this no-op edit. If you run Ruff with `unused-imports` and
	// `sys-exit-alias` over this snippet, it will generate two fixes: (1) remove the unused
	// `sys` import; and (2) replace `quit()` with `sys.exit()`, under the assumption that `sys`
	// is already imported and available.
	//
	// By adding this no-op edit, we force the `unused-imports` fix to conflict with the
	// `sys-exit-alias` fix, and thus will avoid applying both fixes in the same pass.
	let import_edit = Edit::range_replacement(
	self.locator.slice(imported_name.range()).to_string(),
	imported_name.range(),
	);

)

It's still not ideal, though, as it ends up applying this fix:

--- foo.py
+++ foo.py
@@ -1,4 +1,3 @@
-import os  # F401
 
 
 def function():

when ideally it would remove the local import rather than the top-level import

[charliermarsh]

Maybe we should change F811 to only remove the "shadowed" import if they're in the same scope? I need to look back at why we changed that in the first place.