From 8719f3f0303b841565f6d367549295507c1e762b Mon Sep 17 00:00:00 2001 From: John Litborn <11260241+jakkdl@users.noreply.github.com> Date: Wed, 20 Mar 2024 23:08:43 +0100 Subject: [PATCH] ASYNC102 no longer raises warning for calls to `.aclose()` on objects (#222) * ASYNC102 no longer raises warning for calls to `.aclose()` on objects * bump __version__ * fixes after review: allow aclose() in except clauses. Only allow if no args. Don't exempt asyncio-only code. * lol I was so confused why this started failing --- CHANGELOG.md | 5 +++- flake8_async/__init__.py | 2 +- flake8_async/visitors/visitor102.py | 34 ++++++++++++++++++++---- tests/eval_files/async102.py | 13 +++++++++ tests/eval_files/async102_aclose.py | 22 +++++++++++++++ tests/eval_files/async102_aclose_args.py | 24 +++++++++++++++++ tests/test_messages_documented.py | 4 ++- 7 files changed, 96 insertions(+), 8 deletions(-) create mode 100644 tests/eval_files/async102_aclose.py create mode 100644 tests/eval_files/async102_aclose_args.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ecf009..21d9e23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,11 @@ # Changelog *[CalVer, YY.month.patch](https://calver.org/)* +## 24.3.5 +- ASYNC102 (no await inside finally or critical except) no longer raises warnings for calls to `aclose()` on objects in trio/anyio code. See https://github.com/python-trio/flake8-async/issues/156 + ## 24.3.4 -- ASYNC110 (don't loop sleep) now also warns if looping `[trio/anyio].lowlevel.checkpoint()` +- ASYNC110 (don't loop sleep) now also warns if looping `[trio/anyio].lowlevel.checkpoint()`. ## 24.3.3 - Add ASYNC251: `time.sleep()` in async method. diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index eb18bf8..c122402 100644 --- a/flake8_async/__init__.py +++ b/flake8_async/__init__.py @@ -37,7 +37,7 @@ # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" -__version__ = "24.3.4" +__version__ = "24.3.5" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/visitor102.py b/flake8_async/visitors/visitor102.py index cb591da..52177dd 100644 --- a/flake8_async/visitors/visitor102.py +++ b/flake8_async/visitors/visitor102.py @@ -52,15 +52,35 @@ def __init__(self, *args: Any, **kwargs: Any): self._trio_context_managers: list[Visitor102.TrioScope] = [] self.cancelled_caught = False - # if we're inside a finally, and we're not inside a scope that doesn't have - # both a timeout and shield - def visit_Await(self, node: ast.Await | ast.AsyncFor | ast.AsyncWith): + # if we're inside a finally or critical except, and we're not inside a scope that + # doesn't have both a timeout and shield + def async_call_checker( + self, node: ast.Await | ast.AsyncFor | ast.AsyncWith + ) -> None: if self._critical_scope is not None and not any( cm.has_timeout and cm.shielded for cm in self._trio_context_managers ): self.error(node, self._critical_scope) - visit_AsyncFor = visit_Await + def is_safe_aclose_call(self, node: ast.Await) -> bool: + return ( + # don't mark calls safe in asyncio-only files + # a more defensive option would be `asyncio not in self.library` + self.library != ("asyncio",) + and isinstance(node.value, ast.Call) + # only known safe if no arguments + and not node.value.args + and not node.value.keywords + and isinstance(node.value.func, ast.Attribute) + and node.value.func.attr == "aclose" + ) + + def visit_Await(self, node: ast.Await): + # allow calls to `.aclose()` + if not (self.is_safe_aclose_call(node)): + self.async_call_checker(node) + + visit_AsyncFor = async_call_checker def visit_With(self, node: ast.With | ast.AsyncWith): self.save_state(node, "_trio_context_managers", copy=True) @@ -82,7 +102,7 @@ def visit_With(self, node: ast.With | ast.AsyncWith): break def visit_AsyncWith(self, node: ast.AsyncWith): - self.visit_Await(node) + self.async_call_checker(node) self.visit_With(node) def visit_Try(self, node: ast.Try): @@ -94,6 +114,10 @@ def visit_Try(self, node: ast.Try): self.visit_nodes(node.body, node.handlers, node.orelse) self._trio_context_managers = [] + # node.finalbody does not have a lineno, so we give the position of the try + # it'd be possible to estimate the lineno given the last except and the first + # statement in the finally, but it would be very hard to get it perfect with + # comments and empty lines and stuff. self._critical_scope = Statement("try/finally", node.lineno, node.col_offset) self.visit_nodes(node.finalbody) diff --git a/tests/eval_files/async102.py b/tests/eval_files/async102.py index a846716..b6b7278 100644 --- a/tests/eval_files/async102.py +++ b/tests/eval_files/async102.py @@ -252,3 +252,16 @@ async def foo_nested_excepts(): await foo() await foo() await foo() + + +# double check that this works nested inside an async for +async def foo_nested_async_for(): + + async for i in trio.bypasslinters: + try: + ... + except BaseException: + async for ( # error: 12, Statement("BaseException", lineno-1) + j + ) in trio.bypasslinters: + ... diff --git a/tests/eval_files/async102_aclose.py b/tests/eval_files/async102_aclose.py new file mode 100644 index 0000000..df1343d --- /dev/null +++ b/tests/eval_files/async102_aclose.py @@ -0,0 +1,22 @@ +# type: ignore + +# exclude finally: await x.aclose() from async102, with trio/anyio +# ANYIO_NO_ERROR +# TRIO_NO_ERROR +# See also async102_aclose_args.py - which makes sure trio/anyio raises errors if there +# are arguments to aclose(). + + +async def foo(): + # no type tracking in this check, we allow any call that looks like + # `await [...].aclose()` + x = None + + try: + ... + except BaseException: + await x.aclose() # ASYNC102: 8, Statement("BaseException", lineno-1) + await x.y.aclose() # ASYNC102: 8, Statement("BaseException", lineno-2) + finally: + await x.aclose() # ASYNC102: 8, Statement("try/finally", lineno-6) + await x.y.aclose() # ASYNC102: 8, Statement("try/finally", lineno-7) diff --git a/tests/eval_files/async102_aclose_args.py b/tests/eval_files/async102_aclose_args.py new file mode 100644 index 0000000..6db0d9f --- /dev/null +++ b/tests/eval_files/async102_aclose_args.py @@ -0,0 +1,24 @@ +# type: ignore + +# trio/anyio should still raise errors if there's args +# asyncio will always raise errors + +# See also async102_aclose.py, which checks that trio/anyio marks arg-less aclose() as safe + + +async def foo(): + # no type tracking in this check + x = None + + try: + ... + except BaseException: + await x.aclose(foo) # ASYNC102: 8, Statement("BaseException", lineno-1) + await x.aclose(bar=foo) # ASYNC102: 8, Statement("BaseException", lineno-2) + await x.aclose(*foo) # ASYNC102: 8, Statement("BaseException", lineno-3) + await x.aclose(None) # ASYNC102: 8, Statement("BaseException", lineno-4) + finally: + await x.aclose(foo) # ASYNC102: 8, Statement("try/finally", lineno-8) + await x.aclose(bar=foo) # ASYNC102: 8, Statement("try/finally", lineno-9) + await x.aclose(*foo) # ASYNC102: 8, Statement("try/finally", lineno-10) + await x.aclose(None) # ASYNC102: 8, Statement("try/finally", lineno-11) diff --git a/tests/test_messages_documented.py b/tests/test_messages_documented.py index 927fad8..1b24da7 100755 --- a/tests/test_messages_documented.py +++ b/tests/test_messages_documented.py @@ -44,7 +44,9 @@ def test_messages_documented(): if not file_path.is_file(): continue - if m := re.search(r"async\d\d\d", str(file_path)): + # only look in the stem (final part of the path), so as not to get tripped + # up by [git worktree] directories with an exception code in the name + if m := re.search(r"^async\d\d\d", str(file_path.stem)): documented_errors["eval_files"].add(m.group().upper()) with open(file_path) as file: