-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Detect unneeded async
keywords on functions
#9966
Detect unneeded async
keywords on functions
#9966
Conversation
…tects an async function w/o await
…t or async_for or async_with
…ving unused imports
… seem necessary for this rule
…n accept the test snapshot
Sorry, I neglected formatting and linting of the rust code. I'll do that now. |
|
code | total | + violation | - violation | + fix | - fix |
---|---|---|---|---|---|
RUF029 | 314 | 314 | 0 | 0 | 0 |
The |
Ah yeah, the thing we often do there (at the very least) is check if the method has an |
Here's another interesting edge-case false positive https://github.com/zulip/zulip/blob/35098f49597895718343091881fbd6198bd2022d/zerver/tornado/views.py#L35 — this one I'm less sure we can/should do anything about. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
SpuriousAsync { | ||
name: name.to_string(), | ||
}, | ||
range, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, if you have the function_def: &ast::StmtFunctionDef
as an argument to this method, you can use function_def.identifier()
to get just the range of the function name (per your PR summary).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it desirable for the error message to only indicate the name, given that the problem is to the left of the name (the use of the async
keyword) and to the right of it (the function body lacking a reason for the use of the async
keyword)?
Perhaps @AlexWaygood can elucidate whether or not this should trigger for functions with https://peps.python.org/pep-0525 may be helpful. |
Yeah, async functions that contain >>> import asyncio
>>> async def x():
... yield 42
... yield 56
...
>>> x
<function x at 0x00000274CDC52200>
>>> x()
<async_generator object x at 0x00000274CC36BE20>
>>> async def y():
... async for number in x():
... print(number)
...
>>> asyncio.run(y())
42
56 |
I think there's one issue that we might need to look into and it's that the visitor will cross function boundaries when searching for async def unnecessary_async():
async def inner():
await x
async def unnecessary_async2():
class Inner:
async def inner():
await x
I don't think the rule flags |
Thanks for your review! I've looked over the comments above and can work on the changes later today. I need to devote this morning to a different task.
@charliermarsh should I omit this check in both the cases (1) in the context of a class or (2) when the override decorator is present? Based on your description, it seems preferable to limit case (1) to classes with a base class, since they're actually likely to be overriding things, though. |
@MichaReiser These examples make the problem a bit harder. 🙂 I need to check if the visitor has a callback for when an inner-function ends; I could use that to track which scope I'm in when detecting yielding-expressions to attribute them correctly. |
@plredmond I think there are only two cases where this is relevant: classes and functions. You could extend your visitor to handle |
Hi! Sorry to let this PR languish. I have a paper deadline just before I start my astral internship, so I have to put this on ice until then. |
IMO, |
So, I chatted to @carljm about this offline... and he persuaded me that the rationale I gave in #9966 (comment) for excluding async generators from this check basically makes no sense :-) It's true that removing the - x = await function_in_question()
+ x = function_in_question() As such, I'm now persuaded that there's no reason to exclude async generator functions from this check. (Thanks @carljm!) |
Picking this up today. Sorry for the delay. |
Saving this rename for last b/c it'll disrupt the above comments on diffs. |
Ok, I think everything is done except for this:
[EDIT: I'll look at the test failure in the morning. I don't see one when I test locally.] |
@plredmond I'm not entirely sure if that's what you mean but you can test if you're inside a class by using
You can also use the |
Came across this PR when looking at #9951 – thank you for adding this rule, very excited about it! Regarding the latest discussion – I'm curious why not omit it for method marked with |
@kkom we're just cutting scope from the initial rule. We could consider it in the future. |
This looks good to me, but I suggest adding the snippet that Micha posted above. |
…unctions in class definitions using semantic scope info (per @charliermarsh, and thanks to @MichaReiser)
…anation has quotes
@plredmond - Try running |
(I believe that's the cause of your failing test.) |
$ cargo dev generate-all
...
$ git diff
diff --git a/ruff.schema.json b/ruff.schema.json
index 871cce40b..90582fc67 100644
--- a/ruff.schema.json
+++ b/ruff.schema.json
@@ -3951,4 +3951,4 @@
]
}
}
-}
+}
\ No newline at end of file |
Is your editor adding a newline? |
I also tried fetching and merging in the latest I'll look at the CI output. |
It was modified by |
We do expect the Schema to change on this branch. |
I was wrong. I did edit it, back in feb, and that seems to have lead to the CI complaining about the newline. It should be fixed now. |
Summary
This change adds a rule to detect functions declared
async
but lacking any ofawait
,async with
, orasync for
. This resolves #9951.Test Plan
This change was tested by following https://docs.astral.sh/ruff/contributing/#rule-testing-fixtures-and-snapshots and adding positive and negative cases for each of
await
vs nothing,async with
vswith
, andasync for
vsfor
.Questions/todo
yield
appearing in functions. My understanding is that there has been several rounds of iteration in how python async is supposed to be done, and thatyield
is not considered current, and so I haven't added a case for that. Therefore anasync
function containing onlyyield
will cause this rule to fire.async
keywords on functions #9966 (comment)async
keywords on functions #9966 (review)[ ] Needs review by @zanieb