Issue 1600: Forbid `await` in `for` loop by skv0zsneg · Pull Request #3330 · wemake-services/wemake-python-styleguide

skv0zsneg · 2025-03-05T22:14:13Z

I have made things!

Checklist

I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
I have created at least one test case for the changes I have made
I have updated the documentation for the changes I have made
I have added my changes to the CHANGELOG.md

Related issues

Hi! I had a lot of pleasure while working on this issue.

I added new ASTViolation for forbid await in for loop, but not sure about docstring detail. If it need to content more informative, I'll add it.

About documentation I guess it fills automatically using docstrings in ASTViolation subclasses, so I mark as done third point in the checklist

sobolevn

Thanks!

sobolevn · 2025-03-06T07:35:05Z

+async def foo():
+    for _ in range(1):
+        await some()
+"""


Don't forget to test:

list comp [await x for x in ...]

set comp

dict comp

gen exprs

sobolevn · 2025-03-06T07:35:47Z

+
+
+@final
+class AwaitInLoopViolation(ASTViolation):


This is not a consistency violation, this is a best practice.

sobolevn · 2025-03-06T07:36:23Z

+
+code_that_breaks = """
+async def foo():
+    for _ in range(1):


Please, check that await in async for works.

skv0zsneg · 2025-03-06T23:02:26Z

Added comprehensions checking, checked for async for working, added more tests, moved violation to best practice.

sobolevn

👍

sobolevn · 2025-03-07T07:41:57Z

+        await test_function()
+
+
+async def test_await_in_list_comp():
+    list_comprehension = [await test_function() for _ in range(10)]  # noqa: WPS476
+    set_comprehension = {await test_function() for _ in range(10)}  # noqa: WPS476
+    dict_comprehension = {some_hashable: await test_function() for some_hashable in range(10)} # noqa: WPS476
+    generator_comprehension = (await test_function() for _ in range(10))  # noqa: WPS476


Suggested change

await test_function()

async def test_await_in_list_comp():

list_comprehension = [await test_function() for _ in range(10)] # noqa: WPS476

set_comprehension = {await test_function() for _ in range(10)} # noqa: WPS476

dict_comprehension = {some_hashable: await test_function() for some_hashable in range(10)} # noqa: WPS476

generator_comprehension = (await test_function() for _ in range(10)) # noqa: WPS476

Just one error is fine.

sobolevn · 2025-03-07T07:42:08Z

        'WPS473': 0,
        'WPS474': 1,
        'WPS475': 1,
+        'WPS476': 5,


Suggested change

'WPS476': 5,

'WPS476': 1,

sobolevn · 2025-03-07T07:42:47Z

+
+generator_comprehension = """
+async def foo():
+    result = (await some() for i in range(10))


Don't forget about async comprehensions :)
They should allow await

I'm did not work with async Python as much as I wish 😢
Can you please correct me - async comprehensions it is something like this [await some_two() async for _ in some_one()]?

sobolevn · 2025-03-07T07:43:35Z

+        There is a better way to control repeated coroutines in ``for`` loops.
+
+    Solution:
+        Using :function:`asyncio.gather` or :class:`asyncio.TaskGroup`


Suggested change

Using :function:`asyncio.gather` or :class:`asyncio.TaskGroup`

Use :function:`asyncio.gather`,

:function:`asyncio.wait`,

or :class:`asyncio.TaskGroup`

sobolevn · 2025-03-07T07:44:19Z

+        urls = [
+            "https://wemake-python-styleguide.readthedocs.io/",
+            "https://flake8.pycqa.org/",
+            "https://docs.astral.sh/ruff/",
+        ]
+


Suggested change

urls = [

"https://wemake-python-styleguide.readthedocs.io/",

"https://flake8.pycqa.org/",

"https://docs.astral.sh/ruff/",

]

sobolevn · 2025-03-07T07:44:59Z

+        async def request():
+            tasks = []
+            for url in urls:
+                tasks.append(parse_content(url))
+            parsed_content = await asyncio.gather(*tasks)


Suggested change

async def request():

tasks = []

for url in urls:

tasks.append(parse_content(url))

parsed_content = await asyncio.gather(*tasks)

async def request():

tasks = [parse_content(url) for url in urls]

parsed_content = await asyncio.gather(*tasks)

sobolevn · 2025-03-07T07:46:25Z

+        if isinstance(node, ast.While | ast.AsyncFor):
+            return
+
+        for subnode in ast.walk(node):


I think that we should do the other way, we need to search for await and then find For in its parents. See nodes.py for a helper function.

Not sure that I understand you correctly.

In this method we getting nodes that could be AnyLoop | AnyComprehension, so we already in the loop node. Why we need walk through parents? If we found await in child nodes it already triggered violation.

Sorry if it is dumb question I have't work with ast package until your project 🙃

Any way I rewrote method in last commits with walk through parents logic.

Ok, let's keep it this way :)

skv0zsneg · 2025-03-08T18:06:38Z

Well, yes, wps really make me suffer 😄

sobolevn

Thank you! We are almost there.

sobolevn · 2025-03-09T07:01:38Z

+        ):
+            return
+
+        bad_loops = (


we use class-level constants for tuples like this. proper name would be _forbidden_await_loops

sobolevn · 2025-03-09T07:05:28Z

+            | ast.ListComp
+            | ast.SetComp
+        )
+        await_sub_nodes = walk.get_subnodes_by_type(node, ast.Await)


Basically, that's why I said that it would be easier to visit await nodes first and then find forbidden parents from it. See

wemake-python-styleguide/wemake_python_styleguide/logic/walk.py

Lines 24 to 35 in 2c4b96e

def get_closest_parent(

node: ast.AST,

parents: _IsInstanceContainer,

) -> ast.AST | None:

"""Returns the closes parent of a node of requested types."""

parent = get_parent(node)

while True:

if parent is None:

return None

if isinstance(parent, parents):

return parent

parent = get_parent(parent)

Basically, you would just have an await node, pass it into container = get_closest_parent(node, self._forbidden_await_loops) and then check that the container is not an async comprehension. That's it. Three lines of code.

codecov · 2025-03-09T07:08:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (2c4b96e) to head (f63f2e9).
Report is 103 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3330   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          357       358    +1     
  Lines        11708     11761   +53     
  Branches       801       803    +2     
=========================================
+ Hits         11708     11761   +53

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

… in one

…e-python-styleguide into forbid-await-in-for-loop

skv0zsneg · 2025-03-09T12:39:11Z

I'm change method as you said this is really decreese rows number 💯

Also I added test for multiply comprehensions in one including async.

sobolevn

Thanks! Almost there.

sobolevn · 2025-03-09T15:08:53Z

            if not isinstance(evaled, ast.Name) and bool(evaled):
                self.add_violation(InfiniteWhileLoopViolation(node))

+    def _check_await_inside_loop(


It would be easier to use visit_Await method and the call _check_await_inside_loop with ast.Await node :)

Got it! I should put it in WrongLoopVisitor or somewhere else?

if it fits the complexity rules - yes!

It unfortunately did not fit D:
Moved it to new class.

sobolevn

Just one comment :)

sobolevn · 2025-03-10T21:10:58Z

+        if node_parent is not None:
+            if isinstance(node_parent, AnyComprehension) and all(
+                comprehension.is_async
+                for comprehension in node_parent.generators
+            ):
+                return


Suggested change

if node_parent is not None:

if isinstance(node_parent, AnyComprehension) and all(

comprehension.is_async

for comprehension in node_parent.generators

):

return

if (

isinstance(node_parent, AnyComprehension)

and all(

comprehension.is_async

for comprehension in node_parent.generators

):

# async comprehensions are allowed to use `await`

return

You want me to add this comment?
# async comprehensions are allowed to use await

not only a comment, but change how ifs are nested

change how ifs are nested

In this case tests are falling 😢
This will have to do some changes again, if it's ok I'll do it

Push changes 👍

sobolevn

Thank you! Great job!

skv0zsneg · 2025-03-11T07:11:05Z

Thank you! I have a lot of fun and learning new things doing this task.

Hope I can help project solving another issues and bugs, if you don't mind 🤓

sobolevn · 2025-03-11T07:38:15Z

Sure, please, go ahead!

skv0zsneg added 3 commits March 5, 2025 00:18

Add violation for using await in for loop.

2f62a13

Upd AwaitInLoopViolation

3cab9cb

Upd CHANGELOG

3bd4a1b

sobolevn reviewed Mar 6, 2025

View reviewed changes

skv0zsneg added 9 commits March 6, 2025 22:44

Migrate Violation to best practices

9cf38d9

Add more tests and fix await checker

ef6e3a8

Fix violation adding

85fd178

Add new rule to noqa tests

4d67b2b

Upd docstring for new violation

4289f14

Merge branch 'master' into forbid-await-in-for-loop

423557f

Upd error_template in AwaitInLoopViolation

7568e6b

Fix sphinx notation in docstring

5cac80d

Minor docstring fix

92c9d66

sobolevn reviewed Mar 7, 2025

View reviewed changes

skv0zsneg added 4 commits March 8, 2025 12:49

Remover extra noqa tests

ceb431a

Fix docstring in await in loop violation

dd863d5

Refactor await in loop checker

57916fb

Add checking async comp. Add more tests.

4290cdb

sobolevn reviewed Mar 9, 2025

View reviewed changes

skv0zsneg added 4 commits March 9, 2025 12:18

Merge branch 'master' into forbid-await-in-for-loop

d59a121

Refactor checking await in loop. Add tests for multiply comprehension…

c464b52

… in one

Merge branch 'master' into forbid-await-in-for-loop

9173943

Merge branch 'forbid-await-in-for-loop' of github.com:skv0zsneg/wemak…

0f1a574

…e-python-styleguide into forbid-await-in-for-loop

sobolevn reviewed Mar 9, 2025

View reviewed changes

Mover await in loop check to new class.

2e32cfd

sobolevn reviewed Mar 10, 2025

View reviewed changes

Remove nested if

f63f2e9

sobolevn approved these changes Mar 11, 2025

View reviewed changes

sobolevn merged commit 03339e4 into wemake-services:master Mar 11, 2025

	def get_closest_parent(
	node: ast.AST,
	parents: _IsInstanceContainer,
	) -> ast.AST \| None:
	"""Returns the closes parent of a node of requested types."""
	parent = get_parent(node)
	while True:
	if parent is None:
	return None
	if isinstance(parent, parents):
	return parent
	parent = get_parent(parent)



		@final
		class AwaitInLoopViolation(ASTViolation):

Uh oh!

Conversation

skv0zsneg commented Mar 5, 2025

I have made things!

Checklist

Related issues

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skv0zsneg commented Mar 6, 2025

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skv0zsneg Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skv0zsneg commented Mar 8, 2025

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skv0zsneg commented Mar 9, 2025

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skv0zsneg Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skv0zsneg Mar 8, 2025 •

edited

Loading

codecov Bot commented Mar 9, 2025 •

edited

Loading

skv0zsneg Mar 10, 2025 •

edited

Loading