From c16ee1a4b0f6dc858039b2e3091445545f7677e4 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 20 Jan 2024 04:21:14 -0500 Subject: [PATCH 1/7] [`flake8-simplify`] - add fix for `if-with-same-arms` / `SIM114` --- .../test/fixtures/flake8_simplify/SIM114.py | 5 + .../src/rules/flake8_simplify/mod.rs | 1 + .../rules/if_with_same_arms.rs | 62 +++- ...ke8_simplify__tests__SIM114_SIM114.py.snap | 246 +++++++------ ...ify__tests__preview__SIM114_SIM114.py.snap | 330 ++++++++++++++++++ 5 files changed, 530 insertions(+), 114 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py index 180af6086dd7dd..ed222920945e7d 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py @@ -4,6 +4,11 @@ elif c: b +if a: # we preserve comments, too! + b +elif c: # yes, even this one! + b + if x == 1: for _ in range(20): print("hello") diff --git a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs index 3c825ae16359b6..c6b7526ec44f78 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs @@ -60,6 +60,7 @@ mod tests { #[test_case(Rule::YodaConditions, Path::new("SIM300.py"))] #[test_case(Rule::IfElseBlockInsteadOfDictGet, Path::new("SIM401.py"))] #[test_case(Rule::DictGetWithNoneDefault, Path::new("SIM910.py"))] + #[test_case(Rule::IfWithSameArms, Path::new("SIM114.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "preview__{}_{}", diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs index 9ffc34b236a00c..5cafcf23729d77 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs @@ -1,8 +1,10 @@ -use ruff_diagnostics::{Diagnostic, Violation}; +use ast::whitespace::indentation; +use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::comparable::ComparableStmt; use ruff_python_ast::stmt_if::{if_elif_branches, IfElifBranch}; +use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -32,10 +34,15 @@ use crate::checkers::ast::Checker; pub struct IfWithSameArms; impl Violation for IfWithSameArms { + const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; #[derive_message_formats] fn message(&self) -> String { format!("Combine `if` branches using logical `or` operator") } + + fn fix_title(&self) -> Option { + Some("Combine `if` branches using logical `or` operator".to_string()) + } } /// SIM114 @@ -76,10 +83,59 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, locator: &Locator, stmt_i continue; } - checker.diagnostics.push(Diagnostic::new( + let mut diagnostic = Diagnostic::new( IfWithSameArms, TextRange::new(current_branch.start(), following_branch.end()), - )); + ); + + if checker.settings.preview.is_enabled() { + let current_branch_colon = + SimpleTokenizer::starts_at(current_branch.test.end(), checker.locator().contents()) + .find(|token| token.kind == SimpleTokenKind::Colon) + .unwrap(); + + let mut following_branch_tokenizer = SimpleTokenizer::starts_at( + following_branch.test.end(), + checker.locator().contents(), + ); + + let following_branch_colon = following_branch_tokenizer + .find(|token| token.kind == SimpleTokenKind::Colon) + .unwrap(); + + let main_edit = if let Some(following_branch_comment) = + following_branch_tokenizer.find(|token| token.kind == SimpleTokenKind::Comment) + { + let indentation = + indentation(checker.locator(), following_branch.body.first().unwrap()) + .unwrap_or(""); + Edit::range_replacement( + format!("{indentation}"), + TextRange::new( + checker.locator().full_line_end(current_branch_colon.end()), + following_branch_comment.start(), + ), + ) + } else { + Edit::deletion( + checker.locator().full_line_end(current_branch_colon.end()), + checker + .locator() + .full_line_end(following_branch_colon.end()), + ) + }; + + diagnostic.set_fix(Fix::applicable_edits( + main_edit, + [Edit::insertion( + format!(" or {}", checker.generator().expr(following_branch.test)), + current_branch_colon.start(), + )], + Applicability::Safe, + )); + } + + checker.diagnostics.push(diagnostic); } } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap index 48c0df9aad5c2c..578da9fc9de6bb 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -10,158 +10,182 @@ SIM114.py:2:1: SIM114 Combine `if` branches using logical `or` operator 5 | | b | |_____^ SIM114 6 | -7 | if x == 1: +7 | if a: # we preserve comments, too! | + = help: Combine `if` branches using logical `or` operator SIM114.py:7:1: SIM114 Combine `if` branches using logical `or` operator | 5 | b 6 | - 7 | / if x == 1: - 8 | | for _ in range(20): - 9 | | print("hello") -10 | | elif x == 2: -11 | | for _ in range(20): -12 | | print("hello") - | |______________________^ SIM114 -13 | -14 | if x == 1: + 7 | / if a: # we preserve comments, too! + 8 | | b + 9 | | elif c: # yes, even this one! +10 | | b + | |_____^ SIM114 +11 | +12 | if x == 1: | + = help: Combine `if` branches using logical `or` operator -SIM114.py:14:1: SIM114 Combine `if` branches using logical `or` operator - | -12 | print("hello") -13 | -14 | / if x == 1: -15 | | if True: -16 | | for _ in range(20): -17 | | print("hello") -18 | | elif x == 2: -19 | | if True: -20 | | for _ in range(20): -21 | | print("hello") - | |__________________________^ SIM114 -22 | -23 | if x == 1: +SIM114.py:12:1: SIM114 Combine `if` branches using logical `or` operator + | +10 | b +11 | +12 | / if x == 1: +13 | | for _ in range(20): +14 | | print("hello") +15 | | elif x == 2: +16 | | for _ in range(20): +17 | | print("hello") + | |______________________^ SIM114 +18 | +19 | if x == 1: | + = help: Combine `if` branches using logical `or` operator -SIM114.py:23:1: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:19:1: SIM114 Combine `if` branches using logical `or` operator | -21 | print("hello") -22 | -23 | / if x == 1: +17 | print("hello") +18 | +19 | / if x == 1: +20 | | if True: +21 | | for _ in range(20): +22 | | print("hello") +23 | | elif x == 2: 24 | | if True: 25 | | for _ in range(20): 26 | | print("hello") -27 | | elif False: -28 | | for _ in range(20): -29 | | print("hello") -30 | | elif x == 2: -31 | | if True: -32 | | for _ in range(20): -33 | | print("hello") -34 | | elif False: -35 | | for _ in range(20): -36 | | print("hello") | |__________________________^ SIM114 -37 | -38 | if ( +27 | +28 | if x == 1: + | + = help: Combine `if` branches using logical `or` operator + +SIM114.py:28:1: SIM114 Combine `if` branches using logical `or` operator + | +26 | print("hello") +27 | +28 | / if x == 1: +29 | | if True: +30 | | for _ in range(20): +31 | | print("hello") +32 | | elif False: +33 | | for _ in range(20): +34 | | print("hello") +35 | | elif x == 2: +36 | | if True: +37 | | for _ in range(20): +38 | | print("hello") +39 | | elif False: +40 | | for _ in range(20): +41 | | print("hello") + | |__________________________^ SIM114 +42 | +43 | if ( | + = help: Combine `if` branches using logical `or` operator -SIM114.py:24:5: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:29:5: SIM114 Combine `if` branches using logical `or` operator | -23 | if x == 1: -24 | if True: +28 | if x == 1: +29 | if True: | _____^ -25 | | for _ in range(20): -26 | | print("hello") -27 | | elif False: -28 | | for _ in range(20): -29 | | print("hello") +30 | | for _ in range(20): +31 | | print("hello") +32 | | elif False: +33 | | for _ in range(20): +34 | | print("hello") | |__________________________^ SIM114 -30 | elif x == 2: -31 | if True: +35 | elif x == 2: +36 | if True: | + = help: Combine `if` branches using logical `or` operator -SIM114.py:31:5: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:36:5: SIM114 Combine `if` branches using logical `or` operator | -29 | print("hello") -30 | elif x == 2: -31 | if True: +34 | print("hello") +35 | elif x == 2: +36 | if True: | _____^ -32 | | for _ in range(20): -33 | | print("hello") -34 | | elif False: -35 | | for _ in range(20): -36 | | print("hello") +37 | | for _ in range(20): +38 | | print("hello") +39 | | elif False: +40 | | for _ in range(20): +41 | | print("hello") | |__________________________^ SIM114 -37 | -38 | if ( +42 | +43 | if ( | + = help: Combine `if` branches using logical `or` operator -SIM114.py:38:1: SIM114 Combine `if` branches using logical `or` operator - | -36 | print("hello") -37 | -38 | / if ( -39 | | x == 1 -40 | | and y == 2 -41 | | and z == 3 -42 | | and a == 4 -43 | | and b == 5 -44 | | and c == 6 -45 | | and d == 7 -46 | | and e == 8 -47 | | and f == 9 -48 | | and g == 10 -49 | | and h == 11 -50 | | and i == 12 -51 | | and j == 13 -52 | | and k == 14 -53 | | ): -54 | | pass -55 | | elif 1 == 2: -56 | | pass +SIM114.py:43:1: SIM114 Combine `if` branches using logical `or` operator + | +41 | print("hello") +42 | +43 | / if ( +44 | | x == 1 +45 | | and y == 2 +46 | | and z == 3 +47 | | and a == 4 +48 | | and b == 5 +49 | | and c == 6 +50 | | and d == 7 +51 | | and e == 8 +52 | | and f == 9 +53 | | and g == 10 +54 | | and h == 11 +55 | | and i == 12 +56 | | and j == 13 +57 | | and k == 14 +58 | | ): +59 | | pass +60 | | elif 1 == 2: +61 | | pass | |________^ SIM114 -57 | -58 | if result.eofs == "O": +62 | +63 | if result.eofs == "O": | + = help: Combine `if` branches using logical `or` operator -SIM114.py:62:1: SIM114 Combine `if` branches using logical `or` operator - | -60 | elif result.eofs == "S": -61 | skipped = 1 -62 | / elif result.eofs == "F": -63 | | errors = 1 -64 | | elif result.eofs == "E": -65 | | errors = 1 +SIM114.py:67:1: SIM114 Combine `if` branches using logical `or` operator + | +65 | elif result.eofs == "S": +66 | skipped = 1 +67 | / elif result.eofs == "F": +68 | | errors = 1 +69 | | elif result.eofs == "E": +70 | | errors = 1 | |______________^ SIM114 | + = help: Combine `if` branches using logical `or` operator -SIM114.py:109:5: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:114:5: SIM114 Combine `if` branches using logical `or` operator | -107 | a = True -108 | b = False -109 | if a > b: # end-of-line +112 | a = True +113 | b = False +114 | if a > b: # end-of-line | _____^ -110 | | return 3 -111 | | elif a == b: -112 | | return 3 +115 | | return 3 +116 | | elif a == b: +117 | | return 3 | |________________^ SIM114 -113 | elif a < b: # end-of-line -114 | return 4 +118 | elif a < b: # end-of-line +119 | return 4 | + = help: Combine `if` branches using logical `or` operator -SIM114.py:113:5: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:118:5: SIM114 Combine `if` branches using logical `or` operator | -111 | elif a == b: -112 | return 3 -113 | elif a < b: # end-of-line +116 | elif a == b: +117 | return 3 +118 | elif a < b: # end-of-line | _____^ -114 | | return 4 -115 | | elif b is None: -116 | | return 4 +119 | | return 4 +120 | | elif b is None: +121 | | return 4 | |________________^ SIM114 | + = help: Combine `if` branches using logical `or` operator diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap new file mode 100644 index 00000000000000..f5486ecbcd25af --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap @@ -0,0 +1,330 @@ +--- +source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs +--- +SIM114.py:2:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +1 | # Errors +2 | / if a: +3 | | b +4 | | elif c: +5 | | b + | |_____^ SIM114 +6 | +7 | if a: # we preserve comments, too! + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +1 1 | # Errors +2 |-if a: +3 |- b +4 |-elif c: + 2 |+if a or c: +5 3 | b +6 4 | +7 5 | if a: # we preserve comments, too! + +SIM114.py:7:1: SIM114 [*] Combine `if` branches using logical `or` operator + | + 5 | b + 6 | + 7 | / if a: # we preserve comments, too! + 8 | | b + 9 | | elif c: # yes, even this one! +10 | | b + | |_____^ SIM114 +11 | +12 | if x == 1: + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +4 4 | elif c: +5 5 | b +6 6 | +7 |-if a: # we preserve comments, too! +8 |- b +9 |-elif c: # yes, even this one! + 7 |+if a or c: # we preserve comments, too! + 8 |+ # yes, even this one! +10 9 | b +11 10 | +12 11 | if x == 1: + +SIM114.py:12:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +10 | b +11 | +12 | / if x == 1: +13 | | for _ in range(20): +14 | | print("hello") +15 | | elif x == 2: +16 | | for _ in range(20): +17 | | print("hello") + | |______________________^ SIM114 +18 | +19 | if x == 1: + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +9 9 | elif c: # yes, even this one! +10 10 | b +11 11 | +12 |-if x == 1: +13 |- for _ in range(20): +14 |- print("hello") +15 |-elif x == 2: + 12 |+if x == 1 or x == 2: +16 13 | for _ in range(20): +17 14 | print("hello") +18 15 | + +SIM114.py:19:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +17 | print("hello") +18 | +19 | / if x == 1: +20 | | if True: +21 | | for _ in range(20): +22 | | print("hello") +23 | | elif x == 2: +24 | | if True: +25 | | for _ in range(20): +26 | | print("hello") + | |__________________________^ SIM114 +27 | +28 | if x == 1: + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +16 16 | for _ in range(20): +17 17 | print("hello") +18 18 | +19 |-if x == 1: +20 |- if True: +21 |- for _ in range(20): +22 |- print("hello") +23 |-elif x == 2: + 19 |+if x == 1 or x == 2: +24 20 | if True: +25 21 | for _ in range(20): +26 22 | print("hello") + +SIM114.py:28:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +26 | print("hello") +27 | +28 | / if x == 1: +29 | | if True: +30 | | for _ in range(20): +31 | | print("hello") +32 | | elif False: +33 | | for _ in range(20): +34 | | print("hello") +35 | | elif x == 2: +36 | | if True: +37 | | for _ in range(20): +38 | | print("hello") +39 | | elif False: +40 | | for _ in range(20): +41 | | print("hello") + | |__________________________^ SIM114 +42 | +43 | if ( + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +25 25 | for _ in range(20): +26 26 | print("hello") +27 27 | +28 |-if x == 1: +29 |- if True: +30 |- for _ in range(20): +31 |- print("hello") +32 |- elif False: +33 |- for _ in range(20): +34 |- print("hello") +35 |-elif x == 2: + 28 |+if x == 1 or x == 2: +36 29 | if True: +37 30 | for _ in range(20): +38 31 | print("hello") + +SIM114.py:29:5: SIM114 [*] Combine `if` branches using logical `or` operator + | +28 | if x == 1: +29 | if True: + | _____^ +30 | | for _ in range(20): +31 | | print("hello") +32 | | elif False: +33 | | for _ in range(20): +34 | | print("hello") + | |__________________________^ SIM114 +35 | elif x == 2: +36 | if True: + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +26 26 | print("hello") +27 27 | +28 28 | if x == 1: +29 |- if True: +30 |- for _ in range(20): +31 |- print("hello") +32 |- elif False: + 29 |+ if True or False: +33 30 | for _ in range(20): +34 31 | print("hello") +35 32 | elif x == 2: + +SIM114.py:36:5: SIM114 [*] Combine `if` branches using logical `or` operator + | +34 | print("hello") +35 | elif x == 2: +36 | if True: + | _____^ +37 | | for _ in range(20): +38 | | print("hello") +39 | | elif False: +40 | | for _ in range(20): +41 | | print("hello") + | |__________________________^ SIM114 +42 | +43 | if ( + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +33 33 | for _ in range(20): +34 34 | print("hello") +35 35 | elif x == 2: +36 |- if True: +37 |- for _ in range(20): +38 |- print("hello") +39 |- elif False: + 36 |+ if True or False: +40 37 | for _ in range(20): +41 38 | print("hello") +42 39 | + +SIM114.py:43:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +41 | print("hello") +42 | +43 | / if ( +44 | | x == 1 +45 | | and y == 2 +46 | | and z == 3 +47 | | and a == 4 +48 | | and b == 5 +49 | | and c == 6 +50 | | and d == 7 +51 | | and e == 8 +52 | | and f == 9 +53 | | and g == 10 +54 | | and h == 11 +55 | | and i == 12 +56 | | and j == 13 +57 | | and k == 14 +58 | | ): +59 | | pass +60 | | elif 1 == 2: +61 | | pass + | |________^ SIM114 +62 | +63 | if result.eofs == "O": + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +55 55 | and i == 12 +56 56 | and j == 13 +57 57 | and k == 14 +58 |-): +59 |- pass +60 |-elif 1 == 2: + 58 |+) or 1 == 2: +61 59 | pass +62 60 | +63 61 | if result.eofs == "O": + +SIM114.py:67:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +65 | elif result.eofs == "S": +66 | skipped = 1 +67 | / elif result.eofs == "F": +68 | | errors = 1 +69 | | elif result.eofs == "E": +70 | | errors = 1 + | |______________^ SIM114 + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +64 64 | pass +65 65 | elif result.eofs == "S": +66 66 | skipped = 1 +67 |-elif result.eofs == "F": +68 |- errors = 1 +69 |-elif result.eofs == "E": + 67 |+elif result.eofs == "F" or result.eofs == "E": +70 68 | errors = 1 +71 69 | +72 70 | + +SIM114.py:114:5: SIM114 [*] Combine `if` branches using logical `or` operator + | +112 | a = True +113 | b = False +114 | if a > b: # end-of-line + | _____^ +115 | | return 3 +116 | | elif a == b: +117 | | return 3 + | |________________^ SIM114 +118 | elif a < b: # end-of-line +119 | return 4 + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +111 111 | def foo(): +112 112 | a = True +113 113 | b = False +114 |- if a > b: # end-of-line +115 |- return 3 +116 |- elif a == b: + 114 |+ if a > b or a == b: # end-of-line +117 115 | return 3 +118 116 | elif a < b: # end-of-line +119 117 | return 4 + +SIM114.py:118:5: SIM114 [*] Combine `if` branches using logical `or` operator + | +116 | elif a == b: +117 | return 3 +118 | elif a < b: # end-of-line + | _____^ +119 | | return 4 +120 | | elif b is None: +121 | | return 4 + | |________________^ SIM114 + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +115 115 | return 3 +116 116 | elif a == b: +117 117 | return 3 +118 |- elif a < b: # end-of-line +119 |- return 4 +120 |- elif b is None: + 118 |+ elif a < b or b is None: # end-of-line +121 119 | return 4 + + From 318018b5f81865f2ecf6b5a81ce53b139d333757 Mon Sep 17 00:00:00 2001 From: Steve C Date: Sat, 20 Jan 2024 04:44:04 -0500 Subject: [PATCH 2/7] add more test cases --- .../test/fixtures/flake8_simplify/SIM114.py | 4 + ...ke8_simplify__tests__SIM114_SIM114.py.snap | 60 ++++++--- ...ify__tests__preview__SIM114_SIM114.py.snap | 124 +++++++++++++----- 3 files changed, 136 insertions(+), 52 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py index ed222920945e7d..1902640013fbca 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py @@ -68,6 +68,10 @@ errors = 1 elif result.eofs == "E": errors = 1 +elif result.eofs == "X": + errors = 1 +elif result.eofs == "C": + errors = 1 # OK diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap index 578da9fc9de6bb..258e2ed5cd97c1 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -157,33 +157,61 @@ SIM114.py:67:1: SIM114 Combine `if` branches using logical `or` operator 69 | | elif result.eofs == "E": 70 | | errors = 1 | |______________^ SIM114 +71 | elif result.eofs == "X": +72 | errors = 1 | = help: Combine `if` branches using logical `or` operator -SIM114.py:114:5: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:69:1: SIM114 Combine `if` branches using logical `or` operator + | +67 | elif result.eofs == "F": +68 | errors = 1 +69 | / elif result.eofs == "E": +70 | | errors = 1 +71 | | elif result.eofs == "X": +72 | | errors = 1 + | |______________^ SIM114 +73 | elif result.eofs == "C": +74 | errors = 1 + | + = help: Combine `if` branches using logical `or` operator + +SIM114.py:71:1: SIM114 Combine `if` branches using logical `or` operator + | +69 | elif result.eofs == "E": +70 | errors = 1 +71 | / elif result.eofs == "X": +72 | | errors = 1 +73 | | elif result.eofs == "C": +74 | | errors = 1 + | |______________^ SIM114 + | + = help: Combine `if` branches using logical `or` operator + +SIM114.py:118:5: SIM114 Combine `if` branches using logical `or` operator | -112 | a = True -113 | b = False -114 | if a > b: # end-of-line +116 | a = True +117 | b = False +118 | if a > b: # end-of-line | _____^ -115 | | return 3 -116 | | elif a == b: -117 | | return 3 +119 | | return 3 +120 | | elif a == b: +121 | | return 3 | |________________^ SIM114 -118 | elif a < b: # end-of-line -119 | return 4 +122 | elif a < b: # end-of-line +123 | return 4 | = help: Combine `if` branches using logical `or` operator -SIM114.py:118:5: SIM114 Combine `if` branches using logical `or` operator +SIM114.py:122:5: SIM114 Combine `if` branches using logical `or` operator | -116 | elif a == b: -117 | return 3 -118 | elif a < b: # end-of-line +120 | elif a == b: +121 | return 3 +122 | elif a < b: # end-of-line | _____^ -119 | | return 4 -120 | | elif b is None: -121 | | return 4 +123 | | return 4 +124 | | elif b is None: +125 | | return 4 | |________________^ SIM114 | = help: Combine `if` branches using logical `or` operator diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap index f5486ecbcd25af..be5f178e51042e 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap @@ -262,6 +262,8 @@ SIM114.py:67:1: SIM114 [*] Combine `if` branches using logical `or` operator 69 | | elif result.eofs == "E": 70 | | errors = 1 | |______________^ SIM114 +71 | elif result.eofs == "X": +72 | errors = 1 | = help: Combine `if` branches using logical `or` operator @@ -274,57 +276,107 @@ SIM114.py:67:1: SIM114 [*] Combine `if` branches using logical `or` operator 69 |-elif result.eofs == "E": 67 |+elif result.eofs == "F" or result.eofs == "E": 70 68 | errors = 1 -71 69 | -72 70 | +71 69 | elif result.eofs == "X": +72 70 | errors = 1 -SIM114.py:114:5: SIM114 [*] Combine `if` branches using logical `or` operator +SIM114.py:69:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +67 | elif result.eofs == "F": +68 | errors = 1 +69 | / elif result.eofs == "E": +70 | | errors = 1 +71 | | elif result.eofs == "X": +72 | | errors = 1 + | |______________^ SIM114 +73 | elif result.eofs == "C": +74 | errors = 1 + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +66 66 | skipped = 1 +67 67 | elif result.eofs == "F": +68 68 | errors = 1 +69 |-elif result.eofs == "E": +70 |- errors = 1 +71 |-elif result.eofs == "X": + 69 |+elif result.eofs == "E" or result.eofs == "X": +72 70 | errors = 1 +73 71 | elif result.eofs == "C": +74 72 | errors = 1 + +SIM114.py:71:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +69 | elif result.eofs == "E": +70 | errors = 1 +71 | / elif result.eofs == "X": +72 | | errors = 1 +73 | | elif result.eofs == "C": +74 | | errors = 1 + | |______________^ SIM114 + | + = help: Combine `if` branches using logical `or` operator + +ℹ Safe fix +68 68 | errors = 1 +69 69 | elif result.eofs == "E": +70 70 | errors = 1 +71 |-elif result.eofs == "X": +72 |- errors = 1 +73 |-elif result.eofs == "C": + 71 |+elif result.eofs == "X" or result.eofs == "C": +74 72 | errors = 1 +75 73 | +76 74 | + +SIM114.py:118:5: SIM114 [*] Combine `if` branches using logical `or` operator | -112 | a = True -113 | b = False -114 | if a > b: # end-of-line +116 | a = True +117 | b = False +118 | if a > b: # end-of-line | _____^ -115 | | return 3 -116 | | elif a == b: -117 | | return 3 +119 | | return 3 +120 | | elif a == b: +121 | | return 3 | |________________^ SIM114 -118 | elif a < b: # end-of-line -119 | return 4 +122 | elif a < b: # end-of-line +123 | return 4 | = help: Combine `if` branches using logical `or` operator ℹ Safe fix -111 111 | def foo(): -112 112 | a = True -113 113 | b = False -114 |- if a > b: # end-of-line -115 |- return 3 -116 |- elif a == b: - 114 |+ if a > b or a == b: # end-of-line -117 115 | return 3 -118 116 | elif a < b: # end-of-line -119 117 | return 4 +115 115 | def foo(): +116 116 | a = True +117 117 | b = False +118 |- if a > b: # end-of-line +119 |- return 3 +120 |- elif a == b: + 118 |+ if a > b or a == b: # end-of-line +121 119 | return 3 +122 120 | elif a < b: # end-of-line +123 121 | return 4 -SIM114.py:118:5: SIM114 [*] Combine `if` branches using logical `or` operator +SIM114.py:122:5: SIM114 [*] Combine `if` branches using logical `or` operator | -116 | elif a == b: -117 | return 3 -118 | elif a < b: # end-of-line +120 | elif a == b: +121 | return 3 +122 | elif a < b: # end-of-line | _____^ -119 | | return 4 -120 | | elif b is None: -121 | | return 4 +123 | | return 4 +124 | | elif b is None: +125 | | return 4 | |________________^ SIM114 | = help: Combine `if` branches using logical `or` operator ℹ Safe fix -115 115 | return 3 -116 116 | elif a == b: -117 117 | return 3 -118 |- elif a < b: # end-of-line -119 |- return 4 -120 |- elif b is None: - 118 |+ elif a < b or b is None: # end-of-line -121 119 | return 4 +119 119 | return 3 +120 120 | elif a == b: +121 121 | return 3 +122 |- elif a < b: # end-of-line +123 |- return 4 +124 |- elif b is None: + 122 |+ elif a < b or b is None: # end-of-line +125 123 | return 4 From 6ea332ad4209f4d3cd146697dfe98ff13e2edea6 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 23:10:23 -0500 Subject: [PATCH 3/7] Separate method --- .../src/checkers/ast/analyze/statement.rs | 2 +- .../rules/if_with_same_arms.rs | 112 ++++++++++-------- 2 files changed, 64 insertions(+), 50 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index c31f1479976635..e6ac77f1f1a3e8 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1092,7 +1092,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { ); } if checker.enabled(Rule::IfWithSameArms) { - flake8_simplify::rules::if_with_same_arms(checker, checker.locator, if_); + flake8_simplify::rules::if_with_same_arms(checker, if_); } if checker.enabled(Rule::NeedlessBool) { flake8_simplify::rules::needless_bool(checker, if_); diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs index 5cafcf23729d77..1a8892179f47b3 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs @@ -1,9 +1,12 @@ +use anyhow::Result; + use ast::whitespace::indentation; -use ruff_diagnostics::{Applicability, Diagnostic, Edit, Fix, FixAvailability, Violation}; +use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::comparable::ComparableStmt; use ruff_python_ast::stmt_if::{if_elif_branches, IfElifBranch}; +use ruff_python_codegen::Generator; use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -46,7 +49,7 @@ impl Violation for IfWithSameArms { } /// SIM114 -pub(crate) fn if_with_same_arms(checker: &mut Checker, locator: &Locator, stmt_if: &ast::StmtIf) { +pub(crate) fn if_with_same_arms(checker: &mut Checker, stmt_if: &ast::StmtIf) { let mut branches_iter = if_elif_branches(stmt_if).peekable(); while let Some(current_branch) = branches_iter.next() { let Some(following_branch) = branches_iter.peek() else { @@ -70,15 +73,15 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, locator: &Locator, stmt_i let first_comments = checker .indexer() .comment_ranges() - .comments_in_range(body_range(¤t_branch, locator)) + .comments_in_range(body_range(¤t_branch, checker.locator())) .iter() - .map(|range| locator.slice(*range)); + .map(|range| checker.locator().slice(*range)); let second_comments = checker .indexer() .comment_ranges() - .comments_in_range(body_range(following_branch, locator)) + .comments_in_range(body_range(following_branch, checker.locator())) .iter() - .map(|range| locator.slice(*range)); + .map(|range| checker.locator().slice(*range)); if !first_comments.eq(second_comments) { continue; } @@ -89,56 +92,67 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, locator: &Locator, stmt_i ); if checker.settings.preview.is_enabled() { - let current_branch_colon = - SimpleTokenizer::starts_at(current_branch.test.end(), checker.locator().contents()) - .find(|token| token.kind == SimpleTokenKind::Colon) - .unwrap(); - - let mut following_branch_tokenizer = SimpleTokenizer::starts_at( - following_branch.test.end(), - checker.locator().contents(), - ); - - let following_branch_colon = following_branch_tokenizer - .find(|token| token.kind == SimpleTokenKind::Colon) - .unwrap(); - - let main_edit = if let Some(following_branch_comment) = - following_branch_tokenizer.find(|token| token.kind == SimpleTokenKind::Comment) - { - let indentation = - indentation(checker.locator(), following_branch.body.first().unwrap()) - .unwrap_or(""); - Edit::range_replacement( - format!("{indentation}"), - TextRange::new( - checker.locator().full_line_end(current_branch_colon.end()), - following_branch_comment.start(), - ), - ) - } else { - Edit::deletion( - checker.locator().full_line_end(current_branch_colon.end()), - checker - .locator() - .full_line_end(following_branch_colon.end()), + diagnostic.try_set_fix(|| { + merge_branches( + ¤t_branch, + &following_branch, + checker.locator(), + checker.generator(), ) - }; - - diagnostic.set_fix(Fix::applicable_edits( - main_edit, - [Edit::insertion( - format!(" or {}", checker.generator().expr(following_branch.test)), - current_branch_colon.start(), - )], - Applicability::Safe, - )); + }); } checker.diagnostics.push(diagnostic); } } +/// Generate a [`Fix`] to merge two [`IfElifBranch`] branches. +fn merge_branches( + current_branch: &IfElifBranch, + following_branch: &IfElifBranch, + locator: &Locator, + generator: Generator, +) -> Result { + let current_branch_colon = + SimpleTokenizer::starts_at(current_branch.test.end(), locator.contents()) + .find(|token| token.kind == SimpleTokenKind::Colon) + .unwrap(); + + let mut following_branch_tokenizer = + SimpleTokenizer::starts_at(following_branch.test.end(), locator.contents()); + + let following_branch_colon = following_branch_tokenizer + .find(|token| token.kind == SimpleTokenKind::Colon) + .unwrap(); + + let main_edit = if let Some(following_branch_comment) = + following_branch_tokenizer.find(|token| token.kind == SimpleTokenKind::Comment) + { + let indentation = + indentation(locator, following_branch.body.first().unwrap()).unwrap_or(""); + Edit::range_replacement( + format!("{indentation}"), + TextRange::new( + locator.full_line_end(current_branch_colon.end()), + following_branch_comment.start(), + ), + ) + } else { + Edit::deletion( + locator.full_line_end(current_branch_colon.end()), + locator.full_line_end(following_branch_colon.end()), + ) + }; + + Ok(Fix::safe_edits( + main_edit, + [Edit::insertion( + format!(" or {}", generator.expr(following_branch.test)), + current_branch_colon.start(), + )], + )) +} + /// Return the [`TextRange`] of an [`IfElifBranch`]'s body (from the end of the test to the end of /// the body). fn body_range(branch: &IfElifBranch, locator: &Locator) -> TextRange { From f1a972813f2810fbf8aef6544ed7f948e0581e00 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 23:10:43 -0500 Subject: [PATCH 4/7] Tweak message --- .../rules/if_with_same_arms.rs | 3 ++- ...ke8_simplify__tests__SIM114_SIM114.py.snap | 26 +++++++++---------- ...ify__tests__preview__SIM114_SIM114.py.snap | 26 +++++++++---------- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs index 1a8892179f47b3..2fa831efcca2a8 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs @@ -38,13 +38,14 @@ pub struct IfWithSameArms; impl Violation for IfWithSameArms { const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes; + #[derive_message_formats] fn message(&self) -> String { format!("Combine `if` branches using logical `or` operator") } fn fix_title(&self) -> Option { - Some("Combine `if` branches using logical `or` operator".to_string()) + Some("Combine `if` branches".to_string()) } } diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap index 258e2ed5cd97c1..f7ae633d36903f 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -12,7 +12,7 @@ SIM114.py:2:1: SIM114 Combine `if` branches using logical `or` operator 6 | 7 | if a: # we preserve comments, too! | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:7:1: SIM114 Combine `if` branches using logical `or` operator | @@ -26,7 +26,7 @@ SIM114.py:7:1: SIM114 Combine `if` branches using logical `or` operator 11 | 12 | if x == 1: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:12:1: SIM114 Combine `if` branches using logical `or` operator | @@ -42,7 +42,7 @@ SIM114.py:12:1: SIM114 Combine `if` branches using logical `or` operator 18 | 19 | if x == 1: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:19:1: SIM114 Combine `if` branches using logical `or` operator | @@ -60,7 +60,7 @@ SIM114.py:19:1: SIM114 Combine `if` branches using logical `or` operator 27 | 28 | if x == 1: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:28:1: SIM114 Combine `if` branches using logical `or` operator | @@ -84,7 +84,7 @@ SIM114.py:28:1: SIM114 Combine `if` branches using logical `or` operator 42 | 43 | if ( | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:29:5: SIM114 Combine `if` branches using logical `or` operator | @@ -100,7 +100,7 @@ SIM114.py:29:5: SIM114 Combine `if` branches using logical `or` operator 35 | elif x == 2: 36 | if True: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:36:5: SIM114 Combine `if` branches using logical `or` operator | @@ -117,7 +117,7 @@ SIM114.py:36:5: SIM114 Combine `if` branches using logical `or` operator 42 | 43 | if ( | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:43:1: SIM114 Combine `if` branches using logical `or` operator | @@ -146,7 +146,7 @@ SIM114.py:43:1: SIM114 Combine `if` branches using logical `or` operator 62 | 63 | if result.eofs == "O": | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:67:1: SIM114 Combine `if` branches using logical `or` operator | @@ -160,7 +160,7 @@ SIM114.py:67:1: SIM114 Combine `if` branches using logical `or` operator 71 | elif result.eofs == "X": 72 | errors = 1 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:69:1: SIM114 Combine `if` branches using logical `or` operator | @@ -174,7 +174,7 @@ SIM114.py:69:1: SIM114 Combine `if` branches using logical `or` operator 73 | elif result.eofs == "C": 74 | errors = 1 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:71:1: SIM114 Combine `if` branches using logical `or` operator | @@ -186,7 +186,7 @@ SIM114.py:71:1: SIM114 Combine `if` branches using logical `or` operator 74 | | errors = 1 | |______________^ SIM114 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:118:5: SIM114 Combine `if` branches using logical `or` operator | @@ -201,7 +201,7 @@ SIM114.py:118:5: SIM114 Combine `if` branches using logical `or` operator 122 | elif a < b: # end-of-line 123 | return 4 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches SIM114.py:122:5: SIM114 Combine `if` branches using logical `or` operator | @@ -214,6 +214,6 @@ SIM114.py:122:5: SIM114 Combine `if` branches using logical `or` operator 125 | | return 4 | |________________^ SIM114 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap index be5f178e51042e..287f4994769d3f 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap @@ -12,7 +12,7 @@ SIM114.py:2:1: SIM114 [*] Combine `if` branches using logical `or` operator 6 | 7 | if a: # we preserve comments, too! | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 1 1 | # Errors @@ -36,7 +36,7 @@ SIM114.py:7:1: SIM114 [*] Combine `if` branches using logical `or` operator 11 | 12 | if x == 1: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 4 4 | elif c: @@ -65,7 +65,7 @@ SIM114.py:12:1: SIM114 [*] Combine `if` branches using logical `or` operator 18 | 19 | if x == 1: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 9 9 | elif c: # yes, even this one! @@ -96,7 +96,7 @@ SIM114.py:19:1: SIM114 [*] Combine `if` branches using logical `or` operator 27 | 28 | if x == 1: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 16 16 | for _ in range(20): @@ -134,7 +134,7 @@ SIM114.py:28:1: SIM114 [*] Combine `if` branches using logical `or` operator 42 | 43 | if ( | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 25 25 | for _ in range(20): @@ -167,7 +167,7 @@ SIM114.py:29:5: SIM114 [*] Combine `if` branches using logical `or` operator 35 | elif x == 2: 36 | if True: | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 26 26 | print("hello") @@ -197,7 +197,7 @@ SIM114.py:36:5: SIM114 [*] Combine `if` branches using logical `or` operator 42 | 43 | if ( | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 33 33 | for _ in range(20): @@ -239,7 +239,7 @@ SIM114.py:43:1: SIM114 [*] Combine `if` branches using logical `or` operator 62 | 63 | if result.eofs == "O": | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 55 55 | and i == 12 @@ -265,7 +265,7 @@ SIM114.py:67:1: SIM114 [*] Combine `if` branches using logical `or` operator 71 | elif result.eofs == "X": 72 | errors = 1 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 64 64 | pass @@ -291,7 +291,7 @@ SIM114.py:69:1: SIM114 [*] Combine `if` branches using logical `or` operator 73 | elif result.eofs == "C": 74 | errors = 1 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 66 66 | skipped = 1 @@ -315,7 +315,7 @@ SIM114.py:71:1: SIM114 [*] Combine `if` branches using logical `or` operator 74 | | errors = 1 | |______________^ SIM114 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 68 68 | errors = 1 @@ -342,7 +342,7 @@ SIM114.py:118:5: SIM114 [*] Combine `if` branches using logical `or` operator 122 | elif a < b: # end-of-line 123 | return 4 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 115 115 | def foo(): @@ -367,7 +367,7 @@ SIM114.py:122:5: SIM114 [*] Combine `if` branches using logical `or` operator 125 | | return 4 | |________________^ SIM114 | - = help: Combine `if` branches using logical `or` operator + = help: Combine `if` branches ℹ Safe fix 119 119 | return 3 From d0d4907dacac3222efb074bf0009546198d79c5c Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 23:23:08 -0500 Subject: [PATCH 5/7] Parenthesize --- .../test/fixtures/flake8_simplify/SIM114.py | 14 +++++- .../rules/if_with_same_arms.rs | 50 +++++++++++++++---- ...ke8_simplify__tests__SIM114_SIM114.py.snap | 13 +++++ ...ify__tests__preview__SIM114_SIM114.py.snap | 27 +++++++++- 4 files changed, 92 insertions(+), 12 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py index 1902640013fbca..5b9e019c48565e 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py @@ -79,7 +79,7 @@ def complicated_calc(*arg, **kwargs): return 42 -def foo(p): +def func(p): if p == 2: return complicated_calc(microsecond=0) elif p == 3: @@ -112,7 +112,7 @@ def foo(p): x = 1 -def foo(): +def func(): a = True b = False if a > b: # end-of-line @@ -123,3 +123,13 @@ def foo(): return 4 elif b is None: return 4 + + +def func(): + """Ensure that the named expression is parenthesized when merged.""" + a = True + b = False + if a > b: # end-of-line + return 3 + elif a := 1: + return 3 diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs index 2fa831efcca2a8..66755f60e03919 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs @@ -1,12 +1,15 @@ use anyhow::Result; +use std::borrow::Cow; use ast::whitespace::indentation; use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::comparable::ComparableStmt; +use ruff_python_ast::parenthesize::parenthesized_range; use ruff_python_ast::stmt_if::{if_elif_branches, IfElifBranch}; -use ruff_python_codegen::Generator; +use ruff_python_ast::Expr; +use ruff_python_index::Indexer; use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::Locator; use ruff_text_size::{Ranged, TextRange}; @@ -95,10 +98,11 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, stmt_if: &ast::StmtIf) { if checker.settings.preview.is_enabled() { diagnostic.try_set_fix(|| { merge_branches( + stmt_if, ¤t_branch, &following_branch, checker.locator(), - checker.generator(), + checker.indexer(), ) }); } @@ -109,22 +113,29 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, stmt_if: &ast::StmtIf) { /// Generate a [`Fix`] to merge two [`IfElifBranch`] branches. fn merge_branches( + stmt_if: &ast::StmtIf, current_branch: &IfElifBranch, following_branch: &IfElifBranch, locator: &Locator, - generator: Generator, + indexer: &Indexer, ) -> Result { - let current_branch_colon = + // Identify the colon (`:`) at the end of the current branch's test. + let Some(current_branch_colon) = SimpleTokenizer::starts_at(current_branch.test.end(), locator.contents()) .find(|token| token.kind == SimpleTokenKind::Colon) - .unwrap(); + else { + return Err(anyhow::anyhow!("Expected colon after test")); + }; let mut following_branch_tokenizer = SimpleTokenizer::starts_at(following_branch.test.end(), locator.contents()); - let following_branch_colon = following_branch_tokenizer - .find(|token| token.kind == SimpleTokenKind::Colon) - .unwrap(); + // Identify the colon (`:`) at the end of the following branch's test. + let Some(following_branch_colon) = + following_branch_tokenizer.find(|token| token.kind == SimpleTokenKind::Colon) + else { + return Err(anyhow::anyhow!("Expected colon after test")); + }; let main_edit = if let Some(following_branch_comment) = following_branch_tokenizer.find(|token| token.kind == SimpleTokenKind::Comment) @@ -145,10 +156,31 @@ fn merge_branches( ) }; + // If the test isn't parenthesized, consider parenthesizing it. + let following_branch_test = if let Some(range) = parenthesized_range( + following_branch.test.into(), + stmt_if.into(), + indexer.comment_ranges(), + locator.contents(), + ) { + Cow::Borrowed(locator.slice(range)) + } else if matches!( + following_branch.test, + Expr::BoolOp(ast::ExprBoolOp { + op: ast::BoolOp::Or, + .. + }) | Expr::Lambda(_) + | Expr::NamedExpr(_) + ) { + Cow::Owned(format!("({})", locator.slice(following_branch.test))) + } else { + Cow::Borrowed(locator.slice(following_branch.test)) + }; + Ok(Fix::safe_edits( main_edit, [Edit::insertion( - format!(" or {}", generator.expr(following_branch.test)), + format!(" or {following_branch_test}"), current_branch_colon.start(), )], )) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap index f7ae633d36903f..de04172958297a 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -216,4 +216,17 @@ SIM114.py:122:5: SIM114 Combine `if` branches using logical `or` operator | = help: Combine `if` branches +SIM114.py:132:5: SIM114 Combine `if` branches using logical `or` operator + | +130 | a = True +131 | b = False +132 | if a > b: # end-of-line + | _____^ +133 | | return 3 +134 | | elif a := 1: +135 | | return 3 + | |________________^ SIM114 + | + = help: Combine `if` branches + diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap index 287f4994769d3f..9a565333d4bc58 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap @@ -345,7 +345,7 @@ SIM114.py:118:5: SIM114 [*] Combine `if` branches using logical `or` operator = help: Combine `if` branches ℹ Safe fix -115 115 | def foo(): +115 115 | def func(): 116 116 | a = True 117 117 | b = False 118 |- if a > b: # end-of-line @@ -378,5 +378,30 @@ SIM114.py:122:5: SIM114 [*] Combine `if` branches using logical `or` operator 124 |- elif b is None: 122 |+ elif a < b or b is None: # end-of-line 125 123 | return 4 +126 124 | +127 125 | + +SIM114.py:132:5: SIM114 [*] Combine `if` branches using logical `or` operator + | +130 | a = True +131 | b = False +132 | if a > b: # end-of-line + | _____^ +133 | | return 3 +134 | | elif a := 1: +135 | | return 3 + | |________________^ SIM114 + | + = help: Combine `if` branches + +ℹ Safe fix +129 129 | """Ensure that the named expression is parenthesized when merged.""" +130 130 | a = True +131 131 | b = False +132 |- if a > b: # end-of-line +133 |- return 3 +134 |- elif a := 1: + 132 |+ if a > b or (a := 1): # end-of-line +135 133 | return 3 From 32a34ca9404770477db72e336940859f5eb91a3d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 23:29:19 -0500 Subject: [PATCH 6/7] Drop some comments for simplicity --- .../test/fixtures/flake8_simplify/SIM114.py | 12 ++++- .../rules/if_with_same_arms.rs | 29 +++------- ...ke8_simplify__tests__SIM114_SIM114.py.snap | 20 ++++++- ...ify__tests__preview__SIM114_SIM114.py.snap | 53 ++++++++++++++++--- 4 files changed, 83 insertions(+), 31 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py index 5b9e019c48565e..195efb85e9ff0d 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM114.py @@ -6,7 +6,7 @@ if a: # we preserve comments, too! b -elif c: # yes, even this one! +elif c: # but not on the second branch b if x == 1: @@ -133,3 +133,13 @@ def func(): return 3 elif a := 1: return 3 + + +if a: # we preserve comments, too! + b +elif c: # but not on the second branch + b + + +if a: b # here's a comment +elif c: b diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs index 66755f60e03919..7aa95363007612 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs @@ -1,14 +1,13 @@ -use anyhow::Result; use std::borrow::Cow; -use ast::whitespace::indentation; +use anyhow::Result; + use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast as ast; use ruff_python_ast::comparable::ComparableStmt; use ruff_python_ast::parenthesize::parenthesized_range; use ruff_python_ast::stmt_if::{if_elif_branches, IfElifBranch}; -use ruff_python_ast::Expr; +use ruff_python_ast::{self as ast, Expr}; use ruff_python_index::Indexer; use ruff_python_trivia::{SimpleTokenKind, SimpleTokenizer}; use ruff_source_file::Locator; @@ -137,24 +136,10 @@ fn merge_branches( return Err(anyhow::anyhow!("Expected colon after test")); }; - let main_edit = if let Some(following_branch_comment) = - following_branch_tokenizer.find(|token| token.kind == SimpleTokenKind::Comment) - { - let indentation = - indentation(locator, following_branch.body.first().unwrap()).unwrap_or(""); - Edit::range_replacement( - format!("{indentation}"), - TextRange::new( - locator.full_line_end(current_branch_colon.end()), - following_branch_comment.start(), - ), - ) - } else { - Edit::deletion( - locator.full_line_end(current_branch_colon.end()), - locator.full_line_end(following_branch_colon.end()), - ) - }; + let main_edit = Edit::deletion( + locator.full_line_end(current_branch_colon.end()), + locator.full_line_end(following_branch_colon.end()), + ); // If the test isn't parenthesized, consider parenthesizing it. let following_branch_test = if let Some(range) = parenthesized_range( diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap index de04172958297a..781dd3a54a4240 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM114_SIM114.py.snap @@ -20,7 +20,7 @@ SIM114.py:7:1: SIM114 Combine `if` branches using logical `or` operator 6 | 7 | / if a: # we preserve comments, too! 8 | | b - 9 | | elif c: # yes, even this one! + 9 | | elif c: # but not on the second branch 10 | | b | |_____^ SIM114 11 | @@ -229,4 +229,22 @@ SIM114.py:132:5: SIM114 Combine `if` branches using logical `or` operator | = help: Combine `if` branches +SIM114.py:138:1: SIM114 Combine `if` branches using logical `or` operator + | +138 | / if a: # we preserve comments, too! +139 | | b +140 | | elif c: # but not on the second branch +141 | | b + | |_____^ SIM114 + | + = help: Combine `if` branches + +SIM114.py:144:1: SIM114 Combine `if` branches using logical `or` operator + | +144 | / if a: b # here's a comment +145 | | elif c: b + | |_________^ SIM114 + | + = help: Combine `if` branches + diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap index 9a565333d4bc58..4f62aed944f702 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM114_SIM114.py.snap @@ -30,7 +30,7 @@ SIM114.py:7:1: SIM114 [*] Combine `if` branches using logical `or` operator 6 | 7 | / if a: # we preserve comments, too! 8 | | b - 9 | | elif c: # yes, even this one! + 9 | | elif c: # but not on the second branch 10 | | b | |_____^ SIM114 11 | @@ -44,12 +44,11 @@ SIM114.py:7:1: SIM114 [*] Combine `if` branches using logical `or` operator 6 6 | 7 |-if a: # we preserve comments, too! 8 |- b -9 |-elif c: # yes, even this one! +9 |-elif c: # but not on the second branch 7 |+if a or c: # we preserve comments, too! - 8 |+ # yes, even this one! -10 9 | b -11 10 | -12 11 | if x == 1: +10 8 | b +11 9 | +12 10 | if x == 1: SIM114.py:12:1: SIM114 [*] Combine `if` branches using logical `or` operator | @@ -68,7 +67,7 @@ SIM114.py:12:1: SIM114 [*] Combine `if` branches using logical `or` operator = help: Combine `if` branches ℹ Safe fix -9 9 | elif c: # yes, even this one! +9 9 | elif c: # but not on the second branch 10 10 | b 11 11 | 12 |-if x == 1: @@ -403,5 +402,45 @@ SIM114.py:132:5: SIM114 [*] Combine `if` branches using logical `or` operator 134 |- elif a := 1: 132 |+ if a > b or (a := 1): # end-of-line 135 133 | return 3 +136 134 | +137 135 | + +SIM114.py:138:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +138 | / if a: # we preserve comments, too! +139 | | b +140 | | elif c: # but not on the second branch +141 | | b + | |_____^ SIM114 + | + = help: Combine `if` branches + +ℹ Safe fix +135 135 | return 3 +136 136 | +137 137 | +138 |-if a: # we preserve comments, too! +139 |- b +140 |-elif c: # but not on the second branch + 138 |+if a or c: # we preserve comments, too! +141 139 | b +142 140 | +143 141 | + +SIM114.py:144:1: SIM114 [*] Combine `if` branches using logical `or` operator + | +144 | / if a: b # here's a comment +145 | | elif c: b + | |_________^ SIM114 + | + = help: Combine `if` branches + +ℹ Safe fix +141 141 | b +142 142 | +143 143 | +144 |-if a: b # here's a comment +145 |-elif c: b + 144 |+if a or c: b # here's a comment From fc05bb9ea9feb4b43f8dbf297f767933a3ca19e0 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 21 Jan 2024 23:32:06 -0500 Subject: [PATCH 7/7] Fix clippy --- .../src/rules/flake8_simplify/rules/if_with_same_arms.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs index 7aa95363007612..3b5be38e31641d 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/if_with_same_arms.rs @@ -99,7 +99,7 @@ pub(crate) fn if_with_same_arms(checker: &mut Checker, stmt_if: &ast::StmtIf) { merge_branches( stmt_if, ¤t_branch, - &following_branch, + following_branch, checker.locator(), checker.indexer(), )