Avoid inserting redundant None elements

Author: charliermarsh

crates/ruff_linter/resources/test/fixtures/pyupgrade/UP045.py

@@ -82,3 +82,12 @@ class ServiceRefOrValue:
     int
     # text
 ] = None
+
+
+# Regression test for: https://github.com/astral-sh/ruff/issues/23429
+# Optional[None | X] should not produce None | None
+bar: None | Optional[None | int] = None
+bar: Optional[None | int] = None
+bar: Optional[int | None] = None
+bar: Optional[None | int | str] = None
+bar: Optional[None | None] = None

crates/ruff_linter/src/rules/pyupgrade/rules/use_pep604_annotation.rs

@@ -1,7 +1,7 @@
 use ruff_macros::{ViolationMetadata, derive_message_formats};
 use ruff_python_ast::PythonVersion;
 use ruff_python_ast::helpers::{pep_604_optional, pep_604_union};
-use ruff_python_ast::{self as ast, Expr};
+use ruff_python_ast::{self as ast, Expr, Operator};
 use ruff_python_semantic::analyze::typing::{Pep604Operator, to_pep604_operator};
 use ruff_source_file::LineRanges;
 use ruff_text_size::Ranged;
@@ -190,17 +190,40 @@ pub(crate) fn non_pep604_annotation(
                             }
                         }
 
-                        diagnostic.set_fix(Fix::applicable_edit(
-                            Edit::range_replacement(
-                                pad(
-                                    checker.generator().expr(&pep_604_optional(inner)),
+                        // If the inner expression is a `BitOr` union that already
+                        // contains `None`, strip it out and re-add it only at the end.
+                        // This avoids generating `None | None` which is a runtime
+                        // `TypeError`. For example, `Optional[None | int]` should
+                        // become `int | None`, not `None | int | None`.
+                        let fix_expr = if let Expr::BinOp(ast::ExprBinOp {
+                            op: Operator::BitOr,
+                            ..
+                        }) = inner
+                        {
+                            let elements = collect_non_none(inner);
+                            if elements.is_empty() {
+                                // All elements were `None`; don't provide a fix.
+                                None
+                            } else {
+                                Some(pep_604_optional(&pep_604_union(&elements)))
+                            }
+                        } else {
+                            Some(pep_604_optional(inner))
+                        };
+
+                        if let Some(fix_expr) = fix_expr {
+                            diagnostic.set_fix(Fix::applicable_edit(
+                                Edit::range_replacement(
+                                    pad(
+                                        checker.generator().expr(&fix_expr),
+                                        expr.range(),
+                                        checker.locator(),
+                                    ),
                                     expr.range(),
-                                    checker.locator(),
                                 ),
-                                expr.range(),
-                            ),
-                            applicability,
-                        ));
+                                applicability,
+                            ));
+                        }
                     }
                 }
             }
@@ -332,3 +355,27 @@ fn is_named_tuple(checker: &Checker, expr: &Expr) -> bool {
 fn is_optional_none(operator: Pep604Operator, slice: &Expr) -> bool {
     matches!(operator, Pep604Operator::Optional) && matches!(slice, Expr::NoneLiteral(_))
 }
+
+/// Collect all non-`None` leaf elements of a chain of `BitOr` binary operations.
+///
+/// For example, `a | None | b` is collected as `[a, b]`.
+fn collect_non_none(expr: &Expr) -> Vec<Expr> {
+    fn inner(expr: &Expr, elements: &mut Vec<Expr>) {
+        if let Expr::BinOp(ast::ExprBinOp {
+            left,
+            op: Operator::BitOr,
+            right,
+            ..
+        }) = expr
+        {
+            inner(left, elements);
+            inner(right, elements);
+        } else if !expr.is_none_literal_expr() {
+            elements.push(expr.clone());
+        }
+    }
+
+    let mut elements = Vec::new();
+    inner(expr, &mut elements);
+    elements
+}

crates/ruff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP045.py.snap

@@ -311,4 +311,93 @@ help: Convert to `X | None`
    -     # text
    - ] = None
 81 + foo: int | None = None
+82 | 
+83 | 
+84 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
 note: This is an unsafe fix and may change runtime behavior
+
+UP045 [*] Use `X | None` for type annotations
+  --> UP045.py:89:13
+   |
+87 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
+88 | # Optional[None | X] should not produce None | None
+89 | bar: None | Optional[None | int] = None
+   |             ^^^^^^^^^^^^^^^^^^^^
+90 | bar: Optional[None | int] = None
+91 | bar: Optional[int | None] = None
+   |
+help: Convert to `X | None`
+86 | 
+87 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
+88 | # Optional[None | X] should not produce None | None
+   - bar: None | Optional[None | int] = None
+89 + bar: None | int | None = None
+90 | bar: Optional[None | int] = None
+91 | bar: Optional[int | None] = None
+92 | bar: Optional[None | int | str] = None
+
+UP045 [*] Use `X | None` for type annotations
+  --> UP045.py:90:6
+   |
+88 | # Optional[None | X] should not produce None | None
+89 | bar: None | Optional[None | int] = None
+90 | bar: Optional[None | int] = None
+   |      ^^^^^^^^^^^^^^^^^^^^
+91 | bar: Optional[int | None] = None
+92 | bar: Optional[None | int | str] = None
+   |
+help: Convert to `X | None`
+87 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
+88 | # Optional[None | X] should not produce None | None
+89 | bar: None | Optional[None | int] = None
+   - bar: Optional[None | int] = None
+90 + bar: int | None = None
+91 | bar: Optional[int | None] = None
+92 | bar: Optional[None | int | str] = None
+93 | bar: Optional[None | None] = None
+
+UP045 [*] Use `X | None` for type annotations
+  --> UP045.py:91:6
+   |
+89 | bar: None | Optional[None | int] = None
+90 | bar: Optional[None | int] = None
+91 | bar: Optional[int | None] = None
+   |      ^^^^^^^^^^^^^^^^^^^^
+92 | bar: Optional[None | int | str] = None
+93 | bar: Optional[None | None] = None
+   |
+help: Convert to `X | None`
+88 | # Optional[None | X] should not produce None | None
+89 | bar: None | Optional[None | int] = None
+90 | bar: Optional[None | int] = None
+   - bar: Optional[int | None] = None
+91 + bar: int | None = None
+92 | bar: Optional[None | int | str] = None
+93 | bar: Optional[None | None] = None
+
+UP045 [*] Use `X | None` for type annotations
+  --> UP045.py:92:6
+   |
+90 | bar: Optional[None | int] = None
+91 | bar: Optional[int | None] = None
+92 | bar: Optional[None | int | str] = None
+   |      ^^^^^^^^^^^^^^^^^^^^^^^^^^
+93 | bar: Optional[None | None] = None
+   |
+help: Convert to `X | None`
+89 | bar: None | Optional[None | int] = None
+90 | bar: Optional[None | int] = None
+91 | bar: Optional[int | None] = None
+   - bar: Optional[None | int | str] = None
+92 + bar: int | str | None = None
+93 | bar: Optional[None | None] = None
+
+UP045 Use `X | None` for type annotations
+  --> UP045.py:93:6
+   |
+91 | bar: Optional[int | None] = None
+92 | bar: Optional[None | int | str] = None
+93 | bar: Optional[None | None] = None
+   |      ^^^^^^^^^^^^^^^^^^^^^
+   |
+help: Convert to `X | None`