Skip to content

Commit

Permalink
Add rule for NoReturn
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Dec 20, 2023
1 parent cbe3bf9 commit 024a2c5
Show file tree
Hide file tree
Showing 17 changed files with 283 additions and 73 deletions.
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::Union => {
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::Union
};

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]`
Union,
/// 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

0 comments on commit 024a2c5

Please sign in to comment.