Implement flake8-simplify SIM103 (#1712)

Ref #998

Co-authored-by: Charlie Marsh <[email protected]>

README.md

@@ -971,9 +971,10 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/0.19.3/
 | ---- | ---- | ------- | --- |
 | SIM101 | DuplicateIsinstanceCall | Multiple `isinstance` calls for `...`, merge into a single call | 🛠 |
 | SIM102 | NestedIfStatements | Use a single `if` statement instead of nested `if` statements |  |
+| SIM103 | ReturnBoolConditionDirectly | Return the condition `...` directly | 🛠 |
 | SIM105 | UseContextlibSuppress | Use `contextlib.suppress(...)` instead of try-except-pass |  |
 | SIM107 | ReturnInTryExceptFinally | Don't use `return` in `try`/`except` and `finally` |  |
-| SIM108 | UseTernaryOperator | Use ternary operator `..` instead of if-else-block | 🛠 |
+| SIM108 | UseTernaryOperator | Use ternary operator `...` instead of if-else-block | 🛠 |
 | SIM109 | CompareWithTuple | Use `value in (..., ...)` instead of `value == ... or value == ...` | 🛠 |
 | SIM110 | ConvertLoopToAny | Use `return any(x for x in y)` instead of `for` loop | 🛠 |
 | SIM111 | ConvertLoopToAll | Use `return all(x for x in y)` instead of `for` loop | 🛠 |

resources/test/fixtures/flake8_simplify/SIM103.py

@@ -0,0 +1,20 @@
+def f():
+    if a:  # SIM103
+        return True
+    else:
+        return False
+
+
+def f():
+    if a:  # OK
+        foo()
+        return True
+    else:
+        return False
+
+
+def f():
+    if a:  # OK
+        return "foo"
+    else:
+        return False

ruff.schema.json

@@ -941,6 +941,7 @@
         "SIM10",
         "SIM101",
         "SIM102",
+        "SIM103",
         "SIM105",
         "SIM107",
         "SIM108",

src/checkers/ast.rs

@@ -1184,6 +1184,9 @@ where
                 if self.settings.enabled.contains(&CheckCode::SIM102) {
                     flake8_simplify::plugins::nested_if_statements(self, stmt);
                 }
+                if self.settings.enabled.contains(&CheckCode::SIM103) {
+                    flake8_simplify::plugins::return_bool_condition_directly(self, stmt);
+                }
                 if self.settings.enabled.contains(&CheckCode::SIM108) {
                     flake8_simplify::plugins::use_ternary_operator(
                         self,

src/flake8_simplify/mod.rs

@@ -14,8 +14,10 @@ mod tests {
 
     #[test_case(CheckCode::SIM101, Path::new("SIM101.py"); "SIM101")]
     #[test_case(CheckCode::SIM102, Path::new("SIM102.py"); "SIM102")]
+    #[test_case(CheckCode::SIM103, Path::new("SIM103.py"); "SIM103")]
     #[test_case(CheckCode::SIM105, Path::new("SIM105.py"); "SIM105")]
     #[test_case(CheckCode::SIM107, Path::new("SIM107.py"); "SIM107")]
+    #[test_case(CheckCode::SIM108, Path::new("SIM108.py"); "SIM108")]
     #[test_case(CheckCode::SIM109, Path::new("SIM109.py"); "SIM109")]
     #[test_case(CheckCode::SIM110, Path::new("SIM110.py"); "SIM110")]
     #[test_case(CheckCode::SIM111, Path::new("SIM111.py"); "SIM111")]
@@ -29,7 +31,6 @@ mod tests {
     #[test_case(CheckCode::SIM222, Path::new("SIM222.py"); "SIM222")]
     #[test_case(CheckCode::SIM223, Path::new("SIM223.py"); "SIM223")]
     #[test_case(CheckCode::SIM300, Path::new("SIM300.py"); "SIM300")]
-    #[test_case(CheckCode::SIM108, Path::new("SIM108.py"); "SIM108")]
     fn checks(check_code: CheckCode, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", check_code.as_ref(), path.to_string_lossy());
         let checks = test_path(

src/flake8_simplify/plugins/ast_if.rs

@@ -1,6 +1,6 @@
 use rustpython_ast::{Constant, Expr, ExprKind, Stmt, StmtKind};
 
-use crate::ast::helpers::{create_expr, create_stmt, unparse_stmt};
+use crate::ast::helpers::{create_expr, create_stmt, unparse_expr, unparse_stmt};
 use crate::ast::types::Range;
 use crate::autofix::Fix;
 use crate::checkers::ast::Checker;
@@ -65,6 +65,45 @@ pub fn nested_if_statements(checker: &mut Checker, stmt: &Stmt) {
     ));
 }
 
+fn is_one_line_return_bool(stmts: &[Stmt]) -> bool {
+    if stmts.len() != 1 {
+        return false;
+    }
+    let StmtKind::Return { value } = &stmts[0].node else {
+        return false;
+    };
+    let Some(ExprKind::Constant { value, .. }) = value.as_ref().map(|value| &value.node) else {
+        return false;
+    };
+    matches!(value, Constant::Bool(_))
+}
+
+/// SIM103
+pub fn return_bool_condition_directly(checker: &mut Checker, stmt: &Stmt) {
+    let StmtKind::If { test, body, orelse } = &stmt.node else {
+        return;
+    };
+    if !(is_one_line_return_bool(body) && is_one_line_return_bool(orelse)) {
+        return;
+    }
+    let condition = unparse_expr(test, checker.style);
+    let mut check = Check::new(
+        CheckKind::ReturnBoolConditionDirectly(condition),
+        Range::from_located(stmt),
+    );
+    if checker.patch(&CheckCode::SIM103) {
+        let return_stmt = create_stmt(StmtKind::Return {
+            value: Some(test.clone()),
+        });
+        check.amend(Fix::replacement(
+            unparse_stmt(&return_stmt, checker.style),
+            stmt.location,
+            stmt.end_location.unwrap(),
+        ));
+    }
+    checker.checks.push(check);
+}
+
 fn ternary(target_var: &Expr, body_value: &Expr, test: &Expr, orelse_value: &Expr) -> Stmt {
     create_stmt(StmtKind::Assign {
         targets: vec![target_var.clone()],

src/flake8_simplify/plugins/mod.rs

@@ -2,7 +2,7 @@ pub use ast_bool_op::{
     a_and_not_a, a_or_not_a, and_false, compare_with_tuple, duplicate_isinstance_call, or_true,
 };
 pub use ast_for::convert_loop_to_any_all;
-pub use ast_if::{nested_if_statements, use_ternary_operator};
+pub use ast_if::{nested_if_statements, return_bool_condition_directly, use_ternary_operator};
 pub use ast_with::multiple_with_statements;
 pub use key_in_dict::{key_in_dict_compare, key_in_dict_for};
 pub use return_in_try_except_finally::return_in_try_except_finally;

src/flake8_simplify/snapshots/ruff__flake8_simplify__tests__SIM103_SIM103.py.snap

@@ -0,0 +1,22 @@
+---
+source: src/flake8_simplify/mod.rs
+expression: checks
+---
+- kind:
+    ReturnBoolConditionDirectly: a
+  location:
+    row: 2
+    column: 4
+  end_location:
+    row: 5
+    column: 20
+  fix:
+    content: return a
+    location:
+      row: 2
+      column: 4
+    end_location:
+      row: 5
+      column: 20
+  parent: ~
+

src/registry.rs

@@ -220,6 +220,7 @@ pub enum CheckCode {
     // flake8-simplify
     SIM101,
     SIM102,
+    SIM103,
     SIM105,
     SIM107,
     SIM108,
@@ -967,6 +968,7 @@ pub enum CheckKind {
     SysVersionCmpStr10,
     SysVersionSlice1Referenced,
     // flake8-simplify
+    ReturnBoolConditionDirectly(String),
     UseTernaryOperator(String),
     CompareWithTuple(String, Vec<String>, String),
     DuplicateIsinstanceCall(String),
@@ -1418,9 +1420,10 @@ impl CheckCode {
             // flake8-simplify
             CheckCode::SIM101 => CheckKind::DuplicateIsinstanceCall("...".to_string()),
             CheckCode::SIM102 => CheckKind::NestedIfStatements,
+            CheckCode::SIM103 => CheckKind::ReturnBoolConditionDirectly("...".to_string()),
             CheckCode::SIM105 => CheckKind::UseContextlibSuppress("...".to_string()),
             CheckCode::SIM107 => CheckKind::ReturnInTryExceptFinally,
-            CheckCode::SIM108 => CheckKind::UseTernaryOperator("..".to_string()),
+            CheckCode::SIM108 => CheckKind::UseTernaryOperator("...".to_string()),
             CheckCode::SIM109 => CheckKind::CompareWithTuple(
                 "value".to_string(),
                 vec!["...".to_string(), "...".to_string()],
@@ -1978,6 +1981,7 @@ impl CheckCode {
             CheckCode::S501 => CheckCategory::Flake8Bandit,
             CheckCode::S506 => CheckCategory::Flake8Bandit,
             // flake8-simplify
+            CheckCode::SIM103 => CheckCategory::Flake8Simplify,
             CheckCode::SIM101 => CheckCategory::Flake8Simplify,
             CheckCode::SIM102 => CheckCategory::Flake8Simplify,
             CheckCode::SIM105 => CheckCategory::Flake8Simplify,
@@ -2256,6 +2260,7 @@ impl CheckKind {
             CheckKind::NegateNotEqualOp(..) => &CheckCode::SIM202,
             CheckKind::NestedIfStatements => &CheckCode::SIM102,
             CheckKind::OrTrue => &CheckCode::SIM222,
+            CheckKind::ReturnBoolConditionDirectly(..) => &CheckCode::SIM103,
             CheckKind::ReturnInTryExceptFinally => &CheckCode::SIM107,
             CheckKind::UseContextlibSuppress(..) => &CheckCode::SIM105,
             CheckKind::UseTernaryOperator(..) => &CheckCode::SIM108,
@@ -3018,6 +3023,9 @@ impl CheckKind {
                 "`sys.version[:1]` referenced (python10), use `sys.version_info`".to_string()
             }
             // flake8-simplify
+            CheckKind::ReturnBoolConditionDirectly(cond) => {
+                format!("Return the condition `{cond}` directly")
+            }
             CheckKind::CompareWithTuple(value, values, or_op) => {
                 let values = values.join(", ");
                 format!("Use `{value} in ({values})` instead of `{or_op}`")
@@ -3774,6 +3782,7 @@ impl CheckKind {
             | CheckKind::RemoveSixCompat
             | CheckKind::ReplaceStdoutStderr
             | CheckKind::ReplaceUniversalNewlines
+            | CheckKind::ReturnBoolConditionDirectly(..)
             | CheckKind::RewriteCElementTree
             | CheckKind::RewriteListComprehension
             | CheckKind::RewriteMockImport(..)
@@ -3998,6 +4007,9 @@ impl CheckKind {
                 Some(format!("Replace with `except {name}`"))
             }
             CheckKind::RemoveSixCompat => Some("Remove `six` usage".to_string()),
+            CheckKind::ReturnBoolConditionDirectly(cond) => {
+                Some(format!("Replace with `return {cond}`"))
+            }
             CheckKind::SectionNameEndsInColon(name) => Some(format!("Add colon to \"{name}\"")),
             CheckKind::SectionNotOverIndented(name) => {
                 Some(format!("Remove over-indentation from \"{name}\""))