Cyclic0007/ruff
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix issues with SIM101 (adjacent isinstance() calls) (#7798)

Browse Source 
- Only trigger for immediately adjacent isinstance() calls with the same
target
- Preserve order of or conditions

Two existing tests changed:
- One was incorrectly reordering the or conditions, and is now correct.
- Another was combining two non-adjacent isinstance() calls. It's safe
enough in that example,
but this isn't safe to do in general, and it feels low-value to come up
with a heuristic for
when it is safe, so it seems better to not combine the calls in that
case.

Fixes https://github.com/astral-sh/ruff/issues/7797
This commit is contained in:
Jelle Zijlstra
2023-10-03 21:42:13 -07:00
committed by GitHub
parent 5d49d268a0
commit 7b4fb4fb5d
3 changed files with 112 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM101.py vendored
View File

@@ -31,6 +31,15 @@ if isinstance(a, bool) or isinstance(b, str):
if isinstance(a, int) or isinstance(a.b, float):
pass
# OK
if isinstance(a, int) or unrelated_condition or isinstance(a, float):
pass
if x or isinstance(a, int) or isinstance(a, float):
pass
if x or y or isinstance(a, int) or isinstance(a, float) or z:
pass
def f():
# OK

109
crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs
View File

@@ -5,7 +5,6 @@ use itertools::Either::{Left, Right};
use itertools::Itertools;
use ruff_python_ast::{self as ast, Arguments, BoolOp, CmpOp, Expr, ExprContext, UnaryOp};
use ruff_text_size::{Ranged, TextRange};
use rustc_hash::FxHashMap;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
@@ -13,6 +12,7 @@ use ruff_python_ast::comparable::ComparableExpr;
use ruff_python_ast::helpers::{contains_effect, Truthiness};
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_python_codegen::Generator;
use ruff_python_semantic::SemanticModel;
use crate::checkers::ast::Checker;
use crate::registry::AsRule;
@@ -299,6 +299,42 @@ fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> {
None
}
/// If `call` is an `isinstance()` call, return its target.
fn isinstance_target<'a>(call: &'a Expr, semantic: &'a SemanticModel) -> Option<&'a Expr> {
// Verify that this is an `isinstance` call.
let Expr::Call(ast::ExprCall {
func,
arguments:
Arguments {
args,
keywords,
range: _,
},
range: _,
}) = &call
else {
return None;
};
if args.len() != 2 {
return None;
}
if !keywords.is_empty() {
return None;
}
let Expr::Name(ast::ExprName { id: func_name, .. }) = func.as_ref() else {
return None;
};
if func_name != "isinstance" {
return None;
}
if !semantic.is_builtin("isinstance") {
return None;
}
// Collect the target (e.g., `obj` in `isinstance(obj, int)`).
Some(&args[0])
}
/// SIM101
pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
let Expr::BoolOp(ast::ExprBoolOp {
@@ -310,50 +346,32 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
return;
};
// Locate duplicate `isinstance` calls, represented as a map from `ComparableExpr`
// to indices of the relevant `Expr` instances in `values`.
let mut duplicates: FxHashMap<ComparableExpr, Vec<usize>> = FxHashMap::default();
// Locate duplicate `isinstance` calls, represented as a vector of vectors
// of indices of the relevant `Expr` instances in `values`.
let mut duplicates: Vec<Vec<usize>> = Vec::new();
let mut last_target_option: Option<ComparableExpr> = None;
for (index, call) in values.iter().enumerate() {
// Verify that this is an `isinstance` call.
let Expr::Call(ast::ExprCall {
func,
arguments:
Arguments {
args,
keywords,
range: _,
},
range: _,
}) = &call
else {
let Some(target) = isinstance_target(call, checker.semantic()) else {
last_target_option = None;
continue;
};
if args.len() != 2 {
continue;
}
if !keywords.is_empty() {
continue;
}
let Expr::Name(ast::ExprName { id: func_name, .. }) = func.as_ref() else {
continue;
};
if func_name != "isinstance" {
continue;
}
if !checker.semantic().is_builtin("isinstance") {
continue;
}
// Collect the target (e.g., `obj` in `isinstance(obj, int)`).
let target = &args[0];
duplicates
.entry(target.into())
.or_insert_with(Vec::new)
.push(index);
if last_target_option
.as_ref()
.is_some_and(|last_target| *last_target == ComparableExpr::from(target))
{
duplicates
.last_mut()
.expect("last_target should have a corresponding entry")
.push(index);
} else {
last_target_option = Some(target.into());
duplicates.push(vec![index]);
}
}
// Generate a `Diagnostic` for each duplicate.
for indices in duplicates.values() {
for indices in duplicates {
if indices.len() > 1 {
// Grab the target used in each duplicate `isinstance` call (e.g., `obj` in
// `isinstance(obj, int)`).
@@ -429,17 +447,14 @@ pub(crate) fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) {
let call = node2.into();
// Generate the combined `BoolOp`.
let [first, .., last] = indices.as_slice() else {
unreachable!("Indices should have at least two elements")
};
let before = values.iter().take(*first).cloned();
let after = values.iter().skip(last + 1).cloned();
let node = ast::ExprBoolOp {
op: BoolOp::Or,
values: iter::once(call)
.chain(
values
.iter()
.enumerate()
.filter(|(index, _)| !indices.contains(index))
.map(|(_, elt)| elt.clone()),
)
.collect(),
values: before.chain(iter::once(call)).chain(after).collect(),
range: TextRange::default(),
};
let bool_op = node.into();

62
crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM101_SIM101.py.snap
View File

@@ -71,31 +71,11 @@ SIM101.py:10:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a sin
8 8 | pass
9 9 |
10 |-if isinstance(b, bool) or isinstance(a, int) or isinstance(a, float): # SIM101
10 |+if isinstance(a, (int, float)) or isinstance(b, bool): # SIM101
10 |+if isinstance(b, bool) or isinstance(a, (int, float)): # SIM101
11 11 | pass
12 12 |
13 13 | if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float): # SIM101
SIM101.py:13:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
|
11 | pass
12 |
13 | if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float): # SIM101
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM101
14 | pass
|
= help: Merge `isinstance` calls for `a`
ℹ Suggested fix
10 10 | if isinstance(b, bool) or isinstance(a, int) or isinstance(a, float): # SIM101
11 11 | pass
12 12 |
13 |-if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float): # SIM101
13 |+if isinstance(a, (int, float)) or isinstance(b, bool): # SIM101
14 14 | pass
15 15 |
16 16 | if (isinstance(a, int) or isinstance(a, float)) and isinstance(b, bool): # SIM101
SIM101.py:16:5: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
|
14 | pass
@@ -146,4 +126,44 @@ SIM101.py:22:4: SIM101 Multiple `isinstance` calls for expression, merge into a
|
= help: Merge `isinstance` calls
SIM101.py:38:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
|
36 | pass
37 |
38 | if x or isinstance(a, int) or isinstance(a, float):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM101
39 | pass
|
= help: Merge `isinstance` calls for `a`
ℹ Suggested fix
35 35 | if isinstance(a, int) or unrelated_condition or isinstance(a, float):
36 36 | pass
37 37 |
38 |-if x or isinstance(a, int) or isinstance(a, float):
38 |+if x or isinstance(a, (int, float)):
39 39 | pass
40 40 |
41 41 | if x or y or isinstance(a, int) or isinstance(a, float) or z:
SIM101.py:41:4: SIM101 [*] Multiple `isinstance` calls for `a`, merge into a single call
|
39 | pass
40 |
41 | if x or y or isinstance(a, int) or isinstance(a, float) or z:
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ SIM101
42 | pass
|
= help: Merge `isinstance` calls for `a`
ℹ Suggested fix
38 38 | if x or isinstance(a, int) or isinstance(a, float):
39 39 | pass
40 40 |
41 |-if x or y or isinstance(a, int) or isinstance(a, float) or z:
41 |+if x or y or isinstance(a, (int, float)) or z:
42 42 | pass
43 43 |
44 44 | def f():