Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ruff] Improve autofix safety for never-union (RUF020) #14589

Merged
merged 1 commit into from
Nov 25, 2024
Merged

[ruff] Improve autofix safety for never-union (RUF020) #14589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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`
Loading