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

ASYNC113 & ASYNC121 false alarm with nested sync functions #287

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Sep 9, 2024

Fixes #286

I also did a search for visit_FunctionDef and visit_AsyncFunctionDef in the codebase and caught the same error for async113 (though it should very rarely occur for 113). At first glance a couple other visitors had the same problem, until I noticed those were cst visitors (which instead has an asynchronous attribute).

For 113 we could consider having the logic for visit_FunctionDef being the same as visit_AsyncFunctionDef instead of always setting self.aenter = False in case users are transforming sync methods to async methods, but given such exotic stuff we're maybe better off staying away and avoiding super obscure false positives.

We could consider adding a test that checks if any AST visitors only has one of the visitors defined, and could do the same for With/AsyncWith and For/AsyncFor (the latter which was in fact only caught in code review for #282). It will have a decent false-positive ratio though, but not very troublesome to have an ignore list or declare dummy methods.

Another approach is preferring cst over AST, and I've been inconsistent when creating new visitors despite #124. I should try and compile pros/cons and write a comment over there.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Thanks @jakkdl!

@Zac-HD Zac-HD merged commit cee691f into python-trio:main Sep 9, 2024
10 checks passed
@jakkdl jakkdl deleted the async121_false_alarm branch September 10, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASYNC121 false alarm in nested async function definition
2 participants