From 75bb6ad4567f8aca52d64a5240219e9458850a59 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 5 Jan 2023 20:21:40 -0500 Subject: [PATCH] Implement duplicate isinstance detection (SIM101) (#1673) See: #998. --- README.md | 1 + .../test/fixtures/flake8_simplify/SIM101.py | 23 +++ ruff.schema.json | 1 + src/checkers/ast.rs | 3 + src/flake8_simplify/mod.rs | 1 + src/flake8_simplify/plugins/ast_bool_op.rs | 136 +++++++++++++++++- src/flake8_simplify/plugins/mod.rs | 2 +- ...ke8_simplify__tests__SIM101_SIM101.py.snap | 107 ++++++++++++++ src/registry.rs | 12 ++ 9 files changed, 284 insertions(+), 2 deletions(-) create mode 100644 resources/test/fixtures/flake8_simplify/SIM101.py create mode 100644 src/flake8_simplify/snapshots/ruff__flake8_simplify__tests__SIM101_SIM101.py.snap diff --git a/README.md b/README.md index d44b12c82df3c..485d1eba43df7 100644 --- a/README.md +++ b/README.md @@ -967,6 +967,7 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/0.19.3/ | Code | Name | Message | Fix | | ---- | ---- | ------- | --- | +| SIM101 | DuplicateIsinstanceCall | Multiple `isinstance` calls for `...`, merge into a single call | 🛠 | | SIM102 | NestedIfStatements | Use a single `if` statement instead of nested `if` statements | | | SIM105 | UseContextlibSuppress | Use `contextlib.suppress(...)` instead of try-except-pass | | | SIM107 | ReturnInTryExceptFinally | Don't use `return` in `try`/`except` and `finally` | | diff --git a/resources/test/fixtures/flake8_simplify/SIM101.py b/resources/test/fixtures/flake8_simplify/SIM101.py new file mode 100644 index 0000000000000..ca3125ff2c701 --- /dev/null +++ b/resources/test/fixtures/flake8_simplify/SIM101.py @@ -0,0 +1,23 @@ +if isinstance(a, int) or isinstance(a, float): # SIM101 + pass + +if isinstance(a, (int, float)) or isinstance(a, bool): # SIM101 + pass + +if isinstance(a, int) or isinstance(a, float) or isinstance(b, bool): # SIM101 + pass + +if isinstance(b, bool) or isinstance(a, int) or isinstance(a, float): # SIM101 + pass + +if isinstance(a, int) or isinstance(b, bool) or isinstance(a, float): # SIM101 + pass + +if (isinstance(a, int) or isinstance(a, float)) and isinstance(b, bool): # SIM101 + pass + +if isinstance(a, int) and isinstance(b, bool) or isinstance(a, float): + pass + +if isinstance(a, bool) or isinstance(b, str): + pass diff --git a/ruff.schema.json b/ruff.schema.json index 261511533067b..a9fd025d58002 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -936,6 +936,7 @@ "SIM", "SIM1", "SIM10", + "SIM101", "SIM102", "SIM105", "SIM107", diff --git a/src/checkers/ast.rs b/src/checkers/ast.rs index f2860756824cc..09e43cf7c1183 100644 --- a/src/checkers/ast.rs +++ b/src/checkers/ast.rs @@ -2710,6 +2710,9 @@ where if self.settings.enabled.contains(&CheckCode::PLR1701) { pylint::plugins::merge_isinstance(self, expr, op, values); } + if self.settings.enabled.contains(&CheckCode::SIM101) { + flake8_simplify::plugins::duplicate_isinstance_call(self, expr); + } if self.settings.enabled.contains(&CheckCode::SIM220) { flake8_simplify::plugins::a_and_not_a(self, expr); } diff --git a/src/flake8_simplify/mod.rs b/src/flake8_simplify/mod.rs index 91f8bc0f831e0..f8bdef8ea0453 100644 --- a/src/flake8_simplify/mod.rs +++ b/src/flake8_simplify/mod.rs @@ -12,6 +12,7 @@ mod tests { use crate::registry::CheckCode; use crate::settings; + #[test_case(CheckCode::SIM101, Path::new("SIM101.py"); "SIM101")] #[test_case(CheckCode::SIM102, Path::new("SIM102.py"); "SIM102")] #[test_case(CheckCode::SIM105, Path::new("SIM105.py"); "SIM105")] #[test_case(CheckCode::SIM107, Path::new("SIM107.py"); "SIM107")] diff --git a/src/flake8_simplify/plugins/ast_bool_op.rs b/src/flake8_simplify/plugins/ast_bool_op.rs index b98777ca46ce4..5811079a66e74 100644 --- a/src/flake8_simplify/plugins/ast_bool_op.rs +++ b/src/flake8_simplify/plugins/ast_bool_op.rs @@ -1,10 +1,18 @@ -use rustpython_ast::{Boolop, Constant, Expr, ExprKind, Unaryop}; +use std::iter; +use itertools::Either::{Left, Right}; +use rustc_hash::FxHashMap; +use rustpython_ast::{Boolop, Constant, Expr, ExprContext, ExprKind, Unaryop}; + +use crate::ast::helpers::create_expr; use crate::ast::types::Range; use crate::autofix::Fix; use crate::checkers::ast::Checker; use crate::registry::{Check, CheckCode, CheckKind}; +use crate::source_code_generator::SourceCodeGenerator; +use crate::source_code_style::SourceCodeStyleDetector; +/// Return `true` if two `Expr` instances are equivalent names. fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> { if let (ExprKind::Name { id: a, .. }, ExprKind::Name { id: b, .. }) = (&a.node, &b.node) { if a == b { @@ -14,6 +22,132 @@ fn is_same_expr<'a>(a: &'a Expr, b: &'a Expr) -> Option<&'a str> { None } +/// Generate source code from an `Expr`. +fn to_source(expr: &Expr, stylist: &SourceCodeStyleDetector) -> String { + let mut generator = SourceCodeGenerator::new( + stylist.indentation(), + stylist.quote(), + stylist.line_ending(), + ); + generator.unparse_expr(expr, 0); + generator.generate().unwrap() +} + +/// SIM101 +pub fn duplicate_isinstance_call(checker: &mut Checker, expr: &Expr) { + let ExprKind::BoolOp { op: Boolop::Or, values } = &expr.node else { + return; + }; + + // Locate duplicate `isinstance` calls, represented as a map from argument name to indices + // of the relevant `Expr` instances in `values`. + let mut duplicates = FxHashMap::default(); + for (index, call) in values.iter().enumerate() { + // Verify that this is an `isinstance` call. + let ExprKind::Call { func, args, keywords } = &call.node else { + continue; + }; + if args.len() != 2 { + continue; + } + if !keywords.is_empty() { + continue; + } + let ExprKind::Name { id: func_name, .. } = &func.node else { + continue; + }; + if func_name != "isinstance" { + continue; + } + + // Collect the name of the argument. + let ExprKind::Name { id: arg_name, .. } = &args[0].node else { + continue; + }; + duplicates + .entry(arg_name.as_str()) + .or_insert_with(Vec::new) + .push(index); + } + + // Generate a `Check` for each duplicate. + for (arg_name, indices) in duplicates { + if indices.len() > 1 { + let mut check = Check::new( + CheckKind::DuplicateIsinstanceCall(arg_name.to_string()), + Range::from_located(expr), + ); + if checker.patch(&CheckCode::SIM101) { + // Grab the types used in each duplicate `isinstance` call. + let types: Vec<&Expr> = indices + .iter() + .map(|index| &values[*index]) + .map(|expr| { + let ExprKind::Call { args, ..} = &expr.node else { + unreachable!("Indices should only contain `isinstance` calls") + }; + args.get(1).expect("`isinstance` should have two arguments") + }) + .collect(); + + // Flatten all the types used across the `isinstance` cals. + let elts: Vec<&Expr> = types + .iter() + .flat_map(|value| { + if let ExprKind::Tuple { elts, .. } = &value.node { + Left(elts.iter()) + } else { + Right(iter::once(*value)) + } + }) + .collect(); + + // Generate a single `isinstance` call. + let call = create_expr(ExprKind::Call { + func: Box::new(create_expr(ExprKind::Name { + id: "isinstance".to_string(), + ctx: ExprContext::Load, + })), + args: vec![ + create_expr(ExprKind::Name { + id: arg_name.to_string(), + ctx: ExprContext::Load, + }), + create_expr(ExprKind::Tuple { + elts: elts.into_iter().map(Clone::clone).collect(), + ctx: ExprContext::Load, + }), + ], + keywords: vec![], + }); + + // Generate the combined `BoolOp`. + let bool_op = create_expr(ExprKind::BoolOp { + op: Boolop::Or, + values: iter::once(call) + .chain( + values + .iter() + .enumerate() + .filter(|(index, _)| !indices.contains(index)) + .map(|(_, elt)| elt.clone()), + ) + .collect(), + }); + + // Populate the `Fix`. Replace the _entire_ `BoolOp`. Note that if we have + // multiple duplicates, the fixes will conflict. + check.amend(Fix::replacement( + to_source(&bool_op, checker.style), + expr.location, + expr.end_location.unwrap(), + )); + } + checker.add_check(check); + } + } +} + /// SIM220 pub fn a_and_not_a(checker: &mut Checker, expr: &Expr) { let ExprKind::BoolOp { op: Boolop::And, values, } = &expr.node else { diff --git a/src/flake8_simplify/plugins/mod.rs b/src/flake8_simplify/plugins/mod.rs index 8f7c337cf33e3..8f62a5f032b63 100644 --- a/src/flake8_simplify/plugins/mod.rs +++ b/src/flake8_simplify/plugins/mod.rs @@ -1,4 +1,4 @@ -pub use ast_bool_op::{a_and_not_a, a_or_not_a, and_false, or_true}; +pub use ast_bool_op::{a_and_not_a, a_or_not_a, and_false, duplicate_isinstance_call, or_true}; pub use ast_for::convert_loop_to_any_all; pub use ast_if::nested_if_statements; pub use ast_with::multiple_with_statements; diff --git a/src/flake8_simplify/snapshots/ruff__flake8_simplify__tests__SIM101_SIM101.py.snap b/src/flake8_simplify/snapshots/ruff__flake8_simplify__tests__SIM101_SIM101.py.snap new file mode 100644 index 0000000000000..347bb42ceba39 --- /dev/null +++ b/src/flake8_simplify/snapshots/ruff__flake8_simplify__tests__SIM101_SIM101.py.snap @@ -0,0 +1,107 @@ +--- +source: src/flake8_simplify/mod.rs +expression: checks +--- +- kind: + DuplicateIsinstanceCall: a + location: + row: 1 + column: 3 + end_location: + row: 1 + column: 45 + fix: + content: "isinstance(a, (int, float))" + location: + row: 1 + column: 3 + end_location: + row: 1 + column: 45 + parent: ~ +- kind: + DuplicateIsinstanceCall: a + location: + row: 4 + column: 3 + end_location: + row: 4 + column: 53 + fix: + content: "isinstance(a, (int, float, bool))" + location: + row: 4 + column: 3 + end_location: + row: 4 + column: 53 + parent: ~ +- kind: + DuplicateIsinstanceCall: a + location: + row: 7 + column: 3 + end_location: + row: 7 + column: 68 + fix: + content: "isinstance(a, (int, float)) or isinstance(b, bool)" + location: + row: 7 + column: 3 + end_location: + row: 7 + column: 68 + parent: ~ +- kind: + DuplicateIsinstanceCall: a + location: + row: 10 + column: 3 + end_location: + row: 10 + column: 68 + fix: + content: "isinstance(a, (int, float)) or isinstance(b, bool)" + location: + row: 10 + column: 3 + end_location: + row: 10 + column: 68 + parent: ~ +- kind: + DuplicateIsinstanceCall: a + location: + row: 13 + column: 3 + end_location: + row: 13 + column: 68 + fix: + content: "isinstance(a, (int, float)) or isinstance(b, bool)" + location: + row: 13 + column: 3 + end_location: + row: 13 + column: 68 + parent: ~ +- kind: + DuplicateIsinstanceCall: a + location: + row: 16 + column: 4 + end_location: + row: 16 + column: 46 + fix: + content: "isinstance(a, (int, float))" + location: + row: 16 + column: 4 + end_location: + row: 16 + column: 46 + parent: ~ + diff --git a/src/registry.rs b/src/registry.rs index 8549a39502b7b..1c43d18c8c922 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -218,6 +218,7 @@ pub enum CheckCode { YTT302, YTT303, // flake8-simplify + SIM101, SIM102, SIM105, SIM107, @@ -959,6 +960,7 @@ pub enum CheckKind { SysVersionCmpStr10, SysVersionSlice1Referenced, // flake8-simplify + DuplicateIsinstanceCall(String), AAndNotA(String), AOrNotA(String), AndFalse, @@ -1400,6 +1402,7 @@ impl CheckCode { // flake8-blind-except CheckCode::BLE001 => CheckKind::BlindExcept("Exception".to_string()), // flake8-simplify + CheckCode::SIM101 => CheckKind::DuplicateIsinstanceCall("...".to_string()), CheckCode::SIM102 => CheckKind::NestedIfStatements, CheckCode::SIM105 => CheckKind::UseContextlibSuppress("...".to_string()), CheckCode::SIM107 => CheckKind::ReturnInTryExceptFinally, @@ -1946,6 +1949,7 @@ impl CheckCode { CheckCode::S324 => CheckCategory::Flake8Bandit, CheckCode::S506 => CheckCategory::Flake8Bandit, // flake8-simplify + CheckCode::SIM101 => CheckCategory::Flake8Simplify, CheckCode::SIM102 => CheckCategory::Flake8Simplify, CheckCode::SIM105 => CheckCategory::Flake8Simplify, CheckCode::SIM107 => CheckCategory::Flake8Simplify, @@ -2204,6 +2208,7 @@ impl CheckKind { CheckKind::SysVersionCmpStr10 => &CheckCode::YTT302, CheckKind::SysVersionSlice1Referenced => &CheckCode::YTT303, // flake8-simplify + CheckKind::DuplicateIsinstanceCall(..) => &CheckCode::SIM101, CheckKind::AAndNotA(..) => &CheckCode::SIM220, CheckKind::AOrNotA(..) => &CheckCode::SIM221, CheckKind::AndFalse => &CheckCode::SIM223, @@ -2972,6 +2977,9 @@ impl CheckKind { "`sys.version[:1]` referenced (python10), use `sys.version_info`".to_string() } // flake8-simplify + CheckKind::DuplicateIsinstanceCall(name) => { + format!("Multiple `isinstance` calls for `{name}`, merge into a single call") + } CheckKind::UseContextlibSuppress(exception) => { format!("Use `contextlib.suppress({exception})` instead of try-except-pass") } @@ -3641,6 +3649,7 @@ impl CheckKind { | CheckKind::DoNotAssignLambda(..) | CheckKind::DupeClassFieldDefinitions(..) | CheckKind::DuplicateHandlerException(..) + | CheckKind::DuplicateIsinstanceCall(..) | CheckKind::EndsInPeriod | CheckKind::EndsInPunctuation | CheckKind::FStringMissingPlaceholders @@ -3778,6 +3787,9 @@ impl CheckKind { Some(format!("Remove duplicate field definition for `{name}`")) } CheckKind::DuplicateHandlerException(..) => Some("De-duplicate exceptions".to_string()), + CheckKind::DuplicateIsinstanceCall(name) => { + Some(format!("Merge `isinstance` calls for `{name}`")) + } CheckKind::EndsInPeriod => Some("Add period".to_string()), CheckKind::EndsInPunctuation => Some("Add closing punctuation".to_string()), CheckKind::ExtraneousScopeFunction => Some("Remove `scope=` argument".to_string()),