From 7b16239f2940c9fba2be6593514f5d4e3e025d0b Mon Sep 17 00:00:00 2001 From: chiri Date: Tue, 20 Jan 2026 18:02:42 +0300 Subject: [PATCH] [`ruff`] Make fix unsafe if it deletes comments (`RUF020`) (#22664) ## Summary ## Test Plan --- .../resources/test/fixtures/ruff/RUF020.py | 7 ++ .../src/rules/ruff/rules/never_union.rs | 82 ++++++++++++------- ..._rules__ruff__tests__RUF020_RUF020.py.snap | 19 +++++ 3 files changed, 77 insertions(+), 31 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py index e737216218..773f21f6ba 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py @@ -15,3 +15,10 @@ z: None | (Never | None) a: int | Never | None b: Never | Never | None + + +def func() -> ( + Never # text + | # text + int +): ... \ No newline at end of file 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 a13fc69459..58d05af03b 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/never_union.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/never_union.rs @@ -1,3 +1,4 @@ +use ruff_diagnostics::Applicability; use ruff_macros::{ViolationMetadata, derive_message_formats}; use ruff_python_ast::{self as ast, Expr, ExprBinOp, Operator}; use ruff_python_semantic::{SemanticModel, analyze::typing::traverse_union}; @@ -31,6 +32,9 @@ use crate::{Edit, Fix, FixAvailability, Violation}; /// def func() -> int: ... /// ``` /// +/// ## Fix safety +/// This rule's fix is marked as safe, unless the union type contains comments. +/// /// ## References /// - [Python documentation: `typing.Never`](https://docs.python.org/3/library/typing.html#typing.Never) /// - [Python documentation: `typing.NoReturn`](https://docs.python.org/3/library/typing.html#typing.NoReturn) @@ -68,9 +72,15 @@ impl Violation for NeverUnion { /// RUF020 pub(crate) fn never_union(checker: &Checker, expr: &Expr) { + let applicability = if checker.comment_ranges().intersects(expr.range()) { + Applicability::Unsafe + } else { + Applicability::Safe + }; + match expr { // Ex) `typing.NoReturn | int` - Expr::BinOp(ast::ExprBinOp { + Expr::BinOp(ExprBinOp { op: Operator::BitOr, left, right, @@ -92,10 +102,13 @@ pub(crate) fn never_union(checker: &Checker, expr: &Expr) { // as `Union[None, None]` is valid Python. // See https://github.com/astral-sh/ruff/issues/14567. if !is_pep604_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(), - ))); + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement( + checker.locator().slice(right.as_ref()).to_string(), + expr.range(), + ), + applicability, + )); } } @@ -109,10 +122,13 @@ pub(crate) fn never_union(checker: &Checker, expr: &Expr) { right.range(), ); if !is_pep604_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(), - ))); + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement( + checker.locator().slice(left.as_ref()).to_string(), + expr.range(), + ), + applicability, + )); } } } @@ -151,30 +167,34 @@ pub(crate) fn never_union(checker: &Checker, expr: &Expr) { }, elt.range(), ); - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - if let [only] = rest.as_slice() { - // Ex) `typing.Union[typing.NoReturn, int]` -> `int` - checker.locator().slice(only).to_string() - } else { - // Ex) `typing.Union[typing.NoReturn, int, str]` -> `typing.Union[int, str]` - checker - .generator() - .expr(&Expr::Subscript(ast::ExprSubscript { - value: value.clone(), - slice: Box::new(Expr::Tuple(ast::ExprTuple { - elts: rest, + + diagnostic.set_fix(Fix::applicable_edit( + Edit::range_replacement( + if let [only] = rest.as_slice() { + // Ex) `typing.Union[typing.NoReturn, int]` -> `int` + checker.locator().slice(only).to_string() + } else { + // Ex) `typing.Union[typing.NoReturn, int, str]` -> `typing.Union[int, str]` + checker + .generator() + .expr(&Expr::Subscript(ast::ExprSubscript { + value: value.clone(), + slice: Box::new(Expr::Tuple(ast::ExprTuple { + elts: rest, + ctx: ast::ExprContext::Load, + range: TextRange::default(), + node_index: ruff_python_ast::AtomicNodeIndex::NONE, + parenthesized: true, + })), ctx: ast::ExprContext::Load, range: TextRange::default(), node_index: ruff_python_ast::AtomicNodeIndex::NONE, - parenthesized: true, - })), - ctx: ast::ExprContext::Load, - range: TextRange::default(), - node_index: ruff_python_ast::AtomicNodeIndex::NONE, - })) - }, - expr.range(), - ))); + })) + }, + expr.range(), + ), + applicability, + )); } } } @@ -200,7 +220,7 @@ enum NeverLike { } impl NeverLike { - fn from_expr(expr: &Expr, semantic: &ruff_python_semantic::SemanticModel) -> Option { + fn from_expr(expr: &Expr, semantic: &SemanticModel) -> Option { let qualified_name = semantic.resolve_qualified_name(expr)?; if semantic.match_typing_qualified_name(&qualified_name, "NoReturn") { Some(NeverLike::NoReturn) 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 2f048321b6..6b127e0462 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 @@ -197,3 +197,22 @@ RUF020 `Never | T` is equivalent to `T` | ^^^^^ | help: Remove `Never` + +RUF020 [*] `Never | T` is equivalent to `T` + --> RUF020.py:21:9 + | +20 | def func() -> ( +21 | Never # text + | ^^^^^ +22 | | # text +23 | int + | +help: Remove `Never` +18 | +19 | +20 | def func() -> ( + - Never # text + - | # text +21 | int +22 | ): ... +note: This is an unsafe fix and may change runtime behavior