From a40bc6a46003a8de4854b011d82c83e59e606922 Mon Sep 17 00:00:00 2001 From: Simon Brugman Date: Thu, 14 Nov 2024 19:37:13 +0100 Subject: [PATCH] [`ruff`] Implement `none-not-at-end-of-union` (`RUF036`) (#14314) --- .../resources/test/fixtures/ruff/RUF036.py | 47 +++++++++++ .../resources/test/fixtures/ruff/RUF036.pyi | 25 ++++++ .../src/checkers/ast/analyze/expression.rs | 7 ++ crates/ruff_linter/src/codes.rs | 1 + crates/ruff_linter/src/rules/ruff/mod.rs | 2 + .../ruff_linter/src/rules/ruff/rules/mod.rs | 2 + .../ruff/rules/none_not_at_end_of_union.rs | 77 ++++++++++++++++++ ..._rules__ruff__tests__RUF036_RUF036.py.snap | 58 ++++++++++++++ ...rules__ruff__tests__RUF036_RUF036.pyi.snap | 80 +++++++++++++++++++ ruff.schema.json | 1 + 10 files changed, 300 insertions(+) create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF036.py create mode 100644 crates/ruff_linter/resources/test/fixtures/ruff/RUF036.pyi create mode 100644 crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.py.snap create mode 100644 crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.pyi.snap diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF036.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF036.py new file mode 100644 index 0000000000..00aca49ab6 --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF036.py @@ -0,0 +1,47 @@ +from typing import Union as U + + +def func1(arg: None | int): + ... + + +def func2() -> None | int: + ... + + +def func3(arg: None | None | int): + ... + + +def func4(arg: U[None, int]): + ... + + +def func5() -> U[None, int]: + ... + + +def func6(arg: U[None, None, int]): + ... + + +# Ok +def good_func1(arg: int | None): + ... + + +def good_func2() -> int | None: + ... + + +def good_func3(arg: None): + ... + + +def good_func4(arg: U[None]): + ... + + +def good_func5(arg: U[int]): + ... + diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF036.pyi b/crates/ruff_linter/resources/test/fixtures/ruff/RUF036.pyi new file mode 100644 index 0000000000..f4210325bf --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF036.pyi @@ -0,0 +1,25 @@ +from typing import Union as U + + +def func1(arg: None | int): ... + +def func2() -> None | int: ... + +def func3(arg: None | None | int): ... + +def func4(arg: U[None, int]): ... + +def func5() -> U[None, int]: ... + +def func6(arg: U[None, None, int]): ... + +# Ok +def good_func1(arg: int | None): ... + +def good_func2() -> int | None: ... + +def good_func3(arg: None): ... + +def good_func4(arg: U[None]): ... + +def good_func5(arg: U[int]): ... diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 74f860d429..605d974444 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -80,6 +80,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { Rule::DuplicateUnionMember, Rule::RedundantLiteralUnion, Rule::UnnecessaryTypeUnion, + Rule::NoneNotAtEndOfUnion, ]) { // Avoid duplicate checks if the parent is a union, since these rules already // traverse nested unions. @@ -96,6 +97,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::NoneNotAtEndOfUnion) { + ruff::rules::none_not_at_end_of_union(checker, expr); + } } } @@ -1282,6 +1286,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::NoneNotAtEndOfUnion) { + ruff::rules::none_not_at_end_of_union(checker, expr); + } } } Expr::UnaryOp( diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 5d676033ad..97c1dc3eb8 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -968,6 +968,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Ruff, "033") => (RuleGroup::Preview, rules::ruff::rules::PostInitDefault), (Ruff, "034") => (RuleGroup::Preview, rules::ruff::rules::UselessIfElse), (Ruff, "035") => (RuleGroup::Preview, rules::ruff::rules::UnsafeMarkupUse), + (Ruff, "036") => (RuleGroup::Preview, rules::ruff::rules::NoneNotAtEndOfUnion), (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA), (Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA), diff --git a/crates/ruff_linter/src/rules/ruff/mod.rs b/crates/ruff_linter/src/rules/ruff/mod.rs index 95971a0378..5117d935c1 100644 --- a/crates/ruff_linter/src/rules/ruff/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/mod.rs @@ -62,6 +62,8 @@ mod tests { #[test_case(Rule::UselessIfElse, Path::new("RUF034.py"))] #[test_case(Rule::RedirectedNOQA, Path::new("RUF101.py"))] #[test_case(Rule::PostInitDefault, Path::new("RUF033.py"))] + #[test_case(Rule::NoneNotAtEndOfUnion, Path::new("RUF036.py"))] + #[test_case(Rule::NoneNotAtEndOfUnion, Path::new("RUF036.pyi"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/ruff/rules/mod.rs b/crates/ruff_linter/src/rules/ruff/rules/mod.rs index 102174f8e3..db559fe510 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/mod.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/mod.rs @@ -17,6 +17,7 @@ pub(crate) use mutable_class_default::*; pub(crate) use mutable_dataclass_default::*; pub(crate) use mutable_fromkeys_value::*; pub(crate) use never_union::*; +pub(crate) use none_not_at_end_of_union::*; pub(crate) use parenthesize_logical_operators::*; pub(crate) use post_init_default::*; pub(crate) use quadratic_list_summation::*; @@ -55,6 +56,7 @@ mod mutable_class_default; mod mutable_dataclass_default; mod mutable_fromkeys_value; mod never_union; +mod none_not_at_end_of_union; mod parenthesize_logical_operators; mod post_init_default; mod quadratic_list_summation; diff --git a/crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs b/crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs new file mode 100644 index 0000000000..ff6300fb2d --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/rules/none_not_at_end_of_union.rs @@ -0,0 +1,77 @@ +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 smallvec::SmallVec; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for type annotations where `None` is not at the end of an union. +/// +/// ## Why is this bad? +/// Type annotation unions are associative, meaning that the order of the elements +/// does not matter. The `None` literal represents the absence of a value. For +/// readability, it's preferred to write the more informative type expressions first. +/// +/// ## Example +/// ```python +/// def func(arg: None | int): ... +/// ``` +/// +/// Use instead: +/// ```python +/// def func(arg: int | None): ... +/// ``` +/// +/// ## References +/// - [Python documentation: Union type](https://docs.python.org/3/library/stdtypes.html#types-union) +/// - [Python documentation: `typing.Optional`](https://docs.python.org/3/library/typing.html#typing.Optional) +/// - [Python documentation: `None`](https://docs.python.org/3/library/constants.html#None) +#[violation] +pub struct NoneNotAtEndOfUnion; + +impl Violation for NoneNotAtEndOfUnion { + #[derive_message_formats] + fn message(&self) -> String { + "`None` not at the end of the type annotation.".to_string() + } +} + +/// RUF036 +pub(crate) fn none_not_at_end_of_union<'a>(checker: &mut Checker, union: &'a Expr) { + let semantic = checker.semantic(); + let mut none_exprs: SmallVec<[&Expr; 1]> = SmallVec::new(); + + let mut last_expr: Option<&Expr> = None; + let mut find_none = |expr: &'a Expr, _parent: &Expr| { + if matches!(expr, Expr::NoneLiteral(_)) { + none_exprs.push(expr); + } + last_expr = Some(expr); + }; + + // Walk through all type expressions in the union and keep track of `None` literals. + traverse_union(&mut find_none, semantic, union); + + let Some(last_expr) = last_expr else { + return; + }; + + // The must be at least one `None` expression. + let Some(last_none) = none_exprs.last() else { + return; + }; + + // If any of the `None` literals is last we do not emit. + if *last_none == last_expr { + return; + } + + for none_expr in none_exprs { + checker + .diagnostics + .push(Diagnostic::new(NoneNotAtEndOfUnion, none_expr.range())); + } +} diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.py.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.py.snap new file mode 100644 index 0000000000..20eaffafd9 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.py.snap @@ -0,0 +1,58 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF036.py:4:16: RUF036 `None` not at the end of the type annotation. + | +4 | def func1(arg: None | int): + | ^^^^ RUF036 +5 | ... + | + +RUF036.py:8:16: RUF036 `None` not at the end of the type annotation. + | +8 | def func2() -> None | int: + | ^^^^ RUF036 +9 | ... + | + +RUF036.py:12:16: RUF036 `None` not at the end of the type annotation. + | +12 | def func3(arg: None | None | int): + | ^^^^ RUF036 +13 | ... + | + +RUF036.py:12:23: RUF036 `None` not at the end of the type annotation. + | +12 | def func3(arg: None | None | int): + | ^^^^ RUF036 +13 | ... + | + +RUF036.py:16:18: RUF036 `None` not at the end of the type annotation. + | +16 | def func4(arg: U[None, int]): + | ^^^^ RUF036 +17 | ... + | + +RUF036.py:20:18: RUF036 `None` not at the end of the type annotation. + | +20 | def func5() -> U[None, int]: + | ^^^^ RUF036 +21 | ... + | + +RUF036.py:24:18: RUF036 `None` not at the end of the type annotation. + | +24 | def func6(arg: U[None, None, int]): + | ^^^^ RUF036 +25 | ... + | + +RUF036.py:24:24: RUF036 `None` not at the end of the type annotation. + | +24 | def func6(arg: U[None, None, int]): + | ^^^^ RUF036 +25 | ... + | diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.pyi.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.pyi.snap new file mode 100644 index 0000000000..b66df39a84 --- /dev/null +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__RUF036_RUF036.pyi.snap @@ -0,0 +1,80 @@ +--- +source: crates/ruff_linter/src/rules/ruff/mod.rs +--- +RUF036.pyi:4:16: RUF036 `None` not at the end of the type annotation. + | +4 | def func1(arg: None | int): ... + | ^^^^ RUF036 +5 | +6 | def func2() -> None | int: ... + | + +RUF036.pyi:6:16: RUF036 `None` not at the end of the type annotation. + | +4 | def func1(arg: None | int): ... +5 | +6 | def func2() -> None | int: ... + | ^^^^ RUF036 +7 | +8 | def func3(arg: None | None | int): ... + | + +RUF036.pyi:8:16: RUF036 `None` not at the end of the type annotation. + | + 6 | def func2() -> None | int: ... + 7 | + 8 | def func3(arg: None | None | int): ... + | ^^^^ RUF036 + 9 | +10 | def func4(arg: U[None, int]): ... + | + +RUF036.pyi:8:23: RUF036 `None` not at the end of the type annotation. + | + 6 | def func2() -> None | int: ... + 7 | + 8 | def func3(arg: None | None | int): ... + | ^^^^ RUF036 + 9 | +10 | def func4(arg: U[None, int]): ... + | + +RUF036.pyi:10:18: RUF036 `None` not at the end of the type annotation. + | + 8 | def func3(arg: None | None | int): ... + 9 | +10 | def func4(arg: U[None, int]): ... + | ^^^^ RUF036 +11 | +12 | def func5() -> U[None, int]: ... + | + +RUF036.pyi:12:18: RUF036 `None` not at the end of the type annotation. + | +10 | def func4(arg: U[None, int]): ... +11 | +12 | def func5() -> U[None, int]: ... + | ^^^^ RUF036 +13 | +14 | def func6(arg: U[None, None, int]): ... + | + +RUF036.pyi:14:18: RUF036 `None` not at the end of the type annotation. + | +12 | def func5() -> U[None, int]: ... +13 | +14 | def func6(arg: U[None, None, int]): ... + | ^^^^ RUF036 +15 | +16 | # Ok + | + +RUF036.pyi:14:24: RUF036 `None` not at the end of the type annotation. + | +12 | def func5() -> U[None, int]: ... +13 | +14 | def func6(arg: U[None, None, int]): ... + | ^^^^ RUF036 +15 | +16 | # Ok + | diff --git a/ruff.schema.json b/ruff.schema.json index e816780e4f..33bb45ca7e 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -3833,6 +3833,7 @@ "RUF033", "RUF034", "RUF035", + "RUF036", "RUF1", "RUF10", "RUF100",