Trio103: except Cancelled/BaseException must be re-raised

CHANGELOG.md

@@ -1,6 +1,10 @@
 # Changelog
 *[CalVer, YY.month.patch](https://calver.org/)*
 
+## 22.7.4
+- Add TRIO103: `except BaseException` or `except trio.Cancelled` with a code path that doesn't re-raise
+- Add TRIO104: "Cancelled and BaseException must be re-raised" if user tries to return or raise a different exception.
+
 ## 22.7.3
 - Added TRIO102 check for unsafe checkpoints inside `finally:` blocks
 

README.md

@@ -24,3 +24,5 @@ pip install flake8-trio
 - **TRIO101** `yield` inside a nursery or cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling.
 - **TRIO102** it's unsafe to await inside `finally:` unless you use a shielded
   cancel scope with a timeout"
+- **TRIO103** `except BaseException` and `except trio.Cancelled` with a code path that doesn't re-raise
+- **TRIO104** `Cancelled` and `BaseException` must be re-raised - when a user tries to `return` or `raise` a different exception.

flake8_trio.py

@@ -14,7 +14,7 @@
 from typing import Any, Collection, Generator, List, Optional, Tuple, Type, Union
 
 # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1"
-__version__ = "22.7.3"
+__version__ = "22.7.4"
 
 
 Error = Tuple[int, int, str, Type[Any]]
@@ -255,6 +255,135 @@ def check_for_trio100(self, node: Union[ast.With, ast.AsyncWith]) -> None:
                 )
 
 
+# Never have an except Cancelled or except BaseException block with a code path that
+# doesn't re-raise the error
+class Visitor103_104(ast.NodeVisitor):
+    def __init__(self) -> None:
+        super().__init__()
+        self.problems: List[Error] = []
+
+        self.except_name: Optional[str] = None
+        self.unraised: bool = False
+
+    def visit_ExceptHandler(self, node: ast.ExceptHandler):
+        def has_exception(node: Optional[ast.expr]):
+            return (isinstance(node, ast.Name) and node.id == "BaseException") or (
+                isinstance(node, ast.Attribute)
+                and isinstance(node.value, ast.Name)
+                and node.value.id == "trio"
+                and node.attr == "Cancelled"
+            )
+
+        outer_unraised = self.unraised
+        exc_node = None
+
+        if isinstance(node.type, ast.Tuple):
+            for element in node.type.elts:
+                if has_exception(element):
+                    exc_node = element
+                    break
+        elif has_exception(node.type):
+            exc_node = node.type
+
+        if exc_node is not None:
+            self.except_name = node.name
+            self.unraised = True
+
+        self.generic_visit(node)
+
+        if exc_node is not None:
+            if self.unraised:
+                self.problems.append(
+                    make_error(TRIO103, exc_node.lineno, exc_node.col_offset)
+                )
+
+        self.unraised = outer_unraised
+
+    def visit_Raise(self, node: ast.Raise):
+        # if there's an exception that must be raised
+        # and none of the valid ways of re-raising it is done
+        if self.unraised and not (
+            # bare except
+            node.exc is None
+            # re-raised by name
+            or (isinstance(node.exc, ast.Name) and node.exc.id == self.except_name)
+            # new valid exception raised
+            or (
+                isinstance(node.exc, ast.Call)
+                and (
+                    (
+                        isinstance(node.exc.func, ast.Name)
+                        and node.exc.func.id == "BaseException"
+                    )
+                    or (
+                        isinstance(node.exc.func, ast.Attribute)
+                        and isinstance(node.exc.func.value, ast.Name)
+                        and node.exc.func.value.id == "trio"
+                        and node.exc.func.attr == "Cancelled"
+                    )
+                )
+            )
+        ):
+            # Error: something other than the exception was raised
+            self.problems.append(make_error(TRIO104, node.lineno, node.col_offset))
+        self.unraised = False
+        self.generic_visit(node)
+
+    def visit_Return(self, node: ast.Return):
+        if self.unraised:
+            # Error: must re-raise
+            self.problems.append(make_error(TRIO104, node.lineno, node.col_offset))
+        self.generic_visit(node)
+
+    def visit_Try(self, node: ast.Try):
+        if not self.unraised:
+            self.generic_visit(node)
+            return
+
+        # in theory it's okay if the try and all excepts re-raise,
+        # and there is a bare except
+        # but is a pain to parse and would require a special case for bare raises in
+        # nested excepts.
+        for n in (*node.body, *node.handlers, *node.orelse):
+            self.visit(n)
+            # re-set unraised to warn about returns in each block
+            self.unraised = True
+
+        # but it's fine if we raise in finally
+        for n in node.finalbody:
+            self.visit(n)
+
+    def visit_If(self, node: ast.If):
+        if not self.unraised:
+            self.generic_visit(node)
+            return
+
+        body_raised = False
+        for n in node.body:
+            self.visit(n)
+
+        # does body always raise correctly
+        body_raised = not self.unraised
+
+        self.unraised = True
+        for n in node.orelse:
+            self.visit(n)
+
+        # if body didn't raise, or it's unraised after else, set unraise
+        self.unraised = not body_raised or self.unraised
+
+    # disregard any raise's inside loops
+    def visit_For(self, node: ast.For):
+        outer_unraised = self.unraised
+        self.generic_visit(node)
+        self.unraised = outer_unraised
+
+    def visit_While(self, node: ast.While):
+        outer_unraised = self.unraised
+        self.generic_visit(node)
+        self.unraised = outer_unraised
+
+
 class Plugin:
     name = __name__
     version = __version__
@@ -269,7 +398,7 @@ def from_filename(cls, filename: str) -> "Plugin":
         return cls(ast.parse(source))
 
     def run(self) -> Generator[Tuple[int, int, str, Type[Any]], None, None]:
-        for v in (Visitor, Visitor102):
+        for v in (Visitor, Visitor102, Visitor103_104):
             visitor = v()
             visitor.visit(self._tree)
             yield from visitor.problems
@@ -278,3 +407,5 @@ def run(self) -> Generator[Tuple[int, int, str, Type[Any]], None, None]:
 TRIO100 = "TRIO100: {} context contains no checkpoints, add `await trio.sleep(0)`"
 TRIO101 = "TRIO101: yield inside a nursery or cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling"
 TRIO102 = "TRIO102: it's unsafe to await inside `finally:` unless you use a shielded cancel scope with a timeout"
+TRIO103 = "TRIO103: except Cancelled or except BaseException block with a code path that doesn't re-raise the error"
+TRIO104 = "TRIO104: Cancelled and BaseException must be re-raised"

tests/test_flake8_trio.py

@@ -9,7 +9,17 @@
 from hypothesis import HealthCheck, given, settings
 from hypothesmith import from_grammar, from_node
 
-from flake8_trio import TRIO100, TRIO101, TRIO102, Error, Plugin, Visitor, make_error
+from flake8_trio import (
+    TRIO100,
+    TRIO101,
+    TRIO102,
+    TRIO103,
+    TRIO104,
+    Error,
+    Plugin,
+    Visitor,
+    make_error,
+)
 
 
 class Flake8TrioTestCase(unittest.TestCase):
@@ -77,6 +87,28 @@ def test_trio102_py39(self):
             make_error(TRIO102, 15, 12),
         )
 
+    def test_trio103_104(self):
+        self.assert_expected_errors(
+            "trio103_104.py",
+            make_error(TRIO103, 7, 33),
+            make_error(TRIO104, 19, 4),
+            make_error(TRIO103, 25, 11),
+            make_error(TRIO103, 27, 11),
+            make_error(TRIO103, 37, 11),
+            make_error(TRIO104, 46, 8),
+            make_error(TRIO103, 45, 11),
+            make_error(TRIO104, 61, 16),
+            make_error(TRIO103, 55, 11),
+            make_error(TRIO104, 63, 8),
+            make_error(TRIO103, 73, 11),
+            make_error(TRIO104, 84, 12),
+            make_error(TRIO104, 86, 12),
+            make_error(TRIO104, 88, 12),
+            make_error(TRIO104, 90, 12),
+            make_error(TRIO103, 82, 11),
+            make_error(TRIO103, 98, 8),
+        )
+
 
 @pytest.mark.fuzz
 class TestFuzz(unittest.TestCase):

tests/trio103_104.py

@@ -0,0 +1,102 @@
+import trio
+
+try:
+    pass
+except (SyntaxError, ValueError, BaseException):
+    raise
+except (SyntaxError, ValueError, trio.Cancelled) as p:  # error
+    pass
+except (SyntaxError, ValueError):
+    raise
+except trio.Cancelled as e:
+    raise e
+except:
+    pass
+
+try:
+    pass
+except BaseException:
+    raise ValueError()  # error
+
+
+def foo(var):
+    try:
+        pass
+    except trio.Cancelled:  # error
+        pass
+    except BaseException as e:  # error
+        if True:
+            raise e
+        if True:
+            pass
+        else:
+            raise e
+
+    try:
+        pass
+    except BaseException as e:  # error - not guaranteed that try block will raise
+        try:
+            raise e
+        except trio.Cancelled as f:
+            raise f  # safe
+
+    try:
+        pass
+    except BaseException:  # error
+        return  # error
+
+    if True:
+        return
+
+    try:
+        pass
+    except BaseException as e:
+        raise  # acceptable - see https://peps.python.org/pep-0678/#example-usage
+    except trio.Cancelled:  # error
+        while var:
+            raise
+
+        for i in var:
+            if i:
+                return  # error
+    except trio.Cancelled as e:
+        raise ValueError() from e  # error
+    except trio.Cancelled as e:
+        raise BaseException() from e  # safe
+    except trio.Cancelled as e:
+        raise trio.Cancelled()  # safe
+
+    try:
+        pass
+    # in theory safe if the try, and all excepts raises - and there's a bare except.
+    # But is a very weird pattern that we don't handle.
+    except BaseException as e:  # error
+        try:
+            raise e
+        except ValueError:
+            raise e
+        except:
+            raise e
+
+    # check that we properly iterate over all nodes in try
+    except BaseException:  # error
+        try:
+            return  # error
+        except ValueError:
+            return  # error
+        else:
+            return  # error
+        finally:
+            return  # error
+
+    my_super_mega_long_exception_so_it_gets_split = SyntaxError
+    try:
+        pass
+    except (
+        my_super_mega_long_exception_so_it_gets_split,
+        SyntaxError,
+        BaseException,  # error
+        ValueError,
+        trio.Cancelled,  # no complaint on this line
+    ):
+        pass