Expand asyncio-dangling-task (RUF006) to include new_event_loop#9976
Expand asyncio-dangling-task (RUF006) to include new_event_loop#9976charliermarsh merged 2 commits intoastral-sh:mainfrom
asyncio-dangling-task (RUF006) to include new_event_loop#9976Conversation
|
charliermarsh
left a comment
There was a problem hiding this comment.
Thanks! Addressed the TODO.
asyncio-dangling-task (RUF006) to include new_event_loop
586666e to
19ec2be
Compare
|
@charliermarsh What do you think of my comments? |
|
@tyilo - Which comments specifically? Sorry if I missed something. |
| // Ex) `loop = ...; loop.create_task(...)` | ||
| if let Expr::Attribute(ast::ExprAttribute { attr, value, .. }) = func.as_ref() { | ||
| if attr == "create_task" { | ||
| if typing::resolve_assignment(value, semantic).is_some_and(|call_path| { |
There was a problem hiding this comment.
I don't know if ruff's typing analysis is good enough for this, but it would be better to check that the type of value is asyncio.AbstractEventLoop, so that the following would also be flagged:
import asyncio
def create_event_loop() -> asyncio.AbstractEventLoop:
return asyncio.new_event_loop()
loop = create_event_loop()
loop.create_task(asyncio.sleep(1))There was a problem hiding this comment.
Yeah we don't quite support that yet unfortunately.
| }) { | ||
| return Some(Diagnostic::new( | ||
| AsyncioDanglingTask { | ||
| expr: compose_call_path(value).unwrap_or_else(|| "asyncio".to_string()), |
There was a problem hiding this comment.
I think loop would be a better fallback:
| expr: compose_call_path(value).unwrap_or_else(|| "asyncio".to_string()), | |
| expr: compose_call_path(value).unwrap_or_else(|| "loop".to_string()), |
There was a problem hiding this comment.
Will change, though I think in practice this should never happen. I wish it were encoded in the type system, but e.g. value here has to be a Name, and compose_call_path always returns Some given a name.
Ah, sorry I haven't used GitHub's review functionality before, so my comments weren't published 😅 They should be visible now... |
…astral-sh#9976) ## Summary Fixes astral-sh#9974 ## Test Plan I added some new test cases.
Summary
Fixes #9974
Test Plan
I added some new test cases.