ruff icon indicating copy to clipboard operation
ruff copied to clipboard

PERF401 triggers when adding items to collection from function arguments

Open CoolCat467 opened this issue 9 months ago • 8 comments

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.

CoolCat467 avatar May 07 '24 03:05 CoolCat467

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)

charliermarsh avatar May 07 '24 03:05 charliermarsh

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 avatar May 07 '24 03:05 CoolCat467

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?

charliermarsh avatar May 07 '24 04:05 charliermarsh

Correct

CoolCat467 avatar May 08 '24 00:05 CoolCat467

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

Skylion007 avatar May 10 '24 14:05 Skylion007

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

CoolCat467 avatar May 10 '24 22:05 CoolCat467

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...)

A5rocks avatar May 16 '24 06:05 A5rocks