From 0b9cc5cfd5feeb9ee7c2aa6f3acefb57c99b3197 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Mon, 18 Jul 2022 20:29:00 +0200 Subject: [PATCH 1/4] trio101: never yield inside a nursery or cancel scope --- flake8_trio.py | 44 ++++++++++++++++++++++++++++++++++++++- tests/test_flake8_trio.py | 9 +++++++- tests/trio101.py | 25 ++++++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 tests/trio101.py diff --git a/flake8_trio.py b/flake8_trio.py index 0b9145f..16f4ba1 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -11,7 +11,7 @@ import ast import tokenize -from typing import Any, Generator, List, Optional, Tuple, Type, Union +from typing import Any, Generator, List, Optional, Set, Tuple, Type, Union # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" __version__ = "22.7.1" @@ -40,13 +40,24 @@ class Visitor(ast.NodeVisitor): def __init__(self) -> None: super().__init__() self.problems: List[Error] = [] + self.safe_yields: Set[ast.Yield] = set() def visit_With(self, node: ast.With) -> None: self.check_for_trio100(node) + self.check_for_trio101(node) self.generic_visit(node) def visit_AsyncWith(self, node: ast.AsyncWith) -> None: self.check_for_trio100(node) + self.check_for_trio101(node) + self.generic_visit(node) + + def visit_FunctionDef(self, node: ast.FunctionDef) -> None: + self.trio101_mark_yields_safe(node) + self.generic_visit(node) + + def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None: + self.trio101_mark_yields_safe(node) self.generic_visit(node) def check_for_trio100(self, node: Union[ast.With, ast.AsyncWith]) -> None: @@ -58,6 +69,36 @@ def check_for_trio100(self, node: Union[ast.With, ast.AsyncWith]) -> None: make_error(TRIO100, item.lineno, item.col_offset, call) ) + def trio101_mark_yields_safe( + self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef] + ) -> None: + if any( + isinstance(d, ast.Name) + and d.id in ("contextmanager", "asynccontextmanager") + for d in node.decorator_list + ): + self.safe_yields.update( + {x for x in ast.walk(node) if isinstance(x, ast.Yield)} + ) + + def check_for_trio101(self, node: Union[ast.With, ast.AsyncWith]) -> None: + for item in (i.context_expr for i in node.items): + call = is_trio_call( + item, + "open_nursery", + "fail_after", + "fail_at", + "move_on_after", + "move_at", + ) + if call and any( + isinstance(x, ast.Yield) and x not in self.safe_yields + for x in ast.walk(node) + ): + self.problems.append( + make_error(TRIO101, item.lineno, item.col_offset, call) + ) + class Plugin: name = __name__ @@ -79,3 +120,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: {} never yield inside a nursery or cancel scope" diff --git a/tests/test_flake8_trio.py b/tests/test_flake8_trio.py index 19c8d25..4df30ea 100644 --- a/tests/test_flake8_trio.py +++ b/tests/test_flake8_trio.py @@ -9,7 +9,7 @@ from hypothesis import HealthCheck, given, settings from hypothesmith import from_grammar, from_node -from flake8_trio import TRIO100, Error, Plugin, Visitor, make_error +from flake8_trio import TRIO100, TRIO101, Error, Plugin, Visitor, make_error class Flake8TrioTestCase(unittest.TestCase): @@ -40,6 +40,13 @@ def test_trio100_py39(self): make_error(TRIO100, 14, 8, "trio.move_on_after"), ) + def test_trio101(self): + self.assert_expected_errors( + "trio101.py", + make_error(TRIO101, 7, 9, "trio.open_nursery"), + make_error(TRIO101, 12, 15, "trio.open_nursery"), + ) + @pytest.mark.fuzz class TestFuzz(unittest.TestCase): diff --git a/tests/trio101.py b/tests/trio101.py new file mode 100644 index 0000000..0b9a827 --- /dev/null +++ b/tests/trio101.py @@ -0,0 +1,25 @@ +from contextlib import asynccontextmanager, contextmanager + +import trio + + +def foo0(): + with trio.open_nursery() as _: + yield 1 + + +async def foo1(): + async with trio.open_nursery() as _: + yield 1 + + +@contextmanager +def foo2(): + with trio.open_nursery() as _: + yield 1 + + +@asynccontextmanager +async def foo3(): + async with trio.open_nursery() as _: + yield 1 From 0a69165af3c9ae5b917a36608fafe16cb86e68c5 Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Tue, 19 Jul 2022 20:01:03 +0200 Subject: [PATCH 2/4] Update flake8_trio.py better error message Co-authored-by: Zac Hatfield-Dodds --- flake8_trio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flake8_trio.py b/flake8_trio.py index 16f4ba1..f8ef9ab 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -120,4 +120,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: {} never yield inside a nursery or cancel scope" +TRIO101 = "TRIO101: yield inside a {} context is only safe when implementing a context manager - otherwise, it breaks exception handling" From 7574f349073f6d8904b7a576f7b29657f49655ce Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 19 Jul 2022 21:03:06 +0200 Subject: [PATCH 3/4] refactor trio101 checker, update readme --- README.md | 1 + flake8_trio.py | 83 +++++++++++++++++++++------------------ tests/test_flake8_trio.py | 6 ++- tests/trio101.py | 16 +++++--- 4 files changed, 60 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index cc43790..638da95 100644 --- a/README.md +++ b/README.md @@ -27,3 +27,4 @@ pip install git+https://github.com/Zac-HD/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. diff --git a/flake8_trio.py b/flake8_trio.py index f8ef9ab..bb1c3ef 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -18,6 +18,13 @@ Error = Tuple[int, int, str, Type[Any]] +cancel_scope_names = ( + "fail_after", + "fail_at", + "move_on_after", + "move_at", + "CancelScope", +) def make_error(error: str, lineno: int, col: int, *args: Any, **kwargs: Any) -> Error: @@ -41,64 +48,62 @@ def __init__(self) -> None: super().__init__() self.problems: List[Error] = [] self.safe_yields: Set[ast.Yield] = set() + self._yield_is_error = False + self._context_manager = False - def visit_With(self, node: ast.With) -> None: + def visit_generic_with(self, node: Union[ast.With, ast.AsyncWith]): self.check_for_trio100(node) - self.check_for_trio101(node) + + outer = self._yield_is_error + if not self._context_manager and any( + is_trio_call(item, "open_nursery", *cancel_scope_names) + for item in (i.context_expr for i in node.items) + ): + self._yield_is_error = True + self.generic_visit(node) + self._yield_is_error = outer + + def visit_With(self, node: ast.With) -> None: + self.visit_generic_with(node) def visit_AsyncWith(self, node: ast.AsyncWith) -> None: - self.check_for_trio100(node) - self.check_for_trio101(node) + self.visit_generic_with(node) + + def visit_generic_FunctionDef( + self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef] + ): + outer = self._context_manager + if any( + isinstance(d, ast.Name) + and d.id in ("contextmanager", "asynccontextmanager") + for d in node.decorator_list + ): + self._context_manager = True self.generic_visit(node) + self._context_manager = outer def visit_FunctionDef(self, node: ast.FunctionDef) -> None: - self.trio101_mark_yields_safe(node) - self.generic_visit(node) + self.visit_generic_FunctionDef(node) def visit_AsyncFunctionDef(self, node: ast.AsyncFunctionDef) -> None: - self.trio101_mark_yields_safe(node) + self.visit_generic_FunctionDef(node) + + def visit_Yield(self, node: ast.Yield) -> None: + if self._yield_is_error: + self.problems.append(make_error(TRIO101, node.lineno, node.col_offset)) + self.generic_visit(node) def check_for_trio100(self, node: Union[ast.With, ast.AsyncWith]) -> None: # Context manager with no `await` call within for item in (i.context_expr for i in node.items): - call = is_trio_call(item, "fail_after", "move_on_after") + call = is_trio_call(item, *cancel_scope_names) if call and not any(isinstance(x, ast.Await) for x in ast.walk(node)): self.problems.append( make_error(TRIO100, item.lineno, item.col_offset, call) ) - def trio101_mark_yields_safe( - self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef] - ) -> None: - if any( - isinstance(d, ast.Name) - and d.id in ("contextmanager", "asynccontextmanager") - for d in node.decorator_list - ): - self.safe_yields.update( - {x for x in ast.walk(node) if isinstance(x, ast.Yield)} - ) - - def check_for_trio101(self, node: Union[ast.With, ast.AsyncWith]) -> None: - for item in (i.context_expr for i in node.items): - call = is_trio_call( - item, - "open_nursery", - "fail_after", - "fail_at", - "move_on_after", - "move_at", - ) - if call and any( - isinstance(x, ast.Yield) and x not in self.safe_yields - for x in ast.walk(node) - ): - self.problems.append( - make_error(TRIO101, item.lineno, item.col_offset, call) - ) - class Plugin: name = __name__ @@ -120,4 +125,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 {} context is only safe when implementing a context manager - otherwise, it breaks exception handling" +TRIO101 = "TRIO101: yield inside a nursery or cancel scope is only safe when implementing a context manager - otherwise, it breaks exception handling" diff --git a/tests/test_flake8_trio.py b/tests/test_flake8_trio.py index 4df30ea..ea1b22a 100644 --- a/tests/test_flake8_trio.py +++ b/tests/test_flake8_trio.py @@ -41,10 +41,12 @@ def test_trio100_py39(self): ) def test_trio101(self): + self.maxDiff = None self.assert_expected_errors( "trio101.py", - make_error(TRIO101, 7, 9, "trio.open_nursery"), - make_error(TRIO101, 12, 15, "trio.open_nursery"), + make_error(TRIO101, 8, 8), + make_error(TRIO101, 13, 8), + make_error(TRIO101, 25, 8), ) diff --git a/tests/trio101.py b/tests/trio101.py index 0b9a827..d200704 100644 --- a/tests/trio101.py +++ b/tests/trio101.py @@ -5,21 +5,27 @@ def foo0(): with trio.open_nursery() as _: - yield 1 + yield 1 # error async def foo1(): async with trio.open_nursery() as _: - yield 1 + yield 1 # error @contextmanager def foo2(): with trio.open_nursery() as _: - yield 1 + yield 1 # safe -@asynccontextmanager async def foo3(): + async with trio.CancelScope() as _: + await trio.sleep(1) # so trio100 doesn't complain + yield 1 # error + + +@asynccontextmanager +async def foo4(): async with trio.open_nursery() as _: - yield 1 + yield 1 # safe From 5a594228ec955b9376aa391e35f6899215ede2df Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 20 Jul 2022 11:41:16 +0200 Subject: [PATCH 4/4] defining function with a wait within context manager is fine, lowered fuzz examples to 1k, detects contextmanagers in packages, disabled multithreading tests again --- flake8_trio.py | 15 +++++++++++---- tests/test_flake8_trio.py | 9 +++++---- tests/trio101.py | 22 ++++++++++++++++++++++ tox.ini | 4 ++-- 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/flake8_trio.py b/flake8_trio.py index bb1c3ef..fa328d7 100644 --- a/flake8_trio.py +++ b/flake8_trio.py @@ -25,6 +25,10 @@ "move_at", "CancelScope", ) +context_manager_names = ( + "contextmanager", + "asynccontextmanager", +) def make_error(error: str, lineno: int, col: int, *args: Any, **kwargs: Any) -> Error: @@ -73,15 +77,18 @@ def visit_AsyncWith(self, node: ast.AsyncWith) -> None: def visit_generic_FunctionDef( self, node: Union[ast.FunctionDef, ast.AsyncFunctionDef] ): - outer = self._context_manager + outer_cm = self._context_manager + outer_yie = self._yield_is_error + self._yield_is_error = False if any( - isinstance(d, ast.Name) - and d.id in ("contextmanager", "asynccontextmanager") + (isinstance(d, ast.Name) and d.id in context_manager_names) + or (isinstance(d, ast.Attribute) and d.attr in context_manager_names) for d in node.decorator_list ): self._context_manager = True self.generic_visit(node) - self._context_manager = outer + self._context_manager = outer_cm + self._yield_is_error = outer_yie def visit_FunctionDef(self, node: ast.FunctionDef) -> None: self.visit_generic_FunctionDef(node) diff --git a/tests/test_flake8_trio.py b/tests/test_flake8_trio.py index ea1b22a..22b3188 100644 --- a/tests/test_flake8_trio.py +++ b/tests/test_flake8_trio.py @@ -44,15 +44,16 @@ def test_trio101(self): self.maxDiff = None self.assert_expected_errors( "trio101.py", - make_error(TRIO101, 8, 8), - make_error(TRIO101, 13, 8), - make_error(TRIO101, 25, 8), + make_error(TRIO101, 10, 8), + make_error(TRIO101, 15, 8), + make_error(TRIO101, 27, 8), + make_error(TRIO101, 38, 8), ) @pytest.mark.fuzz class TestFuzz(unittest.TestCase): - @settings(max_examples=10_000, suppress_health_check=[HealthCheck.too_slow]) + @settings(max_examples=1_000, suppress_health_check=[HealthCheck.too_slow]) @given((from_grammar() | from_node()).map(ast.parse)) def test_does_not_crash_on_any_valid_code(self, syntax_tree: ast.AST): # Given any syntatically-valid source code, the checker should diff --git a/tests/trio101.py b/tests/trio101.py index d200704..9c68f5e 100644 --- a/tests/trio101.py +++ b/tests/trio101.py @@ -1,3 +1,5 @@ +import contextlib +import contextlib as bla from contextlib import asynccontextmanager, contextmanager import trio @@ -29,3 +31,23 @@ async def foo3(): async def foo4(): async with trio.open_nursery() as _: yield 1 # safe + + +async def foo5(): + async with trio.open_nursery(): + yield 1 # error + + def foo6(): + yield 1 # safe + + +@contextlib.asynccontextmanager +async def foo7(): + async with trio.open_nursery() as _: + yield 1 # safe + + +@bla.contextmanager +def foo8(): + with trio.open_nursery() as _: + yield 1 # safe diff --git a/tox.ini b/tox.ini index e1016fc..7dd2a8a 100644 --- a/tox.ini +++ b/tox.ini @@ -28,11 +28,11 @@ description = Runs pytest, optionally with posargs deps = pytest pytest-cov - pytest-xdist + #pytest-xdist hypothesis hypothesmith commands = - pytest {posargs:-n auto} + pytest #{posargs:-n auto} # Settings for other tools