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.

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] Add never-union rule to detect redundant typing.NoReturn and typing.Never #9217

Merged
merged 1 commit into from
Dec 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF020.py
Original file line number Diff line number Diff line change
@@ -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]]
7 changes: 7 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
}
}
}

Expand Down Expand Up @@ -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(
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),

Expand Down
54 changes: 0 additions & 54 deletions crates/ruff_linter/src/rules/flake8_pyi/helpers.rs

This file was deleted.

1 change: 0 additions & 1 deletion crates/ruff_linter/src/rules/flake8_pyi/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
//! Rules from [flake8-pyi](https://pypi.org/project/flake8-pyi/).
mod helpers;
pub(crate) mod rules;

#[cfg(test)]
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
///
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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.,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand All @@ -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;
Expand All @@ -44,6 +47,5 @@ pub(crate) enum Context {
Docstring,
Comment,
}
pub(crate) use quadratic_list_summation::*;

mod quadratic_list_summation;
131 changes: 131 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/never_union.rs
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR: Hmm, one downside of quoting runtime only type annotations is that it makes all lint-rules useless because we don't parse string annotations...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually do!

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"),
}
}
}
Loading
Loading