Warn about continue/return/break in finally block (#100)

* Refactor check_for_b901 to use a recursive loop instead of list append.

A loop is more resource effective than adding and removing elements from lists.

* B012: report warning when using return, continue or break inside finally blocks

README.rst

@@ -100,6 +100,11 @@ using ``setattr`` if you know the attribute name ahead of time.
 **B011**: Do not call `assert False` since `python -O` removes these calls.
 Instead callers should `raise AssertionError()`.
 
+**B012**: Use of `break`, `continue` or `return` inside `finally` blocks will
+silence exceptions or override return values from the `try` or `except` blocks.
+To silence an exception, do it explicitly in the `except` block. To properly use
+a `break`, `continue` or `return` refactor your code so these statements are not
+in the `finally` block.
 
 Python 3 compatibility warnings
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

bugbear.py

@@ -226,6 +226,10 @@ def visit_ClassDef(self, node):
         self.check_for_b903(node)
         self.generic_visit(node)
 
+    def visit_Try(self, node):
+        self.check_for_b012(node)
+        self.generic_visit(node)
+
     def compose_call_path(self, node):
         if isinstance(node, ast.Attribute):
             yield from self.compose_call_path(node.value)
@@ -280,22 +284,39 @@ def check_for_b011(self, node):
         if isinstance(node.test, ast.NameConstant) and node.test.value is False:
             self.errors.append(B011(node.lineno, node.col_offset))
 
+    def check_for_b012(self, node):
+        def _loop(node):
+            if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)):
+                return
+
+            if isinstance(node, (ast.Return, ast.Continue, ast.Break)):
+                self.errors.append(B012(node.lineno, node.col_offset))
+
+            for child in ast.iter_child_nodes(node):
+                _loop(child)
+
+        for child in node.finalbody:
+            _loop(child)
+
+    def walk_function_body(self, node):
+        def _loop(parent, node):
+            if isinstance(node, (ast.AsyncFunctionDef, ast.FunctionDef)):
+                return
+            yield parent, node
+            for child in ast.iter_child_nodes(node):
+                yield from _loop(node, child)
+
+        for child in node.body:
+            yield from _loop(node, child)
+
     def check_for_b901(self, node):
         if node.name == "__await__":
             return
 
-        xs = [(None, x) for x in node.body]
-
         has_yield = False
         return_node = None
 
-        while xs:
-            parent, x = xs.pop()
-            if isinstance(x, (ast.AsyncFunctionDef, ast.FunctionDef)):
-                # Do not recurse into inner functions (`def` in
-                # `def`).
-                continue
-
+        for parent, x in self.walk_function_body(node):
             # Only consider yield when it is part of an Expr statement.
             if isinstance(parent, ast.Expr) and isinstance(
                 x, (ast.Yield, ast.YieldFrom)
@@ -309,9 +330,6 @@ def check_for_b901(self, node):
                 self.errors.append(B901(return_node.lineno, return_node.col_offset))
                 break
 
-            for x2 in ast.iter_child_nodes(x):
-                xs.append((x, x2))
-
     def check_for_b902(self, node):
         if not isinstance(self.node_stack[-2], ast.ClassDef):
             return
@@ -516,7 +534,11 @@ def visit(self, node):
     message="B011 Do not call assert False since python -O removes these calls. "
     "Instead callers should raise AssertionError()."
 )
-
+B012 = Error(
+    message="B012 return/continue/break inside finally blocks cause exceptions "
+    "to be silenced. Exceptions should be silenced in except blocks. Control "
+    "statements can be moved outside the finally block."
+)
 
 # Those could be false positives but it's more dangerous to let them slip
 # through if they're not.
@@ -562,6 +584,7 @@ def visit(self, node):
     "to the exception."
 )
 
+# Warnings disabled by default.
 B901 = Error(
     message="B901 Using `yield` together with `return x`. Use native "
     "`async def` coroutines or put a `# noqa` comment on this "

tests/b012.py

@@ -0,0 +1,95 @@
+def a():
+    try:
+        pass
+    finally:
+        return  # warning
+
+
+def b():
+    try:
+        pass
+    finally:
+        if 1 + 0 == 2 - 1:
+            return  # warning
+
+
+def c():
+    try:
+        pass
+    finally:
+        try:
+            return  # warning
+        except Exception:
+            pass
+
+
+def d():
+    try:
+        try:
+            pass
+        finally:
+            return  # warning
+    finally:
+        pass
+
+
+def e():
+    if 1 == 2 - 1:
+        try:
+
+            def f():
+                try:
+                    pass
+                finally:
+                    return  # warning
+
+        finally:
+            pass
+
+
+def g():
+    try:
+        pass
+    finally:
+
+        def h():
+            return  # no warning
+
+        e()
+
+
+def i():
+    while True:
+        try:
+            pass
+        finally:
+            break  # warning
+
+            def j():
+                while True:
+                    break  # no warning
+
+
+def h():
+    while True:
+        try:
+            pass
+        finally:
+            continue  # warning
+
+            def j():
+                while True:
+                    continue  # no warning
+
+
+while True:
+    try:
+        pass
+    finally:
+        continue  # warning
+
+while True:
+    try:
+        pass
+    finally:
+        break  # warning

tests/test_bugbear.py

@@ -15,6 +15,7 @@
     B009,
     B010,
     B011,
+    B012,
     B301,
     B302,
     B303,
@@ -119,6 +120,23 @@ def test_b011(self):
             errors, self.errors(B011(8, 0, vars=("i",)), B011(10, 0, vars=("k",)))
         )
 
+    def test_b012(self):
+        filename = Path(__file__).absolute().parent / "b012.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        all_errors = [
+            B012(5, 8),
+            B012(13, 12),
+            B012(21, 12),
+            B012(31, 12),
+            B012(44, 20),
+            B012(66, 12),
+            B012(78, 12),
+            B012(89, 8),
+            B012(95, 8),
+        ]
+        self.assertEqual(errors, self.errors(*all_errors))
+
     def test_b301_b302_b305(self):
         filename = Path(__file__).absolute().parent / "b301_b302_b305.py"
         bbc = BugBearChecker(filename=str(filename))