From fcc89d254c269eee618fc771a4d28bd2a3ca19b9 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 29 Oct 2024 13:34:14 +0100 Subject: [PATCH 1/7] add async124 async-function-could-be-sync --- docs/changelog.rst | 4 ++ docs/rules.rst | 6 ++ flake8_async/visitors/visitor91x.py | 55 ++++++++++++++++-- tests/autofix_files/async124.py | 87 ++++++++++++++++++++++++++++ tests/autofix_files/async124.py.diff | 49 ++++++++++++++++ tests/eval_files/async124.py | 81 ++++++++++++++++++++++++++ tests/test_flake8_async.py | 4 +- 7 files changed, 281 insertions(+), 5 deletions(-) create mode 100644 tests/autofix_files/async124.py create mode 100644 tests/autofix_files/async124.py.diff create mode 100644 tests/eval_files/async124.py diff --git a/docs/changelog.rst b/docs/changelog.rst index 2672b08..8ed36ea 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,10 @@ Changelog `CalVer, YY.month.patch `_ +24.10.3 +======= +- Add :ref:`ASYNC124 ` async-function-could-be-sync + 24.10.2 ======= - :ref:`ASYNC102 ` now also warns about ``await()`` inside ``__aexit__``. diff --git a/docs/rules.rst b/docs/rules.rst index c3b7566..da3b943 100644 --- a/docs/rules.rst +++ b/docs/rules.rst @@ -94,6 +94,12 @@ _`ASYNC123`: bad-exception-group-flattening Dropping this information makes diagnosing errors much more difficult. We recommend ``raise SomeNewError(...) from group`` if possible; or consider using `copy.copy` to shallow-copy the exception before re-raising (for copyable types), or re-raising the error from outside the `except` block. +_`ASYNC124`: async-function-could-be-sync + Triggers when an async function contain none of ``await``, ``async for`` or ``async with``. + Calling an async function is slower than calling regular functions, so if possible you + might want to convert your function to be synchronous. + This currently overlaps with :ref:`ASYNC910 ` and :ref:`ASYNC911 ` which, if enabled, will autofix the function to have :ref:`checkpoint`. + Blocking sync calls in async functions ====================================== diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 539fcee..ea79e1d 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -1,8 +1,14 @@ -"""Contains Visitor91X. +"""Contains Visitor91X and Visitor124. -910 looks for async functions without guaranteed checkpoints (or exceptions), and 911 does -the same except for async iterables - while also requiring that they checkpoint between -each yield. +Visitor91X contains checks for +* ASYNC100 cancel-scope-no-checkpoint +* ASYNC910 async-function-no-checkpoint +* ASYNC911 async-generator-no-checkpoint +* ASYNC912 cancel-scope-no-guaranteed-checkpoint +* ASYNC913 indefinite-loop-no-guaranteed-checkpoint + +Visitor124 contains +* ASYNC124 async-function-could-be-sync """ from __future__ import annotations @@ -63,6 +69,47 @@ def func_empty_body(node: cst.FunctionDef) -> bool: ) +@error_class_cst +class Visitor124(Flake8AsyncVisitor_cst): + error_codes: Mapping[str, str] = { + "ASYNC124": ( + "Async function with no `await` could be sync." + " Async functions are more expensive to call." + ) + } + + def __init__(self, *args: Any, **kwargs: Any): + super().__init__(*args, **kwargs) + self.has_await = False + + def visit_FunctionDef(self, node: cst.FunctionDef): + # await in sync defs are not valid, but handling this will make ASYNC124 + # pop up in parent func as if the child function was async + self.save_state(node, "has_await", copy=False) + self.has_await = False + + def leave_FunctionDef( + self, original_node: cst.FunctionDef, updated_node: cst.FunctionDef + ) -> cst.FunctionDef: + if ( + original_node.asynchronous is not None + and not self.has_await + and not func_empty_body(original_node) + ): + self.error(original_node) + self.restore_state(original_node) + return updated_node + + def visit_Await(self, node: cst.Await): + self.has_await = True + + def visit_With(self, node: cst.With | cst.For): + if node.asynchronous is not None: + self.has_await = True + + visit_For = visit_With + + @dataclass class LoopState: infinite_loop: bool = False diff --git a/tests/autofix_files/async124.py b/tests/autofix_files/async124.py new file mode 100644 index 0000000..cc0e8f4 --- /dev/null +++ b/tests/autofix_files/async124.py @@ -0,0 +1,87 @@ +"""Async function with no awaits could be sync. +It currently does not care if 910/911 would also be triggered.""" + +# ARG --enable=ASYNC124,ASYNC910,ASYNC911 + +# 910/911 will also autofix async124, in the sense of adding a checkpoint. This is perhaps +# not what the user wants though, so this would be a case in favor of making 910/911 not +# trigger when async124 does. +# AUTOFIX +# ASYNCIO_NO_AUTOFIX +from typing import Any +import trio + + +def condition() -> bool: + return False + + +async def foo() -> Any: + await foo() + + +async def foo_print(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + print("hello") + await trio.lowlevel.checkpoint() + + +async def conditional_wait(): # ASYNC910: 0, "exit", Statement("function definition", lineno) + if condition(): + await foo() + await trio.lowlevel.checkpoint() + + +async def foo_gen(): # ASYNC124: 0 # ASYNC911: 0, "exit", Statement("yield", lineno+1) + await trio.lowlevel.checkpoint() + yield # ASYNC911: 4, "yield", Statement("function definition", lineno-1) + await trio.lowlevel.checkpoint() + + +async def foo_async_with(): + async with foo_gen(): + ... + + +async def foo_async_for(): + async for i in foo_gen(): + ... + + +async def foo_nested(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + async def foo_nested_2(): + await foo() + await trio.lowlevel.checkpoint() + + +async def foo_nested_sync(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + def foo_nested_sync_child(): + await foo() # type: ignore[await-not-async] + await trio.lowlevel.checkpoint() + + +# We don't want to trigger on empty/pass functions because of inheritance. +# Uses same logic as async91x. + + +async def foo_empty(): + "blah" + ... + + +async def foo_empty_pass(): + "foo" + pass + + +# we could consider filtering out functions named `test_.*` to not give false alarms on +# tests that use async fixtures. +# For ruff and for running through flake8 we could expect users to use per-file-ignores, +# but running as standalone we don't currently support that. (though probs wouldn't be +# too bad to add support). +# See e.g. https://github.com/agronholm/anyio/issues/803 for why one might want an async +# test without awaits. + + +async def test_async_fixture(my_anyio_fixture): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + assert my_anyio_fixture.setup_worked_correctly + await trio.lowlevel.checkpoint() diff --git a/tests/autofix_files/async124.py.diff b/tests/autofix_files/async124.py.diff new file mode 100644 index 0000000..e796987 --- /dev/null +++ b/tests/autofix_files/async124.py.diff @@ -0,0 +1,49 @@ +--- ++++ +@@ x,6 x,7 @@ + # AUTOFIX + # ASYNCIO_NO_AUTOFIX + from typing import Any ++import trio + + + def condition() -> bool: +@@ x,15 x,19 @@ + + async def foo_print(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + print("hello") ++ await trio.lowlevel.checkpoint() + + + async def conditional_wait(): # ASYNC910: 0, "exit", Statement("function definition", lineno) + if condition(): + await foo() ++ await trio.lowlevel.checkpoint() + + + async def foo_gen(): # ASYNC124: 0 # ASYNC911: 0, "exit", Statement("yield", lineno+1) ++ await trio.lowlevel.checkpoint() + yield # ASYNC911: 4, "yield", Statement("function definition", lineno-1) ++ await trio.lowlevel.checkpoint() + + + async def foo_async_with(): +@@ x,11 x,13 @@ + async def foo_nested(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + async def foo_nested_2(): + await foo() ++ await trio.lowlevel.checkpoint() + + + async def foo_nested_sync(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + def foo_nested_sync_child(): + await foo() # type: ignore[await-not-async] ++ await trio.lowlevel.checkpoint() + + + # We don't want to trigger on empty/pass functions because of inheritance. +@@ x,3 x,4 @@ + + async def test_async_fixture(my_anyio_fixture): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + assert my_anyio_fixture.setup_worked_correctly ++ await trio.lowlevel.checkpoint() diff --git a/tests/eval_files/async124.py b/tests/eval_files/async124.py new file mode 100644 index 0000000..2484c19 --- /dev/null +++ b/tests/eval_files/async124.py @@ -0,0 +1,81 @@ +"""Async function with no awaits could be sync. +It currently does not care if 910/911 would also be triggered.""" + +# ARG --enable=ASYNC124,ASYNC910,ASYNC911 + +# 910/911 will also autofix async124, in the sense of adding a checkpoint. This is perhaps +# not what the user wants though, so this would be a case in favor of making 910/911 not +# trigger when async124 does. +# AUTOFIX +# ASYNCIO_NO_AUTOFIX +from typing import Any + + +def condition() -> bool: + return False + + +async def foo() -> Any: + await foo() + + +async def foo_print(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + print("hello") + + +async def conditional_wait(): # ASYNC910: 0, "exit", Statement("function definition", lineno) + if condition(): + await foo() + + +async def foo_gen(): # ASYNC124: 0 # ASYNC911: 0, "exit", Statement("yield", lineno+1) + yield # ASYNC911: 4, "yield", Statement("function definition", lineno-1) + + +async def foo_async_with(): + async with foo_gen(): + ... + + +async def foo_async_for(): + async for i in foo_gen(): + ... + + +async def foo_nested(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + async def foo_nested_2(): + await foo() + + +async def foo_nested_sync(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + def foo_nested_sync_child(): + await foo() # type: ignore[await-not-async] + + +# We don't want to trigger on empty/pass functions because of inheritance. +# Uses same logic as async91x. + + +async def foo_empty(): + "blah" + ... + + +async def foo_empty_pass(): + "foo" + pass + + +# we could consider filtering out functions named `test_.*` to not give false alarms on +# tests that use async fixtures. +# For ruff and for running through flake8 we could expect users to use per-file-ignores, +# but running as standalone we don't currently support that. (though probs wouldn't be +# too bad to add support). +# See e.g. https://github.com/agronholm/anyio/issues/803 for why one might want an async +# test without awaits. + + +async def test_async_fixture( + my_anyio_fixture, +): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + assert my_anyio_fixture.setup_worked_correctly diff --git a/tests/test_flake8_async.py b/tests/test_flake8_async.py index 9676bbc..4435649 100644 --- a/tests/test_flake8_async.py +++ b/tests/test_flake8_async.py @@ -619,13 +619,14 @@ def assert_correct_lines_and_codes(errors: Iterable[Error], expected: Iterable[E expected_dict[e.line][e.code] += 1 error_count = 0 + printed_header = False for line in all_lines: if error_dict[line] == expected_dict[line]: continue # go through all the codes on the line for code in sorted({*error_dict[line], *expected_dict[line]}): - if error_count == 0: + if not printed_header: print( "Lines with different # of errors:", "-" * 38, @@ -633,6 +634,7 @@ def assert_correct_lines_and_codes(errors: Iterable[Error], expected: Iterable[E sep="\n", file=sys.stderr, ) + printed_header = True print( f"| {line:4}", From d40b064dc289852c9321d04226e7c34d9ef9677c Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 29 Oct 2024 16:27:50 +0100 Subject: [PATCH 2/7] fix tests, add fixture logic --- flake8_async/__init__.py | 2 +- flake8_async/visitors/visitor91x.py | 12 ++++ tests/autofix_files/async124.py | 87 ---------------------------- tests/autofix_files/async124.py.diff | 49 ---------------- tests/autofix_files/async910.py | 7 ++- tests/eval_files/async124.py | 44 ++++++++++---- tests/eval_files/async910.py | 7 ++- tests/trio_options.py | 9 ++- 8 files changed, 63 insertions(+), 154 deletions(-) delete mode 100644 tests/autofix_files/async124.py delete mode 100644 tests/autofix_files/async124.py.diff diff --git a/flake8_async/__init__.py b/flake8_async/__init__.py index 83c5d57..d36800f 100644 --- a/flake8_async/__init__.py +++ b/flake8_async/__init__.py @@ -38,7 +38,7 @@ # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1" -__version__ = "24.10.2" +__version__ = "24.10.3" # taken from https://github.com/Zac-HD/shed diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index ea79e1d..0cdd826 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -95,6 +95,18 @@ def leave_FunctionDef( original_node.asynchronous is not None and not self.has_await and not func_empty_body(original_node) + and not func_has_decorator(original_node, "overload") + # skip functions named 'text_xxx' with params, since they may be relying + # on async fixtures. This is esp bad as sync funcs relying on async fixtures + # is not well handled: https://github.com/pytest-dev/pytest/issues/10839 + # also skip funcs with @fixtures and params + and not ( + original_node.params.params + and ( + original_node.name.value.startswith("test_") + or func_has_decorator(original_node, "fixture") + ) + ) ): self.error(original_node) self.restore_state(original_node) diff --git a/tests/autofix_files/async124.py b/tests/autofix_files/async124.py deleted file mode 100644 index cc0e8f4..0000000 --- a/tests/autofix_files/async124.py +++ /dev/null @@ -1,87 +0,0 @@ -"""Async function with no awaits could be sync. -It currently does not care if 910/911 would also be triggered.""" - -# ARG --enable=ASYNC124,ASYNC910,ASYNC911 - -# 910/911 will also autofix async124, in the sense of adding a checkpoint. This is perhaps -# not what the user wants though, so this would be a case in favor of making 910/911 not -# trigger when async124 does. -# AUTOFIX -# ASYNCIO_NO_AUTOFIX -from typing import Any -import trio - - -def condition() -> bool: - return False - - -async def foo() -> Any: - await foo() - - -async def foo_print(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - print("hello") - await trio.lowlevel.checkpoint() - - -async def conditional_wait(): # ASYNC910: 0, "exit", Statement("function definition", lineno) - if condition(): - await foo() - await trio.lowlevel.checkpoint() - - -async def foo_gen(): # ASYNC124: 0 # ASYNC911: 0, "exit", Statement("yield", lineno+1) - await trio.lowlevel.checkpoint() - yield # ASYNC911: 4, "yield", Statement("function definition", lineno-1) - await trio.lowlevel.checkpoint() - - -async def foo_async_with(): - async with foo_gen(): - ... - - -async def foo_async_for(): - async for i in foo_gen(): - ... - - -async def foo_nested(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - async def foo_nested_2(): - await foo() - await trio.lowlevel.checkpoint() - - -async def foo_nested_sync(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - def foo_nested_sync_child(): - await foo() # type: ignore[await-not-async] - await trio.lowlevel.checkpoint() - - -# We don't want to trigger on empty/pass functions because of inheritance. -# Uses same logic as async91x. - - -async def foo_empty(): - "blah" - ... - - -async def foo_empty_pass(): - "foo" - pass - - -# we could consider filtering out functions named `test_.*` to not give false alarms on -# tests that use async fixtures. -# For ruff and for running through flake8 we could expect users to use per-file-ignores, -# but running as standalone we don't currently support that. (though probs wouldn't be -# too bad to add support). -# See e.g. https://github.com/agronholm/anyio/issues/803 for why one might want an async -# test without awaits. - - -async def test_async_fixture(my_anyio_fixture): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - assert my_anyio_fixture.setup_worked_correctly - await trio.lowlevel.checkpoint() diff --git a/tests/autofix_files/async124.py.diff b/tests/autofix_files/async124.py.diff deleted file mode 100644 index e796987..0000000 --- a/tests/autofix_files/async124.py.diff +++ /dev/null @@ -1,49 +0,0 @@ ---- -+++ -@@ x,6 x,7 @@ - # AUTOFIX - # ASYNCIO_NO_AUTOFIX - from typing import Any -+import trio - - - def condition() -> bool: -@@ x,15 x,19 @@ - - async def foo_print(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - print("hello") -+ await trio.lowlevel.checkpoint() - - - async def conditional_wait(): # ASYNC910: 0, "exit", Statement("function definition", lineno) - if condition(): - await foo() -+ await trio.lowlevel.checkpoint() - - - async def foo_gen(): # ASYNC124: 0 # ASYNC911: 0, "exit", Statement("yield", lineno+1) -+ await trio.lowlevel.checkpoint() - yield # ASYNC911: 4, "yield", Statement("function definition", lineno-1) -+ await trio.lowlevel.checkpoint() - - - async def foo_async_with(): -@@ x,11 x,13 @@ - async def foo_nested(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - async def foo_nested_2(): - await foo() -+ await trio.lowlevel.checkpoint() - - - async def foo_nested_sync(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - def foo_nested_sync_child(): - await foo() # type: ignore[await-not-async] -+ await trio.lowlevel.checkpoint() - - - # We don't want to trigger on empty/pass functions because of inheritance. -@@ x,3 x,4 @@ - - async def test_async_fixture(my_anyio_fixture): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - assert my_anyio_fixture.setup_worked_correctly -+ await trio.lowlevel.checkpoint() diff --git a/tests/autofix_files/async910.py b/tests/autofix_files/async910.py index e8bcf77..95345af 100644 --- a/tests/autofix_files/async910.py +++ b/tests/autofix_files/async910.py @@ -134,9 +134,12 @@ def foo_normal_func_1(): def foo_normal_func_2(): ... -# overload decorator +# overload decorator is skipped +# overload functions should always be empty, so the check is somewhat redundant, +# but making one non-empty to check the logic. @overload -async def foo_overload_1(_: bytes): ... +async def foo_overload_1(_: bytes): + raise NotImplementedError @typing.overload diff --git a/tests/eval_files/async124.py b/tests/eval_files/async124.py index 2484c19..c583aa3 100644 --- a/tests/eval_files/async124.py +++ b/tests/eval_files/async124.py @@ -6,9 +6,9 @@ # 910/911 will also autofix async124, in the sense of adding a checkpoint. This is perhaps # not what the user wants though, so this would be a case in favor of making 910/911 not # trigger when async124 does. -# AUTOFIX -# ASYNCIO_NO_AUTOFIX -from typing import Any +# NOAUTOFIX # all errors get "fixed" except for foo_fix_no_subfix +from typing import Any, overload +from pytest import fixture def condition() -> bool: @@ -66,16 +66,36 @@ async def foo_empty_pass(): pass -# we could consider filtering out functions named `test_.*` to not give false alarms on -# tests that use async fixtures. -# For ruff and for running through flake8 we could expect users to use per-file-ignores, -# but running as standalone we don't currently support that. (though probs wouldn't be -# too bad to add support). # See e.g. https://github.com/agronholm/anyio/issues/803 for why one might want an async # test without awaits. +async def test_async_fixture( + my_async_fixture, +): # ASYNC910: 0, "exit", Statement("function definition", lineno) + assert my_async_fixture.setup_worked_correctly -async def test_async_fixture( - my_anyio_fixture, -): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) - assert my_anyio_fixture.setup_worked_correctly +# no params -> no async fixtures +async def test_no_fixture(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + print("blah") + + +# skip @overload. They should always be empty, but /shrug +@overload +async def foo_overload(): + print("blah") + + +async def foo_overload(): ... + + +# skip @[pytest.]fixture if they have any params, since they might depend on other +# async fixtures +@fixture +async def foo_fix(my_async_fixture): + print("blah") + + +# but @fixture with no params can be converted to sync +@fixture +async def foo_fix_no_subfix(): # ASYNC124: 0 + print("blah") diff --git a/tests/eval_files/async910.py b/tests/eval_files/async910.py index 4853437..e25a52b 100644 --- a/tests/eval_files/async910.py +++ b/tests/eval_files/async910.py @@ -126,9 +126,12 @@ def foo_normal_func_1(): def foo_normal_func_2(): ... -# overload decorator +# overload decorator is skipped +# overload functions should always be empty, so the check is somewhat redundant, +# but making one non-empty to check the logic. @overload -async def foo_overload_1(_: bytes): ... +async def foo_overload_1(_: bytes): + raise NotImplementedError @typing.overload diff --git a/tests/trio_options.py b/tests/trio_options.py index 34d9e68..daad4f8 100644 --- a/tests/trio_options.py +++ b/tests/trio_options.py @@ -1,8 +1,15 @@ """Test file used in tests/test_decator.py to check decorator and command line.""" +import asyncio + app = None +def condition() -> bool: + return False + + @app.route # type: ignore async def f(): - print("hello world") + if condition(): + await asyncio.sleep(0) From 42971f1ac5c88fc9df47c453d828408a25d2aec0 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Tue, 29 Oct 2024 16:34:44 +0100 Subject: [PATCH 3/7] sigh --- tests/eval_files/async124.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/eval_files/async124.py b/tests/eval_files/async124.py index c583aa3..586eeaf 100644 --- a/tests/eval_files/async124.py +++ b/tests/eval_files/async124.py @@ -68,9 +68,9 @@ async def foo_empty_pass(): # See e.g. https://github.com/agronholm/anyio/issues/803 for why one might want an async # test without awaits. -async def test_async_fixture( +async def test_async_fixture( # ASYNC910: 0, "exit", Statement("function definition", lineno) my_async_fixture, -): # ASYNC910: 0, "exit", Statement("function definition", lineno) +): assert my_async_fixture.setup_worked_correctly From 60e30d6b651fa80aaa7e013ce54d148ae06f7bfa Mon Sep 17 00:00:00 2001 From: jakkdl Date: Wed, 30 Oct 2024 16:51:58 +0100 Subject: [PATCH 4/7] async124 now ignores class methods. fixed two bugs in async91x --- docs/changelog.rst | 2 + flake8_async/visitors/visitor91x.py | 81 ++++++++++++++++++++++------ tests/autofix_files/async910.py | 13 +++++ tests/autofix_files/async910.py.diff | 11 +++- tests/eval_files/async124.py | 34 ++++++++++++ tests/eval_files/async910.py | 15 ++++++ 6 files changed, 138 insertions(+), 18 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 8ed36ea..496d2f8 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -7,6 +7,8 @@ Changelog 24.10.3 ======= - Add :ref:`ASYNC124 ` async-function-could-be-sync +- :ref:`ASYNC91x ` now correctly handles ``await()`` in parameter lists. +- Fixed a bug with :ref:`ASYNC91x ` and nested empty functions. 24.10.2 ======= diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 0cdd826..aa254b3 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -69,6 +69,7 @@ def func_empty_body(node: cst.FunctionDef) -> bool: ) +# this could've been implemented as part of visitor91x, but /shrug @error_class_cst class Visitor124(Flake8AsyncVisitor_cst): error_codes: Mapping[str, str] = { @@ -81,12 +82,37 @@ class Visitor124(Flake8AsyncVisitor_cst): def __init__(self, *args: Any, **kwargs: Any): super().__init__(*args, **kwargs) self.has_await = False + self.in_class = False + def visit_ClassDef(self, node: cst.ClassDef): + self.save_state(node, "in_class", copy=False) + self.in_class = True + + def leave_ClassDef( + self, original_node: cst.ClassDef, updated_node: cst.ClassDef + ) -> cst.ClassDef: + self.restore_state(original_node) + return updated_node + + # await in sync defs are not valid, but handling this will make ASYNC124 + # correctly pop up in parent func as if the child function was async def visit_FunctionDef(self, node: cst.FunctionDef): - # await in sync defs are not valid, but handling this will make ASYNC124 - # pop up in parent func as if the child function was async - self.save_state(node, "has_await", copy=False) - self.has_await = False + # default values are evaluated in parent scope + # this visitor has no autofixes, so we can throw away return value + _ = node.params.visit(self) + + self.save_state(node, "has_await", "in_class", copy=False) + + # ignore class methods + self.has_await = self.in_class + + # but not nested functions + self.in_class = False + + _ = node.body.visit(self) + + # we've manually visited subnodes (that we care about). + return False def leave_FunctionDef( self, original_node: cst.FunctionDef, updated_node: cst.FunctionDef @@ -99,7 +125,7 @@ def leave_FunctionDef( # skip functions named 'text_xxx' with params, since they may be relying # on async fixtures. This is esp bad as sync funcs relying on async fixtures # is not well handled: https://github.com/pytest-dev/pytest/issues/10839 - # also skip funcs with @fixtures and params + # also skip funcs with @fixture and params and not ( original_node.params.params and ( @@ -115,11 +141,12 @@ def leave_FunctionDef( def visit_Await(self, node: cst.Await): self.has_await = True - def visit_With(self, node: cst.With | cst.For): + def visit_With(self, node: cst.With | cst.For | cst.CompFor): if node.asynchronous is not None: self.has_await = True visit_For = visit_With + visit_CompFor = visit_With @dataclass @@ -348,6 +375,9 @@ def __init__(self, *args: Any, **kwargs: Any): # --exception-suppress-context-manager self.suppress_imported_as: list[str] = [] + # used to transfer new body between visit_FunctionDef and leave_FunctionDef + self.new_body: cst.BaseSuite | None = None + def should_autofix(self, node: cst.CSTNode, code: str | None = None) -> bool: if code is None: # pragma: no branch code = "ASYNC911" if self.has_yield else "ASYNC910" @@ -388,6 +418,10 @@ def visit_ImportFrom(self, node: cst.ImportFrom) -> None: return def visit_FunctionDef(self, node: cst.FunctionDef) -> bool: + # `await` in default values happen in parent scope + # we also know we don't ever modify parameters so we can ignore the return value + _ = node.params.visit(self) + # don't lint functions whose bodies solely consist of pass or ellipsis # @overload functions are also guaranteed to be empty # we also ignore pytest fixtures @@ -417,36 +451,49 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool: node.decorators, *self.options.no_checkpoint_warning_decorators ) ) - if not self.async_function: - # only visit subnodes if there is an async function defined inside - # this should improve performance on codebases with many sync functions - return any(m.findall(node, m.FunctionDef(asynchronous=m.Asynchronous()))) + # only visit subnodes if there is an async function defined inside + # this should improve performance on codebases with many sync functions + if not self.async_function and not any( + m.findall(node, m.FunctionDef(asynchronous=m.Asynchronous())) + ): + return False pos = self.get_metadata(PositionProvider, node).start # type: ignore self.uncheckpointed_statements = { Statement("function definition", pos.line, pos.column) # type: ignore } - return True + + # visit body + # we're not gonna get FlattenSentinel or RemovalSentinel + self.new_body = cast(cst.BaseSuite, node.body.visit(self)) + + # we know that leave_FunctionDef for this FunctionDef will run immediately after + # this function exits so we don't need to worry about save_state for new_body + return False def leave_FunctionDef( self, original_node: cst.FunctionDef, updated_node: cst.FunctionDef ) -> cst.FunctionDef: if ( - self.async_function + self.new_body is not None + and self.async_function # updated_node does not have a position, so we must send original_node and self.check_function_exit(original_node) and self.should_autofix(original_node) - and isinstance(updated_node.body, cst.IndentedBlock) + and isinstance(self.new_body, cst.IndentedBlock) ): # insert checkpoint at the end of body - new_body = list(updated_node.body.body) - new_body.append(self.checkpoint_statement()) - indentedblock = updated_node.body.with_changes(body=new_body) - updated_node = updated_node.with_changes(body=indentedblock) + new_body_block = list(self.new_body.body) + new_body_block.append(self.checkpoint_statement()) + self.new_body = self.new_body.with_changes(body=new_body_block) self.ensure_imported_library() + if self.new_body is not None: + updated_node = updated_node.with_changes(body=self.new_body) self.restore_state(original_node) + # reset self.new_body + self.new_body = None return updated_node # error if function exit/return/yields with uncheckpointed statements diff --git a/tests/autofix_files/async910.py b/tests/autofix_files/async910.py index 95345af..274307f 100644 --- a/tests/autofix_files/async910.py +++ b/tests/autofix_files/async910.py @@ -619,3 +619,16 @@ async def fn_226(): # error: 0, "exit", Statement("function definition", lineno except Exception: pass await trio.lowlevel.checkpoint() + +# the await() is evaluated in the parent scope +async def foo_default_value_await(): + async def bar(arg=await foo()): # error: 4, "exit", Statement("function definition", lineno) + print() + await trio.lowlevel.checkpoint() + + +async def foo_nested_empty_async(): + # this previously errored because leave_FunctionDef assumed a non-empty body + async def bar(): + ... + await foo() diff --git a/tests/autofix_files/async910.py.diff b/tests/autofix_files/async910.py.diff index 8974f7f..2b06834 100644 --- a/tests/autofix_files/async910.py.diff +++ b/tests/autofix_files/async910.py.diff @@ -213,8 +213,17 @@ # Issue #226 -@@ x,3 x,4 @@ +@@ x,11 x,13 @@ pass except Exception: pass + await trio.lowlevel.checkpoint() + + # the await() is evaluated in the parent scope + async def foo_default_value_await(): + async def bar(arg=await foo()): # error: 4, "exit", Statement("function definition", lineno) + print() ++ await trio.lowlevel.checkpoint() + + + async def foo_nested_empty_async(): diff --git a/tests/eval_files/async124.py b/tests/eval_files/async124.py index 586eeaf..45017dd 100644 --- a/tests/eval_files/async124.py +++ b/tests/eval_files/async124.py @@ -99,3 +99,37 @@ async def foo_fix(my_async_fixture): @fixture async def foo_fix_no_subfix(): # ASYNC124: 0 print("blah") + + +async def default_value(): + def foo(arg=await foo()): ... + + +# 124 doesn't care if you evaluate the comprehension or not +# 910 is stingy +async def foo_async_gen(): + return ( + a async for a in foo_gen() + ) # ASYNC910: 4, "return", Statement("function definition", lineno-1) + + +async def foo_async_for_comprehension(): + return [a async for a in foo_gen()] + + +class Foo: + # async124 ignores class methods + async def bar( + self, + ): # ASYNC910: 4, "exit", Statement("function definition", lineno) + async def bee(): # ASYNC124: 8 # ASYNC910: 8, "exit", Statement("function definition", lineno) + print("blah") + + async def later_in_class( + self, + ): # ASYNC910: 4, "exit", Statement("function definition", lineno) + print() + + +async def after_class(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) + print() diff --git a/tests/eval_files/async910.py b/tests/eval_files/async910.py index e25a52b..2e399b3 100644 --- a/tests/eval_files/async910.py +++ b/tests/eval_files/async910.py @@ -589,3 +589,18 @@ async def fn_226(): # error: 0, "exit", Statement("function definition", lineno pass except Exception: pass + + +# the await() is evaluated in the parent scope +async def foo_default_value_await(): + async def bar( + arg=await foo(), + ): # error: 4, "exit", Statement("function definition", lineno) + print() + + +async def foo_nested_empty_async(): + # this previously errored because leave_FunctionDef assumed a non-empty body + async def bar(): ... + + await foo() From 9d30727999f752a9248177ee65cedba489f15c13 Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 31 Oct 2024 11:18:03 +0100 Subject: [PATCH 5/7] ugh, I've gotten out of the habit of watching the black reformats --- tests/autofix_files/async910.py | 9 ++++++--- tests/autofix_files/async910.py.diff | 8 +++++--- tests/eval_files/async910.py | 4 ++-- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/tests/autofix_files/async910.py b/tests/autofix_files/async910.py index 274307f..4d97ec3 100644 --- a/tests/autofix_files/async910.py +++ b/tests/autofix_files/async910.py @@ -620,15 +620,18 @@ async def fn_226(): # error: 0, "exit", Statement("function definition", lineno pass await trio.lowlevel.checkpoint() + # the await() is evaluated in the parent scope async def foo_default_value_await(): - async def bar(arg=await foo()): # error: 4, "exit", Statement("function definition", lineno) + async def bar( # error: 4, "exit", Statement("function definition", lineno) + arg=await foo(), + ): print() await trio.lowlevel.checkpoint() async def foo_nested_empty_async(): # this previously errored because leave_FunctionDef assumed a non-empty body - async def bar(): - ... + async def bar(): ... + await foo() diff --git a/tests/autofix_files/async910.py.diff b/tests/autofix_files/async910.py.diff index 2b06834..3fb6131 100644 --- a/tests/autofix_files/async910.py.diff +++ b/tests/autofix_files/async910.py.diff @@ -213,15 +213,17 @@ # Issue #226 -@@ x,11 x,13 @@ +@@ x,6 x,7 @@ pass except Exception: pass + await trio.lowlevel.checkpoint() + # the await() is evaluated in the parent scope - async def foo_default_value_await(): - async def bar(arg=await foo()): # error: 4, "exit", Statement("function definition", lineno) +@@ x,6 x,7 @@ + arg=await foo(), + ): print() + await trio.lowlevel.checkpoint() diff --git a/tests/eval_files/async910.py b/tests/eval_files/async910.py index 2e399b3..f8d7680 100644 --- a/tests/eval_files/async910.py +++ b/tests/eval_files/async910.py @@ -593,9 +593,9 @@ async def fn_226(): # error: 0, "exit", Statement("function definition", lineno # the await() is evaluated in the parent scope async def foo_default_value_await(): - async def bar( + async def bar( # error: 4, "exit", Statement("function definition", lineno) arg=await foo(), - ): # error: 4, "exit", Statement("function definition", lineno) + ): print() From c40b2538c13e03e06f85ed402ccad03abb0de8eb Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 31 Oct 2024 11:27:36 +0100 Subject: [PATCH 6/7] enable no_checkpoint_warning_decorators for async124. Remove unused Visitor91x.safe_decorator. --- flake8_async/visitors/visitor91x.py | 8 +++++--- tests/eval_files/async124.py | 20 ++++++++++++++------ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index aa254b3..0eaaab9 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -133,6 +133,10 @@ def leave_FunctionDef( or func_has_decorator(original_node, "fixture") ) ) + # ignore functions with no_checkpoint_warning_decorators + and not fnmatch_qualified_name_cst( + original_node.decorators, *self.options.no_checkpoint_warning_decorators + ) ): self.error(original_node) self.restore_state(original_node) @@ -360,7 +364,6 @@ class Visitor91X(Flake8AsyncVisitor_cst, CommonVisitors): def __init__(self, *args: Any, **kwargs: Any): super().__init__(*args, **kwargs) self.has_yield = False - self.safe_decorator = False self.async_function = False self.uncheckpointed_statements: set[Statement] = set() self.comp_unknown = False @@ -431,7 +434,6 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool: self.save_state( node, "has_yield", - "safe_decorator", "async_function", "uncheckpointed_statements", "loop_state", @@ -442,7 +444,7 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool: ) self.uncheckpointed_statements = set() self.has_checkpoint_stack = [] - self.has_yield = self.safe_decorator = False + self.has_yield = False self.loop_state = LoopState() self.async_function = ( diff --git a/tests/eval_files/async124.py b/tests/eval_files/async124.py index 45017dd..398acf0 100644 --- a/tests/eval_files/async124.py +++ b/tests/eval_files/async124.py @@ -2,6 +2,7 @@ It currently does not care if 910/911 would also be triggered.""" # ARG --enable=ASYNC124,ASYNC910,ASYNC911 +# ARG --no-checkpoint-warning-decorator=custom_disabled_decorator # 910/911 will also autofix async124, in the sense of adding a checkpoint. This is perhaps # not what the user wants though, so this would be a case in favor of making 910/911 not @@ -10,6 +11,8 @@ from typing import Any, overload from pytest import fixture +custom_disabled_decorator: Any = ... + def condition() -> bool: return False @@ -108,9 +111,9 @@ def foo(arg=await foo()): ... # 124 doesn't care if you evaluate the comprehension or not # 910 is stingy async def foo_async_gen(): - return ( + return ( # ASYNC910: 4, "return", Statement("function definition", lineno-1) a async for a in foo_gen() - ) # ASYNC910: 4, "return", Statement("function definition", lineno-1) + ) async def foo_async_for_comprehension(): @@ -119,17 +122,22 @@ async def foo_async_for_comprehension(): class Foo: # async124 ignores class methods - async def bar( + async def bar( # ASYNC910: 4, "exit", Statement("function definition", lineno) self, - ): # ASYNC910: 4, "exit", Statement("function definition", lineno) + ): async def bee(): # ASYNC124: 8 # ASYNC910: 8, "exit", Statement("function definition", lineno) print("blah") - async def later_in_class( + async def later_in_class( # ASYNC910: 4, "exit", Statement("function definition", lineno) self, - ): # ASYNC910: 4, "exit", Statement("function definition", lineno) + ): print() async def after_class(): # ASYNC124: 0 # ASYNC910: 0, "exit", Statement("function definition", lineno) print() + + +@custom_disabled_decorator +async def foo_has_custom_disabled_decorator(): + print() From 7f7d1cb4f4ea65b8af274c647ae52c211e8e6eaf Mon Sep 17 00:00:00 2001 From: jakkdl Date: Thu, 7 Nov 2024 18:34:33 +0100 Subject: [PATCH 7/7] fix genexp --- flake8_async/visitors/visitor91x.py | 11 +++++++++-- tests/eval_files/async124.py | 11 +++++++---- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/flake8_async/visitors/visitor91x.py b/flake8_async/visitors/visitor91x.py index 0eaaab9..613cdf9 100644 --- a/flake8_async/visitors/visitor91x.py +++ b/flake8_async/visitors/visitor91x.py @@ -152,6 +152,13 @@ def visit_With(self, node: cst.With | cst.For | cst.CompFor): visit_For = visit_With visit_CompFor = visit_With + # The generator target will be immediately evaluated, but the other + # elements will not be evaluated at the point of defining the GenExp. + # To consume those needs an explicit syntactic checkpoint + def visit_GeneratorExp(self, node: cst.GeneratorExp): + node.for_in.iter.visit(self) + return False + @dataclass class LoopState: @@ -1086,8 +1093,8 @@ def visit_CompFor(self, node: cst.CompFor): return False # The generator target will be immediately evaluated, but the other - # elements will be lazily evaluated as the generator is consumed so we don't - # visit them as any checkpoints in them are not guaranteed to execute. + # elements will not be evaluated at the point of defining the GenExp. + # To consume those needs an explicit syntactic checkpoint def visit_GeneratorExp(self, node: cst.GeneratorExp): node.for_in.iter.visit(self) return False diff --git a/tests/eval_files/async124.py b/tests/eval_files/async124.py index 398acf0..5815f16 100644 --- a/tests/eval_files/async124.py +++ b/tests/eval_files/async124.py @@ -108,14 +108,17 @@ async def default_value(): def foo(arg=await foo()): ... -# 124 doesn't care if you evaluate the comprehension or not -# 910 is stingy -async def foo_async_gen(): +# only the expression in genexp's get checked +async def foo_async_gen(): # ASYNC124: 0 return ( # ASYNC910: 4, "return", Statement("function definition", lineno-1) - a async for a in foo_gen() + await a async for a in foo_gen() ) +async def foo_async_gen_await(): + return (a for a in await foo_gen()) + + async def foo_async_for_comprehension(): return [a async for a in foo_gen()]