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

PERF401 triggers when adding items to collection from function arguments #11316

Open
CoolCat467 opened this issue May 7, 2024 · 8 comments
Open

Comments

@CoolCat467
Copy link

CoolCat467 commented May 7, 2024

PERF401 is triggered and suggests rewriting as async for loop when modifying a collection from arguments, which makes this not possible.

example.py

async def read_output(
    stream: ReceiveStream,
    chunks: list[bytes | bytearray],
) -> None:
    async with stream:
        async for chunk in stream:
            chunks.append(chunk)
ruff check example.py --isolated --select PERF401

example.py:7:13: PERF401 Use an async list comprehension to create a transformed list

ruff --version: ruff 0.4.3

Suggested fixes:

  • Don't raise issue for this circumstance
  • ?Suggest extending collection with async comprehension if argument is a list

Worries I have about 2nd solution is that this way of ordering your code makes it seem like you care quite a lot about having the function caller having complete control over memory allocation, and then there is the matter of what about collections that are not lists like sets and how are we even finding out if argument is a list or not if it doesn't have type annotations.

@charliermarsh
Copy link
Member

Can it not be written with something like:

async def read_output(
    stream: ReceiveStream,
    chunks: list[bytes | bytearray],
) -> None:
    async with stream:
        chunks.extend(chunk async for chunk  in stream)

@CoolCat467
Copy link
Author

That's what I'm mentioning in second idea for suggested fix, ruff telling people they could use extend, but as I noted, what if people are passing in a set object? Sets don't have an extend method.

@CoolCat467
Copy link
Author

@charliermarsh ^

@charliermarsh
Copy link
Member

Ah yeah, I think the message could be better here.

On the second point, thankfully sets also don't have an append method, so we wouldn't trigger on a set here I don't think?

@CoolCat467
Copy link
Author

Correct

@Skylion007
Copy link

@CoolCat467 sets and dicts do have an update() method though.

@CoolCat467
Copy link
Author

Not tuple though. The point is, giving one single solution for all collections will probably not work for at least one somewhere.

@A5rocks
Copy link

A5rocks commented May 16, 2024

Can it not be written with something like:

async def read_output(
    stream: ReceiveStream,
    chunks: list[bytes | bytearray],
) -> None:
    async with stream:
        chunks.extend(chunk async for chunk  in stream)

No, async for in a generator like that returns an async generator, which .extend doesn't take. Instead, you could do chunks.extend([chunk async for chunk in stream]) which is like... I don't know, is that slower? Faster? You're making an extra list and discarding it, I can't imagine that's faster.

>>> import asyncio
>>> async def agenerator():
...   yield 1
...   await asyncio.sleep(2)
...   yield 3
...
>>> async def main():
...   b = []
...   b.extend(i async for i in agenerator())
...
>>> asyncio.run(main())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\A5rocks\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "C:\Users\A5rocks\AppData\Local\Programs\Python\Python311\Lib\asyncio\runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\A5rocks\AppData\Local\Programs\Python\Python311\Lib\asyncio\base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "<stdin>", line 3, in main
TypeError: 'async_generator' object is not iterable

(I comment this because I thought that would work too, but then tried it just to be sure...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants