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

add async124 async-function-could-be-sync #309

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Oct 29, 2024

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.

@jakkdl jakkdl requested review from oremanj and Zac-HD October 29, 2024 12:42
@jakkdl jakkdl force-pushed the async_fun_could_be_sync branch from 6f2e070 to d40b064 Compare October 29, 2024 15:29
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jakkdl
Copy link
Member Author

jakkdl commented Oct 30, 2024

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

Hmhm, I also feel like this would lose a bunch of the upside of it. Maybe a separate error code?

  • If there's a convenient way to try it out we could get some empirical feedback? I can't think of one though.

push a release and wait for complaints? 😆
We could push it as default-disabled and mention in changelog + ask on anyio/trio gitter and python discord (started hanging out a bit in the async channel) for people to try it out and give feedback if it should be default-enabled or not.
I should also add the linter to the trio/ repo

@jakkdl
Copy link
Member Author

jakkdl commented Oct 30, 2024

after messing around in trio:

  1. this is implemented in ruff preview as https://docs.astral.sh/ruff/rules/unused-async/
  2. We definitely need to suppress it for methods, cray false alarm rate. If one wanted to be very specific you could specifically look for __aiter__ and stuff but it's probably not worth it. RUF029 is disabled for class methods
  3. RUF029 further has lots of edge cases brought up in RUF029 (unneeded async) needs nuance for class methods astral-sh/ruff#10980, but we probably don't need to be as thorough.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me; optional nitpick below.

Comment on lines 111 to 116
# 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()
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jakkdl jakkdl Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oooh, thank you!
And turns out neither mypy nor ruff handles this properly, hah

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened astral-sh/ruff#14167 and python/mypy#18124

I've now opened 7 issues today in mypy/ruff/pyright/typeshed 😇

@Zac-HD
Copy link
Member

Zac-HD commented Nov 1, 2024

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.

@jakkdl
Copy link
Member Author

jakkdl commented Nov 7, 2024

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 >_<
I guess that's a point in favor of default-disabling it for async124 in particular regardless of no-checkpoint-decorator

@jakkdl
Copy link
Member Author

jakkdl commented Nov 8, 2024

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 >_< I guess that's a point in favor of default-disabling it for async124 in particular regardless of no-checkpoint-decorator

opened #313 to procrastinate on this while I have way-too-many-balls in the air simultaneously 😇

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

Successfully merging this pull request may close these issues.

New rule: async function with no await could be sync
2 participants