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.

Sign up for GitHub

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

[flake8-pyi]: More autofixes for redundant-none-literal (PYI061) #14872

Merged
merged 8 commits into from
Dec 12, 2024
Merged

[flake8-pyi]: More autofixes for redundant-none-literal (PYI061) #14872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1f2e93b
fix: Correct implementation for PYI061
kiran-4444 Dec 9, 2024
2363e7f
fix: use first Literal Expr instead of creating one
kiran-4444 Dec 10, 2024
baf46c9
fix: handle single element Literal seperately from multi-element Literal
kiran-4444 Dec 11, 2024
0f27765
chore: update snapshots
kiran-4444 Dec 11, 2024
8600002
chore: accept new snapshots
kiran-4444 Dec 11, 2024
e2fd2c2
chore: update snapshots
kiran-4444 Dec 11, 2024
8663352
lint: make clippy happy
kiran-4444 Dec 12, 2024
c9150d4
Consolidate the two fix functions and offer no fix on Python <=3.9
AlexWaygood Dec 12, 2024
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
107 changes: 83 additions & 24 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_none_literal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_python_ast::{Expr, ExprBinOp, ExprNoneLiteral, ExprSubscript, Operator};
use ruff_python_semantic::{
analyze::typing::{traverse_literal, traverse_union},
SemanticModel,
use ruff_python_ast::{
self as ast, Expr, ExprBinOp, ExprContext, ExprNoneLiteral, ExprSubscript, Operator,
};
use ruff_text_size::Ranged;
use ruff_python_semantic::analyze::typing::{traverse_literal, traverse_union};
use ruff_text_size::{Ranged, TextRange};

use smallvec::SmallVec;

use crate::checkers::ast::Checker;
use crate::{checkers::ast::Checker, settings::types::PythonVersion};

/// ## What it does
/// Checks for redundant `Literal[None]` annotations.
Expand All @@ -34,9 +33,14 @@ use crate::checkers::ast::Checker;
/// Literal[1, 2, 3, "foo", 5] | None
/// ```
///
/// ## Fix safety
/// ## Fix safety and availability
/// This rule's fix is marked as safe unless the literal contains comments.
///
/// There is currently no fix available if there are other elements in the `Literal` slice aside
/// from `None` and [`target-version`] is set to Python 3.9 or lower, as the fix always uses the
/// `|` syntax to create unions rather than `typing.Union`, and the `|` syntax for unions was added
/// in Python 3.10.
///
/// ## References
/// - [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)
#[derive(ViolationMetadata)]
Expand Down Expand Up @@ -65,35 +69,44 @@ impl Violation for RedundantNoneLiteral {
}
}

/// RUF037
/// PYI061
pub(crate) fn redundant_none_literal<'a>(checker: &mut Checker, literal_expr: &'a Expr) {
if !checker.semantic().seen_typing() {
let semantic = checker.semantic();

if !semantic.seen_typing() {
return;
}

let Expr::Subscript(ast::ExprSubscript {
value: literal_subscript,
..
}) = literal_expr
else {
return;
};

let mut none_exprs: SmallVec<[&ExprNoneLiteral; 1]> = SmallVec::new();
let mut other_literal_elements_seen = false;
let mut literal_elements = vec![];

let mut find_none = |expr: &'a Expr, _parent: &'a Expr| {
let mut partition_literal_elements = |expr: &'a Expr, _parent: &'a Expr| {
if let Expr::NoneLiteral(none_expr) = expr {
none_exprs.push(none_expr);
} else {
other_literal_elements_seen = true;
literal_elements.push(expr);
}
};

traverse_literal(&mut find_none, checker.semantic(), literal_expr);
traverse_literal(&mut partition_literal_elements, semantic, literal_expr);

if none_exprs.is_empty() {
return;
}

// Provide a [`Fix`] when the complete `Literal` can be replaced. Applying the fix
// can leave an unused import to be fixed by the `unused-import` rule.
let fix = if other_literal_elements_seen {
None
} else {
create_fix_edit(checker.semantic(), literal_expr).map(|edit| {
let other_literal_elements_seen = !literal_elements.is_empty();

// N.B. Applying the fix can leave an unused import to be fixed by the `unused-import` rule.
let fix =
create_fix_edit(checker, literal_expr, literal_subscript, literal_elements).map(|edit| {
Fix::applicable_edit(
edit,
if checker.comment_ranges().intersects(literal_expr.range()) {
Expand All @@ -102,8 +115,7 @@ pub(crate) fn redundant_none_literal<'a>(checker: &mut Checker, literal_expr: &'
Applicability::Safe
},
)
})
};
});

for none_expr in none_exprs {
let mut diagnostic = Diagnostic::new(
Expand All @@ -128,7 +140,14 @@ pub(crate) fn redundant_none_literal<'a>(checker: &mut Checker, literal_expr: &'
/// See <https://github.com/astral-sh/ruff/issues/14567>.
///
/// [`typing.Union`]: https://docs.python.org/3/library/typing.html#typing.Union
fn create_fix_edit(semantic: &SemanticModel, literal_expr: &Expr) -> Option<Edit> {
fn create_fix_edit(
checker: &Checker,
literal_expr: &Expr,
literal_subscript: &Expr,
literal_elements: Vec<&Expr>,
) -> Option<Edit> {
let semantic = checker.semantic();

let enclosing_pep604_union = semantic
.current_expressions()
.skip(1)
Expand All @@ -143,8 +162,9 @@ fn create_fix_edit(semantic: &SemanticModel, literal_expr: &Expr) -> Option<Edit
})
.last();

let mut is_fixable = true;
if let Some(enclosing_pep604_union) = enclosing_pep604_union {
let mut is_fixable = true;

traverse_union(
&mut |expr, _| {
if matches!(expr, Expr::NoneLiteral(_)) {
Expand All @@ -163,7 +183,46 @@ fn create_fix_edit(semantic: &SemanticModel, literal_expr: &Expr) -> Option<Edit
semantic,
enclosing_pep604_union,
);

if !is_fixable {
return None;
}
}

if literal_elements.is_empty() {
return Some(Edit::range_replacement(
"None".to_string(),
literal_expr.range(),
));
}

if checker.settings.target_version < PythonVersion::Py310 {
return None;
}

is_fixable.then(|| Edit::range_replacement("None".to_string(), literal_expr.range()))
let bin_or = Expr::BinOp(ExprBinOp {
range: TextRange::default(),
left: Box::new(Expr::Subscript(ast::ExprSubscript {
value: Box::new(literal_subscript.clone()),
range: TextRange::default(),
ctx: ExprContext::Load,
slice: Box::new(if literal_elements.len() > 1 {
Expr::Tuple(ast::ExprTuple {
elts: literal_elements.into_iter().cloned().collect(),
range: TextRange::default(),
ctx: ExprContext::Load,
parenthesized: true,
})
} else {
literal_elements[0].clone()
}),
})),
op: ruff_python_ast::Operator::BitOr,
right: Box::new(Expr::NoneLiteral(ExprNoneLiteral {
range: TextRange::default(),
})),
});

let content = checker.generator().expr(&bin_or);
Some(Edit::range_replacement(content, literal_expr.range()))
}
89 changes: 82 additions & 7 deletions ...c/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI061_PYI061.py.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
---
source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs
snapshot_kind: text
---
PYI061.py:4:25: PYI061 [*] `Literal[None]` can be replaced with `None`
|
Expand Down Expand Up @@ -55,14 +56,24 @@ PYI061.py:12:24: PYI061 [*] `Literal[None]` can be replaced with `None`
14 14 |
15 15 |

PYI061.py:16:30: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] | None`
PYI061.py:16:30: PYI061 [*] `Literal[None, ...]` can be replaced with `Literal[...] | None`
|
16 | def func4(arg1: Literal[int, None, float]):
| ^^^^ PYI061
17 | ...
|
= help: Replace with `Literal[...] | None`

ℹ Safe fix
13 13 | ...
14 14 |
15 15 |
16 |-def func4(arg1: Literal[int, None, float]):
16 |+def func4(arg1: Literal[int, float] | None):
17 17 | ...
18 18 |
19 19 |

PYI061.py:20:25: PYI061 [*] `Literal[None]` can be replaced with `None`
|
20 | def func5(arg1: Literal[None, None]):
Expand Down Expand Up @@ -99,7 +110,7 @@ PYI061.py:20:31: PYI061 [*] `Literal[None]` can be replaced with `None`
22 22 |
23 23 |

PYI061.py:26:5: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] | None`
PYI061.py:26:5: PYI061 [*] `Literal[None, ...]` can be replaced with `Literal[...] | None`
|
24 | def func6(arg1: Literal[
25 | "hello",
Expand All @@ -110,6 +121,20 @@ PYI061.py:26:5: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] |
|
= help: Replace with `Literal[...] | None`

ℹ Unsafe fix
21 21 | ...
22 22 |
23 23 |
24 |-def func6(arg1: Literal[
25 |- "hello",
26 |- None # Comment 1
27 |- , "world"
28 |- ]):
24 |+def func6(arg1: Literal["hello", "world"] | None):
29 25 | ...
30 26 |
31 27 |

PYI061.py:33:5: PYI061 [*] `Literal[None]` can be replaced with `None`
|
32 | def func7(arg1: Literal[
Expand Down Expand Up @@ -177,7 +202,7 @@ PYI061.py:52:9: PYI061 [*] `Literal[None]` can be replaced with `None`
54 54 |
55 55 | ###

PYI061.py:53:15: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] | None`
PYI061.py:53:15: PYI061 [*] `Literal[None, ...]` can be replaced with `Literal[...] | None`
|
51 | # From flake8-pyi
52 | Literal[None] # Y061 None inside "Literal[]" expression. Replace with "None"
Expand All @@ -188,6 +213,16 @@ PYI061.py:53:15: PYI061 `Literal[None, ...]` can be replaced with `Literal[...]
|
= help: Replace with `Literal[...] | None`

ℹ Safe fix
50 50 |
51 51 | # From flake8-pyi
52 52 | Literal[None] # Y061 None inside "Literal[]" expression. Replace with "None"
53 |-Literal[True, None] # Y061 None inside "Literal[]" expression. Replace with "Literal[True] | None"
53 |+Literal[True] | None # Y061 None inside "Literal[]" expression. Replace with "Literal[True] | None"
54 54 |
55 55 | ###
56 56 | # The following rules here are slightly subtle,

PYI061.py:62:9: PYI061 [*] `Literal[None]` can be replaced with `None`
|
60 | # If Y061 and Y062 both apply, but all the duplicate members are None,
Expand Down Expand Up @@ -228,7 +263,7 @@ PYI061.py:62:15: PYI061 [*] `Literal[None]` can be replaced with `None`
64 64 |
65 65 | # ... but if Y061 and Y062 both apply

PYI061.py:63:12: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] | None`
PYI061.py:63:12: PYI061 [*] `Literal[None, ...]` can be replaced with `Literal[...] | None`
|
61 | # only emit Y061...
62 | Literal[None, None] # Y061 None inside "Literal[]" expression. Replace with "None"
Expand All @@ -239,7 +274,17 @@ PYI061.py:63:12: PYI061 `Literal[None, ...]` can be replaced with `Literal[...]
|
= help: Replace with `Literal[...] | None`

PYI061.py:63:25: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] | None`
ℹ Safe fix
60 60 | # If Y061 and Y062 both apply, but all the duplicate members are None,
61 61 | # only emit Y061...
62 62 | Literal[None, None] # Y061 None inside "Literal[]" expression. Replace with "None"
63 |-Literal[1, None, "foo", None] # Y061 None inside "Literal[]" expression. Replace with "Literal[1, 'foo'] | None"
63 |+Literal[1, "foo"] | None # Y061 None inside "Literal[]" expression. Replace with "Literal[1, 'foo'] | None"
64 64 |
65 65 | # ... but if Y061 and Y062 both apply
66 66 | # and there are no None members in the Literal[] slice,

PYI061.py:63:25: PYI061 [*] `Literal[None, ...]` can be replaced with `Literal[...] | None`
|
61 | # only emit Y061...
62 | Literal[None, None] # Y061 None inside "Literal[]" expression. Replace with "None"
Expand All @@ -250,7 +295,17 @@ PYI061.py:63:25: PYI061 `Literal[None, ...]` can be replaced with `Literal[...]
|
= help: Replace with `Literal[...] | None`

PYI061.py:68:9: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] | None`
ℹ Safe fix
60 60 | # If Y061 and Y062 both apply, but all the duplicate members are None,
61 61 | # only emit Y061...
62 62 | Literal[None, None] # Y061 None inside "Literal[]" expression. Replace with "None"
63 |-Literal[1, None, "foo", None] # Y061 None inside "Literal[]" expression. Replace with "Literal[1, 'foo'] | None"
63 |+Literal[1, "foo"] | None # Y061 None inside "Literal[]" expression. Replace with "Literal[1, 'foo'] | None"
64 64 |
65 65 | # ... but if Y061 and Y062 both apply
66 66 | # and there are no None members in the Literal[] slice,

PYI061.py:68:9: PYI061 [*] `Literal[None, ...]` can be replaced with `Literal[...] | None`
|
66 | # and there are no None members in the Literal[] slice,
67 | # only emit Y062:
Expand All @@ -259,7 +314,17 @@ PYI061.py:68:9: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] |
|
= help: Replace with `Literal[...] | None`

PYI061.py:68:21: PYI061 `Literal[None, ...]` can be replaced with `Literal[...] | None`
ℹ Safe fix
65 65 | # ... but if Y061 and Y062 both apply
66 66 | # and there are no None members in the Literal[] slice,
67 67 | # only emit Y062:
68 |-Literal[None, True, None, True] # Y062 Duplicate "Literal[]" member "True"
68 |+Literal[True, True] | None # Y062 Duplicate "Literal[]" member "True"
69 69 |
70 70 |
71 71 | # Regression tests for https://github.com/astral-sh/ruff/issues/14567

PYI061.py:68:21: PYI061 [*] `Literal[None, ...]` can be replaced with `Literal[...] | None`
|
66 | # and there are no None members in the Literal[] slice,
67 | # only emit Y062:
Expand All @@ -268,6 +333,16 @@ PYI061.py:68:21: PYI061 `Literal[None, ...]` can be replaced with `Literal[...]
|
= help: Replace with `Literal[...] | None`

ℹ Safe fix
65 65 | # ... but if Y061 and Y062 both apply
66 66 | # and there are no None members in the Literal[] slice,
67 67 | # only emit Y062:
68 |-Literal[None, True, None, True] # Y062 Duplicate "Literal[]" member "True"
68 |+Literal[True, True] | None # Y062 Duplicate "Literal[]" member "True"
69 69 |
70 70 |
71 71 | # Regression tests for https://github.com/astral-sh/ruff/issues/14567

PYI061.py:72:12: PYI061 `Literal[None]` can be replaced with `None`
|
71 | # Regression tests for https://github.com/astral-sh/ruff/issues/14567
Expand Down
Loading
Loading