Skip to content

Commit aeb4480

Browse files
charliermarshcharliermarsh
committed
Avoid inserting redundant None elements
1 parent 66defe9 commit aeb4480

3 files changed

Lines changed: 167 additions & 10 deletions

File tree

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,3 +82,12 @@ class ServiceRefOrValue:
8282
int
8383
# text
8484
] = None
85+
86+
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
92+
bar: Optional[None | int | str] = None
93+
bar: Optional[None | None] = None

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

Lines changed: 69 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use ruff_macros::{ViolationMetadata, derive_message_formats};
22
use ruff_python_ast::PythonVersion;
33
use ruff_python_ast::helpers::{pep_604_optional, pep_604_union};
4-
use ruff_python_ast::{self as ast, Expr};
4+
use ruff_python_ast::{self as ast, Expr, Operator};
55
use ruff_python_semantic::analyze::typing::{Pep604Operator, to_pep604_operator};
66
use ruff_source_file::LineRanges;
77
use ruff_text_size::Ranged;
@@ -190,17 +190,52 @@ pub(crate) fn non_pep604_annotation(
190190
}
191191
}
192192

193-
diagnostic.set_fix(Fix::applicable_edit(
194-
Edit::range_replacement(
195-
pad(
196-
checker.generator().expr(&pep_604_optional(inner)),
193+
// If the inner expression is a `BitOr` union that already
194+
// contains `None`, strip it out and re-add it only at the end.
195+
// This avoids generating `None | None` which is a runtime
196+
// `TypeError`. For example, `Optional[None | int]` should
197+
// become `int | None`, not `None | int | None`.
198+
let fix_expr = if let Expr::BinOp(ast::ExprBinOp {
199+
op: Operator::BitOr,
200+
..
201+
}) = inner
202+
{
203+
let elements = collect_bitor_elements(inner);
204+
if elements
205+
.iter()
206+
.any(|element| matches!(element, Expr::NoneLiteral(_)))
207+
{
208+
let non_none: Vec<Expr> = elements
209+
.into_iter()
210+
.filter(|element| !matches!(element, Expr::NoneLiteral(_)))
211+
.cloned()
212+
.collect();
213+
if non_none.is_empty() {
214+
// All elements were `None`; don't provide a fix.
215+
None
216+
} else {
217+
Some(pep_604_optional(&pep_604_union(&non_none)))
218+
}
219+
} else {
220+
Some(pep_604_optional(inner))
221+
}
222+
} else {
223+
Some(pep_604_optional(inner))
224+
};
225+
226+
if let Some(fix_expr) = fix_expr {
227+
diagnostic.set_fix(Fix::applicable_edit(
228+
Edit::range_replacement(
229+
pad(
230+
checker.generator().expr(&fix_expr),
231+
expr.range(),
232+
checker.locator(),
233+
),
197234
expr.range(),
198-
checker.locator(),
199235
),
200-
expr.range(),
201-
),
202-
applicability,
203-
));
236+
applicability,
237+
));
238+
}
204239
}
205240
}
206241
}
@@ -332,3 +367,27 @@ fn is_named_tuple(checker: &Checker, expr: &Expr) -> bool {
332367
fn is_optional_none(operator: Pep604Operator, slice: &Expr) -> bool {
333368
matches!(operator, Pep604Operator::Optional) && matches!(slice, Expr::NoneLiteral(_))
334369
}
370+
371+
/// Collect all leaf elements of a chain of `BitOr` binary operations.
372+
///
373+
/// For example, `a | b | c` is collected as `[a, b, c]`.
374+
fn collect_bitor_elements(expr: &Expr) -> Vec<&Expr> {
375+
fn inner<'a>(expr: &'a Expr, elements: &mut Vec<&'a Expr>) {
376+
if let Expr::BinOp(ast::ExprBinOp {
377+
left,
378+
op: Operator::BitOr,
379+
right,
380+
..
381+
}) = expr
382+
{
383+
inner(left, elements);
384+
inner(right, elements);
385+
} else {
386+
elements.push(expr);
387+
}
388+
}
389+
390+
let mut elements = Vec::new();
391+
inner(expr, &mut elements);
392+
elements
393+
}

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

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,4 +311,93 @@ help: Convert to `X | None`
311311
- # text
312312
- ] = None
313313
81 + foo: int | None = None
314+
82 |
315+
83 |
316+
84 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
314317
note: This is an unsafe fix and may change runtime behavior
318+
319+
UP045 [*] Use `X | None` for type annotations
320+
--> UP045.py:89:13
321+
|
322+
87 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
323+
88 | # Optional[None | X] should not produce None | None
324+
89 | bar: None | Optional[None | int] = None
325+
| ^^^^^^^^^^^^^^^^^^^^
326+
90 | bar: Optional[None | int] = None
327+
91 | bar: Optional[int | None] = None
328+
|
329+
help: Convert to `X | None`
330+
86 |
331+
87 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
332+
88 | # Optional[None | X] should not produce None | None
333+
- bar: None | Optional[None | int] = None
334+
89 + bar: None | int | None = None
335+
90 | bar: Optional[None | int] = None
336+
91 | bar: Optional[int | None] = None
337+
92 | bar: Optional[None | int | str] = None
338+
339+
UP045 [*] Use `X | None` for type annotations
340+
--> UP045.py:90:6
341+
|
342+
88 | # Optional[None | X] should not produce None | None
343+
89 | bar: None | Optional[None | int] = None
344+
90 | bar: Optional[None | int] = None
345+
| ^^^^^^^^^^^^^^^^^^^^
346+
91 | bar: Optional[int | None] = None
347+
92 | bar: Optional[None | int | str] = None
348+
|
349+
help: Convert to `X | None`
350+
87 | # Regression test for: https://github.com/astral-sh/ruff/issues/23429
351+
88 | # Optional[None | X] should not produce None | None
352+
89 | bar: None | Optional[None | int] = None
353+
- bar: Optional[None | int] = None
354+
90 + bar: int | None = None
355+
91 | bar: Optional[int | None] = None
356+
92 | bar: Optional[None | int | str] = None
357+
93 | bar: Optional[None | None] = None
358+
359+
UP045 [*] Use `X | None` for type annotations
360+
--> UP045.py:91:6
361+
|
362+
89 | bar: None | Optional[None | int] = None
363+
90 | bar: Optional[None | int] = None
364+
91 | bar: Optional[int | None] = None
365+
| ^^^^^^^^^^^^^^^^^^^^
366+
92 | bar: Optional[None | int | str] = None
367+
93 | bar: Optional[None | None] = None
368+
|
369+
help: Convert to `X | None`
370+
88 | # Optional[None | X] should not produce None | None
371+
89 | bar: None | Optional[None | int] = None
372+
90 | bar: Optional[None | int] = None
373+
- bar: Optional[int | None] = None
374+
91 + bar: int | None = None
375+
92 | bar: Optional[None | int | str] = None
376+
93 | bar: Optional[None | None] = None
377+
378+
UP045 [*] Use `X | None` for type annotations
379+
--> UP045.py:92:6
380+
|
381+
90 | bar: Optional[None | int] = None
382+
91 | bar: Optional[int | None] = None
383+
92 | bar: Optional[None | int | str] = None
384+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
385+
93 | bar: Optional[None | None] = None
386+
|
387+
help: Convert to `X | None`
388+
89 | bar: None | Optional[None | int] = None
389+
90 | bar: Optional[None | int] = None
390+
91 | bar: Optional[int | None] = None
391+
- bar: Optional[None | int | str] = None
392+
92 + bar: int | str | None = None
393+
93 | bar: Optional[None | None] = None
394+
395+
UP045 Use `X | None` for type annotations
396+
--> UP045.py:93:6
397+
|
398+
91 | bar: Optional[int | None] = None
399+
92 | bar: Optional[None | int | str] = None
400+
93 | bar: Optional[None | None] = None
401+
| ^^^^^^^^^^^^^^^^^^^^^
402+
|
403+
help: Convert to `X | None`

0 commit comments

Comments
 (0)
⚡