From 526abebbaec7ac2ed86144bbfa0ae8f69d8fb054 Mon Sep 17 00:00:00 2001 From: Ottavio Hartman Date: Sun, 17 Mar 2024 20:15:28 -0400 Subject: [PATCH] [`flake8-simplify`] Detect implicit `else` cases in `needless-bool` (`SIM103`) (#10414) Fixes #10402 ## Summary For SIM103, detect and simplify the following case: [playground link](https://play.ruff.rs/d98570aa-b180-495b-8600-5c4c3fd02526) ```python def main(): if foo > 5: return True return False ``` ## Test Plan Unit tested only. --- .../test/fixtures/flake8_simplify/SIM103.py | 19 ++ .../src/checkers/ast/analyze/statement.rs | 2 +- .../src/rules/flake8_simplify/mod.rs | 1 + .../flake8_simplify/rules/needless_bool.rs | 77 ++++++- ...ke8_simplify__tests__SIM103_SIM103.py.snap | 2 - ...ify__tests__preview__SIM103_SIM103.py.snap | 189 ++++++++++++++++++ 6 files changed, 280 insertions(+), 10 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM103_SIM103.py.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py index 98172e597b450..85f00aec21530 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py @@ -84,3 +84,22 @@ def bool(): return True else: return False + + +### +# Positive cases (preview) +### + + +def f(): + # SIM103 + if a: + return True + return False + + +def f(): + # SIM103 + if a: + return False + return True diff --git a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs index d4f6287bf95e9..e3bb10fa67c68 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/statement.rs @@ -1079,7 +1079,7 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { flake8_simplify::rules::if_with_same_arms(checker, if_); } if checker.enabled(Rule::NeedlessBool) { - flake8_simplify::rules::needless_bool(checker, if_); + flake8_simplify::rules::needless_bool(checker, stmt); } if checker.enabled(Rule::IfElseBlockInsteadOfDictLookup) { flake8_simplify::rules::if_else_block_instead_of_dict_lookup(checker, if_); diff --git a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs index e68c9d6b471ca..c5243428c2992 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/mod.rs @@ -56,6 +56,7 @@ mod tests { Ok(()) } + #[test_case(Rule::NeedlessBool, Path::new("SIM103.py"))] #[test_case(Rule::YodaConditions, Path::new("SIM300.py"))] fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs index 656ed70059bd7..93f533de3f88e 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs +++ b/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs @@ -1,5 +1,6 @@ use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::traversal; use ruff_python_ast::{self as ast, Arguments, ElifElseClause, Expr, ExprContext, Stmt}; use ruff_python_semantic::analyze::typing::{is_sys_version_block, is_type_checking_block}; use ruff_text_size::{Ranged, TextRange}; @@ -16,7 +17,7 @@ use crate::fix::snippet::SourceCodeSnippet; /// /// ## Example /// ```python -/// if foo: +/// if x > 0: /// return True /// else: /// return False @@ -24,11 +25,20 @@ use crate::fix::snippet::SourceCodeSnippet; /// /// Use instead: /// ```python -/// return bool(foo) +/// return x > 0 +/// ``` +/// +/// In [preview], this rule will also flag implicit `else` cases, as in: +/// ```python +/// if x > 0: +/// return True +/// return False /// ``` /// /// ## References /// - [Python documentation: Truth Value Testing](https://docs.python.org/3/library/stdtypes.html#truth-value-testing) +/// +/// [preview]: https://docs.astral.sh/ruff/preview/ #[violation] pub struct NeedlessBool { condition: SourceCodeSnippet, @@ -62,23 +72,41 @@ impl Violation for NeedlessBool { } /// SIM103 -pub(crate) fn needless_bool(checker: &mut Checker, stmt_if: &ast::StmtIf) { +pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) { + let Stmt::If(stmt_if) = stmt else { return }; let ast::StmtIf { test: if_test, body: if_body, elif_else_clauses, - range: _, + .. } = stmt_if; // Extract an `if` or `elif` (that returns) followed by an else (that returns the same value) let (if_test, if_body, else_body, range) = match elif_else_clauses.as_slice() { - // if-else case + // if-else case: + // ```python + // if x > 0: + // return True + // else: + // return False + // ``` [ElifElseClause { body: else_body, test: None, .. - }] => (if_test.as_ref(), if_body, else_body, stmt_if.range()), + }] => ( + if_test.as_ref(), + if_body, + else_body.as_slice(), + stmt_if.range(), + ), // elif-else case + // ```python + // if x > 0: + // return True + // elif x < 0: + // return False + // ``` [.., ElifElseClause { body: elif_body, test: Some(elif_test), @@ -90,12 +118,47 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt_if: &ast::StmtIf) { }] => ( elif_test, elif_body, - else_body, + else_body.as_slice(), TextRange::new(elif_range.start(), else_range.end()), ), + // if-implicit-else case: + // ```python + // if x > 0: + // return True + // return False + // ``` + [] if checker.settings.preview.is_enabled() => { + // Fetching the next sibling is expensive, so do some validation early. + if is_one_line_return_bool(if_body).is_none() { + return; + } + + // Fetch the next sibling statement. + let Some(next_stmt) = checker + .semantic() + .current_statement_parent() + .and_then(|parent| traversal::suite(stmt, parent)) + .and_then(|suite| traversal::next_sibling(stmt, suite)) + else { + return; + }; + + // If the next sibling is not a return statement, abort. + if !next_stmt.is_return_stmt() { + return; + } + + ( + if_test.as_ref(), + if_body, + std::slice::from_ref(next_stmt), + TextRange::new(stmt_if.start(), next_stmt.end()), + ) + } _ => return, }; + // Both branches must be one-liners that return a boolean. let (Some(if_return), Some(else_return)) = ( is_one_line_return_bool(if_body), is_one_line_return_bool(else_body), diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap index 174d22184c7b4..2ed80d20b0880 100644 --- a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap @@ -142,5 +142,3 @@ SIM103.py:83:5: SIM103 Return the condition `a` directly | |____________________^ SIM103 | = help: Inline condition - - diff --git a/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM103_SIM103.py.snap b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM103_SIM103.py.snap new file mode 100644 index 0000000000000..868129a6d16bf --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM103_SIM103.py.snap @@ -0,0 +1,189 @@ +--- +source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs +--- +SIM103.py:3:5: SIM103 [*] Return the condition `a` directly + | +1 | def f(): +2 | # SIM103 +3 | if a: + | _____^ +4 | | return True +5 | | else: +6 | | return False + | |____________________^ SIM103 + | + = help: Replace with `return bool(a)` + +ℹ Unsafe fix +1 1 | def f(): +2 2 | # SIM103 +3 |- if a: +4 |- return True +5 |- else: +6 |- return False + 3 |+ return bool(a) +7 4 | +8 5 | +9 6 | def f(): + +SIM103.py:11:5: SIM103 [*] Return the condition `a == b` directly + | + 9 | def f(): +10 | # SIM103 +11 | if a == b: + | _____^ +12 | | return True +13 | | else: +14 | | return False + | |____________________^ SIM103 + | + = help: Replace with `return a == b` + +ℹ Unsafe fix +8 8 | +9 9 | def f(): +10 10 | # SIM103 +11 |- if a == b: +12 |- return True +13 |- else: +14 |- return False + 11 |+ return a == b +15 12 | +16 13 | +17 14 | def f(): + +SIM103.py:21:5: SIM103 [*] Return the condition `b` directly + | +19 | if a: +20 | return 1 +21 | elif b: + | _____^ +22 | | return True +23 | | else: +24 | | return False + | |____________________^ SIM103 + | + = help: Replace with `return bool(b)` + +ℹ Unsafe fix +18 18 | # SIM103 +19 19 | if a: +20 20 | return 1 +21 |- elif b: +22 |- return True +23 |- else: +24 |- return False + 21 |+ return bool(b) +25 22 | +26 23 | +27 24 | def f(): + +SIM103.py:32:9: SIM103 [*] Return the condition `b` directly + | +30 | return 1 +31 | else: +32 | if b: + | _________^ +33 | | return True +34 | | else: +35 | | return False + | |________________________^ SIM103 + | + = help: Replace with `return bool(b)` + +ℹ Unsafe fix +29 29 | if a: +30 30 | return 1 +31 31 | else: +32 |- if b: +33 |- return True +34 |- else: +35 |- return False + 32 |+ return bool(b) +36 33 | +37 34 | +38 35 | def f(): + +SIM103.py:57:5: SIM103 [*] Return the condition `a` directly + | +55 | def f(): +56 | # SIM103 (but not fixable) +57 | if a: + | _____^ +58 | | return False +59 | | else: +60 | | return True + | |___________________^ SIM103 + | + = help: Replace with `return not a` + +ℹ Unsafe fix +54 54 | +55 55 | def f(): +56 56 | # SIM103 (but not fixable) +57 |- if a: +58 |- return False +59 |- else: +60 |- return True + 57 |+ return not a +61 58 | +62 59 | +63 60 | def f(): + +SIM103.py:83:5: SIM103 Return the condition `a` directly + | +81 | def bool(): +82 | return False +83 | if a: + | _____^ +84 | | return True +85 | | else: +86 | | return False + | |____________________^ SIM103 + | + = help: Inline condition + +SIM103.py:96:5: SIM103 [*] Return the condition `a` directly + | +94 | def f(): +95 | # SIM103 +96 | if a: + | _____^ +97 | | return True +98 | | return False + | |________________^ SIM103 + | + = help: Replace with `return bool(a)` + +ℹ Unsafe fix +93 93 | +94 94 | def f(): +95 95 | # SIM103 +96 |- if a: +97 |- return True +98 |- return False + 96 |+ return bool(a) +99 97 | +100 98 | +101 99 | def f(): + +SIM103.py:103:5: SIM103 [*] Return the condition `a` directly + | +101 | def f(): +102 | # SIM103 +103 | if a: + | _____^ +104 | | return False +105 | | return True + | |_______________^ SIM103 + | + = help: Replace with `return not a` + +ℹ Unsafe fix +100 100 | +101 101 | def f(): +102 102 | # SIM103 +103 |- if a: +104 |- return False +105 |- return True + 103 |+ return not a