Skip to content

Avoid unused async when context manager includes TaskGroup#12605

Merged
charliermarsh merged 1 commit intomainfrom
charlie/tg
Aug 1, 2024
Merged

Avoid unused async when context manager includes TaskGroup#12605
charliermarsh merged 1 commit intomainfrom
charlie/tg

Conversation

@charliermarsh
Copy link
Copy Markdown
Member

Summary

Closes #12354.

@charliermarsh charliermarsh added the bug Something isn't working label Aug 1, 2024
@charliermarsh charliermarsh enabled auto-merge (squash) August 1, 2024 02:09
@charliermarsh charliermarsh merged commit d774a3b into main Aug 1, 2024
@charliermarsh charliermarsh deleted the charlie/tg branch August 1, 2024 02:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 1, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

.semantic()
.resolve_qualified_name(call.func.as_ref())
.is_some_and(|qualified_name| {
matches!(qualified_name.segments(), ["asyncio", "TaskGroup"])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it hold for any async context manager? I don't think there is something unique about asyncio.TaskGroup.

If the async with is explicitly nested then ruff doesn't trigger the error. So perhaps the fix should be to make

async with asyncio.timeout(), foo():
    ...

be treated the same as

async with asyncio.timeout():
    async with foo():
        ...

According to the Language Reference they are semantically equivalent (this is for with, the async with ref seems to have an omission here):

image

I also checked async for and it's detected correctly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interested in changing it? :) We might need to accommodate cases like async with asyncio.timeout(), asyncio.timeout(), I guess those should still trigger even if weird.

charliermarsh pushed a commit that referenced this pull request Aug 2, 2024
… (`ASYNC100`) (#12643)

## Summary

Please see
#12605 (comment) for
a description of the issue.

They way I fixed it is to get the *last* timeout item in the `with`, and
if it's an `async with` and there are items after it, then don't trigger
the lint.

## Test Plan

Updated the fixture with some more cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False positive ASYNC100 when pairing asyncio.timeout with asyncio.TaskGroup

2 participants