The PR is split into two commits, the first does the refactoring, the second splits up VisitorMiscChecks (turns out some comments were wrong in what visitors in it were necessary for which checks...) so most of those checks should be easier to read. So I'd recommend reading the diffs for those separately.
Was somewhat tricky to get this working with some of the very particular checks - and so it doesn't fully only iterate through the tree once. If a visit_ function calls self.visit() (either directly or through self.generic_visit()/self.visit_nodes()) it will set a flag self.novisit which tells the external runner to exclude that class when visiting subnodes so they're not visited twice.
To not have to do that too often I added Flake8TrioVisitor.save_state(node) which serves the same purpose as get_state/set_state did previously
outer = self.get_state(...)
# do stuff before visiting child nodes
...
# visit child nodes
self.generic_visit(node)
# restore state
self.set_state(outer)
so that can now be written
self.save_state(node, ...)
# do stuff before visiting child nodes
...
# generic_visit and set_state are both handled by external runner
get/set state are still used in a couple places when manually visiting, but should be possible to get rid of them completely and only have save_state().
I think I could get rid of nodes visiting their own subnodes entirely by adding
exit_Xxx which is called for a node after it's subnodes have been visited (would clean up all cases of self.generic_visit(node) being used)
visit functions for node parts that in the AST don't have dedicated nodes, e.g. visit_Finally or visit_WhileElse. This would split up several long functions and solve the last cases of revisits - only trouble child would be 107_108.visit_loop, which explicitly wants to visit the body twice atm, but I could probably rewrite that.
This would maybe give some performance gains, but should make the code simpler as well and can entirely remove Flake8TrioVisitor.visit() and .visit_nodes() - at the cost of adding some more "magic" method names or other construction (realizing as I'm writing I can have visitor classes implement a visit_Xxx as a pseudo-contextmanager, with enter and exit as magic names, and other names called as they match up with the _fields of the Xxx node. And the external runner handles a visit different depending on if it's isinstance function or class)
I'm not super happy about the current novisit solution with lots of set updating, and I have some alternative solutions, but getting rid of it entirely by implementing 1&2 above would ofc be even nicer.
Looking at runtime I'm not seeing much if any improvement in performance, not even just looking at --runfuzz -k site_code, so maybe reiterating through the tree multiple times wasn't that much of an issues, or my external runner adds enough overhead it's voided, but there could maybe be gains on code with large files and complicated functions - or at least when 1+2 are implemented.
I'm also starting to want to split up flake8_trio.py into different files, 1500 lines is starting to be quite inconvenient to navigate, and get any overview on, but not gonna do that in this PR as it would completely mess up any remaining diff logic - I also know you are hesitant to split given installation complications so would have to take care when doing that.
There's two TODO's in the code, probably not gonna touch them in this PR
Exclude subclasses for disabled checks - probably not a priority for Anthropic since you're always gonna use all checks (for now), but would help when debugging a single runner and could help to find out if any one visitor is eating a ton of time (and e.g. find out if 1+2 are worth it for performance reasons).
don't hard code unnecessary variables in get_state - elected not to touch this for now as it would most likely just mean renaming a lot of variables and that'd make a big diff even bigger. But is bound to introduce bugs in it's current state so plan to fix it soon after merge.
The PR is split into two commits, the first does the refactoring, the second splits up VisitorMiscChecks (turns out some comments were wrong in what visitors in it were necessary for which checks...) so most of those checks should be easier to read. So I'd recommend reading the diffs for those separately.
Was somewhat tricky to get this working with some of the very particular checks - and so it doesn't fully only iterate through the tree once. If a
visit_function callsself.visit()(either directly or throughself.generic_visit()/self.visit_nodes()) it will set a flagself.novisitwhich tells the external runner to exclude that class when visiting subnodes so they're not visited twice.To not have to do that too often I added
Flake8TrioVisitor.save_state(node)which serves the same purpose asget_state/set_statedid previouslyso that can now be written
get/set state are still used in a couple places when manually visiting, but should be possible to get rid of them completely and only have
save_state().I think I could get rid of nodes visiting their own subnodes entirely by adding
exit_Xxxwhich is called for a node after it's subnodes have been visited (would clean up all cases ofself.generic_visit(node)being used)visit_Finallyorvisit_WhileElse. This would split up several long functions and solve the last cases of revisits - only trouble child would be 107_108.visit_loop, which explicitly wants to visit the body twice atm, but I could probably rewrite that.This would maybe give some performance gains, but should make the code simpler as well and can entirely remove
Flake8TrioVisitor.visit()and.visit_nodes()- at the cost of adding some more "magic" method names or other construction (realizing as I'm writing I can have visitor classes implement avisit_Xxxas a pseudo-contextmanager, withenterandexitas magic names, and other names called as they match up with the_fieldsof theXxxnode. And the external runner handles a visit different depending on if it'sisinstancefunction or class)I'm not super happy about the current
novisitsolution with lots ofsetupdating, and I have some alternative solutions, but getting rid of it entirely by implementing 1&2 above would ofc be even nicer.Looking at runtime I'm not seeing much if any improvement in performance, not even just looking at
--runfuzz -k site_code, so maybe reiterating through the tree multiple times wasn't that much of an issues, or my external runner adds enough overhead it's voided, but there could maybe be gains on code with large files and complicated functions - or at least when 1+2 are implemented.I'm also starting to want to split up flake8_trio.py into different files, 1500 lines is starting to be quite inconvenient to navigate, and get any overview on, but not gonna do that in this PR as it would completely mess up any remaining diff logic - I also know you are hesitant to split given installation complications so would have to take care when doing that.
There's two TODO's in the code, probably not gonna touch them in this PR
get_state- elected not to touch this for now as it would most likely just mean renaming a lot of variables and that'd make a big diff even bigger. But is bound to introduce bugs in it's current state so plan to fix it soon after merge.