ASYNC912: timeout/cancelscope with only conditional checkpoints #242
Conversation
|
Got the major shit working, but needs some cleanup, comments, documentation and a self-review pass. So no need to review yet. |
| elif strip_error_subidentifier(error_code) not in self.options.enabled_codes: | ||
| elif ( | ||
| (ec_no_sub := strip_error_subidentifier(error_code)) | ||
| not in self.options.enabled_codes | ||
| and ec_no_sub not in self.options.autofix_codes | ||
| ): |
There was a problem hiding this comment.
it's not great that there's duplication of logic between the visitors (it confused me for a bit while developing). I originally expected all visitors to be rewritten to use libcst, but given that's not going to happen anytime soon (or at all), I should probably refactor these two and put common code in a parent class.
| # ASYNC100 | ||
| self.has_checkpoint_stack: list[bool] = [] | ||
| self.node_dict: dict[cst.With, list[AttributeCall]] = {} | ||
|
|
There was a problem hiding this comment.
the primary ASYNC100 logic is simply copy-pasted from visitor100.py
flake8_async/visitors/visitor91x.py
Outdated
| def should_autofix(self, node: cst.CSTNode, code: str | None = None) -> bool: | ||
| if code is None: | ||
| code = "ASYNC911" if self.has_yield else "ASYNC910" | ||
|
|
||
| return ( | ||
| not self.noautofix | ||
| and super().should_autofix( | ||
| node, "ASYNC911" if self.has_yield else "ASYNC910" | ||
| ) | ||
| and super().should_autofix(node, code) | ||
| and self.library != ("asyncio",) | ||
| ) |
There was a problem hiding this comment.
This change was needed in a previous implementation, but because async100 does work on asyncio (now) and does not care about self.noautofix (which I still haven't figured out why it was introduced), it now doesn't use this method at all. But good to respect an explicitly defined code anyway, which it didn't before.
jakkdl
changed the title
| elif strip_error_subidentifier(error_code) not in self.options.enabled_codes: | ||
| elif ( | ||
| (ec_no_sub := strip_error_subidentifier(error_code)) | ||
| not in self.options.enabled_codes | ||
| and ec_no_sub not in self.options.autofix_codes | ||
| ): |
flake8_async/visitors/helpers.py
Outdated
| if ( | ||
| isinstance(library_node, cst.Name) | ||
| and library_str == library_node.value | ||
| ): | ||
| break |
There was a problem hiding this comment.
These checks suggest to me that using a libcst matcher might be more efficient.
Could we have a helper function which parses a string ("name", or "x.y", or "x.y.z", or...) into the nodes, and then walks the nodes to return a corresponding matcher?
There was a problem hiding this comment.
yes, though sounds complicated to engineer. I'll give it a try and see if I get anywhere easily, otherwise throw in a TODO and maybe deal with it at a later point.
There was a problem hiding this comment.
Ended up being straightforward to write, but got sidetracked by Instagram/LibCST#1143
tests/eval_files/async912.py
Outdated
| # We don't know which cancelscope will trigger first, so to avoid false | ||
| # positives on tricky-but-valid cases we don't raise any error for the outer one. |
There was a problem hiding this comment.
nit, b/c clear in this case: I prefer "false alarm" vs "missed alarm", which is immediately descriptive.
By comparison 'false positive' vs 'false negative' is easy to confuse (is detecting a problem positive?), although still better than the abuse 'Type {1,2,I,II} error' terminology which gives absolutely no hints to the listener.
There was a problem hiding this comment.
Haha, I've seen you point this out about a dozen times and keep misremembering/forgetting which way to do it :D
Fixing
|
Added a couple corner cases, I don't think wrapping calls is terribly common - but if it is feel free to open an issue and I'll make it search for the matching calls differently. # wrapped calls do not raise errors
with customWrapper(trio.fail_at(10)):
...
with (res := trio.fail_at(10)):
...
# but saving with `as` does
with trio.fail_at(10) as res: # ASYNC912: 9
if bar():
await trio.lowlevel.checkpoint() |
oh, turns out #183 was already planning on using ASYNC912.
It looks like I broke some stuff, but I gotta go for the day so I thought I'd push in case you have time to review
tests/eval_files/async912.pyand have any opinions on the behavior in some of the edge cases in there.