Trio103: except Cancelled/BaseException must be re-raised

CHANGELOG.md

@@ -1,6 +1,10 @@
 # Changelog
 *[CalVer, YY.month.patch](https://calver.org/)*
 
+## Future
+- 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.4
 - Added TRIO105 check for not immediately `await`ing async trio functions.
 - Added TRIO106 check that trio is imported in a form that the plugin can easily parse.

README.md

@@ -21,7 +21,10 @@ pip install flake8-trio
 - **TRIO100**: a `with trio.fail_after(...):` or `with trio.move_on_after(...):`
   context does not contain any `await` statements.  This makes it pointless, as
   the timeout can only be triggered by a checkpoint.
-- **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
+- **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"
-- **TRIO105** Calling a trio async function without immediately `await`ing it.
+- **TRIO103**: `except BaseException` and `except trio.Cancelled` with a code path that doesn't re-raise. Note that any `raise` statements in loops are ignored since it's tricky to parse loop flow with `break`, `continue` and/or the zero-iteration case.
+- **TRIO104**: `Cancelled` and `BaseException` must be re-raised - when a user tries to `return` or `raise` a different exception.
+- **TRIO105**: Calling a trio async function without immediately `await`ing it.
+- **TRIO106**: trio must be imported with `import trio` for the linter to work

flake8_trio.py

@@ -41,6 +41,12 @@ def __init__(self) -> None:
         super().__init__()
         self.problems: List[Error] = []
 
+    @classmethod
+    def run(cls, tree: ast.AST) -> Generator[Error, None, None]:
+        visitor = cls()
+        visitor.visit(tree)
+        yield from visitor.problems
+
 
 class TrioScope:
     def __init__(self, node: ast.Call, funcname: str, packagename: str):
@@ -91,6 +97,7 @@ def has_decorator(decorator_list: List[ast.expr], names: Collection[str]):
     return False
 
 
+# handles 100, 101 and 106
 class VisitorMiscChecks(Flake8TrioVisitor):
     def __init__(self) -> None:
         super().__init__()
@@ -150,7 +157,8 @@ def check_for_trio100(self, node: Union[ast.With, ast.AsyncWith]) -> None:
         for item in (i.context_expr for i in node.items):
             call = get_trio_scope(item, *cancel_scope_names)
             if call and not any(
-                isinstance(x, checkpoint_node_types) for x in ast.walk(node)
+                isinstance(x, checkpoint_node_types) and x != node
+                for x in ast.walk(node)
             ):
                 self.problems.append(
                     make_error(TRIO100, item.lineno, item.col_offset, call)
@@ -269,6 +277,136 @@ def check_for_trio102(self, node: Union[ast.Await, ast.AsyncFor, ast.AsyncWith])
             self.problems.append(make_error(TRIO102, node.lineno, node.col_offset))
 
 
+# Never have an except Cancelled or except BaseException block with a code path that
+# doesn't re-raise the error
+class Visitor103_104(Flake8TrioVisitor):
+    def __init__(self) -> None:
+        super().__init__()
+        self.except_name: Optional[str] = ""
+        self.unraised: bool = False
+
+    # If an `except` is bare, catches `BaseException`, or `trio.Cancelled`
+    # set self.unraised, and if it's still set after visiting child nodes
+    # then there might be a code path that doesn't re-raise.
+    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 = (self.unraised, self.except_name)
+        marker = None
+
+        # we need to not unset self.unraised if this is non-critical to still
+        # warn about `return`s
+
+        # bare except
+        if node.type is None:
+            self.unraised = True
+            marker = (node.lineno, node.col_offset)
+        # several exceptions
+        elif isinstance(node.type, ast.Tuple):
+            for element in node.type.elts:
+                if has_exception(element):
+                    self.unraised = True
+                    marker = element.lineno, element.col_offset
+                    break
+        # single exception, either a Name or an Attribute
+        elif has_exception(node.type):
+            self.unraised = True
+            marker = node.type.lineno, node.type.col_offset
+
+        if marker is None:
+            self.generic_visit(node)
+            (self.unraised, self.except_name) = outer
+            return
+
+        # save name `as <except_name>`
+        self.except_name = node.name
+
+        # visit child nodes. Will unset self.unraised if all code paths `raise`
+        self.generic_visit(node)
+
+        if self.unraised:
+            self.problems.append(make_error(TRIO103, *marker))
+
+        (self.unraised, self.except_name) = outer
+
+    def visit_Raise(self, node: ast.Raise):
+        # if there's an unraised critical exception, the raise isn't bare,
+        # and the name doesn't match, signal a problem.
+        if (
+            self.unraised
+            and node.exc is not None
+            and not (isinstance(node.exc, ast.Name) and node.exc.id == self.except_name)
+        ):
+            self.problems.append(make_error(TRIO104, node.lineno, node.col_offset))
+
+        # treat it as safe regardless, to avoid unnecessary error messages.
+        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)
+
+    # Treat Try's as fully covering only if `finally` always raises.
+    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)
+
+    # Treat if's as fully covering if both `if` and `else` raise.
+    # `elif` is parsed by the ast as a new if statement inside the else.
+    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
+
+    # It's hard to check for full coverage of `raise`s inside loops, so
+    # we completely disregard them when checking coverage by resetting the
+    # effects of them afterwards
+    def visit_For(self, node: Union[ast.For, ast.While]):
+        outer_unraised = self.unraised
+        self.generic_visit(node)
+        self.unraised = outer_unraised
+
+    visit_While = visit_For
+
+
 trio_async_functions = (
     "aclose_forcefully",
     "open_file",
@@ -328,14 +466,14 @@ 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 cls in Flake8TrioVisitor.__subclasses__():
-            visitor = cls()
-            visitor.visit(self._tree)
-            yield from visitor.problems
+        for v in Flake8TrioVisitor.__subclasses__():
+            yield from v.run(self._tree)
 
 
 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 therefore BaseException) must be re-raised"
 TRIO105 = "TRIO105: Trio async function {} must be immediately awaited"
 TRIO106 = "TRIO106: trio must be imported with `import trio` for the linter to work"

tests/test_changelog_and_version.py

@@ -1,7 +1,8 @@
 """Tests for flake8-trio package metadata."""
 import re
+import unittest
 from pathlib import Path
-from typing import Iterable, NamedTuple
+from typing import Dict, Iterable, NamedTuple, Set
 
 import flake8_trio
 
@@ -43,3 +44,25 @@ def test_version_increments_are_correct():
             assert prev[:2] < current[:2], msg
         else:
             assert current == prev._replace(patch=prev.patch + 1), msg
+
+
+class test_messages_documented(unittest.TestCase):
+    def runTest(self):
+        documented_errors: Dict[str, Set[str]] = {}
+        for filename in (
+            "CHANGELOG.md",
+            "README.md",
+            "flake8_trio.py",
+            "tests/test_flake8_trio.py",
+        ):
+            with open(Path(__file__).parent.parent / filename) as f:
+                lines = f.readlines()
+            documented_errors[filename] = set()
+            for line in lines:
+                for error_msg in re.findall(r"TRIO\d\d\d", line):
+                    documented_errors[filename].add(error_msg)
+
+        file_items = list(documented_errors.items())
+        first = file_items.pop()
+        for file, documented in file_items:
+            self.assertSetEqual(first[1], documented, msg=(first[0], file))

tests/test_flake8_trio.py

@@ -5,6 +5,7 @@
 import sys
 import unittest
 from pathlib import Path
+from typing import Iterable
 
 import pytest
 import trio  # type: ignore
@@ -15,6 +16,8 @@
     TRIO100,
     TRIO101,
     TRIO102,
+    TRIO103,
+    TRIO104,
     TRIO105,
     TRIO106,
     Error,
@@ -26,21 +29,33 @@
 
 class Flake8TrioTestCase(unittest.TestCase):
     def assert_expected_errors(self, test_file: str, *expected: Error) -> None:
+        def trim_messages(messages: Iterable[Error]):
+            return tuple(((line, col, msg[:7]) for line, col, msg, _ in messages))
+
         filename = Path(__file__).absolute().parent / test_file
         plugin = Plugin.from_filename(str(filename))
+
         errors = tuple(plugin.run())
-        self.assertEqual(errors, expected)
+
+        # start with a check with trimmed errors that will make for smaller diff messages
+        trim_errors = trim_messages(plugin.run())
+        trim_expected = trim_messages(expected)
+        self.assertTupleEqual(trim_errors, trim_expected)
+
+        # full check
+        self.assertTupleEqual(errors, expected)
 
     def test_tree(self):
         plugin = Plugin(ast.parse(""))
         errors = list(plugin.run())
-        self.assertEqual(errors, [])
+        self.assertSequenceEqual(errors, [])
 
     def test_trio100(self):
         self.assert_expected_errors(
             "trio100.py",
             make_error(TRIO100, 3, 5, "trio.move_on_after"),
-            make_error(TRIO100, 23, 9, "trio.fail_after"),
+            make_error(TRIO100, 8, 15, "trio.fail_after"),
+            make_error(TRIO100, 26, 9, "trio.fail_after"),
         )
 
     @unittest.skipIf(sys.version_info < (3, 9), "requires 3.9+")
@@ -83,6 +98,44 @@ def test_trio102(self):
             make_error(TRIO102, 123, 12),
         )
 
+    def test_trio103_104(self):
+        self.assert_expected_errors(
+            "trio103_104.py",
+            make_error(TRIO103, 7, 33),
+            make_error(TRIO103, 15, 7),
+            # raise different exception
+            make_error(TRIO104, 20, 4),
+            make_error(TRIO104, 22, 4),
+            make_error(TRIO104, 25, 4),
+            # if
+            make_error(TRIO103, 28, 7),
+            make_error(TRIO103, 35, 7),
+            # loops
+            make_error(TRIO103, 47, 7),
+            make_error(TRIO103, 52, 7),
+            # nested exceptions
+            make_error(TRIO104, 67, 8),  # weird case, unsure if error
+            make_error(TRIO103, 61, 7),
+            make_error(TRIO104, 92, 8),
+            # make_error(TRIO104, 94, 8), # TODO: not implemented
+            # bare except
+            make_error(TRIO103, 97, 0),
+            # multi-line
+            make_error(TRIO103, 111, 4),
+            # re-raise parent
+            make_error(TRIO104, 124, 8),
+            # return
+            make_error(TRIO104, 134, 8),
+            make_error(TRIO103, 133, 11),
+            make_error(TRIO104, 139, 12),
+            make_error(TRIO104, 141, 12),
+            make_error(TRIO104, 143, 12),
+            make_error(TRIO104, 145, 12),
+            make_error(TRIO103, 137, 11),
+            # make_error(TRIO104, 154, 7), # TODO: not implemented
+            # make_error(TRIO104, 162, 7), # TODO: not implemented
+        )
+
     def test_trio105(self):
         self.assert_expected_errors(
             "trio105.py",
@@ -100,11 +153,11 @@ def test_trio105(self):
             make_error(TRIO105, 36, 4, "sleep"),
             make_error(TRIO105, 37, 4, "sleep_forever"),
             make_error(TRIO105, 38, 4, "sleep_until"),
-            make_error(TRIO105, 45, 15, "open_file"),
-            make_error(TRIO105, 50, 8, "open_file"),
+            make_error(TRIO105, 44, 15, "open_file"),
+            make_error(TRIO105, 49, 8, "open_file"),
         )
 
-        self.assertEqual(
+        self.assertSetEqual(
             set(trio_async_functions),
             {
                 o[0]