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

Avoid unused async when context manager includes TaskGroup #12605

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

charliermarsh
Copy link
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
19 checks passed
@charliermarsh charliermarsh deleted the charlie/tg branch August 1, 2024 02:12
Copy link
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
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
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
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