add async124 async-function-could-be-sync (#309)

* add async124 async-function-could-be-sync

* fixed two bugs in async91x

* enable no_checkpoint_warning_decorators for async124. Remove unused Visitor91x.safe_decorator.

* mention ASYNC124 in docs/usage.rst, and convert mentions of PEP to :pep:

Author: jakkdl

docs/changelog.rst

@@ -4,6 +4,12 @@ Changelog
 
 `CalVer, YY.month.patch <https://calver.org/>`_
 
+25.1.1
+=======
+- Add :ref:`ASYNC124 <async124>` async-function-could-be-sync
+- :ref:`ASYNC91x <ASYNC910>` now correctly handles ``await()`` in parameter lists.
+- Fixed a bug with :ref:`ASYNC91x <ASYNC910>` and nested empty functions.
+
 24.11.4
 =======
 - :ref:`ASYNC100 <async100>` once again ignores :func:`trio.open_nursery` and :func:`anyio.create_task_group`, unless we find a call to ``.start_soon()``.
@@ -238,7 +244,7 @@ Changelog
 
 22.9.2
 ======
-- Fix a crash on nontrivial decorator expressions (calls, PEP-614) and document behavior.
+- Fix a crash on nontrivial decorator expressions (calls, :pep:`614`) and document behavior.
 
 22.9.1
 ======

docs/rules.rst

@@ -19,7 +19,7 @@ _`ASYNC100` : cancel-scope-no-checkpoint
 _`ASYNC101` : yield-in-cancel-scope
     ``yield`` inside a :ref:`taskgroup_nursery` or :ref:`timeout_context` is only safe when implementing a context manager - otherwise, it breaks exception handling.
     See `this thread <https://discuss.python.org/t/preventing-yield-inside-certain-context-managers/1091/23>`_ for discussion of a future PEP.
-    This has substantial overlap with :ref:`ASYNC119 <ASYNC119>`, which will warn on almost all instances of ASYNC101, but ASYNC101 is about a conceptually different problem that will not get resolved by `PEP 533 <https://peps.python.org/pep-0533/>`_.
+    This has substantial overlap with :ref:`ASYNC119 <ASYNC119>`, which will warn on almost all instances of ASYNC101, but ASYNC101 is about a conceptually different problem that will not get resolved by :pep:`533`.
 
 _`ASYNC102` : await-in-finally-or-cancelled
     ``await`` inside ``finally``, :ref:`cancelled-catching <cancelled>` ``except:``, or ``__aexit__`` must have shielded :ref:`cancel scope <cancel_scope>` with timeout.
@@ -76,7 +76,7 @@ ASYNC118 : cancelled-class-saved
 
 _`ASYNC119` : yield-in-cm-in-async-gen
    ``yield`` in context manager in async generator is unsafe, the cleanup may be delayed until ``await`` is no longer allowed.
-   We strongly encourage you to read `PEP 533 <https://peps.python.org/pep-0533/>`_ and use `async with aclosing(...) <https://docs.python.org/3/library/contextlib.html#contextlib.aclosing>`_, or better yet avoid async generators entirely (see `ASYNC900`_ ) in favor of context managers which return an iterable :ref:`channel/stream/queue <channel_stream_queue>`.
+   We strongly encourage you to read :pep:`533` and use `async with aclosing(...) <https://docs.python.org/3/library/contextlib.html#contextlib.aclosing>`_, or better yet avoid async generators entirely (see `ASYNC900`_ ) in favor of context managers which return an iterable :ref:`channel/stream/queue <channel_stream_queue>`.
 
 _`ASYNC120` : await-in-except
     Dangerous :ref:`checkpoint` inside an ``except`` block.
@@ -95,6 +95,13 @@ _`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 <ASYNC910>` and :ref:`ASYNC911 <ASYNC911>` which, if enabled, will autofix the function to have :ref:`checkpoint`.
+    This excludes class methods as they often have to be async for other reasons, if you really do want to check those you could manually run :ref:`ASYNC910 <ASYNC910>` and/or :ref:`ASYNC911 <ASYNC911>` and check the methods they trigger on.
+
 Blocking sync calls in async functions
 ======================================
 

docs/usage.rst

@@ -33,7 +33,7 @@ adding the following to your ``.pre-commit-config.yaml``:
    minimum_pre_commit_version: '2.9.0'
    repos:
    - repo: https://github.com/python-trio/flake8-async
-     rev: 24.11.4
+     rev: 25.1.1
      hooks:
        - id: flake8-async
          # args: [--enable=ASYNC, --disable=ASYNC9, --autofix=ASYNC]
@@ -215,11 +215,11 @@ Example
 ``no-checkpoint-warning-decorators``
 ------------------------------------
 
-Comma-separated list of decorators to disable checkpointing checks for, turning off :ref:`ASYNC910 <async910>` and :ref:`ASYNC911 <async911>` warnings for functions decorated with any decorator matching against an entry in the list.
+Comma-separated list of decorators to disable checkpointing checks for, turning off :ref:`ASYNC910 <async910>`, :ref:`ASYNC911 <async911>`, and :ref:`ASYNC124 <async124>` warnings for functions decorated with any decorator matching against an entry in the list.
 Matching is done with `fnmatch <https://docs.python.org/3/library/fnmatch.html>`_.
 Defaults to disabling for ``asynccontextmanager``.
 
-Decorators-to-match must be identifiers or dotted names only (not PEP-614 expressions), and will match against the name only - e.g. ``foo.bar`` matches ``foo.bar``, ``foo.bar()``, and ``foo.bar(args, here)``, etc.
+Decorators-to-match must be identifiers or dotted names only (not :pep:`614` expressions), and will match against the name only - e.g. ``foo.bar`` matches ``foo.bar``, ``foo.bar()``, and ``foo.bar(args, here)``, etc.
 
 Example
 ^^^^^^^

flake8_async/__init__.py

@@ -38,7 +38,7 @@
 
 
 # CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1"
-__version__ = "24.11.4"
+__version__ = "25.1.1"
 
 
 # taken from https://github.com/Zac-HD/shed

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
@@ -64,6 +70,92 @@ 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] = {
+        "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
+        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):
+        # 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
+    ) -> cst.FunctionDef:
+        if (
+            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 with @fixture and params since they may be relying
+            # on async fixtures.
+            and not (
+                original_node.params.params
+                and 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)
+        return updated_node
+
+    def visit_Await(self, node: cst.Await):
+        self.has_await = True
+
+    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
+
+    # 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:
     infinite_loop: bool = False
@@ -275,7 +367,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
@@ -291,6 +382,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"
@@ -349,6 +443,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
@@ -358,7 +456,6 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool:
         self.save_state(
             node,
             "has_yield",
-            "safe_decorator",
             "async_function",
             "uncheckpointed_statements",
             # comp_unknown does not need to be saved
@@ -370,11 +467,11 @@ def visit_FunctionDef(self, node: cst.FunctionDef) -> bool:
             "suppress_imported_as",  # a copy is saved, but state is not reset
             copy=True,
         )
-        self.has_yield = self.safe_decorator = False
         self.uncheckpointed_statements = set()
+        self.has_checkpoint_stack = []
+        self.has_yield = False
         self.loop_state = LoopState()
         # try_state is reset upon entering try
-        self.has_checkpoint_stack = []
         self.taskgroup_has_start_soon = {}
 
         self.async_function = (
@@ -383,36 +480,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
@@ -1039,8 +1149,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

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
@@ -616,3 +619,19 @@ 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(  # 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(): ...
+
+    await foo()

tests/autofix_files/async910.py.diff

@@ -213,8 +213,19 @@
 
 
  # Issue #226
-@@ x,3 x,4 @@
+@@ x,6 x,7 @@
                  pass
      except Exception:
          pass
 +    await trio.lowlevel.checkpoint()
+
+
+ # the await() is evaluated in the parent scope
+@@ x,6 x,7 @@
+         arg=await foo(),
+     ):
+         print()
++        await trio.lowlevel.checkpoint()
+
+
+ async def foo_nested_empty_async():