open-alias (UP020) has unexpected auto-fix behaviour

[RasmusNygren]

Description

Running the UP020 rule with --fix produces weird and unexpected results.

Example 1 (ImportStyle::ImportFrom)
When running ruff check --fix --select --isolated UP020 /path/to/file.py on the particular file below:

from io import open
open("Cargo.toml")

you end up with this result:

from io import open
import builtins
builtins.open("Cargo.toml"): ...

Not only is the from io import open statement still there, but it also imports builtins which is not needed in this minimal example.

Example 2 (ImportStyle::Import)
When running ruff check --fix --select --isolated UP020 /path/to/file.py on the particular file below:

import io
io.open("Cargo.toml")

you end up with the following:

import io
open("Cargo.toml")

where the import io statement is still there despite being unused.

ruff --version: ruff 0.9.4

[ntBre]

I think both of these examples boil down to the rule not removing the io import. Also enabling F401 - unused-import should take care of the second case:

$ cat <<EOF | ruff check --select UP020,F401 --diff -
import io
io.open("Cargo.toml")
EOF

Output

@@ -1,2 +1 @@
-import io
-io.open("Cargo.toml")
+open("Cargo.toml")

Would fix 2 errors.

But I agree that the first case is not really ideal. Even with F401, builtins still ends up imported:

$ cat <<EOF | ruff check --select UP020,F401 --diff --no-cache -
from io import open
open("Cargo.toml")   
EOF

Output

@@ -1,2 +1,2 @@
-from io import open
-open("Cargo.toml")
+import builtins
+builtins.open("Cargo.toml")

Would fix 2 errors.

[ntBre]

There is another rule that looks promising for cleaning up the builtins import: UP029 - unnecessary-builtin-import, but it doesn't actually help here, unfortunately, as it looks for imports like from builtins import open. That might be the best rule to update to clean this up? What do you think @MichaReiser or @AlexWaygood ?

[MichaReiser]

Adjusting UP029 seems reasonable to me. I'm a bit surprised that it wouldn't catch from io import open because there's an io, open in the match statement here

ruff/crates/ruff_linter/src/rules/pyupgrade/rules/unnecessary_builtin_import.rs

Line 102 in 14ba469

) | ("io", "open")

[ntBre]

Ahhh, with --unsafe-fixes UP029 does cause the desired behavior:

cat <<EOF | ruff check --select UP020,UP029,F401 --diff --no-cache --unsafe-fixes -
from io import open
open("Cargo.toml")
EOF

@@ -1,2 +1 @@
-from io import open
 open("Cargo.toml")

Would fix 1 error.

But without --unsafe-fixes and with UP020 also selected, UP020 runs and doesn't mention the unsafe fix. I only got the (1 fix available with --unsafe-fixes) when I didn't select UP020 too.

[MichaReiser]

Ah nice find. I wonder why the fix is marked as unsafe.

[ntBre]

Not sure either, but it's being tracked for improving that in the docs in #15584 at least.