From c8575374a78f2b66434436376d7d8a25ed6c0e08 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 20 Dec 2023 15:03:23 -0500 Subject: [PATCH] Add a fix for never-union --- .../resources/test/fixtures/ruff/RUF020.py | 1 + .../src/checkers/ast/analyze/expression.rs | 14 +- .../src/rules/ruff/rules/never_union.rs | 161 +++++++++++++----- ..._rules__ruff__tests__RUF020_RUF020.py.snap | 91 +++++++++- 4 files changed, 215 insertions(+), 52 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py index 3775afa8ec471d..84203b0cc90403 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py @@ -5,3 +5,4 @@ Never | int NoReturn | int Union[Union[Never, int], Union[NoReturn, int]] +Union[NoReturn, int, float] diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index fbde5c58803b3a..f42f577ea8b672 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -98,12 +98,13 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::UnnecessaryTypeUnion) { flake8_pyi::rules::unnecessary_type_union(checker, expr); } - if checker.enabled(Rule::NeverUnion) { - ruff::rules::never_union(checker, expr); - } } } + if checker.enabled(Rule::NeverUnion) { + ruff::rules::never_union(checker, expr); + } + if checker.any_enabled(&[ Rule::SysVersionSlice3, Rule::SysVersion2, @@ -1158,6 +1159,10 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { } } + if checker.enabled(Rule::NeverUnion) { + ruff::rules::never_union(checker, expr); + } + // Avoid duplicate checks if the parent is a union, since these rules already // traverse nested unions. if !checker.semantic.in_nested_union() { @@ -1178,9 +1183,6 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { if checker.enabled(Rule::RuntimeStringUnion) { flake8_type_checking::rules::runtime_string_union(checker, expr); } - if checker.enabled(Rule::NeverUnion) { - ruff::rules::never_union(checker, expr); - } } } Expr::UnaryOp( 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 0fcccf3e4560ce..88c10f58778930 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/never_union.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/never_union.rs @@ -1,8 +1,7 @@ -use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::Expr; -use ruff_python_semantic::analyze::typing::traverse_union; -use ruff_text_size::Ranged; +use ruff_python_ast::{self as ast, Expr, Operator}; +use ruff_text_size::{Ranged, TextRange}; use crate::checkers::ast::Checker; @@ -40,7 +39,7 @@ pub struct NeverUnion { union_like: UnionLike, } -impl Violation for NeverUnion { +impl AlwaysFixableViolation for NeverUnion { #[derive_message_formats] fn message(&self) -> String { let Self { @@ -56,52 +55,121 @@ impl Violation for NeverUnion { } } } + + fn fix_title(&self) -> String { + let Self { never_like, .. } = self; + format!("Remove `{never_like}`") + } } /// RUF020 pub(crate) fn never_union<'a>(checker: &mut Checker, expr: &'a Expr) { - let mut expressions: Vec<(NeverLike, UnionLike, &'a Expr)> = Vec::new(); - let mut rest: Vec<&'a Expr> = Vec::new(); + match expr { + // Ex) `typing.NoReturn | int` + Expr::BinOp(ast::ExprBinOp { + op: Operator::BitOr, + left, + right, + range: _, + }) => { + // Analyze the left-hand side of the `|` operator. + if let Some(never_like) = NeverLike::from_expr(left, checker.semantic()) { + let mut diagnostic = Diagnostic::new( + NeverUnion { + never_like, + union_like: UnionLike::BinOp, + }, + left.range(), + ); + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + checker.locator().slice(right.as_ref()).to_string(), + expr.range(), + ))); + checker.diagnostics.push(diagnostic); + } - // Find all `typing.Never` and `typing.NoReturn` expressions. - let semantic = checker.semantic(); - let mut collect_never = |expr: &'a Expr, parent: Option<&'a Expr>| { - if let Some(call_path) = semantic.resolve_call_path(expr) { - let union_like = if parent.is_some_and(Expr::is_bin_op_expr) { - UnionLike::BinOp - } else { - UnionLike::Union - }; + // Analyze the right-hand side of the `|` operator. + if let Some(never_like) = NeverLike::from_expr(right, checker.semantic()) { + let mut diagnostic = Diagnostic::new( + NeverUnion { + never_like, + union_like: UnionLike::BinOp, + }, + right.range(), + ); + diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( + checker.locator().slice(left.as_ref()).to_string(), + expr.range(), + ))); + checker.diagnostics.push(diagnostic); + } + } - let never_like = if semantic.match_typing_call_path(&call_path, "NoReturn") { - Some(NeverLike::NoReturn) - } else if semantic.match_typing_call_path(&call_path, "Never") { - Some(NeverLike::Never) - } else { - None + // Ex) `typing.Union[typing.NoReturn, int]` + Expr::Subscript(ast::ExprSubscript { + value, + slice, + ctx: _, + range: _, + }) if checker.semantic().match_typing_expr(value, "Union") => { + let Expr::Tuple(ast::ExprTuple { + elts, + ctx: _, + range: _, + }) = slice.as_ref() + else { + return; }; - if let Some(never_like) = never_like { - expressions.push((never_like, union_like, expr)); - return; - } - } + // Analyze each element of the `Union`. + for elt in elts { + if let Some(never_like) = NeverLike::from_expr(elt, checker.semantic()) { + // Collect the other elements of the `Union`. + let rest = elts + .iter() + .filter(|other| *other != elt) + .cloned() + .collect::>(); - rest.push(expr); - }; + // Ignore, e.g., `typing.Union[typing.NoReturn]`. + if rest.is_empty() { + return; + } - traverse_union(&mut collect_never, checker.semantic(), expr, None); + let mut diagnostic = Diagnostic::new( + NeverUnion { + never_like, + union_like: UnionLike::Union, + }, + 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, + ctx: ast::ExprContext::Load, + range: TextRange::default(), + })), + ctx: ast::ExprContext::Load, + range: TextRange::default(), + })) + }, + expr.range(), + ))); + checker.diagnostics.push(diagnostic); + } + } + } - // Create a diagnostic for each `typing.Never` and `typing.NoReturn` expression. - for (never_like, union_like, child) in expressions { - let diagnostic = Diagnostic::new( - NeverUnion { - never_like, - union_like, - }, - child.range(), - ); - checker.diagnostics.push(diagnostic); + _ => {} } } @@ -121,6 +189,19 @@ enum NeverLike { Never, } +impl NeverLike { + fn from_expr(expr: &Expr, semantic: &ruff_python_semantic::SemanticModel) -> Option { + let call_path = semantic.resolve_call_path(expr)?; + if semantic.match_typing_call_path(&call_path, "NoReturn") { + Some(NeverLike::NoReturn) + } else if semantic.match_typing_call_path(&call_path, "Never") { + Some(NeverLike::Never) + } else { + None + } + } +} + impl std::fmt::Display for NeverLike { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { 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 a742caaf5b8e1f..64fd30b4ed4d2c 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,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/ruff/mod.rs --- -RUF020.py:3:7: RUF020 `Union[Never, T]` is equivalent to `T` +RUF020.py:3:7: RUF020 [*] `Union[Never, T]` is equivalent to `T` | 1 | from typing import Never, NoReturn, Union 2 | @@ -10,8 +10,18 @@ RUF020.py:3:7: RUF020 `Union[Never, T]` is equivalent to `T` 4 | Union[NoReturn, int] 5 | Never | int | + = help: Remove `Never` -RUF020.py:4:7: RUF020 `Union[NoReturn, T]` is equivalent to `T` +ℹ Safe fix +1 1 | from typing import Never, NoReturn, Union +2 2 | +3 |-Union[Never, int] + 3 |+int +4 4 | Union[NoReturn, int] +5 5 | Never | int +6 6 | NoReturn | int + +RUF020.py:4:7: RUF020 [*] `Union[NoReturn, T]` is equivalent to `T` | 3 | Union[Never, int] 4 | Union[NoReturn, int] @@ -19,8 +29,19 @@ RUF020.py:4:7: RUF020 `Union[NoReturn, T]` is equivalent to `T` 5 | Never | int 6 | NoReturn | int | + = help: Remove `NoReturn` + +ℹ Safe fix +1 1 | from typing import Never, NoReturn, Union +2 2 | +3 3 | Union[Never, int] +4 |-Union[NoReturn, int] + 4 |+int +5 5 | Never | int +6 6 | NoReturn | int +7 7 | Union[Union[Never, int], Union[NoReturn, int]] -RUF020.py:5:1: RUF020 `Never | T` is equivalent to `T` +RUF020.py:5:1: RUF020 [*] `Never | T` is equivalent to `T` | 3 | Union[Never, int] 4 | Union[NoReturn, int] @@ -29,30 +50,88 @@ RUF020.py:5:1: RUF020 `Never | T` is equivalent to `T` 6 | NoReturn | int 7 | Union[Union[Never, int], Union[NoReturn, int]] | + = help: Remove `Never` -RUF020.py:6:1: RUF020 `NoReturn | T` is equivalent to `T` +ℹ Safe fix +2 2 | +3 3 | Union[Never, int] +4 4 | Union[NoReturn, int] +5 |-Never | int + 5 |+int +6 6 | NoReturn | int +7 7 | Union[Union[Never, int], Union[NoReturn, int]] +8 8 | Union[NoReturn, int, float] + +RUF020.py:6:1: RUF020 [*] `NoReturn | T` is equivalent to `T` | 4 | Union[NoReturn, int] 5 | Never | int 6 | NoReturn | int | ^^^^^^^^ RUF020 7 | Union[Union[Never, int], Union[NoReturn, int]] +8 | Union[NoReturn, int, float] | + = help: Remove `NoReturn` + +ℹ Safe fix +3 3 | Union[Never, int] +4 4 | Union[NoReturn, int] +5 5 | Never | int +6 |-NoReturn | int + 6 |+int +7 7 | Union[Union[Never, int], Union[NoReturn, int]] +8 8 | Union[NoReturn, int, float] -RUF020.py:7:13: RUF020 `Union[Never, T]` is equivalent to `T` +RUF020.py:7:13: RUF020 [*] `Union[Never, T]` is equivalent to `T` | 5 | Never | int 6 | NoReturn | int 7 | Union[Union[Never, int], Union[NoReturn, int]] | ^^^^^ RUF020 +8 | Union[NoReturn, int, float] | + = help: Remove `Never` -RUF020.py:7:32: RUF020 `Union[NoReturn, T]` is equivalent to `T` +ℹ Safe fix +4 4 | Union[NoReturn, int] +5 5 | Never | int +6 6 | NoReturn | int +7 |-Union[Union[Never, int], Union[NoReturn, int]] + 7 |+Union[int, Union[NoReturn, int]] +8 8 | Union[NoReturn, int, float] + +RUF020.py:7:32: RUF020 [*] `Union[NoReturn, T]` is equivalent to `T` | 5 | Never | int 6 | NoReturn | int 7 | Union[Union[Never, int], Union[NoReturn, int]] | ^^^^^^^^ RUF020 +8 | Union[NoReturn, int, float] | + = help: Remove `NoReturn` + +ℹ Safe fix +4 4 | Union[NoReturn, int] +5 5 | Never | int +6 6 | NoReturn | int +7 |-Union[Union[Never, int], Union[NoReturn, int]] + 7 |+Union[Union[Never, int], int] +8 8 | Union[NoReturn, int, float] + +RUF020.py:8:7: RUF020 [*] `Union[NoReturn, T]` is equivalent to `T` + | +6 | NoReturn | int +7 | Union[Union[Never, int], Union[NoReturn, int]] +8 | Union[NoReturn, int, float] + | ^^^^^^^^ RUF020 + | + = help: Remove `NoReturn` + +ℹ Safe fix +5 5 | Never | int +6 6 | NoReturn | int +7 7 | Union[Union[Never, int], Union[NoReturn, int]] +8 |-Union[NoReturn, int, float] + 8 |+Union[int, float]