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

[refurb] Avoid None | None as well as better detection and fix (FURB168) #15779

Merged
merged 4 commits into from
Jan 31, 2025
Merged
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Per review
InSyncWithFoo committed Jan 28, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit 7dff9a74e132397417e8a16380e2bc5689702439
20 changes: 19 additions & 1 deletion crates/ruff_linter/resources/test/fixtures/refurb/FURB168.py
Original file line number Diff line number Diff line change
@@ -20,7 +20,7 @@
if isinstance(foo, None | type(None)):
pass

if isinstance(foo, (None | type(None))):
if isinstance(foo, type(None) | type(None)):
pass

# A bit contrived, but is both technically valid and equivalent to the above.
@@ -33,6 +33,17 @@
):
...

from typing import Union

if isinstance(foo, Union[None]):
...

if isinstance(foo, Union[None, None]):
...

if isinstance(foo, Union[None, type(None)]):
...


# Okay.

@@ -47,6 +58,13 @@

if isinstance(foo, (int, type(None), str)):
pass
pass

if isinstance(foo, str | None):
pass

if isinstance(foo, Union[None, str]):
...

# This is a TypeError, which the rule ignores.
if isinstance(foo, None):
Original file line number Diff line number Diff line change
@@ -90,6 +90,10 @@ fn is_none(expr: &Expr, semantic: &SemanticModel) -> bool {
return false;
}

if !arguments.keywords.is_empty() {
return false;
}

matches!(arguments.args.as_ref(), [Expr::NoneLiteral(_)])
}

@@ -111,14 +115,28 @@ fn is_none(expr: &Expr, semantic: &SemanticModel) -> bool {
inner(left, true, semantic) && inner(right, true, semantic)
}

// Ex) `Union[None, ...]`
Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
if !semantic.match_typing_expr(value, "Union") {
return false;
}

match slice.as_ref() {
Expr::Tuple(ast::ExprTuple { elts, .. }) => {
elts.iter().all(|element| inner(element, true, semantic))
}
slice => inner(slice, true, semantic),
}
}

// Otherwise, return false.
_ => false,
}
}
inner(expr, false, semantic)
}

fn replace_with_identity_check(expr: &Expr, range: TextRange, checker: &mut Checker) -> Fix {
fn replace_with_identity_check(expr: &Expr, range: TextRange, checker: &Checker) -> Fix {
let (semantic, generator) = (checker.semantic(), checker.generator());

let new_expr = Expr::Compare(ast::ExprCompare {
Original file line number Diff line number Diff line change
@@ -119,14 +119,14 @@ FURB168.py:20:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
20 |+if foo is None:
21 21 | pass
22 22 |
23 23 | if isinstance(foo, (None | type(None))):
23 23 | if isinstance(foo, type(None) | type(None)):

FURB168.py:23:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
21 | pass
22 |
23 | if isinstance(foo, (None | type(None))):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
23 | if isinstance(foo, type(None) | type(None)):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
24 | pass
|
= help: Replace with `is` operator
@@ -135,7 +135,7 @@ FURB168.py:23:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
20 20 | if isinstance(foo, None | type(None)):
21 21 | pass
22 22 |
23 |-if isinstance(foo, (None | type(None))):
23 |-if isinstance(foo, type(None) | type(None)):
23 |+if foo is None:
24 24 | pass
25 25 |
@@ -159,3 +159,63 @@ FURB168.py:27:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if
28 28 | pass
29 29 |
30 30 | if isinstance(

FURB168.py:38:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
36 | from typing import Union
37 |
38 | if isinstance(foo, Union[None]):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
39 | ...
|
= help: Replace with `is` operator

ℹ Safe fix
35 35 |
36 36 | from typing import Union
37 37 |
38 |-if isinstance(foo, Union[None]):
38 |+if foo is None:
39 39 | ...
40 40 |
41 41 | if isinstance(foo, Union[None, None]):

FURB168.py:41:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
39 | ...
40 |
41 | if isinstance(foo, Union[None, None]):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
42 | ...
|
= help: Replace with `is` operator

ℹ Safe fix
38 38 | if isinstance(foo, Union[None]):
39 39 | ...
40 40 |
41 |-if isinstance(foo, Union[None, None]):
41 |+if foo is None:
42 42 | ...
43 43 |
44 44 | if isinstance(foo, Union[None, type(None)]):

FURB168.py:44:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
42 | ...
43 |
44 | if isinstance(foo, Union[None, type(None)]):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
45 | ...
|
= help: Replace with `is` operator

ℹ Safe fix
41 41 | if isinstance(foo, Union[None, None]):
42 42 | ...
43 43 |
44 |-if isinstance(foo, Union[None, type(None)]):
44 |+if foo is None:
45 45 | ...
46 46 |
47 47 |