Strange autofix for PYI025 if "Set" is included in __all__

[AlexWaygood]

The following snippet correctly causes Ruff to emit PYI025:

from collections.abc import Set as Set

__all__ = ["Set"]

But if you apply the autofix, it "fixes" it to this, which is... not correct:

from collections.abc import Set as AbstractSet

AbstractSet = ["Set"]

[MichaReiser]

What's the correct fix in your view or should the rule not emit a fix if it's a module scoped variable named __all__?

[AlexWaygood]

Including the name "Set" in __all__ marks the symbol as being publicly exported from the module, but it doesn't create a binding in and of itself -- the Set binding is created via the import. So we definitely shouldn't be removing the __all__ variable from the module here.

If we stipulate that it's correct for us to emit the violation here at all, the correct fix would be to modify the original snippet to

from collections.abc import Set as AbstractSet

__all__ = ["AbstractSet"]

rather than what we currently do, which is to modify the snippet to this:

from collections.abc import Set as AbstractSet

AbstractSet = ["Set"]

This is an edge case that's unlikely to come up. However, I'm somewhat concerned that it points to a broader issue with the autofix machinery that could also impact other rules.

Should we be emitting PYI025 on this snippet at all?

This is debatable. However, I think it's fine to emit PYI025 even if the "Set" symbol appears in __all__, since that's very much an edge case that's extremely unlikely to actually come up. I'm not really concerned about this.

[AlexWaygood]

Note that the code will actually trigger F811 after being "autofixed" for PYI025:

from collections.abc import Set as AbstractSet

AbstractSet = ["Set"]  # F811: Redefinition of unused `AbstractSet` from line 1

[AlexWaygood]

The autofix is done using the renamer here:

ruff/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs

Line 80 in 4f06d59

let (edit, rest) = Renamer::rename(name, "AbstractSet", scope, checker.semantic())?;

The issue in the renamer is here where, after renaming the binding itself (from collections.abc import Set), we then proceed to try to rename all references to the binding that we've just renamed:

ruff/crates/ruff_linter/src/renamer.rs

Lines 202 to 206 in 4f06d59

	// Rename the references to the binding.
	edits.extend(binding.references().map(|reference_id| {
	let reference = semantic.reference(reference_id);
	Edit::range_replacement(target.to_string(), reference.range())
	}));

The renamer correctly recognises that __all__ = ["Set"] counts as a resolved reference to the Set symbol that we've just renamed. But it then incorrectly renames the target of the assignment rather than the string in the assignment's value.

[AlexWaygood]

Adding this debug print:

diff --git a/crates/ruff_linter/src/renamer.rs b/crates/ruff_linter/src/renamer.rs
index 8571d7f53..e8f4d1295 100644
--- a/crates/ruff_linter/src/renamer.rs
+++ b/crates/ruff_linter/src/renamer.rs
@@ -202,6 +202,7 @@ impl Renamer {
                 // Rename the references to the binding.
                 edits.extend(binding.references().map(|reference_id| {
                     let reference = semantic.reference(reference_id);
+                    dbg!(reference);
                     Edit::range_replacement(target.to_string(), reference.range())
                 }));

reveals that the range of the ResolvedReference is 33..40, which is the range of __all__ in the expression __all__ = ["Set"], rather than the range of "Set". The incorrect range is then what causes the renamer to change __all__ = ["Set"] to AbstractSet = ["Set"] rather than to __all__ = ["AbstractSet"].

This TODO comment looks relevant @charliermarsh ;)

ruff/crates/ruff_linter/src/checkers/ast/mod.rs

Lines 2114 to 2120 in 4f06d59

	for (name, range) in exports {
	if let Some(binding_id) = self.semantic.global_scope().get(name) {
	// Mark anything referenced in `__all__` as used.
	// TODO(charlie): `range` here should be the range of the name in `__all__`, not
	// the range of `__all__` itself.
	self.semantic
	.add_global_reference(binding_id, ExprContext::Load, range);