[flake8-pyi] Ensure Literal[None,] | Literal[None,] is not autofixed to None | None (PYI061)

[LaBatata101]

Handles the case where the None literal inside Literal is part of a tuple, e.g. List[None,]. We don't create the fix for this case.

Fixes #16177.

Summary

Test Plan

Snapshot tests.

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[AlexWaygood]

thanks!

[AlexWaygood]

Looking at it again, I think this maybe fixes things the wrong way. If we have a foo.pyi file like this:

from typing import Literal

f: Literal[None,] | Literal[None,]
g: Literal[None,] | None
h: None | Literal[None,]

and I run cargo run -- check foo.pyi --select=PYI061 --preview --no-cache --diff from the main branch, Ruff says it would apply these fixes:

--- foo.pyi
+++ foo.pyi
@@ -1,5 +1,5 @@
 from typing import Literal
 
-f: Literal[None,] | Literal[None,]
+f: None | None
 g: Literal[None,] | None
 h: None | Literal[None,]

I.e., the second and third of these are fine, because this rule already knows that converting Literal[None,] | None to None | None will produce a TypeError. The reason why the first expression gets converted into one that will produce a TypeError is because there are actually two fixes being applied to the same expression as part of a single iteration of diagnostics-plus-fixes from Ruff. The second autofix isn't aware of the changes that the first autofix made (it isn't aware that the first element in the union has already been converted to None) so it thinks it's still safe to convert the second element in the union to None.

That indicates to me that the better fix here is to ensure that each element's autofix is forced to happen in a separate iteration of the diagnostics-plus-autofixes loop that Ruff does until there are no more autofixes to apply. I.e., this diff applied to main also fixes the issue for me, and feels to me like it more directly addresses the cause of the problem here:

diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
index 16a48bc5f..e3fafac62 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
@@ -130,6 +130,9 @@ pub(crate) fn redundant_none_literal<'a>(checker: &Checker, literal_expr: &'a Ex
                 literal_elements.clone(),
                 union_kind,
             )
+            .map(|fix| {
+                fix.map(|fix| fix.isolate(Checker::isolation(semantic.current_statement_id())))
+            })
         });
         checker.report_diagnostic(diagnostic);
     }

[LaBatata101]

Looking at it again, I think this maybe fixes things the wrong way. If we have a foo.pyi file like this:

from typing import Literal

f: Literal[None,] | Literal[None,]
g: Literal[None,] | None
h: None | Literal[None,]

and I run cargo run -- check foo.pyi --select=PYI061 --preview --no-cache --diff from the main branch, Ruff says it would apply these fixes:

--- foo.pyi
+++ foo.pyi
@@ -1,5 +1,5 @@
 from typing import Literal
 
-f: Literal[None,] | Literal[None,]
+f: None | None
 g: Literal[None,] | None
 h: None | Literal[None,]

I.e., the second and third of these are fine, because this rule already knows that converting Literal[None,] | None to None | None will produce a TypeError. The reason why the first expression gets converted into one that will produce a TypeError is because there are actually two fixes being applied to the same expression as part of a single iteration of diagnostics-plus-fixes from Ruff. The second autofix isn't aware of the changes that the first autofix made (it isn't aware that the first element in the union has already been converted to None) so it thinks it's still safe to convert the second element in the union to None.

That indicates to me that the better fix here is to ensure that each element's autofix is forced to happen in a separate iteration of the diagnostics-plus-autofixes loop that Ruff does until there are no more autofixes to apply. I.e., this diff applied to main also fixes the issue for me, and feels to me like it more directly addresses the cause of the problem here:

diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
index 16a48bc5f..e3fafac62 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
@@ -130,6 +130,9 @@ pub(crate) fn redundant_none_literal<'a>(checker: &Checker, literal_expr: &'a Ex
                 literal_elements.clone(),
                 union_kind,
             )
+            .map(|fix| {
+                fix.map(|fix| fix.isolate(Checker::isolation(semantic.current_statement_id())))
+            })
         });
         checker.report_diagnostic(diagnostic);
     }

Makes sense, thanks!

[AlexWaygood]

I realised that we could simplify some of the logic added in #14583, because ensuring that the individual autofixes are isolated provides a more general fix! I pushed the change to your branch here

[AlexWaygood]

Thank you!