reimplement 91X in libcst #143
Conversation
flake8_trio/visitors/visitor91x.py
Outdated
| self.set_state(outer) | ||
| self.outer[node]["uncheckpointed_statements"] = self.uncheckpointed_statements | ||
| self.restore_state(node) | ||
| ... |
There was a problem hiding this comment.
surprised no flake8 plugin warned about this ellipsis
|
Oh, I didn't check coverage before pushing. Will look at that another day. |
Switched to using CSTTransformer instead of m.MatcherDecoratableTransformer and that slashed the runtime of 910_permutations from 41.82s to 14.35s - so I'll default to CSTTransformer unless the decorators are actually useful. (Sidenote: I looked into writing my own, and it's not that easy). 14s is still enough to be kind of a hassle though, so added it to fuzz only. |
|
I had to add a bunch of stuff to eval files for coverage, since multiple visitors using the same helper shared the burden of covering them - but now that some of them are converted to libcst all the cases have to be covered both by cst visitors and ast visitors. |
| @@ -27,6 +28,7 @@ | |||
|
|
|||
| T = TypeVar("T", bound=Flake8TrioVisitor) | |||
| T_CST = TypeVar("T_CST", bound=Flake8TrioVisitor_cst) | |||
| T_EITHER = TypeVar("T_EITHER", Flake8TrioVisitor, Flake8TrioVisitor_cst) | |||
There was a problem hiding this comment.
I think here you mean bound=Union[...]? The multi-arg form means "must be an exact match to one of these types".
There was a problem hiding this comment.
Confused me for a minute why the multi-arg form doesn't give errors when passed subtypes, but figured out that it transforms the type.
And mypy gave me errors on the union one, but that's a bug with a PR in the pipe: python/mypy#14755
| def cst_literal_eval(node: cst.BaseExpression) -> Any: | ||
| ast_node = cst.Module([cst.SimpleStatementLine([cst.Expr(node)])]).code | ||
| try: | ||
| return ast.literal_eval(ast_node) | ||
| except Exception: # noqa: PIE786 | ||
| return None |
There was a problem hiding this comment.
Seems surprising to return None on failure, instead of raising an exception - you lose the ability to distinguish None from foobar() (which raises ValueError as it's not a literal).
We might also want to open an upstream issue - there's Integer.evaluated_value for example, but no such property on e.g. lists or tuples and fixing that would let future projects avoid this helper function.
This one is chonky
I might go over it tomorrow and add review comments to help reviewing it. Although there's not really much to do other than grind through it - and running it on existing code, I wouldn't be shocked if there's crashes or bugs that didn't exist before, so maybe do that before releasing.
I generally optimized for minimizing the diff, rather than writing the best / most logical code - might leave that for a second commit, or do it when implementing autofix. Esp any
isinstances should generally not be there when working against libcst.I rewrote the loop checker to not require two passes, by injecting a fake statement and doing some fancy logic. Quite happy I came up with this solution, much better than the previous super messy one.
Removing redundant checkpoints might be more doable with this approach as well.
Having to access metadata to get the linenos of nodes is quite ugly, with the redundant checkpoint I'll be looking to store nodes instead of
Statements inuncheckpointed_statementstest_910_permutations is now extremely slow, previously being so fast I never noticed it, but now it takes 50 seconds on my machine. I did some rudimentary timing, and the
parse_modulecall took 5 of the seconds, and plugin.run() taking 45 seconds. Now that is across ~4000 calls, so nothing too crazy, but I'll probably put it behind--runfuzzif I don't find anything responsible for the slowdown. 91X currently doesn't use any decorator markers, so changing it to be a simple transformer might do wonders.A few changes in eval_files:
fmt: offinstead of fighting over and over with black/shed