diff --git a/README.md b/README.md index 6a898c29bf..6569d225bb 100644 --- a/README.md +++ b/README.md @@ -1259,6 +1259,7 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/) on Py | SIM110 | convert-loop-to-any | Use `{any}` instead of `for` loop | 🛠 | | SIM111 | convert-loop-to-all | Use `{all}` instead of `for` loop | 🛠 | | SIM112 | use-capital-environment-variables | Use capitalized environment variable `{expected}` instead of `{original}` | 🛠 | +| SIM114 | [if-with-same-arms](https://github.com/charliermarsh/ruff/blob/main/docs/rules/if-with-same-arms.md) | Combine `if` statements using `or` | | | SIM115 | open-file-with-context-handler | Use context handler for opening files | | | SIM117 | multiple-with-statements | Use a single `with` statement with multiple contexts instead of nested `with` statements | 🛠 | | SIM118 | key-in-dict | Use `{key} in {dict}` instead of `{key} in {dict}.keys()` | 🛠 | diff --git a/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py new file mode 100644 index 0000000000..c66dd33ad2 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_simplify/SIM114.py @@ -0,0 +1,79 @@ +# These SHOULD change +if a: + b +elif c: + b + +if x == 1: + for _ in range(20): + print("hello") +elif x == 2: + for _ in range(20): + print("hello") + +if x == 1: + if True: + for _ in range(20): + print("hello") +elif x == 2: + if True: + for _ in range(20): + print("hello") + +if x == 1: + if True: + for _ in range(20): + print("hello") + elif False: + for _ in range(20): + print("hello") +elif x == 2: + if True: + for _ in range(20): + print("hello") + elif False: + for _ in range(20): + print("hello") + +if ( + x == 1 + and y == 2 + and z == 3 + and a == 4 + and b == 5 + and c == 6 + and d == 7 + and e == 8 + and f == 9 + and g == 10 + and h == 11 + and i == 12 + and j == 13 + and k == 14 +): + pass +elif 1 == 2: pass + +# These SHOULD NOT change +def complicated_calc(*arg, **kwargs): + return 42 + + +def foo(p): + if p == 2: + return complicated_calc(microsecond=0) + elif p == 3: + return complicated_calc(microsecond=0, second=0) + return None + + +a = False +b = True +c = True + +if a: + z = 1 +elif b: + z = 2 +elif c: + z = 1 diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 3a35c27221..13295219b6 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -1512,6 +1512,9 @@ where self.current_stmt_parent().map(Into::into), ); } + if self.settings.rules.enabled(&Rule::IfWithSameArms) { + flake8_simplify::rules::if_with_same_arms(self, body, orelse); + } if self.settings.rules.enabled(&Rule::NeedlessBool) { flake8_simplify::rules::return_bool_condition_directly(self, stmt); } diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index d915e0f6a0..9f84f44152 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -251,7 +251,6 @@ ruff_macros::define_rule_mapping!( YTT302 => rules::flake8_2020::rules::SysVersionCmpStr10, YTT303 => rules::flake8_2020::rules::SysVersionSlice1Referenced, // flake8-simplify - SIM115 => rules::flake8_simplify::rules::OpenFileWithContextHandler, SIM101 => rules::flake8_simplify::rules::DuplicateIsinstanceCall, SIM102 => rules::flake8_simplify::rules::CollapsibleIf, SIM103 => rules::flake8_simplify::rules::NeedlessBool, @@ -262,6 +261,8 @@ ruff_macros::define_rule_mapping!( SIM110 => rules::flake8_simplify::rules::ConvertLoopToAny, SIM111 => rules::flake8_simplify::rules::ConvertLoopToAll, SIM112 => rules::flake8_simplify::rules::UseCapitalEnvironmentVariables, + SIM114 => rules::flake8_simplify::rules::IfWithSameArms, + SIM115 => rules::flake8_simplify::rules::OpenFileWithContextHandler, SIM117 => rules::flake8_simplify::rules::MultipleWithStatements, SIM118 => rules::flake8_simplify::rules::KeyInDict, SIM201 => rules::flake8_simplify::rules::NegateEqualOp, diff --git a/crates/ruff/src/rules/flake8_simplify/mod.rs b/crates/ruff/src/rules/flake8_simplify/mod.rs index 09cb9e38d2..3f7c3db9fb 100644 --- a/crates/ruff/src/rules/flake8_simplify/mod.rs +++ b/crates/ruff/src/rules/flake8_simplify/mod.rs @@ -37,6 +37,7 @@ mod tests { #[test_case(Rule::AndFalse, Path::new("SIM223.py"); "SIM223")] #[test_case(Rule::YodaConditions, Path::new("SIM300.py"); "SIM300")] #[test_case(Rule::DictGetWithDefault, Path::new("SIM401.py"); "SIM401")] + #[test_case(Rule::IfWithSameArms, Path::new("SIM114.py"); "SIM114")] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs index 99c2705512..820530e93b 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs @@ -1,5 +1,7 @@ use log::error; use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprContext, ExprKind, Stmt, StmtKind}; +use rustpython_parser::lexer; +use rustpython_parser::lexer::Tok; use ruff_macros::{define_violation, derive_message_formats}; @@ -13,6 +15,7 @@ use crate::checkers::ast::Checker; use crate::fix::Fix; use crate::registry::Diagnostic; use crate::rules::flake8_simplify::rules::fix_if; +use crate::source_code::Locator; use crate::violation::{AutofixKind, Availability, Violation}; define_violation!( @@ -90,6 +93,36 @@ impl Violation for UseTernaryOperator { } } +define_violation!( + /// ### What it does + /// Checks for `if` branches with identical arm bodies. + /// + /// ### Why is this bad? + /// If multiple arms of an `if` statement have the same body, using `or` + /// better signals the intent of the statement. + /// + /// ### Example + /// ```python + /// if x = 1: + /// print("Hello") + /// elif x = 2: + /// print("Hello") + /// ``` + /// + /// Use instead: + /// ```python + /// if x = 1 or x = 2 + /// print("Hello") + /// ``` + pub struct IfWithSameArms; +); +impl Violation for IfWithSameArms { + #[derive_message_formats] + fn message(&self) -> String { + format!("Combine `if` statements using `or`") + } +} + define_violation!( pub struct DictGetWithDefault { pub contents: String, @@ -453,6 +486,84 @@ fn compare_expr(expr1: &ComparableExpr, expr2: &ComparableExpr) -> bool { expr1.eq(expr2) } +fn get_if_body_pairs(orelse: &[Stmt], result: &mut Vec>) { + if orelse.is_empty() { + return; + } + let mut current_vec: Vec = vec![]; + for if_line in orelse { + if let StmtKind::If { + body: orelse_body, + orelse: orelse_orelse, + .. + } = &if_line.node + { + if !current_vec.is_empty() { + result.push(current_vec.clone()); + current_vec = vec![]; + } + if !orelse_body.is_empty() { + result.push(orelse_body.clone()); + } + get_if_body_pairs(orelse_orelse, result); + } else { + current_vec.push(if_line.clone()); + } + } + if !current_vec.is_empty() { + result.push(current_vec.clone()); + } +} + +pub fn is_equal(locator: &Locator, stmts1: &[Stmt], stmts2: &[Stmt]) -> bool { + if stmts1.len() != stmts2.len() { + return false; + } + for (stmt1, stmt2) in stmts1.iter().zip(stmts2.iter()) { + let text1 = locator.slice_source_code_range(&Range::from_located(stmt1)); + let text2 = locator.slice_source_code_range(&Range::from_located(stmt2)); + let lexer1: Vec = lexer::make_tokenizer(text1) + .flatten() + .map(|(_, tok, _)| tok) + .collect(); + let lexer2: Vec = lexer::make_tokenizer(text2) + .flatten() + .map(|(_, tok, _)| tok) + .collect(); + if lexer1 != lexer2 { + return false; + } + } + true +} + +/// SIM114 +pub fn if_with_same_arms(checker: &mut Checker, body: &[Stmt], orelse: &[Stmt]) { + if orelse.is_empty() { + return; + } + + // It's not all combinations because of this: + // https://github.com/MartinThoma/flake8-simplify/issues/70#issuecomment-924074984 + let mut final_stmts: Vec> = vec![body.to_vec()]; + get_if_body_pairs(orelse, &mut final_stmts); + let if_statements = final_stmts.len(); + if if_statements <= 1 { + return; + } + + for i in 0..(if_statements - 1) { + if is_equal(checker.locator, &final_stmts[i], &final_stmts[i + 1]) { + let first = &final_stmts[i].first().unwrap(); + let last = &final_stmts[i].last().unwrap(); + checker.diagnostics.push(Diagnostic::new( + IfWithSameArms, + Range::new(first.location, last.end_location.unwrap()), + )); + } + } +} + /// SIM401 pub fn use_dict_get_with_default( checker: &mut Checker, diff --git a/crates/ruff/src/rules/flake8_simplify/rules/mod.rs b/crates/ruff/src/rules/flake8_simplify/rules/mod.rs index 86b7c862c9..c28e6e2b30 100644 --- a/crates/ruff/src/rules/flake8_simplify/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_simplify/rules/mod.rs @@ -5,8 +5,9 @@ pub use ast_bool_op::{ pub use ast_expr::{use_capital_environment_variables, UseCapitalEnvironmentVariables}; pub use ast_for::{convert_for_loop_to_any_all, ConvertLoopToAll, ConvertLoopToAny}; pub use ast_if::{ - nested_if_statements, return_bool_condition_directly, use_dict_get_with_default, - use_ternary_operator, CollapsibleIf, DictGetWithDefault, NeedlessBool, UseTernaryOperator, + if_with_same_arms, nested_if_statements, return_bool_condition_directly, + use_dict_get_with_default, use_ternary_operator, CollapsibleIf, DictGetWithDefault, + IfWithSameArms, NeedlessBool, UseTernaryOperator, }; pub use ast_ifexp::{ explicit_false_true_in_ifexpr, explicit_true_false_in_ifexpr, twisted_arms_in_ifexpr, diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap new file mode 100644 index 0000000000..fc9964771d --- /dev/null +++ b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -0,0 +1,75 @@ +--- +source: crates/ruff/src/rules/flake8_simplify/mod.rs +expression: diagnostics +--- +- kind: + IfWithSameArms: ~ + location: + row: 3 + column: 4 + end_location: + row: 3 + column: 5 + fix: ~ + parent: ~ +- kind: + IfWithSameArms: ~ + location: + row: 8 + column: 4 + end_location: + row: 9 + column: 22 + fix: ~ + parent: ~ +- kind: + IfWithSameArms: ~ + location: + row: 15 + column: 4 + end_location: + row: 17 + column: 26 + fix: ~ + parent: ~ +- kind: + IfWithSameArms: ~ + location: + row: 24 + column: 4 + end_location: + row: 29 + column: 26 + fix: ~ + parent: ~ +- kind: + IfWithSameArms: ~ + location: + row: 25 + column: 8 + end_location: + row: 26 + column: 26 + fix: ~ + parent: ~ +- kind: + IfWithSameArms: ~ + location: + row: 32 + column: 8 + end_location: + row: 33 + column: 26 + fix: ~ + parent: ~ +- kind: + IfWithSameArms: ~ + location: + row: 54 + column: 4 + end_location: + row: 54 + column: 8 + fix: ~ + parent: ~ + diff --git a/docs/rules/combine-if-conditions.md b/docs/rules/combine-if-conditions.md new file mode 100644 index 0000000000..f3805a7f0b --- /dev/null +++ b/docs/rules/combine-if-conditions.md @@ -0,0 +1,23 @@ +# combine-if-conditions (SIM114) + +Derived from the **flake8-simplify** linter. + +### What it does +Checks if consecutive `if` branches have the same body. + +### Why is this bad? +These branches can be combine using the python `or` statement + +### Example +```python +if x = 1: + print("Hello") +elif x = 2: + print("Hello") +``` + +Use instead: +```python +if x = 1 or x = 2 + print("Hello") +``` \ No newline at end of file diff --git a/docs/rules/if-with-same-arms.md b/docs/rules/if-with-same-arms.md new file mode 100644 index 0000000000..10a62afdc4 --- /dev/null +++ b/docs/rules/if-with-same-arms.md @@ -0,0 +1,24 @@ +# if-with-same-arms (SIM114) + +Derived from the **flake8-simplify** linter. + +### What it does +Checks for `if` branches with identical arm bodies. + +### Why is this bad? +If multiple arms of an `if` statement have the same body, using `or` +better signals the intent of the statement. + +### Example +```python +if x = 1: + print("Hello") +elif x = 2: + print("Hello") +``` + +Use instead: +```python +if x = 1 or x = 2 + print("Hello") +``` \ No newline at end of file diff --git a/ruff.schema.json b/ruff.schema.json index 33710223ae..2b4a3dc463 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1911,6 +1911,7 @@ "SIM110", "SIM111", "SIM112", + "SIM114", "SIM115", "SIM117", "SIM118",