Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make ASYNC912 and 913 care about cancel points, and ignore schedule points #344

Merged
merged 3 commits into from
Feb 3, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ Changelog

`CalVer, YY.month.patch <https://calver.org/>`_

25.2.1
=======
- :ref:`ASYNC912 <async912>` and :ref:`ASYNC913 <async913>` will now trigger if there's no *cancel* points. This means that :func:`trio.open_nursery`/`anyio.create_task_group` will not silence them on their own, unless they're guaranteed to start tasks.

25.1.1
=======
- Add :ref:`ASYNC124 <async124>` async-function-could-be-sync
Expand Down
5 changes: 3 additions & 2 deletions docs/rules.rst
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,13 @@ _`ASYNC911` : async-generator-no-checkpoint
Exit, ``yield`` or ``return`` from async iterable with no guaranteed :ref:`checkpoint` since possible function entry (``yield`` or function definition).

_`ASYNC912` : cancel-scope-no-guaranteed-checkpoint
A timeout/cancelscope has :ref:`checkpoints <checkpoint>`, but they're not guaranteed to run.
Similar to `ASYNC100`_, but it does not warn on trivial cases where there is no checkpoint at all.
A timeout/cancelscope has :ref:`cancel points <cancel_point>`, but they're not guaranteed to run.
Similar to `ASYNC100`_, but it does not warn on trivial cases where there is no cancel point 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.
This will also error if there's no guaranteed :ref:`cancel point <cancel_point>`, where even though it won't deadlock the loop might become an uncancelable dry-run loop.

.. _autofix-support:

Expand Down
2 changes: 1 addition & 1 deletion docs/usage.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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: 25.1.1
rev: 25.2.1
hooks:
- id: flake8-async
# args: [--enable=ASYNC, --disable=ASYNC9, --autofix=ASYNC]
Expand Down
2 changes: 1 addition & 1 deletion flake8_async/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@


# CalVer: YY.month.patch, e.g. first release of July 2022 == "22.7.1"
__version__ = "25.1.1"
__version__ = "25.2.1"


# taken from https://github.com/Zac-HD/shed
Expand Down
14 changes: 10 additions & 4 deletions flake8_async/visitors/visitor91x.py
Original file line number Diff line number Diff line change
Expand Up @@ -354,10 +354,10 @@ class Visitor91X(Flake8AsyncVisitor_cst, CommonVisitors):
"on line {1.lineno}."
),
"ASYNC912": (
"CancelScope with no guaranteed checkpoint. This makes it potentially "
"CancelScope with no guaranteed cancel point. This makes it potentially "
"impossible to cancel."
),
"ASYNC913": ("Indefinite loop with no guaranteed checkpoint."),
"ASYNC913": ("Indefinite loop with no guaranteed cancel points."),
"ASYNC100": (
"{0}.{1} context contains no checkpoints, remove the context or add"
" `await {0}.lowlevel.checkpoint()`."
Expand Down Expand Up @@ -401,10 +401,16 @@ def checkpoint_cancel_point(self) -> None:
self.taskgroup_has_start_soon.clear()

def checkpoint_schedule_point(self) -> None:
self.uncheckpointed_statements = set()
# ASYNC912&ASYNC913 only cares about cancel points, so don't remove
# them if we only do a schedule point
self.uncheckpointed_statements = {
s
for s in self.uncheckpointed_statements
if isinstance(s, ArtificialStatement)
}

def checkpoint(self) -> None:
self.checkpoint_schedule_point()
self.uncheckpointed_statements = set()
self.checkpoint_cancel_point()

def checkpoint_statement(self) -> cst.SimpleStatementLine:
Expand Down
12 changes: 12 additions & 0 deletions tests/autofix_files/async913_trio_anyio.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# ARG --enable=ASYNC913
# AUTOFIX
# ASYNCIO_NO_ERROR

import trio


async def nursery_no_cancel_point():
while True: # ASYNC913: 4
await trio.lowlevel.checkpoint()
async with trio.open_nursery():
pass
9 changes: 9 additions & 0 deletions tests/autofix_files/async913_trio_anyio.py.diff
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
+++
@@ x,5 x,6 @@

async def nursery_no_cancel_point():
while True: # ASYNC913: 4
+ await trio.lowlevel.checkpoint()
async with trio.open_nursery():
pass
9 changes: 8 additions & 1 deletion tests/eval_files/async912.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def customWrapper(a: T) -> T:
with (res := trio.fail_at(10)):
...
# but saving with `as` does
with trio.fail_at(10) as res: # ASYNC912: 9
with trio.fail_at(10) as res2: # ASYNC912: 9
if bar():
await trio.lowlevel.checkpoint()

Expand Down Expand Up @@ -189,3 +189,10 @@ async def check_yield_logic():
if bar():
await trio.lowlevel.checkpoint()
yield


async def nursery_no_cancel_point():
with trio.move_on_after(10): # ASYNC912: 9
async with trio.open_nursery():
if bar():
await trio.lowlevel.checkpoint()
11 changes: 11 additions & 0 deletions tests/eval_files/async913_trio_anyio.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# ARG --enable=ASYNC913
# AUTOFIX
# ASYNCIO_NO_ERROR

import trio


async def nursery_no_cancel_point():
while True: # ASYNC913: 4
async with trio.open_nursery():
pass
33 changes: 28 additions & 5 deletions tests/test_flake8_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,21 @@ def diff_strings(first: str, second: str, /) -> str:

# replaces all instances of `original` with `new` in string
# unless it's preceded by a `-`, which indicates it's part of a command-line flag
def replace_library(string: str, original: str = "trio", new: str = "anyio") -> str:
return re.sub(rf"(?<!-){original}", new, string)
def replace_library(
string: str,
original: str = "trio",
new: str = "anyio",
replace_nursery: bool = False,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the replace_nursery parameter is temporary and removed in #345

) -> str:
def replace_str(string: str, original: str, new: str) -> str:
return re.sub(rf"(?<!-){original}", new, string)

if replace_nursery:
if original == "trio" and new == "anyio":
string = replace_str(string, "trio.open_nursery", "anyio.create_task_group")
elif original == "anyio" and new == "trio":
string = replace_str(string, "anyio.create_task_group", "trio.open_nursery")
return replace_str(string, original, new)


def check_autofix(
Expand Down Expand Up @@ -160,11 +173,18 @@ def check_autofix(
# meaning it's replaced in visited_code, we also replace it in previous generated code
# and in the previous diff
if base_library != library:
replace_nursery = "913" in test or "912" in test
previous_autofixed = replace_library(
previous_autofixed, original=base_library, new=library
previous_autofixed,
original=base_library,
new=library,
replace_nursery=replace_nursery,
)
autofix_diff_content = replace_library(
autofix_diff_content, original=base_library, new=library
autofix_diff_content,
original=base_library,
new=library,
replace_nursery=replace_nursery,
)

# save any difference in the autofixed code
Expand Down Expand Up @@ -274,7 +294,10 @@ def test_eval(

if library != magic_markers.BASE_LIBRARY:
content = replace_library(
content, original=magic_markers.BASE_LIBRARY, new=library
content,
original=magic_markers.BASE_LIBRARY,
new=library,
replace_nursery="912" in test or "913" in test,
)

# if substituting we're messing up columns
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ allowlist_externals = make
changedir = docs
skip_install = True
commands =
make clean
make html

# Settings for other tools
[pytest]
addopts =
--tb=native
--cov=flake8_async
--cov-branch
--cov-report=term-missing:skip-covered
Expand Down
Loading