add ASYNC300 create-task-no-reference (#256)

* add ASYNC120 create-task-no-reference

* fix crash when subscripts are in attributes

* codecov

* Rename 120->300, add rules notes

---------

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>

Author: jakkdl

docs/changelog.rst

@@ -4,13 +4,17 @@ Changelog
 
 *[CalVer, YY.month.patch](https://calver.org/)*
 
+24.5.5
+======
+- Add :ref:`ASYNC300 <async300>` create-task-no-reference
+
 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:`ASYNC913 <async913>`: Indefinite loop with no guaranteed checkpoint.
+- Fix bugs in :ref:`ASYNC910 <async910>` and :ref:`ASYNC911 <async911>` autofixing where they sometimes didn't add a library import.
+- Fix crash in :ref:`ASYNC911 <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.
+- :ref:`ASYNC91x <ASYNC910>` now treats checkpoints inside ``with contextlib.suppress`` as unreliable.
 
 24.5.3
 ======

docs/rules.rst

@@ -6,6 +6,9 @@ Rules
 General rules
 =============
 
+Our ``ASYNC1xx`` rules check for semantic problems ranging from fatal errors (e.g. 101),
+to idioms for clearer code (e.g. 116).
+
 _`ASYNC100` : cancel-scope-no-checkpoint
     A :ref:`timeout_context` does not contain any :ref:`checkpoints <checkpoint>`.
     This makes it pointless, as the timeout can only be triggered by a checkpoint.
@@ -71,10 +74,13 @@ _`ASYNC119` : yield-in-cm-in-async-gen
    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>`.
 
 
-
 Blocking sync calls in async functions
 ======================================
 
+Our 2xx lint rules warn you to use the async equivalent for slow sync calls which
+would otherwise block the event loop (and therefore cause performance problems,
+or even deadlock).
+
 .. _httpx.Client: https://www.python-httpx.org/api/#client
 .. _httpx.AsyncClient: https://www.python-httpx.org/api/#asyncclient
 .. _urllib3: https://github.com/urllib3/urllib3
@@ -126,9 +132,27 @@ ASYNC251 : blocking-sleep
     Use :func:`trio.sleep`/:func:`anyio.sleep`/:func:`asyncio.sleep`.
 
 
+Asyncio-specific rules
+======================
+
+Asyncio *encourages* structured concurrency, with :obj:`asyncio.TaskGroup`, but does not *require* it.
+We therefore provide some additional lint rules for common problems - although we'd also recommend a
+gradual migration to AnyIO, which is much less error-prone.
+
+_`ASYNC300` : create-task-no-reference
+    Calling :func:`asyncio.create_task` without saving the result. A task that isn't referenced elsewhere may get garbage collected at any time, even before it's done.
+    Note that this rule won't check whether the variable the result is saved in is susceptible to being garbage-collected itself. See the asyncio documentation for best practices.
+    You might consider instead using a :ref:`TaskGroup <taskgroup_nursery>` and calling :meth:`asyncio.TaskGroup.create_task` to avoid this problem, and gain the advantages of structured concurrency with e.g. better cancellation semantics.
+
+
 Optional rules disabled by default
 ==================================
 
+Our 9xx rules check for semantics issues, like 1xx rules, but are disabled by default due
+to the higher volume of warnings.  We encourage you to enable them - without guaranteed
+:ref:`checkpoint`\ s timeouts and cancellation can be arbitrarily delayed, and async
+generators are prone to the problems described in :pep:`533`.
+
 _`ASYNC900` : unsafe-async-generator
        Async generator without :func:`@asynccontextmanager <contextlib.asynccontextmanager>` not allowed.
        You might want to enable this on a codebase since async generators are inherently unsafe and cleanup logic might not be performed.

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.4"
+__version__ = "24.5.5"
 
 
 # taken from https://github.com/Zac-HD/shed

flake8_async/visitors/helpers.py

@@ -337,11 +337,15 @@ def build_cst_matcher(attr: str) -> m.BaseExpression:
     return m.Attribute(value=build_cst_matcher(body), attr=m.Name(value=tail))
 
 
-def identifier_to_string(attr: cst.Name | cst.Attribute) -> str:
+def identifier_to_string(attr: cst.Name | cst.Attribute) -> str | None:
     if isinstance(attr, cst.Name):
         return attr.value
-    assert isinstance(attr.value, (cst.Attribute, cst.Name))
-    return identifier_to_string(attr.value) + "." + attr.attr.value
+    if not isinstance(attr.value, (cst.Attribute, cst.Name)):
+        return None
+    lhs = identifier_to_string(attr.value)
+    if lhs is None:
+        return None
+    return lhs + "." + attr.attr.value
 
 
 def with_has_call(
@@ -383,10 +387,10 @@ def with_has_call(
             assert isinstance(item.item, cst.Call)
             assert isinstance(res["base"], (cst.Name, cst.Attribute))
             assert isinstance(res["function"], cst.Name)
+            base_string = identifier_to_string(res["base"])
+            assert base_string is not None, "subscripts should never get matched"
             res_list.append(
-                AttributeCall(
-                    item.item, identifier_to_string(res["base"]), res["function"].value
-                )
+                AttributeCall(item.item, base_string, res["function"].value)
             )
     return res_list
 

flake8_async/visitors/visitors.py

@@ -5,8 +5,17 @@
 import ast
 from typing import TYPE_CHECKING, Any, cast
 
-from .flake8asyncvisitor import Flake8AsyncVisitor
-from .helpers import disabled_by_default, error_class, get_matching_call, has_decorator
+import libcst as cst
+
+from .flake8asyncvisitor import Flake8AsyncVisitor, Flake8AsyncVisitor_cst
+from .helpers import (
+    disabled_by_default,
+    error_class,
+    error_class_cst,
+    get_matching_call,
+    has_decorator,
+    identifier_to_string,
+)
 
 if TYPE_CHECKING:
     from collections.abc import Mapping
@@ -332,6 +341,53 @@ def visit_Yield(self, node: ast.Yield):
     visit_Lambda = visit_AsyncFunctionDef
 
 
+@error_class_cst
+class Visitor300(Flake8AsyncVisitor_cst):
+    error_codes: Mapping[str, str] = {
+        "ASYNC300": "asyncio.create_task() called without saving the result"
+    }
+
+    def __init__(self, *args: Any, **kwargs: Any):
+        super().__init__(*args, **kwargs)
+        self.safe_to_create_task: bool = False
+
+    def visit_Assign(self, node: cst.CSTNode):
+        self.save_state(node, "safe_to_create_task")
+        self.safe_to_create_task = True
+
+    def visit_CompIf(self, node: cst.CSTNode):
+        self.save_state(node, "safe_to_create_task")
+        self.safe_to_create_task = False
+
+    def visit_Call(self, node: cst.Call):
+        if (
+            isinstance(node.func, (cst.Name, cst.Attribute))
+            and identifier_to_string(node.func) == "asyncio.create_task"
+            and not self.safe_to_create_task
+        ):
+            self.error(node)
+        self.visit_Assign(node)
+
+    visit_NamedExpr = visit_Assign
+    visit_AugAssign = visit_Assign
+    visit_IfExp_test = visit_CompIf
+
+    # because this is a Flake8AsyncVisitor_cst, we need to manually call restore_state
+    def leave_Assign(
+        self, original_node: cst.CSTNode, updated_node: cst.CSTNode
+    ) -> Any:
+        self.restore_state(original_node)
+        return updated_node
+
+    leave_Call = leave_Assign
+    leave_CompIf = leave_Assign
+    leave_NamedExpr = leave_Assign
+    leave_AugAssign = leave_Assign
+
+    def leave_IfExp_test(self, node: cst.IfExp):
+        self.restore_state(node)
+
+
 @error_class
 @disabled_by_default
 class Visitor900(Flake8AsyncVisitor):

tests/eval_files/async300.py

@@ -0,0 +1,80 @@
+# BASE_LIBRARY asyncio
+# TRIO_NO_ERROR
+# ANYIO_NO_ERROR
+
+from typing import Any
+
+import asyncio
+
+
+def handle_things(*args: object): ...
+
+
+class TaskStorer:
+    def __init__(self):
+        self.tasks: set[Any] = set()
+
+    def __ior__(self, obj: object):
+        self.tasks.add(obj)
+
+    def __iadd__(self, obj: object):
+        self.tasks.add(obj)
+
+
+async def foo():
+    args: Any
+    asyncio.create_task(*args)  # ASYNC300: 4
+
+    k = asyncio.create_task(*args)
+
+    mylist = []
+    mylist.append(asyncio.create_task(*args))
+
+    handle_things(asyncio.create_task(*args))
+
+    (l := asyncio.create_task(*args))
+
+    mylist = [asyncio.create_task(*args)]
+
+    task_storer = TaskStorer()
+    task_storer |= asyncio.create_task(*args)
+    task_storer += asyncio.create_task(*args)
+
+    mylist = [asyncio.create_task(*args) for i in range(10)]
+
+    # non-call usage is fine
+    asyncio.create_task
+    asyncio.create_task = args
+
+    # more or less esoteric ways of not saving the value
+
+    [asyncio.create_task(*args)]  # ASYNC300: 5
+
+    (asyncio.create_task(*args) for i in range(10))  # ASYNC300: 5
+
+    args = 1 if asyncio.create_task(*args) else 2  # ASYNC300: 16
+
+    args = (i for i in range(10) if asyncio.create_task(*args))  # ASYNC300: 36
+
+    # not supported, it can't be used as a context manager
+    with asyncio.create_task(*args) as k:  # type: ignore[attr-defined]  # ASYNC300: 9
+        ...
+
+    # import aliasing is not supported (this would raise ASYNC106 bad-async-library-import)
+    from asyncio import create_task
+
+    create_task(*args)
+
+    # nor is assigning it
+    boo = asyncio.create_task
+    boo(*args)
+
+    # or any lambda thing
+    my_lambda = lambda: asyncio.create_task(*args)
+    my_lambda(*args)
+
+    # don't crash
+
+    args.nodes[args].append(args)
+    args[1].nodes()
+    args[1].abc.nodes()

tests/test_flake8_async.py

@@ -476,6 +476,7 @@ def _parse_eval_file(
     "ASYNC116",
     "ASYNC117",
     "ASYNC118",
+    "ASYNC300",
     "ASYNC912",
 }