add async124 async-function-could-be-sync #309
Conversation
jakkdl
force-pushed
the
async_fun_could_be_sync
branch
from
6f2e070
to
d40b064
Compare
There was a problem hiding this comment.
Implementation looks good, but I'm pretty concerned about the false-alarm rate.
- Let's also exclude methods on a class; in those cases it's reasonably likely that it 'has to be async' in order to match the interface of some other type
- If there's a convenient way to try it out we could get some empirical feedback? I can't think of one though.
Hmhm, I also feel like this would lose a bunch of the upside of it. Maybe a separate error code?
push a release and wait for complaints? 😆 |
|
after messing around in trio:
|
tests/eval_files/async124.py
Outdated
| # 124 doesn't care if you evaluate the comprehension or not | ||
| # 910 is stingy | ||
| async def foo_async_gen(): | ||
| return ( # ASYNC910: 4, "return", Statement("function definition", lineno-1) | ||
| a async for a in foo_gen() | ||
| ) |
There was a problem hiding this comment.
I think the behavior of 910 is clearly correct here; there are no checkpoints until after foo_async_gen() has returned, so that function could be sync.
There was a problem hiding this comment.
yeah, but the problem is differentiating that case from
k = (a async for a in foo_gen())
return list(k)and similar. For 910 we can simply ignore the generator and tell the user to insert explicit checkpoints, but for a default-enabled check I think ASYNC124 should err on the side of expecting the generator to be consumed in the same function.
There was a problem hiding this comment.
Snippet is invalid; list() doesn't do async iteration. Which is a nitpick but relevant in that it means we probably don't need to distinguish such cases; consuming an async generator requires a syntactic checkpoint.
There was a problem hiding this comment.
oooh, thank you!
And turns out neither mypy nor ruff handles this properly, hah
There was a problem hiding this comment.
Opened astral-sh/ruff#14167 and python/mypy#18124
I've now opened 7 issues today in mypy/ruff/pyright/typeshed 😇
|
I'm also inclined to include FastAPI support in the default no-checkpoint-decorators; reducing the low-value-alarm rate for users is more important than my personal distaste for missing checkpoints 😅. That can easily go in a future PR though. |
hrm. suppressing it for async124 makes lots of sense, but less so for async9xx... if they got suppressed for cases where async124 would get suppressed >_< |
opened #313 to procrastinate on this while I have way-too-many-balls in the air simultaneously 😇 |
fixes #253
given that converting an async test to a sync test will give weird errors agronholm/anyio#803 pytest-dev/pytest#10839 we should probably err on the side of not erroring, at least until pytest/anyio is fixed,
which would extend to 910 and 911.