From d606c9b17fa58e98c834acca53c0b44507b0eb70 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 15 Feb 2023 13:53:33 -0500 Subject: [PATCH] Implement asyncio-dangling-task to track asyncio.create_task calls --- README.md | 1 + .../resources/test/fixtures/ruff/RUF006.py | 52 +++++++++++++ crates/ruff/src/checkers/ast.rs | 7 ++ crates/ruff/src/codes.rs | 1 + crates/ruff/src/registry.rs | 1 + crates/ruff/src/rules/ruff/mod.rs | 1 + .../rules/ruff/rules/asyncio_dangling_task.rs | 78 +++++++++++++++++++ crates/ruff/src/rules/ruff/rules/mod.rs | 2 + ..._rules__ruff__tests__RUF006_RUF006.py.snap | 15 ++++ ruff.schema.json | 1 + 10 files changed, 159 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/ruff/RUF006.py create mode 100644 crates/ruff/src/rules/ruff/rules/asyncio_dangling_task.rs create mode 100644 crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF006_RUF006.py.snap diff --git a/README.md b/README.md index 04c74fb03e2ca1..10420c49df4be5 100644 --- a/README.md +++ b/README.md @@ -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/) | Dangling task | | | RUF100 | unused-noqa | Unused `noqa` directive | 🛠 | diff --git a/crates/ruff/resources/test/fixtures/ruff/RUF006.py b/crates/ruff/resources/test/fixtures/ruff/RUF006.py new file mode 100644 index 00000000000000..2ad938a5e7b550 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/ruff/RUF006.py @@ -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())) diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index a2f5ead600afad..67ddb98d08e3ab 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -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); + } + } } _ => {} } diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index e883a9aa325ccf..c506e43dd83b8a 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -596,6 +596,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option { (Ruff, "003") => Rule::AmbiguousUnicodeCharacterComment, (Ruff, "004") => Rule::KeywordArgumentBeforeStarArgument, (Ruff, "005") => Rule::UnpackInsteadOfConcatenatingToCollectionLiteral, + (Ruff, "006") => Rule::AsyncioDanglingTask, (Ruff, "100") => Rule::UnusedNOQA, // flake8-django diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 9679f17580f221..60c6d0a4974a65 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -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, diff --git a/crates/ruff/src/rules/ruff/mod.rs b/crates/ruff/src/rules/ruff/mod.rs index d0661c8d6a2bf3..fba05e02126ebb 100644 --- a/crates/ruff/src/rules/ruff/mod.rs +++ b/crates/ruff/src/rules/ruff/mod.rs @@ -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( diff --git a/crates/ruff/src/rules/ruff/rules/asyncio_dangling_task.rs b/crates/ruff/src/rules/ruff/rules/asyncio_dangling_task.rs new file mode 100644 index 00000000000000..7528070001c7b8 --- /dev/null +++ b/crates/ruff/src/rules/ruff/rules/asyncio_dangling_task.rs @@ -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!("Dangling task") + } +} + +/// RUF006 +pub fn asyncio_dangling_task<'a, F>(expr: &'a Expr, resolve_call_path: F) -> Option +where + F: FnOnce(&'a Expr) -> Option>, +{ + 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 +} diff --git a/crates/ruff/src/rules/ruff/rules/mod.rs b/crates/ruff/src/rules/ruff/rules/mod.rs index a038b320d3f19c..287ede18620d27 100644 --- a/crates/ruff/src/rules/ruff/rules/mod.rs +++ b/crates/ruff/src/rules/ruff/rules/mod.rs @@ -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; @@ -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, }; diff --git a/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF006_RUF006.py.snap b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF006_RUF006.py.snap new file mode 100644 index 00000000000000..fb6ef9a9dfe2b0 --- /dev/null +++ b/crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__RUF006_RUF006.py.snap @@ -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: ~ + diff --git a/ruff.schema.json b/ruff.schema.json index a81715c809c2b4..e07abfd8beb455 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1864,6 +1864,7 @@ "RUF003", "RUF004", "RUF005", + "RUF006", "RUF1", "RUF10", "RUF100",