python-trio / flake8-async

Highly opinionated linter for Trio code
https://flake8-async.readthedocs.io
MIT License
17 stars 2 forks source link

reimplement 91X in libcst #143

Closed jakkdl closed 1 year ago

jakkdl commented 1 year ago

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 in uncheckpointed_statements

test_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_module call 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 --runfuzz if 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:

  1. realized that comprehensions aren't handled, added regression tests.
  2. I think the nested error test ... was wrong? I don't see why that should give two errors per line.
  3. multiple yields in a single boolop is no longer handled as before. I maybe should've just done fmt: off instead of fighting over and over with black/shed
jakkdl commented 1 year ago

Oh, I didn't check coverage before pushing. Will look at that another day.

jakkdl commented 1 year ago

test_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_module call 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 --runfuzz if 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.

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. The existing fuzz tests takes ... literal forever now though. I had it run for 17 minutes just now before ast.parse crashed on unicode/escape sequence crap.

jakkdl commented 1 year ago

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.