Skip to content

Commit

Permalink
Abstain from autofix in union with bare none.
Browse files Browse the repository at this point in the history
  • Loading branch information
@sbrugman
sbrugman committed Nov 25, 2024
1 parent fa22bd6 commit 174df8d
Show file tree
Hide file tree
Showing 3 changed files with 128 additions and 14 deletions.
9 changes: 9 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,12 @@
NoReturn | int
Union[Union[Never, int], Union[NoReturn, int]]
Union[NoReturn, int, float]


# Regression tests for https://github.com/astral-sh/ruff/issues/14567
x: None | Never | None
y: (None | Never) | None
z: None | (Never | None)

a: int | Never | None
b: Never | Never | None
67 changes: 54 additions & 13 deletions crates/ruff_linter/src/rules/ruff/rules/never_union.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, Operator};
use ruff_python_ast::{self as ast, Expr, ExprBinOp, Operator};
use ruff_python_semantic::{analyze::typing::traverse_union, SemanticModel};
use ruff_text_size::{Ranged, TextRange};

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -39,7 +40,9 @@ pub struct NeverUnion {
union_like: UnionLike,
}

impl AlwaysFixableViolation for NeverUnion {
impl Violation for NeverUnion {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let Self {
Expand All @@ -56,9 +59,9 @@ impl AlwaysFixableViolation for NeverUnion {
}
}

fn fix_title(&self) -> String {
fn fix_title(&self) -> Option<String> {
let Self { never_like, .. } = self;
format!("Remove `{never_like}`")
Some(format!("Remove `{never_like}`"))
}
}

Expand All @@ -81,10 +84,17 @@ pub(crate) fn never_union(checker: &mut Checker, expr: &Expr) {
},
left.range(),
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
checker.locator().slice(right.as_ref()).to_string(),
expr.range(),
)));
// Avoid producing code that would raise an exception when
// `Never | None` would be fixed to `None | None`.
// Instead do not provide a fix. No action needed for `typing.Union`,
// as `Union[None, None]` is valid Python.
// See https://github.com/astral-sh/ruff/issues/14567.
if !in_union_with_bare_none(checker.semantic()) {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
checker.locator().slice(right.as_ref()).to_string(),
expr.range(),
)));
}
checker.diagnostics.push(diagnostic);
}

Expand All @@ -97,10 +107,12 @@ pub(crate) fn never_union(checker: &mut Checker, expr: &Expr) {
},
right.range(),
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
checker.locator().slice(left.as_ref()).to_string(),
expr.range(),
)));
if !in_union_with_bare_none(checker.semantic()) {
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
checker.locator().slice(left.as_ref()).to_string(),
expr.range(),
)));
}
checker.diagnostics.push(diagnostic);
}
}
Expand Down Expand Up @@ -206,3 +218,32 @@ impl std::fmt::Display for NeverLike {
}
}
}

fn in_union_with_bare_none(semantic: &SemanticModel) -> bool {
let mut enclosing_union = None;
let mut expression_ancestors = semantic.current_expressions().skip(1);
let mut parent_expr = expression_ancestors.next();
while let Some(Expr::BinOp(ExprBinOp {
op: Operator::BitOr,
..
})) = parent_expr
{
enclosing_union = parent_expr;
parent_expr = expression_ancestors.next();
}

let mut is_union_with_bare_none = false;
if let Some(enclosing_union) = enclosing_union {
traverse_union(
&mut |expr, _| {
if matches!(expr, Expr::NoneLiteral(_)) {
is_union_with_bare_none = true;
}
},
semantic,
enclosing_union,
);
}

is_union_with_bare_none
}
66 changes: 65 additions & 1 deletion ...ff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF020_RUF020.py.snap
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
snapshot_kind: text
---
RUF020.py:3:7: RUF020 [*] `Union[Never, T]` is equivalent to `T`
|
Expand Down Expand Up @@ -82,6 +81,7 @@ RUF020.py:6:1: RUF020 [*] `NoReturn | T` is equivalent to `T`
6 |+int
7 7 | Union[Union[Never, int], Union[NoReturn, int]]
8 8 | Union[NoReturn, int, float]
9 9 |

RUF020.py:7:13: RUF020 [*] `Union[Never, T]` is equivalent to `T`
|
Expand All @@ -100,6 +100,8 @@ RUF020.py:7:13: RUF020 [*] `Union[Never, T]` is equivalent to `T`
7 |-Union[Union[Never, int], Union[NoReturn, int]]
7 |+Union[int, Union[NoReturn, int]]
8 8 | Union[NoReturn, int, float]
9 9 |
10 10 |

RUF020.py:7:32: RUF020 [*] `Union[NoReturn, T]` is equivalent to `T`
|
Expand All @@ -118,6 +120,8 @@ RUF020.py:7:32: RUF020 [*] `Union[NoReturn, T]` is equivalent to `T`
7 |-Union[Union[Never, int], Union[NoReturn, int]]
7 |+Union[Union[Never, int], int]
8 8 | Union[NoReturn, int, float]
9 9 |
10 10 |

RUF020.py:8:7: RUF020 [*] `Union[NoReturn, T]` is equivalent to `T`
|
Expand All @@ -134,3 +138,63 @@ RUF020.py:8:7: RUF020 [*] `Union[NoReturn, T]` is equivalent to `T`
7 7 | Union[Union[Never, int], Union[NoReturn, int]]
8 |-Union[NoReturn, int, float]
8 |+Union[int, float]
9 9 |
10 10 |
11 11 | # Regression tests for https://github.com/astral-sh/ruff/issues/14567

RUF020.py:12:11: RUF020 `Never | T` is equivalent to `T`
|
11 | # Regression tests for https://github.com/astral-sh/ruff/issues/14567
12 | x: None | Never | None
| ^^^^^ RUF020
13 | y: (None | Never) | None
14 | z: None | (Never | None)
|
= help: Remove `Never`

RUF020.py:13:12: RUF020 `Never | T` is equivalent to `T`
|
11 | # Regression tests for https://github.com/astral-sh/ruff/issues/14567
12 | x: None | Never | None
13 | y: (None | Never) | None
| ^^^^^ RUF020
14 | z: None | (Never | None)
|
= help: Remove `Never`

RUF020.py:14:12: RUF020 `Never | T` is equivalent to `T`
|
12 | x: None | Never | None
13 | y: (None | Never) | None
14 | z: None | (Never | None)
| ^^^^^ RUF020
15 |
16 | a: int | Never | None
|
= help: Remove `Never`

RUF020.py:16:10: RUF020 `Never | T` is equivalent to `T`
|
14 | z: None | (Never | None)
15 |
16 | a: int | Never | None
| ^^^^^ RUF020
17 | b: Never | Never | None
|
= help: Remove `Never`

RUF020.py:17:4: RUF020 `Never | T` is equivalent to `T`
|
16 | a: int | Never | None
17 | b: Never | Never | None
| ^^^^^ RUF020
|
= help: Remove `Never`

RUF020.py:17:12: RUF020 `Never | T` is equivalent to `T`
|
16 | a: int | Never | None
17 | b: Never | Never | None
| ^^^^^ RUF020
|
= help: Remove `Never`

0 comments on commit 174df8d

Please sign in to comment.