[refurb] Implement isinstance-type-none (FURB168) (#8308)

## Summary

Implement
[`no-isinstance-type-none`](https://github.com/dosisod/refurb/blob/master/refurb/checks/builtin/no_isinstance_type_none.py)
as `isinstance-type-none` (`FURB168`).

Auto-fixes calls to `isinstance` to check if an object is `None` to a
`None` identity check. For example,

```python
isinstance(foo, type(None))
```

becomes

```python
foo is None
```

Related to #1348.

## Test Plan

`cargo test`

---------

Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
This commit is contained in:
Tom Kuson 2023-10-28 23:37:02 +01:00 committed by GitHub
parent a151e50ad3
commit 10a50bf1e2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 368 additions and 0 deletions

View file

@ -911,6 +911,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
if checker.enabled(Rule::ExceptionWithoutExcInfo) {
flake8_logging::rules::exception_without_exc_info(checker, call);
}
if checker.enabled(Rule::IsinstanceTypeNone) {
refurb::rules::isinstance_type_none(checker, call);
}
if checker.enabled(Rule::ImplicitCwd) {
refurb::rules::no_implicit_cwd(checker, call);
}

View file

@ -936,6 +936,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Refurb, "140") => (RuleGroup::Preview, rules::refurb::rules::ReimplementedStarmap),
(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, "171") => (RuleGroup::Preview, rules::refurb::rules::SingleItemMembershipTest),
(Refurb, "177") => (RuleGroup::Preview, rules::refurb::rules::ImplicitCwd),

View file

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

View file

@ -0,0 +1,144 @@
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 crate::checkers::ast::Checker;
use crate::fix::edits::pad;
/// ## What it does
/// Checks for uses of `isinstance` that check if an object is of type `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
/// isinstance(obj, 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 IsinstanceTypeNone;
impl Violation for IsinstanceTypeNone {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
format!("Prefer `is` operator over `isinstance` to check if an object is `None`")
}
fn fix_title(&self) -> Option<String> {
Some("Replace with `is` operator".to_string())
}
}
/// FURB168
pub(crate) fn isinstance_type_none(checker: &mut Checker, call: &ast::ExprCall) {
let Expr::Name(ast::ExprName { id, .. }) = call.func.as_ref() else {
return;
};
if id.as_str() != "isinstance" {
return;
}
if !checker.semantic().is_builtin(id) {
return;
}
let Some(types) = call.arguments.find_positional(1) else {
return;
};
if is_none(types) {
let Some(Expr::Name(ast::ExprName {
id: object_name, ..
})) = call.arguments.find_positional(0)
else {
return;
};
let mut diagnostic = Diagnostic::new(IsinstanceTypeNone, call.range());
let replacement = generate_replacement(object_name, checker.generator());
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
pad(replacement, call.range(), checker.locator()),
call.range(),
)));
checker.diagnostics.push(diagnostic);
}
}
/// Returns `true` if the given expression is equivalent to checking if the
/// object type is `None` when used with the `isinstance` builtin.
fn is_none(expr: &Expr) -> bool {
fn inner(expr: &Expr, in_union_context: bool) -> bool {
match expr {
// Ex) `None`
// Note: `isinstance` only accepts `None` as a type when used with
// the union operator, so we need to check if we're in a union.
Expr::Constant(ast::ExprConstant { value, .. }) if in_union_context => value.is_none(),
// Ex) `type(None)`
Expr::Call(ast::ExprCall {
func, arguments, ..
}) if arguments.len() == 1 => {
if let Expr::Name(ast::ExprName { id, .. }) = func.as_ref() {
if id.as_str() == "type" {
if let Some(Expr::Constant(ast::ExprConstant { value, .. })) =
arguments.args.get(0)
{
return value.is_none();
}
}
}
false
}
// Ex) `(type(None),)`
Expr::Tuple(ast::ExprTuple { elts, .. }) => elts.iter().all(|elt| inner(elt, false)),
// Ex) `type(None) | type(None)`
Expr::BinOp(ast::ExprBinOp {
left,
op: Operator::BitOr,
right,
..
}) => inner(left, true) && inner(right, true),
// Otherwise, return false.
_ => false,
}
}
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::Constant(ast::ExprConstant {
value: ast::Constant::None,
range: TextRange::default(),
})],
range: TextRange::default(),
};
generator.expr(&compare.into())
}

View file

@ -1,6 +1,7 @@
pub(crate) use check_and_remove_from_set::*;
pub(crate) use delete_full_slice::*;
pub(crate) use implicit_cwd::*;
pub(crate) use isinstance_type_none::*;
pub(crate) use print_empty_string::*;
pub(crate) use read_whole_file::*;
pub(crate) use reimplemented_starmap::*;
@ -12,6 +13,7 @@ pub(crate) use unnecessary_enumerate::*;
mod check_and_remove_from_set;
mod delete_full_slice;
mod implicit_cwd;
mod isinstance_type_none;
mod print_empty_string;
mod read_whole_file;
mod reimplemented_starmap;

View file

@ -0,0 +1,163 @@
---
source: crates/ruff_linter/src/rules/refurb/mod.rs
---
FURB168.py:5:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
3 | # Errors.
4 |
5 | if isinstance(foo, type(None)):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
6 | pass
|
= help: Replace with `is` operator
Fix
2 2 |
3 3 | # Errors.
4 4 |
5 |-if isinstance(foo, type(None)):
5 |+if foo is None:
6 6 | pass
7 7 |
8 8 | if isinstance(foo, (type(None))):
FURB168.py:8:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
6 | pass
7 |
8 | if isinstance(foo, (type(None))):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
9 | pass
|
= help: Replace with `is` operator
Fix
5 5 | if isinstance(foo, type(None)):
6 6 | pass
7 7 |
8 |-if isinstance(foo, (type(None))):
8 |+if foo is None:
9 9 | pass
10 10 |
11 11 | if isinstance(foo, (type(None), type(None), type(None))):
FURB168.py:11:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
9 | pass
10 |
11 | if isinstance(foo, (type(None), type(None), type(None))):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
12 | pass
|
= help: Replace with `is` operator
Fix
8 8 | if isinstance(foo, (type(None))):
9 9 | pass
10 10 |
11 |-if isinstance(foo, (type(None), type(None), type(None))):
11 |+if foo is None:
12 12 | pass
13 13 |
14 14 | if isinstance(foo, None | None):
FURB168.py:14:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
12 | pass
13 |
14 | if isinstance(foo, None | None):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
15 | pass
|
= help: Replace with `is` operator
Fix
11 11 | if isinstance(foo, (type(None), type(None), type(None))):
12 12 | pass
13 13 |
14 |-if isinstance(foo, None | None):
14 |+if foo is None:
15 15 | pass
16 16 |
17 17 | if isinstance(foo, (None | None)):
FURB168.py:17:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
15 | pass
16 |
17 | if isinstance(foo, (None | None)):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
18 | pass
|
= help: Replace with `is` operator
Fix
14 14 | if isinstance(foo, None | None):
15 15 | pass
16 16 |
17 |-if isinstance(foo, (None | None)):
17 |+if foo is None:
18 18 | pass
19 19 |
20 20 | if isinstance(foo, None | type(None)):
FURB168.py:20:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
18 | pass
19 |
20 | if isinstance(foo, None | type(None)):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
21 | pass
|
= help: Replace with `is` operator
Fix
17 17 | if isinstance(foo, (None | None)):
18 18 | pass
19 19 |
20 |-if isinstance(foo, None | type(None)):
20 |+if foo is None:
21 21 | pass
22 22 |
23 23 | if isinstance(foo, (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
24 | pass
|
= help: Replace with `is` operator
Fix
20 20 | if isinstance(foo, None | type(None)):
21 21 | pass
22 22 |
23 |-if isinstance(foo, (None | type(None))):
23 |+if foo is None:
24 24 | pass
25 25 |
26 26 | # A bit contrived, but is both technically valid and equivalent to the above.
FURB168.py:27:4: FURB168 [*] Prefer `is` operator over `isinstance` to check if an object is `None`
|
26 | # A bit contrived, but is both technically valid and equivalent to the above.
27 | if isinstance(foo, (type(None) | ((((type(None))))) | ((None | type(None))))):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FURB168
28 | pass
|
= help: Replace with `is` operator
Fix
24 24 | pass
25 25 |
26 26 | # A bit contrived, but is both technically valid and equivalent to the above.
27 |-if isinstance(foo, (type(None) | ((((type(None))))) | ((None | type(None))))):
27 |+if foo is None:
28 28 | pass
29 29 |
30 30 |