Skip to content

Commit 59e96ff

Browse files
sbrugmanAlexWaygoodcharliermarsh
authored andcommitted
[flake8-pyi] Implement redundant-none-literal (PYI061) (astral-sh#14316)
## Summary `Literal[None]` can be simplified into `None` in type annotations. Surprising to see that this is not that rare: - https://github.com/langchain-ai/langchain/blob/master/libs/langchain/langchain/chat_models/base.py#L54 - https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/sql/annotation.py#L69 - https://github.com/jax-ml/jax/blob/main/jax/numpy/__init__.pyi#L961 - https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/inference/_common.py#L179 ## Test Plan `cargo test` Reviewed all ecosystem results, and they are true positives. --------- Co-authored-by: Alex Waygood <[email protected]> Co-authored-by: Charlie Marsh <[email protected]>
1 parent 5c797e6 commit 59e96ff

File tree

11 files changed

+618
-2
lines changed

11 files changed

+618
-2
lines changed

crates/red_knot_workspace/tests/check.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,7 @@ const KNOWN_FAILURES: &[(&str, bool, bool)] = &[
256256
("crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI052.py", true, true),
257257
("crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.py", true, true),
258258
("crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI055.py", true, true),
259+
("crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI061.py", true, true),
259260
("crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI062.py", true, true),
260261
("crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI062.py", true, true),
261262
("crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI063.py", true, true),
Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
from typing import Literal
2+
3+
4+
def func1(arg1: Literal[None]):
5+
...
6+
7+
8+
def func2(arg1: Literal[None] | int):
9+
...
10+
11+
12+
def func3() -> Literal[None]:
13+
...
14+
15+
16+
def func4(arg1: Literal[int, None, float]):
17+
...
18+
19+
20+
def func5(arg1: Literal[None, None]):
21+
...
22+
23+
24+
def func6(arg1: Literal[
25+
"hello",
26+
None # Comment 1
27+
, "world"
28+
]):
29+
...
30+
31+
32+
def func7(arg1: Literal[
33+
None # Comment 1
34+
]):
35+
...
36+
37+
38+
# OK
39+
def good_func(arg1: Literal[int] | None):
40+
...
41+
42+
43+
# From flake8-pyi
44+
Literal[None] # Y061 None inside "Literal[]" expression. Replace with "None"
45+
Literal[True, None] # Y061 None inside "Literal[]" expression. Replace with "Literal[True] | None"
46+
47+
###
48+
# The following rules here are slightly subtle,
49+
# but make sense when it comes to giving the best suggestions to users of flake8-pyi.
50+
###
51+
52+
# If Y061 and Y062 both apply, but all the duplicate members are None,
53+
# only emit Y061...
54+
Literal[None, None] # Y061 None inside "Literal[]" expression. Replace with "None"
55+
Literal[1, None, "foo", None] # Y061 None inside "Literal[]" expression. Replace with "Literal[1, 'foo'] | None"
56+
57+
# ... but if Y061 and Y062 both apply
58+
# and there are no None members in the Literal[] slice,
59+
# only emit Y062:
60+
Literal[None, True, None, True] # Y062 Duplicate "Literal[]" member "True"
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
from typing import Literal
2+
3+
4+
def func1(arg1: Literal[None]): ...
5+
6+
7+
def func2(arg1: Literal[None] | int): ...
8+
9+
10+
def func3() -> Literal[None]: ...
11+
12+
13+
def func4(arg1: Literal[int, None, float]): ...
14+
15+
16+
def func5(arg1: Literal[None, None]): ...
17+
18+
19+
def func6(arg1: Literal[
20+
"hello",
21+
None # Comment 1
22+
, "world"
23+
]): ...
24+
25+
26+
def func7(arg1: Literal[
27+
None # Comment 1
28+
]): ...
29+
30+
31+
# OK
32+
def good_func(arg1: Literal[int] | None): ...
33+
34+
35+
# From flake8-pyi
36+
Literal[None] # PYI061 None inside "Literal[]" expression. Replace with "None"
37+
Literal[True, None] # PYI061 None inside "Literal[]" expression. Replace with "Literal[True] | None"

crates/ruff_linter/src/checkers/ast/analyze/expression.rs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,9 +104,14 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
104104
}
105105

106106
// Ex) Literal[...]
107-
if checker.enabled(Rule::DuplicateLiteralMember) {
107+
if checker.any_enabled(&[Rule::DuplicateLiteralMember, Rule::RedundantNoneLiteral]) {
108108
if !checker.semantic.in_nested_literal() {
109-
flake8_pyi::rules::duplicate_literal_member(checker, expr);
109+
if checker.enabled(Rule::DuplicateLiteralMember) {
110+
flake8_pyi::rules::duplicate_literal_member(checker, expr);
111+
}
112+
if checker.enabled(Rule::RedundantNoneLiteral) {
113+
flake8_pyi::rules::redundant_none_literal(checker, expr);
114+
}
110115
}
111116
}
112117

crates/ruff_linter/src/codes.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -785,6 +785,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
785785
(Flake8Pyi, "058") => (RuleGroup::Stable, rules::flake8_pyi::rules::GeneratorReturnFromIterMethod),
786786
(Flake8Pyi, "057") => (RuleGroup::Stable, rules::flake8_pyi::rules::ByteStringUsage),
787787
(Flake8Pyi, "059") => (RuleGroup::Preview, rules::flake8_pyi::rules::GenericNotLastBaseClass),
788+
(Flake8Pyi, "061") => (RuleGroup::Preview, rules::flake8_pyi::rules::RedundantNoneLiteral),
788789
(Flake8Pyi, "062") => (RuleGroup::Stable, rules::flake8_pyi::rules::DuplicateLiteralMember),
789790
(Flake8Pyi, "063") => (RuleGroup::Preview, rules::flake8_pyi::rules::PrePep570PositionalArgument),
790791
(Flake8Pyi, "064") => (RuleGroup::Preview, rules::flake8_pyi::rules::RedundantFinalLiteral),

crates/ruff_linter/src/rules/flake8_pyi/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,8 @@ mod tests {
124124
#[test_case(Rule::UnusedPrivateTypedDict, Path::new("PYI049.pyi"))]
125125
#[test_case(Rule::WrongTupleLengthVersionComparison, Path::new("PYI005.py"))]
126126
#[test_case(Rule::WrongTupleLengthVersionComparison, Path::new("PYI005.pyi"))]
127+
#[test_case(Rule::RedundantNoneLiteral, Path::new("PYI061.py"))]
128+
#[test_case(Rule::RedundantNoneLiteral, Path::new("PYI061.pyi"))]
127129
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
128130
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
129131
let diagnostics = test_path(

crates/ruff_linter/src/rules/flake8_pyi/rules/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ pub(crate) use prefix_type_params::*;
2727
pub(crate) use quoted_annotation_in_stub::*;
2828
pub(crate) use redundant_final_literal::*;
2929
pub(crate) use redundant_literal_union::*;
30+
pub(crate) use redundant_none_literal::*;
3031
pub(crate) use redundant_numeric_union::*;
3132
pub(crate) use simple_defaults::*;
3233
pub(crate) use str_or_repr_defined_in_stub::*;
@@ -69,6 +70,7 @@ mod prefix_type_params;
6970
mod quoted_annotation_in_stub;
7071
mod redundant_final_literal;
7172
mod redundant_literal_union;
73+
mod redundant_none_literal;
7274
mod redundant_numeric_union;
7375
mod simple_defaults;
7476
mod str_or_repr_defined_in_stub;
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
2+
use ruff_macros::{derive_message_formats, violation};
3+
use ruff_python_ast::{Expr, ExprNoneLiteral};
4+
use ruff_python_semantic::analyze::typing::traverse_literal;
5+
use ruff_text_size::Ranged;
6+
7+
use smallvec::SmallVec;
8+
9+
use crate::checkers::ast::Checker;
10+
11+
/// ## What it does
12+
/// Checks for redundant `Literal[None]` annotations.
13+
///
14+
/// ## Why is this bad?
15+
/// While `Literal[None]` is a valid type annotation, it is semantically equivalent to `None`.
16+
/// Prefer `None` over `Literal[None]` for both consistency and readability.
17+
///
18+
/// ## Example
19+
/// ```python
20+
/// from typing import Literal
21+
///
22+
/// Literal[None]
23+
/// Literal[1, 2, 3, "foo", 5, None]
24+
/// ```
25+
///
26+
/// Use instead:
27+
/// ```python
28+
/// from typing import Literal
29+
///
30+
/// None
31+
/// Literal[1, 2, 3, "foo", 5] | None
32+
/// ```
33+
///
34+
/// ## References
35+
/// - [Typing documentation: Legal parameters for `Literal` at type check time](https://typing.readthedocs.io/en/latest/spec/literal.html#legal-parameters-for-literal-at-type-check-time)
36+
#[violation]
37+
pub struct RedundantNoneLiteral {
38+
other_literal_elements_seen: bool,
39+
}
40+
41+
impl Violation for RedundantNoneLiteral {
42+
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
43+
44+
#[derive_message_formats]
45+
fn message(&self) -> String {
46+
if self.other_literal_elements_seen {
47+
"`Literal[None, ...]` can be replaced with `Literal[...] | None`".to_string()
48+
} else {
49+
"`Literal[None]` can be replaced with `None`".to_string()
50+
}
51+
}
52+
53+
fn fix_title(&self) -> Option<String> {
54+
Some(if self.other_literal_elements_seen {
55+
"Replace with `Literal[...] | None`".to_string()
56+
} else {
57+
"Replace with `None`".to_string()
58+
})
59+
}
60+
}
61+
62+
/// RUF037
63+
pub(crate) fn redundant_none_literal<'a>(checker: &mut Checker, literal_expr: &'a Expr) {
64+
if !checker.semantic().seen_typing() {
65+
return;
66+
}
67+
68+
let mut none_exprs: SmallVec<[&ExprNoneLiteral; 1]> = SmallVec::new();
69+
let mut other_literal_elements_seen = false;
70+
71+
let mut find_none = |expr: &'a Expr, _parent: &'a Expr| {
72+
if let Expr::NoneLiteral(none_expr) = expr {
73+
none_exprs.push(none_expr);
74+
} else {
75+
other_literal_elements_seen = true;
76+
}
77+
};
78+
79+
traverse_literal(&mut find_none, checker.semantic(), literal_expr);
80+
81+
if none_exprs.is_empty() {
82+
return;
83+
}
84+
85+
// Provide a [`Fix`] when the complete `Literal` can be replaced. Applying the fix
86+
// can leave an unused import to be fixed by the `unused-import` rule.
87+
let fix = if other_literal_elements_seen {
88+
None
89+
} else {
90+
Some(Fix::applicable_edit(
91+
Edit::range_replacement("None".to_string(), literal_expr.range()),
92+
if checker.comment_ranges().intersects(literal_expr.range()) {
93+
Applicability::Unsafe
94+
} else {
95+
Applicability::Safe
96+
},
97+
))
98+
};
99+
100+
for none_expr in none_exprs {
101+
let mut diagnostic = Diagnostic::new(
102+
RedundantNoneLiteral {
103+
other_literal_elements_seen,
104+
},
105+
none_expr.range(),
106+
);
107+
if let Some(ref fix) = fix {
108+
diagnostic.set_fix(fix.clone());
109+
}
110+
checker.diagnostics.push(diagnostic);
111+
}
112+
}

0 commit comments

Comments
 (0)