Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[`refurb`] Implement `type-none-comparison` (`FURB169`) (#8487) 

## Summary

Implement
[`no-is-type-none`](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/no_is_type_none.py)
as `type-none-comparison` (`FURB169`).

Auto-fixes comparisons that use `type` to compare the type of an object
to `type(None)` to a `None` identity check. For example,

```python
type(foo) is type(None)
```

becomes

```python
foo is None
```

Related to #1348.

## Test Plan

`cargo test`
This commit is contained in:
Tom Kuson 2023-11-06 00:56:20 +00:00 committed by GitHub
parent bcb737dd80
commit de2d7e97b1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 515 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
crates/ruff_linter/resources/test/fixtures/refurb/FURB169.py vendored Normal file
View File

@ -0,0 +1,67 @@
foo = None
# Error.
type(foo) is type(None)
type(None) is type(foo)
type(None) is type(None)
type(foo) is not type(None)
type(None) is not type(foo)
type(None) is not type(None)
type(foo) == type(None)
type(None) == type(foo)
type(None) == type(None)
type(foo) != type(None)
type(None) != type(foo)
type(None) != type(None)
# Ok.
foo is None
foo is not None
None is foo
None is not foo
None is None
None is not None
foo is type(None)
type(foo) is None
type(None) is None
foo is not type(None)
type(foo) is not None
type(None) is not None
foo == type(None)
type(foo) == None
type(None) == None
foo != type(None)
type(foo) != None
type(None) != None
type(foo) > type(None)

3
crates/ruff_linter/src/checkers/ast/analyze/expression.rs
View File

@ -1238,6 +1238,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
comparators,
);
}
if checker.enabled(Rule::TypeNoneComparison) {
refurb::rules::type_none_comparison(checker, compare);
}
if checker.enabled(Rule::SingleItemMembershipTest) {
refurb::rules::single_item_membership_test(checker, expr, left, ops, comparators);
}

1
crates/ruff_linter/src/codes.rs
View File

@ -945,6 +945,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Refurb, "145") => (RuleGroup::Preview, rules::refurb::rules::SliceCopy),
(Refurb, "148") => (RuleGroup::Preview, rules::refurb::rules::UnnecessaryEnumerate),
(Refurb, "168") => (RuleGroup::Preview, rules::refurb::rules::IsinstanceTypeNone),
(Refurb, "169") => (RuleGroup::Preview, rules::refurb::rules::TypeNoneComparison),
(Refurb, "171") => (RuleGroup::Preview, rules::refurb::rules::SingleItemMembershipTest),
(Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd),

27
crates/ruff_linter/src/rules/refurb/helpers.rs
View File

@ -34,3 +34,30 @@ pub(super) fn generate_method_call(name: &str, method: &str, generator: Generato
};
generator.stmt(&stmt.into())
}
/// Format a code snippet comparing `name` to `None` (e.g., `name is None`).
pub(super) fn generate_none_identity_comparison(
name: &str,
negate: bool,
generator: Generator,
) -> String {
// Construct `name`.
let var = ast::ExprName {
id: name.to_string(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
// Construct `name is None` or `name is not None`.
let op = if negate {
ast::CmpOp::IsNot
} else {
ast::CmpOp::Is
};
let compare = ast::ExprCompare {
left: Box::new(var.into()),
ops: vec![op],
comparators: vec![ast::Expr::NoneLiteral(ast::ExprNoneLiteral::default())],
range: TextRange::default(),
};
generator.expr(&compare.into())
}

1
crates/ruff_linter/src/rules/refurb/mod.rs
View File

@ -25,6 +25,7 @@ mod tests {
#[test_case(Rule::ImplicitCwd, Path::new("FURB177.py"))]
#[test_case(Rule::SingleItemMembershipTest, Path::new("FURB171.py"))]
#[test_case(Rule::IsinstanceTypeNone, Path::new("FURB168.py"))]
#[test_case(Rule::TypeNoneComparison, Path::new("FURB169.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(

25
crates/ruff_linter/src/rules/refurb/rules/isinstance_type_none.rs
View File

@ -2,11 +2,11 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_python_ast::{self as ast, Expr, Operator};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_codegen::Generator;
use ruff_text_size::{Ranged, TextRange};
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::fix::edits::pad;
use crate::rules::refurb::helpers::generate_none_identity_comparison;
/// ## What it does
/// Checks for uses of `isinstance` that check if an object is of type `None`.
@ -69,7 +69,8 @@ pub(crate) fn isinstance_type_none(checker: &mut Checker, call: &ast::ExprCall)
return;
};
let mut diagnostic = Diagnostic::new(IsinstanceTypeNone, call.range());
let replacement = generate_replacement(object_name, checker.generator());
let replacement =
generate_none_identity_comparison(object_name, false, checker.generator());
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
pad(replacement, call.range(), checker.locator()),
call.range(),
@ -117,21 +118,3 @@ fn is_none(expr: &Expr) -> bool {
}
inner(expr, false)
}
/// Format a code snippet comparing `name` to `None` (e.g., `name is None`).
fn generate_replacement(name: &str, generator: Generator) -> String {
// Construct `name`.
let var = ast::ExprName {
id: name.to_string(),
ctx: ast::ExprContext::Load,
range: TextRange::default(),
};
// Construct `name is None`.
let compare = ast::ExprCompare {
left: Box::new(var.into()),
ops: vec![ast::CmpOp::Is],
comparators: vec![ast::Expr::NoneLiteral(ast::ExprNoneLiteral::default())],
range: TextRange::default(),
};
generator.expr(&compare.into())
}

2
crates/ruff_linter/src/rules/refurb/rules/mod.rs
View File

@ -8,6 +8,7 @@ pub(crate) use reimplemented_starmap::*;
pub(crate) use repeated_append::*;
pub(crate) use single_item_membership_test::*;
pub(crate) use slice_copy::*;
pub(crate) use type_none_comparison::*;
pub(crate) use unnecessary_enumerate::*;
mod check_and_remove_from_set;
@ -20,4 +21,5 @@ mod reimplemented_starmap;
mod repeated_append;
mod single_item_membership_test;
mod slice_copy;
mod type_none_comparison;
mod unnecessary_enumerate;

153
crates/ruff_linter/src/rules/refurb/rules/type_none_comparison.rs Normal file
View File

@ -0,0 +1,153 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::{self as ast, CmpOp, Expr};
use ruff_python_semantic::SemanticModel;
use ruff_text_size::Ranged;
use crate::checkers::ast::Checker;
use crate::fix::edits::pad;
use crate::rules::refurb::helpers::generate_none_identity_comparison;
/// ## What it does
/// Checks for uses of `type` that compare the type of an object to the type of
/// `None`.
///
/// ## Why is this bad?
/// There is only ever one instance of `None`, so it is more efficient and
/// readable to use the `is` operator to check if an object is `None`.
///
/// ## Example
/// ```python
/// type(obj) is type(None)
/// ```
///
/// Use instead:
/// ```python
/// obj is None
/// ```
///
/// ## References
/// - [Python documentation: `isinstance`](https://docs.python.org/3/library/functions.html#isinstance)
/// - [Python documentation: `None`](https://docs.python.org/3/library/constants.html#None)
/// - [Python documentation: `type`](https://docs.python.org/3/library/functions.html#type)
/// - [Python documentation: Identity comparisons](https://docs.python.org/3/reference/expressions.html#is-not)
#[violation]
pub struct TypeNoneComparison {
object: String,
comparison: Comparison,
}
impl Violation for TypeNoneComparison {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
let TypeNoneComparison { object, .. } = self;
format!("Compare the identities of `{object}` and `None` instead of their respective types")
}
fn fix_title(&self) -> Option<String> {
let TypeNoneComparison { object, comparison } = self;
match comparison {
Comparison::Is | Comparison::Eq => Some(format!("Replace with `{object} is None`")),
Comparison::IsNot | Comparison::NotEq => {
Some(format!("Replace with `{object} is not None`"))
}
}
}
}
/// FURB169
pub(crate) fn type_none_comparison(checker: &mut Checker, compare: &ast::ExprCompare) {
let ([op], [right]) = (compare.ops.as_slice(), compare.comparators.as_slice()) else {
return;
};
// Ensure that the comparison is an identity or equality test.
let comparison = match op {
CmpOp::Is => Comparison::Is,
CmpOp::IsNot => Comparison::IsNot,
CmpOp::Eq => Comparison::Eq,
CmpOp::NotEq => Comparison::NotEq,
_ => return,
};
// Get the objects whose types are being compared.
let Some(left_arg) = type_call_arg(&compare.left, checker.semantic()) else {
return;
};
let Some(right_arg) = type_call_arg(right, checker.semantic()) else {
return;
};
// If one of the objects is `None`, get the other object; else, return.
let other_arg = match (
left_arg.is_none_literal_expr(),
right_arg.is_none_literal_expr(),
) {
(true, false) => right_arg,
(false, true) => left_arg,
// If both are `None`, just pick one.
(true, true) => left_arg,
_ => return,
};
// Get the name of the other object (or `None` if both were `None`).
let other_arg_name = match other_arg {
Expr::Name(ast::ExprName { id, .. }) => id.as_str(),
Expr::NoneLiteral { .. } => "None",
_ => return,
};
let mut diagnostic = Diagnostic::new(
TypeNoneComparison {
object: other_arg_name.to_string(),
comparison,
},
compare.range(),
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
pad(
match comparison {
Comparison::Is | Comparison::Eq => {
generate_none_identity_comparison(other_arg_name, false, checker.generator())
}
Comparison::IsNot | Comparison::NotEq => {
generate_none_identity_comparison(other_arg_name, true, checker.generator())
}
},
compare.range(),
checker.locator(),
),
compare.range(),
)));
checker.diagnostics.push(diagnostic);
}
/// Returns the object passed to the function, if the expression is a call to
/// `type` with a single argument.
fn type_call_arg<'a>(expr: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a Expr> {
// The expression must be a single-argument call to `type`.
let ast::ExprCall {
func, arguments, ..
} = expr.as_call_expr()?;
if arguments.len() != 1 {
return None;
}
// The function itself must be the builtin `type`.
let ast::ExprName { id, .. } = func.as_name_expr()?;
if id.as_str() != "type" || !semantic.is_builtin(id) {
return None;
}
arguments.find_positional(0)
}
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum Comparison {
Is,
IsNot,
Eq,
NotEq,
}

256
crates/ruff_linter/src/rules/refurb/snapshots/ruff_linter__rules__refurb__tests__FURB169_FURB169.py.snap Normal file
View File

@ -0,0 +1,256 @@
---
source: crates/ruff_linter/src/rules/refurb/mod.rs
---
FURB169.py:5:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
3 | # Error.
4 |
5 | type(foo) is type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^ FURB169
6 |
7 | type(None) is type(foo)
|
= help: Replace with `foo is None`
ℹ Fix
2 2 |
3 3 | # Error.
4 4 |
5 |-type(foo) is type(None)
5 |+foo is None
6 6 |
7 7 | type(None) is type(foo)
8 8 |
FURB169.py:7:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
5 | type(foo) is type(None)
6 |
7 | type(None) is type(foo)
| ^^^^^^^^^^^^^^^^^^^^^^^ FURB169
8 |
9 | type(None) is type(None)
|
= help: Replace with `foo is None`
ℹ Fix
4 4 |
5 5 | type(foo) is type(None)
6 6 |
7 |-type(None) is type(foo)
7 |+foo is None
8 8 |
9 9 | type(None) is type(None)
10 10 |
FURB169.py:9:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types
|
7 | type(None) is type(foo)
8 |
9 | type(None) is type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^^ FURB169
10 |
11 | type(foo) is not type(None)
|
= help: Replace with `None is None`
ℹ Fix
6 6 |
7 7 | type(None) is type(foo)
8 8 |
9 |-type(None) is type(None)
9 |+None is None
10 10 |
11 11 | type(foo) is not type(None)
12 12 |
FURB169.py:11:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
9 | type(None) is type(None)
10 |
11 | type(foo) is not type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB169
12 |
13 | type(None) is not type(foo)
|
= help: Replace with `foo is not None`
ℹ Fix
8 8 |
9 9 | type(None) is type(None)
10 10 |
11 |-type(foo) is not type(None)
11 |+foo is not None
12 12 |
13 13 | type(None) is not type(foo)
14 14 |
FURB169.py:13:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
11 | type(foo) is not type(None)
12 |
13 | type(None) is not type(foo)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB169
14 |
15 | type(None) is not type(None)
|
= help: Replace with `foo is not None`
ℹ Fix
10 10 |
11 11 | type(foo) is not type(None)
12 12 |
13 |-type(None) is not type(foo)
13 |+foo is not None
14 14 |
15 15 | type(None) is not type(None)
16 16 |
FURB169.py:15:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types
|
13 | type(None) is not type(foo)
14 |
15 | type(None) is not type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB169
16 |
17 | type(foo) == type(None)
|
= help: Replace with `None is not None`
ℹ Fix
12 12 |
13 13 | type(None) is not type(foo)
14 14 |
15 |-type(None) is not type(None)
15 |+None is not None
16 16 |
17 17 | type(foo) == type(None)
18 18 |
FURB169.py:17:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
15 | type(None) is not type(None)
16 |
17 | type(foo) == type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^ FURB169
18 |
19 | type(None) == type(foo)
|
= help: Replace with `foo is None`
ℹ Fix
14 14 |
15 15 | type(None) is not type(None)
16 16 |
17 |-type(foo) == type(None)
17 |+foo is None
18 18 |
19 19 | type(None) == type(foo)
20 20 |
FURB169.py:19:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
17 | type(foo) == type(None)
18 |
19 | type(None) == type(foo)
| ^^^^^^^^^^^^^^^^^^^^^^^ FURB169
20 |
21 | type(None) == type(None)
|
= help: Replace with `foo is None`
ℹ Fix
16 16 |
17 17 | type(foo) == type(None)
18 18 |
19 |-type(None) == type(foo)
19 |+foo is None
20 20 |
21 21 | type(None) == type(None)
22 22 |
FURB169.py:21:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types
|
19 | type(None) == type(foo)
20 |
21 | type(None) == type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^^ FURB169
22 |
23 | type(foo) != type(None)
|
= help: Replace with `None is None`
ℹ Fix
18 18 |
19 19 | type(None) == type(foo)
20 20 |
21 |-type(None) == type(None)
21 |+None is None
22 22 |
23 23 | type(foo) != type(None)
24 24 |
FURB169.py:23:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
21 | type(None) == type(None)
22 |
23 | type(foo) != type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^ FURB169
24 |
25 | type(None) != type(foo)
|
= help: Replace with `foo is not None`
ℹ Fix
20 20 |
21 21 | type(None) == type(None)
22 22 |
23 |-type(foo) != type(None)
23 |+foo is not None
24 24 |
25 25 | type(None) != type(foo)
26 26 |
FURB169.py:25:1: FURB169 [*] Compare the identities of `foo` and `None` instead of their respective types
|
23 | type(foo) != type(None)
24 |
25 | type(None) != type(foo)
| ^^^^^^^^^^^^^^^^^^^^^^^ FURB169
26 |
27 | type(None) != type(None)
|
= help: Replace with `foo is not None`
ℹ Fix
22 22 |
23 23 | type(foo) != type(None)
24 24 |
25 |-type(None) != type(foo)
25 |+foo is not None
26 26 |
27 27 | type(None) != type(None)
28 28 |
FURB169.py:27:1: FURB169 [*] Compare the identities of `None` and `None` instead of their respective types
|
25 | type(None) != type(foo)
26 |
27 | type(None) != type(None)
| ^^^^^^^^^^^^^^^^^^^^^^^^ FURB169
28 |
29 | # Ok.
|
= help: Replace with `None is not None`
ℹ Fix
24 24 |
25 25 | type(None) != type(foo)
26 26 |
27 |-type(None) != type(None)
27 |+None is not None
28 28 |
29 29 | # Ok.
30 30 |

1
ruff.schema.json generated
View File

@ -2822,6 +2822,7 @@
"FURB148",
"FURB16",
"FURB168",
"FURB169",
"FURB17",
"FURB171",
"FURB177",