From 7b4fb4fb5dfe53866239c29ba770218aa0c90fcf Mon Sep 17 00:00:00 2001 From: Jelle Zijlstra Date: Tue, 3 Oct 2023 21:42:13 -0700 Subject: [PATCH] Fix issues with SIM101 (adjacent isinstance() calls) (#7798) - 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 --- .../test/fixtures/flake8_simplify/SIM101.py | 9 ++ .../flake8_simplify/rules/ast_bool_op.rs | 109 ++++++++++-------- ...ke8_simplify__tests__SIM101_SIM101.py.snap | 62 ++++++---- 3 files changed, 112 insertions(+), 68 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM101.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM101.py index 1d9066f5ce896..12e26c4c76c7d 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM101.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM101.py @@ -31,6 +31,15 @@ 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 diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs index eb41bcad7f2bb..9a798ca1ab532 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/ast_bool_op.rs @@ -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> = 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::new(); + let mut last_target_option: Option = 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(); diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM101_SIM101.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM101_SIM101.py.snap index 1368303ffa0b8..11dda2b013cce 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM101_SIM101.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM101_SIM101.py.snap @@ -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(): +