-
Notifications
You must be signed in to change notification settings - Fork 179
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
Flatten handlers #70
base: main
Are you sure you want to change the base?
Flatten handlers #70
Conversation
This is just a WIP; while passes all tests, it is very likely that more tests are needed, and some of the handlers need to be switched back to being |
e3b323c
to
ea032f4
Compare
Not really a WIP any more. tests now exist for every case which didnt fail when they should have before. I've run it against against ~300 of the largest OSS Python codebases, and it has no differences except ordering of errors within the same line. A slightly older version of this change had a minor perf hit, of 3-5 mins increase for what was a 55 min task of processing all of those ~300 OSS codebases . I expect that perf hit has been reduced slightly with this version, and I am rerunning that task now, but this isnt properly isolated perf testing so if this is a serious concern we'll need to do proper perf testing. |
@sigmavirus24, it looks like an infrastructure failure on Appveyor, due to BitBucket. I've successfully run the build twice on Appveyor. Could you restart it, to see if it is working again now on your account. Also a review would be lovely, especially since you looked at this general problem a while back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this yet but here's a first pass at a review.
if handler is False: | ||
return | ||
|
||
if not handler: | ||
handler = self.getNodeHandler(node.__class__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've moved this outside of the try
. I suspect it was in there for a reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need to do better than suspect. :P
pyflakes/checker.py
Outdated
DELETE = PRINT = EXEC = EXPR = RAISE = handleChildrenFlattened | ||
ASSIGN = TRYFINALLY = handleChildren | ||
FOR = ASYNCFOR = WHILE = IF = WITH = ASYNCWITH = handleChildren | ||
WITHITEM = ASYNCWITHITEM = handleChildrenFlattened |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not put all of the nodes using handleChildrenFlattened
together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have grouped them by type of node in a readible manner, as best I could with this subset of all nodes
I felt the "with-item" nodes should go after the "with" nodes and other branching nodes.
if handler and handler not in _may_flatten: | ||
yield node, parent, handler | ||
else: | ||
nodes[:] += ((child, node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the [:]
do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List resizing. https://stackoverflow.com/a/10155987
pyflakes has traditionally recursed with a handler for every level of the ast. The ast depth can become very large, especially for an expression containing many binary operators. Python has a maximum recursion limit, defaulting to a low number like 1000, which resulted in a RuntimeError for the ast of: x = 1 + 2 + 3 + ... + 1001 This change avoids recursing for nodes that do not have a specific handler. Checker.nodeDepth and node.depth changes from always being the ast depth, which varied between Python version due to ast differences, to being the number of nested handlers within pyflakes.
pyflakes has traditionally recursed with a handler for every
level of the ast. The ast depth can become very large, especially
for an expression containing many binary operators.
Python has a maximum recursion limit, defaulting to a low number
like 1000, which resulted in a RuntimeError for the ast of:
This change avoids recursing for nodes that do not have a specific
handler.
Checker.nodeDepth and node.depth change from always being the
ast depth, which varied based on Python version due to ast differences,
to being the number of nested handlers within pyflakes.