Add ASYNC913, indefinite-loop-no-guaranteed-checkpoint. Fix async91x bugs (#255)

* Add ASYNC913, indefinite-loop-no-guaranteed-checkpoint.
* Fix autofix bugs in async910 & 911.

Author: jakkdl

docs/changelog.rst

@@ -4,6 +4,14 @@ Changelog
 
 *[CalVer, YY.month.patch](https://calver.org/)*
 
+24.5.4
+======
+- Add ASYNC913: Indefinite loop with no guaranteed checkpoint.
+- Fix bugs in ASYNC910 and ASYNC911 autofixing where they sometimes didn't add a library import.
+- Fix crash in ASYNC911 when trying to autofix a one-line ``while ...: yield``
+- Add :ref:`exception-suppress-context-managers`. Contextmanagers that may suppress exceptions.
+- ASYNC91x now treats checkpoints inside ``with contextlib.suppress`` as unreliable.
+
 24.5.3
 ======
 - Rename config option ``trio200-blocking-calls`` to :ref:`async200-blocking-calls`.

docs/rules.rst

@@ -146,6 +146,9 @@ _`ASYNC912` : cancel-scope-no-guaranteed-checkpoint
     Similar to `ASYNC100`_, but it does not warn on trivial cases where there is no checkpoint at all.
     It instead shares logic with `ASYNC910`_ and `ASYNC911`_ for parsing conditionals and branches.
 
+_`ASYNC913` : indefinite-loop-no-guaranteed-checkpoint
+    An indefinite loop (e.g. ``while True``) has no guaranteed :ref:`checkpoint <checkpoint>`. This could potentially cause a deadlock.
+
 .. _autofix-support:
 
 Autofix support
@@ -154,6 +157,7 @@ The following rules support :ref:`autofixing <autofix>`.
 - :ref:`ASYNC100 <ASYNC100>`
 - :ref:`ASYNC910 <ASYNC910>`
 - :ref:`ASYNC911 <ASYNC911>`
+- :ref:`ASYNC913 <ASYNC913>`
 
 Removed rules
 ================

docs/usage.rst

@@ -232,6 +232,8 @@ Example
      ign*,
      *.ignore,
 
+.. _exception-suppress-context-managers:
+
 ``exception-suppress-context-managers``
 ---------------------------------------
 

flake8_async/__init__.py

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

flake8_async/visitors/visitor91x.py

@@ -9,7 +9,7 @@
 
 from abc import ABC, abstractmethod
 from dataclasses import dataclass, field
-from typing import TYPE_CHECKING, Any
+from typing import TYPE_CHECKING, Any, cast
 
 import libcst as cst
 import libcst.matchers as m
@@ -73,8 +73,8 @@ class LoopState:
     uncheckpointed_before_break: set[Statement] = field(default_factory=set)
 
     artificial_errors: set[cst.Return | cst.Yield] = field(default_factory=set)
-    nodes_needing_checkpoints: list[cst.Return | cst.Yield] = field(
-        default_factory=list
+    nodes_needing_checkpoints: list[cst.Return | cst.Yield | ArtificialStatement] = (
+        field(default_factory=list)
     )
 
     def copy(self):
@@ -215,8 +215,10 @@ def __init__(
         self,
         nodes_needing_checkpoint: Sequence[cst.Yield | cst.Return],
         library: tuple[str, ...],
+        explicitly_imported: dict[str, bool],
     ):
         super().__init__()
+        self.explicitly_imported_library = explicitly_imported
         self.nodes_needing_checkpoint = nodes_needing_checkpoint
         self.__library = library
 
@@ -262,6 +264,7 @@ class Visitor91X(Flake8AsyncVisitor_cst, CommonVisitors):
             "CancelScope with no guaranteed checkpoint. This makes it potentially "
             "impossible to cancel."
         ),
+        "ASYNC913": ("Indefinite loop with no guaranteed checkpoint."),
         "ASYNC100": (
             "{0}.{1} context contains no checkpoints, remove the context or add"
             " `await {0}.lowlevel.checkpoint()`."
@@ -382,6 +385,8 @@ def leave_FunctionDef(
             indentedblock = updated_node.body.with_changes(body=new_body)
             updated_node = updated_node.with_changes(body=indentedblock)
 
+            self.ensure_imported_library()
+
         self.restore_state(original_node)
         return updated_node
 
@@ -772,6 +777,18 @@ def leave_While_body(self, node: cst.For | cst.While):
         if not any_error:
             self.loop_state.nodes_needing_checkpoints = []
 
+        if (
+            self.loop_state.infinite_loop
+            and not self.loop_state.has_break
+            and ARTIFICIAL_STATEMENT in self.uncheckpointed_statements
+            and self.error(node, error_code="ASYNC913")
+        ):
+            # We can override nodes_needing_checkpoints, as that's solely for checkpoints
+            # that error because of the artificial statement injected at the start of
+            # the loop. When inserting a checkpoint at the start of the loop, those
+            # will be remedied
+            self.loop_state.nodes_needing_checkpoints = [ARTIFICIAL_STATEMENT]
+
         # replace artificial statements in else with prebody uncheckpointed statements
         # non-artificial stmts before continue/break/at body end will already be in them
         for stmts in (
@@ -832,15 +849,38 @@ def leave_While(
         | cst.FlattenSentinel[cst.For | cst.While]
         | cst.RemovalSentinel
     ):
-        if self.loop_state.nodes_needing_checkpoints:
+        # don't bother autofixing same-line loops
+        if isinstance(updated_node.body, cst.SimpleStatementSuite):
+            self.restore_state(original_node)
+            return updated_node
+
+        # ASYNC913, indefinite loop with no guaranteed checkpoint
+        if self.loop_state.nodes_needing_checkpoints == [ARTIFICIAL_STATEMENT]:
+            if self.should_autofix(original_node, code="ASYNC913"):
+                # insert checkpoint at start of body
+                new_body = list(updated_node.body.body)
+                new_body.insert(0, self.checkpoint_statement())
+                indentedblock = updated_node.body.with_changes(body=new_body)
+                updated_node = updated_node.with_changes(body=indentedblock)
+
+                self.ensure_imported_library()
+        elif self.loop_state.nodes_needing_checkpoints:
+            assert ARTIFICIAL_STATEMENT not in self.loop_state.nodes_needing_checkpoints
             transformer = InsertCheckpointsInLoopBody(
-                self.loop_state.nodes_needing_checkpoints, self.library
+                cast(
+                    "list[cst.Yield | cst.Return]",
+                    self.loop_state.nodes_needing_checkpoints,
+                ),
+                self.library,
+                self.explicitly_imported_library,
             )
             # type of updated_node expanded to the return type
             updated_node = updated_node.visit(transformer)  # type: ignore
 
+            # include any necessary import added
+            self.add_import.update(transformer.add_import)
+
         self.restore_state(original_node)
-        # https://github.com/afonasev/flake8-return/issues/133
         return updated_node
 
     leave_For = leave_While

pyproject.toml

@@ -7,6 +7,10 @@ remove-all-unused-imports = true
 remove-duplicate-keys = true
 remove-unused-variables = true
 
+[tool.black]
+# need to use force-exclude since pre-commit directly specifies files
+force-exclude = "tests/autofix_files/.*.py"
+
 [tool.codespell]
 ignore-words-list = 'spawnve'
 
@@ -16,7 +20,7 @@ profile = "black"
 py_version = "311"
 quiet = true
 skip_gitignore = true
-skip_glob = "tests/eval_files/*"
+skip_glob = "tests/*_files/*"
 
 [tool.mypy]
 check_untyped_defs = true

tests/autofix_files/async910_insert_library.py

@@ -0,0 +1,14 @@
+# ensure that import gets added when adding checkpoints at end of function body
+
+# AUTOFIX
+# ASYNCIO_NO_AUTOFIX
+
+
+import trio
+def condition() -> bool:
+    return False
+
+
+async def foo():  # ASYNC910: 0, "exit", Stmt("function definition", line)
+    print()
+    await trio.lowlevel.checkpoint()

tests/autofix_files/async910_insert_library.py.diff

@@ -0,0 +1,14 @@
+---
++++
+@@ x,9 x,11 @@
+ # ASYNCIO_NO_AUTOFIX
+
+
++import trio
+ def condition() -> bool:
+     return False
+
+
+ async def foo():  # ASYNC910: 0, "exit", Stmt("function definition", line)
+     print()
++    await trio.lowlevel.checkpoint()

tests/autofix_files/async911_insert_library.py

@@ -0,0 +1,17 @@
+# ensure that import gets added when adding checkpoints in loop body
+
+# AUTOFIX
+# ASYNCIO_NO_AUTOFIX
+
+
+import trio
+def condition() -> bool:
+    return False
+
+
+async def foo():
+    await foo()
+    while condition():
+        await trio.lowlevel.checkpoint()
+        yield  # ASYNC911: 8, "yield", Stmt("yield", lineno)
+    await foo()

tests/autofix_files/async911_insert_library.py.diff

@@ -0,0 +1,17 @@
+---
++++
+@@ x,6 x,7 @@
+ # ASYNCIO_NO_AUTOFIX
+
+
++import trio
+ def condition() -> bool:
+     return False
+
+@@ x,5 x,6 @@
+ async def foo():
+     await foo()
+     while condition():
++        await trio.lowlevel.checkpoint()
+         yield  # ASYNC911: 8, "yield", Stmt("yield", lineno)
+     await foo()

tests/autofix_files/async913.py

@@ -0,0 +1,91 @@
+# ARG --enable=ASYNC910,ASYNC911,ASYNC913
+# AUTOFIX
+# ASYNCIO_NO_AUTOFIX
+
+
+import trio
+def condition() -> bool:
+    return False
+
+
+async def foo():
+    while True:  # ASYNC913: 4
+        await trio.lowlevel.checkpoint()
+        ...
+
+
+async def foo2():
+    while True:
+        await foo()
+
+
+async def foo3():
+    while True:  # ASYNC913: 4
+        await trio.lowlevel.checkpoint()
+        if condition():
+            await foo()
+
+
+# ASYNC913 does not trigger on loops with break, but those will generally be handled
+# by 910/911/912 if necessary
+async def foo_break():  # ASYNC910: 0, "exit", Statement("function definition", lineno)
+    while True:
+        if condition():
+            break
+    await trio.lowlevel.checkpoint()
+
+
+# the inner loop will suppress the error in the outer loop
+async def foo_nested():
+    while True:
+        while True:  # ASYNC913: 8
+            await trio.lowlevel.checkpoint()
+            ...
+
+
+async def foo_conditional_nested():
+    while True:  # ASYNC913: 4
+        await trio.lowlevel.checkpoint()
+        if condition():
+            while True:  # ASYNC913: 12
+                await trio.lowlevel.checkpoint()
+                ...
+
+
+# various checks I added for my own sanity to ensure autofixes worked when multiple
+# codes simultaneously want to autofix.
+
+
+async def foo_indef_and_910():
+    while True:  # ASYNC913: 4
+        await trio.lowlevel.checkpoint()
+        if ...:
+            await foo()
+            return
+
+
+async def foo_indef_and_910_2():
+    while True:  # ASYNC913: 4
+        await trio.lowlevel.checkpoint()
+        if ...:
+            return  # ASYNC910: 12, "return", Stmt("function definition", line-3)
+
+
+async def foo_indef_and_911():
+    await foo()
+    while True:  # ASYNC913: 4
+        await trio.lowlevel.checkpoint()
+        if condition():
+            yield  # ASYNC911: 12, "yield", Stmt("yield", line)  # ASYNC911: 12, "yield", Stmt("yield", line+2)
+        if condition():
+            await trio.lowlevel.checkpoint()
+            yield  # ASYNC911: 12, "yield", Stmt("yield", line)  # ASYNC911: 12, "yield", Stmt("yield", line-2)  # ASYNC911: 12, "yield", Stmt("yield", line-2)
+
+
+async def foo_indef_and_911_2():
+    await foo()
+    while True:  # ASYNC913: 4
+        await trio.lowlevel.checkpoint()
+        while condition():
+            await trio.lowlevel.checkpoint()
+            yield  # ASYNC911: 12, "yield", Stmt("yield", line)