Implement flake8-simplify SIM107 (#1650)

README.md

@@ -968,6 +968,7 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/0.19.3/
 | SIM117 | MultipleWithStatements | Use a single `with` statement with multiple contexts instead of nested `with` statements |  |
 | 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` |  |
 | SIM118 | KeyInDict | Use `key in dict` instead of `key in dict.keys()` | 🛠 |
 | SIM220 | AAndNotA | Use `False` instead of `... and not ...` | 🛠 |
 | SIM221 | AOrNotA | Use `True` instead of `... or not ...` | 🛠 |

resources/test/fixtures/flake8_simplify/SIM107.py

@@ -0,0 +1,22 @@
+# Bad: This example returns 3!
+def foo():
+    try:
+        1 / 0
+        return "1"
+    except:
+        return "2"
+    finally:
+        return "3"
+
+
+# Good
+def foo():
+    return_value = None
+    try:
+        1 / 0
+        return_value = "1"
+    except:
+        return_value = "2"
+    finally:
+        return_value = "3"  # you might also want to check if "except" happened
+    return return_value

ruff.schema.json

@@ -932,6 +932,7 @@
         "SIM10",
         "SIM102",
         "SIM105",
+        "SIM107",
         "SIM11",
         "SIM117",
         "SIM118",

src/checkers/ast.rs

@@ -1259,6 +1259,7 @@ where
                 }
             }
             StmtKind::Try {
+                body,
                 handlers,
                 orelse,
                 finalbody,
@@ -1293,6 +1294,11 @@ where
                         self, stmt, handlers, orelse, finalbody,
                     );
                 }
+                if self.settings.enabled.contains(&CheckCode::SIM107) {
+                    flake8_simplify::plugins::return_in_try_except_finally(
+                        self, body, handlers, finalbody,
+                    );
+                }
             }
             StmtKind::Assign { targets, value, .. } => {
                 if self.settings.enabled.contains(&CheckCode::E731) {

src/flake8_simplify/mod.rs

@@ -14,6 +14,7 @@ mod tests {
 
     #[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")]
     #[test_case(CheckCode::SIM117, Path::new("SIM117.py"); "SIM117")]
     #[test_case(CheckCode::SIM118, Path::new("SIM118.py"); "SIM118")]
     #[test_case(CheckCode::SIM220, Path::new("SIM220.py"); "SIM220")]

src/flake8_simplify/plugins/mod.rs

@@ -2,11 +2,14 @@ pub use ast_bool_op::{a_and_not_a, a_or_not_a, and_false, or_true};
 pub use ast_if::nested_if_statements;
 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;
 pub use use_contextlib_suppress::use_contextlib_suppress;
 pub use yoda_conditions::yoda_conditions;
+
 mod ast_bool_op;
 mod ast_if;
 mod ast_with;
 mod key_in_dict;
+mod return_in_try_except_finally;
 mod use_contextlib_suppress;
 mod yoda_conditions;

src/flake8_simplify/plugins/return_in_try_except_finally.rs

@@ -0,0 +1,34 @@
+use rustpython_ast::{Excepthandler, ExcepthandlerKind, Stmt, StmtKind};
+
+use crate::ast::types::Range;
+use crate::checkers::ast::Checker;
+use crate::registry::{Check, CheckKind};
+
+fn find_return(stmts: &[Stmt]) -> Option<&Stmt> {
+    stmts
+        .iter()
+        .find(|stmt| matches!(stmt.node, StmtKind::Return { .. }))
+}
+
+/// SIM107
+pub fn return_in_try_except_finally(
+    checker: &mut Checker,
+    body: &[Stmt],
+    handlers: &[Excepthandler],
+    finalbody: &[Stmt],
+) {
+    let try_has_return = find_return(body).is_some();
+    let except_has_return = handlers.iter().any(|handler| {
+        let ExcepthandlerKind::ExceptHandler { body, .. } = &handler.node;
+        find_return(body).is_some()
+    });
+
+    if let Some(finally_return) = find_return(finalbody) {
+        if try_has_return || except_has_return {
+            checker.add_check(Check::new(
+                CheckKind::ReturnInTryExceptFinally,
+                Range::from_located(finally_return),
+            ));
+        }
+    }
+}

src/flake8_simplify/snapshots/ruff__flake8_simplify__tests__SIM107_SIM107.py.snap

@@ -0,0 +1,14 @@
+---
+source: src/flake8_simplify/mod.rs
+expression: checks
+---
+- kind: ReturnInTryExceptFinally
+  location:
+    row: 9
+    column: 8
+  end_location:
+    row: 9
+    column: 18
+  fix: ~
+  parent: ~
+

src/registry.rs

@@ -220,6 +220,7 @@ pub enum CheckCode {
     SIM117,
     SIM102,
     SIM105,
+    SIM107,
     SIM118,
     SIM220,
     SIM221,
@@ -882,7 +883,6 @@ pub enum CheckKind {
     UnaryPrefixIncrement,
     UnreliableCallableCheck,
     UnusedLoopControlVariable(String),
-    UseContextlibSuppress(String),
     UselessComparison,
     UselessContextlibSuppress,
     UselessExpression,
@@ -956,6 +956,8 @@ pub enum CheckKind {
     // flake8-simplify
     MultipleWithStatements,
     NestedIfStatements,
+    UseContextlibSuppress(String),
+    ReturnInTryExceptFinally,
     AAndNotA(String),
     AOrNotA(String),
     KeyInDict(String, String),
@@ -1391,6 +1393,7 @@ impl CheckCode {
             // flake8-simplify
             CheckCode::SIM102 => CheckKind::NestedIfStatements,
             CheckCode::SIM105 => CheckKind::UseContextlibSuppress("...".to_string()),
+            CheckCode::SIM107 => CheckKind::ReturnInTryExceptFinally,
             CheckCode::SIM117 => CheckKind::MultipleWithStatements,
             CheckCode::SIM118 => CheckKind::KeyInDict("key".to_string(), "dict".to_string()),
             CheckCode::SIM220 => CheckKind::AAndNotA("...".to_string()),
@@ -1926,6 +1929,7 @@ impl CheckCode {
             // flake8-simplify
             CheckCode::SIM102 => CheckCategory::Flake8Simplify,
             CheckCode::SIM105 => CheckCategory::Flake8Simplify,
+            CheckCode::SIM107 => CheckCategory::Flake8Simplify,
             CheckCode::SIM117 => CheckCategory::Flake8Simplify,
             CheckCode::SIM118 => CheckCategory::Flake8Simplify,
             CheckCode::SIM220 => CheckCategory::Flake8Simplify,
@@ -2179,14 +2183,15 @@ impl CheckKind {
             CheckKind::SysVersionCmpStr10 => &CheckCode::YTT302,
             CheckKind::SysVersionSlice1Referenced => &CheckCode::YTT303,
             // flake8-simplify
+            CheckKind::NestedIfStatements => &CheckCode::SIM102,
+            CheckKind::UseContextlibSuppress(..) => &CheckCode::SIM105,
+            CheckKind::ReturnInTryExceptFinally => &CheckCode::SIM107,
+            CheckKind::MultipleWithStatements => &CheckCode::SIM117,
+            CheckKind::KeyInDict(..) => &CheckCode::SIM118,
             CheckKind::AAndNotA(..) => &CheckCode::SIM220,
             CheckKind::AOrNotA(..) => &CheckCode::SIM221,
-            CheckKind::AndFalse => &CheckCode::SIM223,
-            CheckKind::KeyInDict(..) => &CheckCode::SIM118,
-            CheckKind::MultipleWithStatements => &CheckCode::SIM117,
-            CheckKind::NestedIfStatements => &CheckCode::SIM102,
             CheckKind::OrTrue => &CheckCode::SIM222,
-            CheckKind::UseContextlibSuppress(..) => &CheckCode::SIM105,
+            CheckKind::AndFalse => &CheckCode::SIM223,
             CheckKind::YodaConditions(..) => &CheckCode::SIM300,
             // pyupgrade
             CheckKind::ConvertNamedTupleFunctionalToClass(..) => &CheckCode::UP014,
@@ -2698,9 +2703,6 @@ impl CheckKind {
             CheckKind::FStringDocstring => "f-string used as docstring. This will be interpreted \
                                             by python as a joined string rather than a docstring."
                 .to_string(),
-            CheckKind::UseContextlibSuppress(exception) => {
-                format!("Use `contextlib.suppress({exception})` instead of try-except-pass")
-            }
             CheckKind::UselessContextlibSuppress => {
                 "No arguments passed to `contextlib.suppress`. No exceptions will be suppressed \
                  and therefore this context manager is redundant"
@@ -2945,6 +2947,12 @@ impl CheckKind {
                 "`sys.version[:1]` referenced (python10), use `sys.version_info`".to_string()
             }
             // flake8-simplify
+            CheckKind::UseContextlibSuppress(exception) => {
+                format!("Use `contextlib.suppress({exception})` instead of try-except-pass")
+            }
+            CheckKind::ReturnInTryExceptFinally => {
+                "Don't use `return` in `try`/`except` and `finally`".to_string()
+            }
             CheckKind::AAndNotA(name) => format!("Use `False` instead of `{name} and not {name}`"),
             CheckKind::AOrNotA(name) => format!("Use `True` instead of `{name} or not {name}`"),
             CheckKind::AndFalse => "Use `False` instead of `... and False`".to_string(),

src/registry_gen.rs

@@ -520,6 +520,7 @@ pub enum CheckCodePrefix {
     SIM10,
     SIM102,
     SIM105,
+    SIM107,
     SIM11,
     SIM117,
     SIM118,
@@ -811,6 +812,7 @@ impl CheckCodePrefix {
                 CheckCode::SIM117,
                 CheckCode::SIM102,
                 CheckCode::SIM105,
+                CheckCode::SIM107,
                 CheckCode::SIM118,
                 CheckCode::SIM220,
                 CheckCode::SIM221,
@@ -2636,6 +2638,7 @@ impl CheckCodePrefix {
                 CheckCode::SIM117,
                 CheckCode::SIM102,
                 CheckCode::SIM105,
+                CheckCode::SIM107,
                 CheckCode::SIM118,
                 CheckCode::SIM220,
                 CheckCode::SIM221,
@@ -2647,11 +2650,13 @@ impl CheckCodePrefix {
                 CheckCode::SIM117,
                 CheckCode::SIM102,
                 CheckCode::SIM105,
+                CheckCode::SIM107,
                 CheckCode::SIM118,
             ],
-            CheckCodePrefix::SIM10 => vec![CheckCode::SIM102, CheckCode::SIM105],
+            CheckCodePrefix::SIM10 => vec![CheckCode::SIM102, CheckCode::SIM105, CheckCode::SIM107],
             CheckCodePrefix::SIM102 => vec![CheckCode::SIM102],
             CheckCodePrefix::SIM105 => vec![CheckCode::SIM105],
+            CheckCodePrefix::SIM107 => vec![CheckCode::SIM107],
             CheckCodePrefix::SIM11 => vec![CheckCode::SIM117, CheckCode::SIM118],
             CheckCodePrefix::SIM117 => vec![CheckCode::SIM117],
             CheckCodePrefix::SIM118 => vec![CheckCode::SIM118],
@@ -3627,6 +3632,7 @@ impl CheckCodePrefix {
             CheckCodePrefix::SIM10 => SuffixLength::Two,
             CheckCodePrefix::SIM102 => SuffixLength::Three,
             CheckCodePrefix::SIM105 => SuffixLength::Three,
+            CheckCodePrefix::SIM107 => SuffixLength::Three,
             CheckCodePrefix::SIM11 => SuffixLength::Two,
             CheckCodePrefix::SIM117 => SuffixLength::Three,
             CheckCodePrefix::SIM118 => SuffixLength::Three,