From 174df8d7714a7f734cd375230451a756ff0e3e08 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Mon, 25 Nov 2024 18:07:30 +0100 Subject: [PATCH] Abstain from autofix in union with bare none. --- .../resources/test/fixtures/ruff/RUF020.py | 9 +++ .../src/rules/ruff/rules/never_union.rs | 67 +++++++++++++++---- ..._rules__ruff__tests__RUF020_RUF020.py.snap | 66 +++++++++++++++++- 3 files changed, 128 insertions(+), 14 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py index 84203b0cc9040..e737216218e20 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py @@ -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 diff --git a/crates/ruff_linter/src/rules/ruff/rules/never_union.rs b/crates/ruff_linter/src/rules/ruff/rules/never_union.rs index 7485848b97f05..2e099e0481e93 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/never_union.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/never_union.rs @@ -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; @@ -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 { @@ -56,9 +59,9 @@ impl AlwaysFixableViolation for NeverUnion { } } - fn fix_title(&self) -> String { + fn fix_title(&self) -> Option { let Self { never_like, .. } = self; - format!("Remove `{never_like}`") + Some(format!("Remove `{never_like}`")) } } @@ -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); } @@ -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); } } @@ -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 +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF020_RUF020.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF020_RUF020.py.snap index 683c802ab387b..8163d3d72c0e1 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF020_RUF020.py.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF020_RUF020.py.snap @@ -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` | @@ -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` | @@ -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` | @@ -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` | @@ -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`