Trio103: except Cancelled/BaseException must be re-raised

flake8_trio.py

@@ -255,6 +255,102 @@ def check_for_trio100(self, node: Union[ast.With, ast.AsyncWith]) -> None:
                 )
 
 
+def has_exception(node: 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"
+    )
+
+
+# Never have an except Cancelled or except BaseException block with a code path that
+# doesn't re-raise the error
+class Visitor103(ast.NodeVisitor):
+    def __init__(self) -> None:
+        super().__init__()
+        self.problems: List[Error] = []
+
+        # Likely overkill, but need to figure out nested try's first.
+        self.except_names: List[Optional[str]] = []
+        self.unraised: bool = False
+
+    def visit_ExceptHandler(self, node: ast.ExceptHandler):
+        outer_unraised = self.unraised
+        must_raise = False
+        if node.type is not None:
+            if (
+                isinstance(node.type, ast.Tuple)
+                and any(has_exception(v) for v in node.type.elts)
+            ) or has_exception(node.type):
+                must_raise = True
+                self.except_names.append(node.name)
+                self.unraised = True
+
+        self.generic_visit(node)
+        if must_raise:
+            if self.unraised:
+                # TODO: point at correct exception
+                self.problems.append(make_error(TRIO103, node.lineno, node.col_offset))
+            self.except_names.pop()
+
+        self.unraised = outer_unraised
+
+    def visit_Raise(self, node: ast.Raise):
+        if self.unraised:
+            assert self.except_names
+            name = self.except_names[-1]
+            if (name is None and node.exc is None) or (
+                isinstance(node.exc, ast.Name) and node.exc.id == name
+            ):
+                self.unraised = False
+            else:
+                # Error: something other than the exception was raised
+                # TODO: give separate error message
+                self.problems.append(make_error(TRIO103, node.lineno, node.col_offset))
+                self.unraised = False
+        self.generic_visit(node)
+
+    def visit_Return(self, node: ast.Return):
+        if self.unraised:
+            # Error, only good way of stopping the function is to raise
+            self.problems.append(make_error(TRIO103, node.lineno, node.col_offset))
+            self.unraised = False
+        self.generic_visit(node)
+
+    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)
+        # does orelse always raise correctly
+        orelse_raised = not self.unraised
+
+        # if both paths always raises, unset unraised.
+        self.unraised = not (body_raised and orelse_raised)
+
+    # 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 +365,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):
             visitor = v()
             visitor.visit(self._tree)
             yield from visitor.problems
@@ -278,3 +374,4 @@ 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"

tests/test_flake8_trio.py

@@ -9,7 +9,16 @@
 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,
+    Error,
+    Plugin,
+    Visitor,
+    make_error,
+)
 
 
 class Flake8TrioTestCase(unittest.TestCase):
@@ -77,6 +86,19 @@ def test_trio102_py39(self):
             make_error(TRIO102, 15, 12),
         )
 
+    def test_trio103(self):
+        self.assert_expected_errors(
+            "trio103.py",
+            make_error(TRIO103, 7, 0),
+            make_error(TRIO103, 19, 4),
+            make_error(TRIO103, 25, 4),
+            make_error(TRIO103, 27, 4),
+            make_error(TRIO103, 46, 8),
+            make_error(TRIO103, 80, 8),
+            make_error(TRIO103, 87, 16),
+            make_error(TRIO103, 81, 4),
+        )
+
 
 @pytest.mark.fuzz
 class TestFuzz(unittest.TestCase):

tests/trio103.py

@@ -0,0 +1,90 @@
+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:
+        try:
+            raise e
+        except trio.Cancelled as f:
+            raise f  # ???
+
+    try:
+        pass
+    except BaseException:
+        return  # error
+
+    if True:
+        return
+
+    try:
+        pass
+    except BaseException as e:
+        try:
+            raise e
+        except:
+            raise e
+
+    try:
+        pass
+    except BaseException as e:  # TODO: nested try's
+        try:
+            raise e
+        except:
+            raise
+
+    try:
+        pass
+    except BaseException as e:
+        try:
+            pass
+        except:
+            pass
+        finally:
+            raise e
+
+    try:
+        pass
+    except BaseException as e:
+        raise  # In theory safe? But godawful ugly so treat as error anyway
+    except trio.Cancelled:  # error
+        while var:
+            raise
+
+        for i in var:
+            if var:
+                return  # error
+
+
+# TODO: how to handle raise from?