Create rule S7521: Coroutine objects should be awaited #5105
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
guillaume-dequenne
force-pushed
the
rule/add-RSPEC-S7521
branch
from
e3e2d68 to
a1fd11a
Compare
| (visible only on this page) | ||
|
|
||
| === Message | ||
| Await this coroutine or store it in a variable to be awaited later. |
There was a problem hiding this comment.
With this approach I actually think we could implement it in sonar-python without DBD...
Finding the absence of await on the stored object would actually be difficult, even with DBD.
Probably worth discussing with someone from DBD before any implementation though, and depending on that, we can adapt the message.
There was a problem hiding this comment.
With this approach, I agree.
The behavior and limitations should be clearly defined in the tests.
In the long term, I really think DBD or a taint analysis engine are the more robust implementation
There was a problem hiding this comment.
That makes sense!
guillaume-dequenne
changed the title
rules/S7521/python/rule.adoc
Outdated
|
|
||
| == How to fix it | ||
|
|
||
| Always use the `await` keyword when calling a coroutine function or any other awaitable object (like a `Task` or `Future`). Alternatively, you can schedule the coroutine to run using methods like `asyncio.create_task()`, but be sure to save and await the resulting task. |
There was a problem hiding this comment.
I think the recommendation should be broader and use keywords like TaskGroup or Nursery to help people look for the correct replacements
There was a problem hiding this comment.
I made a slight change in the recommendation. That said, I think the rule is a bit more generic than the others we have created so far, and probably targets someone who simply forgot to add the await keyword. I feel that by referencing Nursery, I'm now tempted to have a "How to fix in Trio" section which may not be super relevant here.
Hopefully we hit a middle ground with the new wording.
| (visible only on this page) | ||
|
|
||
| === Message | ||
| Await this coroutine or store it in a variable to be awaited later. |
There was a problem hiding this comment.
With this approach, I agree.
The behavior and limitations should be clearly defined in the tests.
In the long term, I really think DBD or a taint analysis engine are the more robust implementation
| }, | ||
| "tags": [ | ||
| "async", | ||
| "asyncio", |
There was a problem hiding this comment.
We might as well add the two other libraries, since it it not specific to any of them
guillaume-dequenne
force-pushed
the
rule/add-RSPEC-S7521
branch
from
00bb150 to
b1d5941
Compare
|
|
You can preview this rule here (updated a few minutes after each push).
Review
A dedicated reviewer checked the rule description successfully for: