[ruff] Add never-union rule to detect redundant typing.NoReturn and typing.Never (#9217)

## Summary

Adds a rule to detect unions that include `typing.NoReturn` or
`typing.Never`. In such cases, the use of the bottom type is redundant.

Closes https://github.com/astral-sh/ruff/issues/9113.

## Test Plan

`cargo test`
This commit is contained in:
Charlie Marsh 2023-12-21 15:53:31 -05:00 committed by GitHub
parent a3e06e5a9d
commit a9ceef5b5d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
17 changed files with 283 additions and 73 deletions

View file

@ -0,0 +1,7 @@
from typing import Never, NoReturn, Union
Union[Never, int]
Union[NoReturn, int]
Never | int
NoReturn | int
Union[Union[Never, int], Union[NoReturn, int]]

View file

@ -81,6 +81,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
Rule::DuplicateUnionMember,
Rule::RedundantLiteralUnion,
Rule::UnnecessaryTypeUnion,
Rule::NeverUnion,
]) {
// Avoid duplicate checks if the parent is a union, since these rules already
// traverse nested unions.
@ -97,6 +98,9 @@ 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);
}
}
}
@ -1174,6 +1178,9 @@ 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(

View file

@ -901,6 +901,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "017") => (RuleGroup::Nursery, rules::ruff::rules::QuadraticListSummation),
(Ruff, "018") => (RuleGroup::Preview, rules::ruff::rules::AssignmentInAssert),
(Ruff, "019") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryKeyCheck),
(Ruff, "020") => (RuleGroup::Preview, rules::ruff::rules::NeverUnion),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),

View file

@ -1,54 +0,0 @@
use ruff_python_ast::{self as ast, Expr, Operator};
use ruff_python_semantic::SemanticModel;
/// Traverse a "union" type annotation, applying `func` to each union member.
/// Supports traversal of `Union` and `|` union expressions.
/// The function is called with each expression in the union (excluding declarations of nested unions)
/// and the parent expression (if any).
pub(super) fn traverse_union<'a, F>(
func: &mut F,
semantic: &SemanticModel,
expr: &'a Expr,
parent: Option<&'a Expr>,
) where
F: FnMut(&'a Expr, Option<&'a Expr>),
{
// Ex) x | y
if let Expr::BinOp(ast::ExprBinOp {
op: Operator::BitOr,
left,
right,
range: _,
}) = expr
{
// The union data structure usually looks like this:
// a | b | c -> (a | b) | c
//
// However, parenthesized expressions can coerce it into any structure:
// a | (b | c)
//
// So we have to traverse both branches in order (left, then right), to report members
// in the order they appear in the source code.
// Traverse the left then right arms
traverse_union(func, semantic, left, Some(expr));
traverse_union(func, semantic, right, Some(expr));
return;
}
// Ex) Union[x, y]
if let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr {
if semantic.match_typing_expr(value, "Union") {
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() {
// Traverse each element of the tuple within the union recursively to handle cases
// such as `Union[..., Union[...]]
elts.iter()
.for_each(|elt| traverse_union(func, semantic, elt, Some(expr)));
return;
}
}
}
// Otherwise, call the function on expression
func(expr, parent);
}

View file

@ -1,5 +1,4 @@
//! Rules from [flake8-pyi](https://pypi.org/project/flake8-pyi/).
mod helpers;
pub(crate) mod rules;
#[cfg(test)]

View file

@ -1,15 +1,16 @@
use ruff_python_ast::{self as ast, Expr};
use rustc_hash::FxHashSet;
use std::collections::HashSet;
use crate::checkers::ast::Checker;
use rustc_hash::FxHashSet;
use crate::rules::flake8_pyi::helpers::traverse_union;
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::analyze::typing::traverse_union;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for duplicate union members.
///

View file

@ -1,14 +1,16 @@
use rustc_hash::FxHashSet;
use std::fmt;
use rustc_hash::FxHashSet;
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr, LiteralExpressionRef};
use ruff_python_semantic::analyze::typing::traverse_union;
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::fix::snippet::SourceCodeSnippet;
use crate::{checkers::ast::Checker, rules::flake8_pyi::helpers::traverse_union};
/// ## What it does
/// Checks for the presence of redundant `Literal` types and builtin super

View file

@ -1,11 +1,10 @@
use ruff_python_ast::{Expr, Parameters};
use ruff_diagnostics::{Diagnostic, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{Expr, Parameters};
use ruff_python_semantic::analyze::typing::traverse_union;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::rules::flake8_pyi::helpers::traverse_union;
/// ## What it does
/// Checks for union annotations that contain redundant numeric types (e.g.,

View file

@ -2,12 +2,11 @@ use ast::{ExprSubscript, Operator};
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::analyze::typing::traverse_union;
use ruff_text_size::{Ranged, TextRange};
use crate::checkers::ast::Checker;
use crate::rules::flake8_pyi::helpers::traverse_union;
/// ## What it does
/// Checks for the presence of multiple literal types in a union.
///

View file

@ -2,9 +2,10 @@ use ast::{ExprContext, Operator};
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, Expr};
use ruff_python_semantic::analyze::typing::traverse_union;
use ruff_text_size::{Ranged, TextRange};
use crate::{checkers::ast::Checker, rules::flake8_pyi::helpers::traverse_union};
use crate::checkers::ast::Checker;
/// ## What it does
/// Checks for the presence of multiple `type`s in a union.

View file

@ -44,6 +44,7 @@ mod tests {
#[test_case(Rule::QuadraticListSummation, Path::new("RUF017_0.py"))]
#[test_case(Rule::AssignmentInAssert, Path::new("RUF018.py"))]
#[test_case(Rule::UnnecessaryKeyCheck, Path::new("RUF019.py"))]
#[test_case(Rule::NeverUnion, Path::new("RUF020.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(

View file

@ -9,7 +9,9 @@ pub(crate) use invalid_index_type::*;
pub(crate) use invalid_pyproject_toml::*;
pub(crate) use mutable_class_default::*;
pub(crate) use mutable_dataclass_default::*;
pub(crate) use never_union::*;
pub(crate) use pairwise_over_zipped::*;
pub(crate) use quadratic_list_summation::*;
pub(crate) use static_key_dict_comprehension::*;
pub(crate) use unnecessary_iterable_allocation_for_first_element::*;
pub(crate) use unnecessary_key_check::*;
@ -30,6 +32,7 @@ mod invalid_index_type;
mod invalid_pyproject_toml;
mod mutable_class_default;
mod mutable_dataclass_default;
mod never_union;
mod pairwise_over_zipped;
mod static_key_dict_comprehension;
mod unnecessary_iterable_allocation_for_first_element;
@ -44,6 +47,5 @@ pub(crate) enum Context {
Docstring,
Comment,
}
pub(crate) use quadratic_list_summation::*;
mod quadratic_list_summation;

View file

@ -0,0 +1,131 @@
use ruff_diagnostics::{Diagnostic, Violation};
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 crate::checkers::ast::Checker;
/// ## What it does
/// Checks for uses of `typing.NoReturn` and `typing.Never` in union types.
///
/// ## Why is this bad?
/// `typing.NoReturn` and `typing.Never` are special types, used to indicate
/// that a function never returns, or that a type has no values.
///
/// Including `typing.NoReturn` or `typing.Never` in a union type is redundant,
/// as, e.g., `typing.Never | T` is equivalent to `T`.
///
/// ## Example
/// ```python
/// from typing import Never
///
///
/// def func() -> Never | int:
/// ...
/// ```
///
/// Use instead:
/// ```python
/// def func() -> int:
/// ...
/// ```
///
/// ## Options
/// - [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)
#[violation]
pub struct NeverUnion {
never_like: NeverLike,
union_like: UnionLike,
}
impl Violation for NeverUnion {
#[derive_message_formats]
fn message(&self) -> String {
let Self {
never_like,
union_like,
} = self;
match union_like {
UnionLike::BinOp => {
format!("`{never_like} | T` is equivalent to `T`")
}
UnionLike::TypingUnion => {
format!("`Union[{never_like}, T]` is equivalent to `T`")
}
}
}
}
/// 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();
// 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::TypingUnion
};
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
};
if let Some(never_like) = never_like {
expressions.push((never_like, union_like, expr));
return;
}
}
rest.push(expr);
};
traverse_union(&mut collect_never, checker.semantic(), expr, None);
// 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);
}
}
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum UnionLike {
/// E.g., `typing.Union[int, str]`
TypingUnion,
/// E.g., `int | str`
BinOp,
}
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum NeverLike {
/// E.g., `typing.NoReturn`
NoReturn,
/// E.g., `typing.Never`
Never,
}
impl std::fmt::Display for NeverLike {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
NeverLike::NoReturn => f.write_str("NoReturn"),
NeverLike::Never => f.write_str("Never"),
}
}
}

View file

@ -0,0 +1,58 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF020.py:3:7: RUF020 `Union[Never, T]` is equivalent to `T`
|
1 | from typing import Never, NoReturn, Union
2 |
3 | Union[Never, int]
| ^^^^^ RUF020
4 | Union[NoReturn, int]
5 | Never | int
|
RUF020.py:4:7: RUF020 `Union[NoReturn, T]` is equivalent to `T`
|
3 | Union[Never, int]
4 | Union[NoReturn, int]
| ^^^^^^^^ RUF020
5 | Never | int
6 | NoReturn | int
|
RUF020.py:5:1: RUF020 `Never | T` is equivalent to `T`
|
3 | Union[Never, int]
4 | Union[NoReturn, int]
5 | Never | int
| ^^^^^ RUF020
6 | NoReturn | int
7 | Union[Union[Never, int], Union[NoReturn, int]]
|
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]]
|
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
|
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
|

View file

@ -1505,6 +1505,7 @@ pub fn pep_604_union(elts: &[Expr]) -> Expr {
}
}
/// Format the expression as a `typing.Optional`-style optional.
pub fn typing_optional(elt: Expr, binding: String) -> Expr {
Expr::Subscript(ast::ExprSubscript {
value: Box::new(Expr::Name(ast::ExprName {
@ -1518,18 +1519,19 @@ pub fn typing_optional(elt: Expr, binding: String) -> Expr {
})
}
/// Format the expressions as a `typing.Union`-style union.
pub fn typing_union(elts: &[Expr], binding: String) -> Expr {
fn tuple(elts: &[Expr]) -> Expr {
fn tuple(elts: &[Expr], binding: String) -> Expr {
match elts {
[] => Expr::Tuple(ast::ExprTuple {
elts: vec![],
ctx: ExprContext::Load,
range: TextRange::default(),
}),
[Expr::Tuple(ast::ExprTuple { elts, .. })] => pep_604_union(elts),
[Expr::Tuple(ast::ExprTuple { elts, .. })] => typing_union(elts, binding),
[elt] => elt.clone(),
[rest @ .., elt] => Expr::BinOp(ast::ExprBinOp {
left: Box::new(tuple(rest)),
left: Box::new(tuple(rest, binding)),
op: Operator::BitOr,
right: Box::new(elt.clone()),
range: TextRange::default(),
@ -1539,11 +1541,11 @@ pub fn typing_union(elts: &[Expr], binding: String) -> Expr {
Expr::Subscript(ast::ExprSubscript {
value: Box::new(Expr::Name(ast::ExprName {
id: binding,
id: binding.clone(),
range: TextRange::default(),
ctx: ExprContext::Load,
})),
slice: Box::new(tuple(elts)),
slice: Box::new(tuple(elts, binding)),
ctx: ExprContext::Load,
range: TextRange::default(),
})

View file

@ -333,6 +333,58 @@ pub fn is_sys_version_block(stmt: &ast::StmtIf, semantic: &SemanticModel) -> boo
})
}
/// Traverse a "union" type annotation, applying `func` to each union member.
/// Supports traversal of `Union` and `|` union expressions.
/// The function is called with each expression in the union (excluding declarations of nested unions)
/// and the parent expression (if any).
pub fn traverse_union<'a, F>(
func: &mut F,
semantic: &SemanticModel,
expr: &'a Expr,
parent: Option<&'a Expr>,
) where
F: FnMut(&'a Expr, Option<&'a Expr>),
{
// Ex) x | y
if let Expr::BinOp(ast::ExprBinOp {
op: Operator::BitOr,
left,
right,
range: _,
}) = expr
{
// The union data structure usually looks like this:
// a | b | c -> (a | b) | c
//
// However, parenthesized expressions can coerce it into any structure:
// a | (b | c)
//
// So we have to traverse both branches in order (left, then right), to report members
// in the order they appear in the source code.
// Traverse the left then right arms
traverse_union(func, semantic, left, Some(expr));
traverse_union(func, semantic, right, Some(expr));
return;
}
// Ex) Union[x, y]
if let Expr::Subscript(ast::ExprSubscript { value, slice, .. }) = expr {
if semantic.match_typing_expr(value, "Union") {
if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() {
// Traverse each element of the tuple within the union recursively to handle cases
// such as `Union[..., Union[...]]
elts.iter()
.for_each(|elt| traverse_union(func, semantic, elt, Some(expr)));
return;
}
}
}
// Otherwise, call the function on expression
func(expr, parent);
}
/// Abstraction for a type checker, conservatively checks for the intended type(s).
trait TypeChecker {
/// Check annotation expression to match the intended type(s).

2
ruff.schema.json generated
View file

@ -3429,6 +3429,8 @@
"RUF017",
"RUF018",
"RUF019",
"RUF02",
"RUF020",
"RUF1",
"RUF10",
"RUF100",