astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
30.78k stars 1.02k forks source link

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

Open CoolCat467 opened 4 months ago

CoolCat467 commented 4 months ago

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:

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

@charliermarsh ^

charliermarsh commented 4 months ago

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 commented 4 months ago

Correct

Skylion007 commented 4 months ago

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

CoolCat467 commented 4 months ago

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

A5rocks commented 3 months ago

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