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

Implement asyncio-dangling-task to track asyncio.create_task calls #2935

Merged
merged 1 commit into from
Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,7 @@ For more, see [tryceratops](https://pypi.org/project/tryceratops/1.1.0/) on PyPI
| RUF003 | ambiguous-unicode-character-comment | Comment contains ambiguous unicode character `{confusable}` (did you mean `{representant}`?) | 🛠 |
| RUF004 | keyword-argument-before-star-argument | Keyword argument `{name}` must come after starred arguments | |
| RUF005 | unpack-instead-of-concatenating-to-collection-literal | Consider `{expr}` instead of concatenation | 🛠 |
| RUF006 | [asyncio-dangling-task](https://beta.ruff.rs/docs/rules/asyncio-dangling-task/) | Store a reference to the return value of `asyncio.create_task` | |
| RUF100 | unused-noqa | Unused `noqa` directive | 🛠 |

<!-- End auto-generated sections. -->
Expand Down
52 changes: 52 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF006.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import asyncio


# Error
def f():
asyncio.create_task(coordinator.ws_connect()) # Error


# OK
def f():
background_tasks = set()

for i in range(10):
task = asyncio.create_task(some_coro(param=i))

# Add task to the set. This creates a strong reference.
background_tasks.add(task)

# To prevent keeping references to finished tasks forever,
# make each task remove its own reference from the set after
# completion:
task.add_done_callback(background_tasks.discard)


# OK
def f():
ctx.task = asyncio.create_task(make_request())


# OK
def f():
tasks.append(asyncio.create_task(self._populate_collection(coll, coll_info)))


# OK
def f():
asyncio.wait([asyncio.create_task(client.close()) for client in clients.values()])


# OK
def f():
tasks = [asyncio.create_task(task) for task in tasks]


# Ok (false negative)
def f():
task = asyncio.create_task(coordinator.ws_connect())


# Ok (potential false negative)
def f():
do_nothing_with_the_task(asyncio.create_task(coordinator.ws_connect()))
7 changes: 7 additions & 0 deletions crates/ruff/src/checkers/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1795,6 +1795,13 @@ where
{
flake8_simplify::rules::use_capital_environment_variables(self, value);
}
if self.settings.rules.enabled(&Rule::AsyncioDanglingTask) {
if let Some(diagnostic) = ruff::rules::asyncio_dangling_task(value, |expr| {
self.resolve_call_path(expr)
}) {
self.diagnostics.push(diagnostic);
}
}
}
_ => {}
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<Rule> {
(Ruff, "003") => Rule::AmbiguousUnicodeCharacterComment,
(Ruff, "004") => Rule::KeywordArgumentBeforeStarArgument,
(Ruff, "005") => Rule::UnpackInsteadOfConcatenatingToCollectionLiteral,
(Ruff, "006") => Rule::AsyncioDanglingTask,
(Ruff, "100") => Rule::UnusedNOQA,

// flake8-django
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,7 @@ ruff_macros::register_rules!(
rules::ruff::rules::AmbiguousUnicodeCharacterComment,
rules::ruff::rules::KeywordArgumentBeforeStarArgument,
rules::ruff::rules::UnpackInsteadOfConcatenatingToCollectionLiteral,
rules::ruff::rules::AsyncioDanglingTask,
rules::ruff::rules::UnusedNOQA,
// flake8-django
rules::flake8_django::rules::NullableModelStringField,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ mod tests {

#[test_case(Rule::KeywordArgumentBeforeStarArgument, Path::new("RUF004.py"); "RUF004")]
#[test_case(Rule::UnpackInsteadOfConcatenatingToCollectionLiteral, Path::new("RUF005.py"); "RUF005")]
#[test_case(Rule::AsyncioDanglingTask, Path::new("RUF006.py"); "RUF006")]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
78 changes: 78 additions & 0 deletions crates/ruff/src/rules/ruff/rules/asyncio_dangling_task.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
use rustpython_parser::ast::{Expr, ExprKind};

use ruff_macros::{define_violation, derive_message_formats};

use crate::ast::types::{CallPath, Range};
use crate::registry::Diagnostic;
use crate::violation::Violation;

define_violation!(
/// ## What it does
/// Checks for `asyncio.create_task` calls that do not store a reference
/// to the returned result.
///
/// ## Why is this bad?
/// Per the `asyncio` documentation, the event loop only retains a weak
/// reference to tasks. If the task returned by `asyncio.create_task` is
/// not stored in a variable, or a collection, or otherwise referenced, it
/// may be garbage collected at any time. This can lead to unexpected and
/// inconsistent behavior, as your tasks may or may not run to completion.
///
/// ## Example
/// ```python
/// import asyncio
///
/// for i in range(10):
/// # This creates a weak reference to the task, which may be garbage
/// # collected at any time.
/// asyncio.create_task(some_coro(param=i))
/// ```
///
/// Use instead:
/// ```python
/// import asyncio
///
/// background_tasks = set()
///
/// for i in range(10):
/// task = asyncio.create_task(some_coro(param=i))
///
/// # Add task to the set. This creates a strong reference.
/// background_tasks.add(task)
///
/// # To prevent keeping references to finished tasks forever,
/// # make each task remove its own reference from the set after
/// # completion:
/// task.add_done_callback(background_tasks.discard)
/// ```
///
/// ## References
/// * [_The Heisenbug lurking in your async code_](https://textual.textualize.io/blog/2023/02/11/the-heisenbug-lurking-in-your-async-code/)
/// * [`asyncio.create_task`](https://docs.python.org/3/library/asyncio-task.html#asyncio.create_task)
pub struct AsyncioDanglingTask;
);

impl Violation for AsyncioDanglingTask {
#[derive_message_formats]
fn message(&self) -> String {
format!("Store a reference to the return value of `asyncio.create_task`")
}
}

/// RUF006
pub fn asyncio_dangling_task<'a, F>(expr: &'a Expr, resolve_call_path: F) -> Option<Diagnostic>
where
F: FnOnce(&'a Expr) -> Option<CallPath<'a>>,
{
if let ExprKind::Call { func, .. } = &expr.node {
if resolve_call_path(func).map_or(false, |call_path| {
call_path.as_slice() == ["asyncio", "create_task"]
}) {
return Some(Diagnostic::new(
AsyncioDanglingTask,
Range::from_located(expr),
));
}
}
None
}
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
mod ambiguous_unicode_character;
mod asyncio_dangling_task;
mod keyword_argument_before_star_argument;
mod unpack_instead_of_concatenating_to_collection_literal;
mod unused_noqa;
Expand All @@ -7,6 +8,7 @@ pub use ambiguous_unicode_character::{
ambiguous_unicode_character, AmbiguousUnicodeCharacterComment,
AmbiguousUnicodeCharacterDocstring, AmbiguousUnicodeCharacterString,
};
pub use asyncio_dangling_task::{asyncio_dangling_task, AsyncioDanglingTask};
pub use keyword_argument_before_star_argument::{
keyword_argument_before_star_argument, KeywordArgumentBeforeStarArgument,
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
source: crates/ruff/src/rules/ruff/mod.rs
expression: diagnostics
---
- kind:
AsyncioDanglingTask: ~
location:
row: 6
column: 4
end_location:
row: 6
column: 49
fix: ~
parent: ~

1 change: 1 addition & 0 deletions ruff.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1864,6 +1864,7 @@
"RUF003",
"RUF004",
"RUF005",
"RUF006",
"RUF1",
"RUF10",
"RUF100",
Expand Down