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

reimplement 91X in libcst #143

Merged
merged 1 commit into from
Feb 28, 2023
Merged

reimplement 91X in libcst #143

merged 1 commit into from
Feb 28, 2023

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Feb 24, 2023

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

self.set_state(outer)
self.outer[node]["uncheckpointed_statements"] = self.uncheckpointed_statements
self.restore_state(node)
...
Copy link
Member Author

@jakkdl jakkdl Feb 24, 2023

Choose a reason for hiding this comment

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

surprised no flake8 plugin warned about this ellipsis

@jakkdl
Copy link
Member Author

jakkdl commented Feb 24, 2023

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

@jakkdl
Copy link
Member Author

jakkdl commented Feb 27, 2023

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
Copy link
Member Author

jakkdl commented Feb 27, 2023

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.

Copy link
Member Author

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

comments on the diff to ease review

flake8_trio/visitors/flake8triovisitor.py Show resolved Hide resolved
flake8_trio/visitors/flake8triovisitor.py Show resolved Hide resolved
flake8_trio/visitors/flake8triovisitor.py Show resolved Hide resolved
flake8_trio/visitors/flake8triovisitor.py Show resolved Hide resolved
flake8_trio/visitors/helpers.py Show resolved Hide resolved
tests/eval_files/trio911.py Show resolved Hide resolved
tests/eval_files/trio911.py Show resolved Hide resolved
tests/eval_files/trio911.py Show resolved Hide resolved
tests/test_flake8_trio.py Show resolved Hide resolved
tests/test_flake8_trio.py Show resolved Hide resolved
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.

With tests passing, this is IMO a clear step forward from the status quo (🎉), so given the nit-picky nature of remaining comments I'll merge this now and we can follow up in a future PR - it'll be easier than re-reviewing this one. Thanks again!

flake8_trio/visitors/helpers.py Show resolved Hide resolved
flake8_trio/visitors/flake8triovisitor.py Show resolved Hide resolved
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

I think here you mean bound=Union[...]? The multi-arg form means "must be an exact match to one of these types".

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Comment on lines +145 to +150
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

flake8_trio/visitors/helpers.py Show resolved Hide resolved
tests/eval_files/trio910.py Show resolved Hide resolved
tests/eval_files/trio911.py Show resolved Hide resolved
flake8_trio/visitors/visitor91x.py Show resolved Hide resolved
flake8_trio/visitors/visitor91x.py Show resolved Hide resolved
flake8_trio/visitors/visitor91x.py Show resolved Hide resolved
@Zac-HD Zac-HD merged commit 7417b83 into python-trio:main Feb 28, 2023
@jakkdl jakkdl mentioned this pull request Feb 28, 2023
@jakkdl jakkdl mentioned this pull request Mar 11, 2023
63 tasks
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.

2 participants