-
Notifications
You must be signed in to change notification settings - Fork 179
Checking for non-ast syntax errors #20
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
Closed
Closed
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
da26b7c
Handle return outside of function at the module level
asmeurer 26abc96
Catch most instances of continue and break outside a loop
asmeurer 58fd813
Add tests for continue/break nested in a loop
asmeurer 7dc7bd0
Fix flake8 warnings
asmeurer 4b158fb
Catch continue and break in a function definition in a loop
asmeurer fc8acc7
Move some tests to the proper test function
asmeurer 696f5a6
Handle continue and break nested in an for/while else clause correctly
asmeurer 120bf3a
Handle 'continue' in a 'finally' block
asmeurer 2a021cc
Fix flake8 errors
asmeurer e8d726a
Make the __future__ warning match the Python SyntaxError error message
asmeurer 8fca681
Detect default 'except:' that isn't last (a SyntaxError)
asmeurer 892596f
Fix whitespace
asmeurer ac7a245
Add some tests for multiple 'except:' not last instances
asmeurer 72d3885
Fix PEP 8 warning
asmeurer 14e81a1
Add support for non-ast syntax errors around starred assignments
asmeurer d844b9b
Detect yield and yield from outside a function
asmeurer d222013
The 'yield from' outside of function error message is the same as 'yi…
asmeurer 590d54f
Clean up and make more efficient the CONTINUE and BREAK algorithm
asmeurer bc758dd
Merge branch 'master' into nonast2
asmeurer dc4c84f
Fix flake8 "errors"
asmeurer db6954f
Merge branch 'master' into nonast2
asmeurer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Could this be broken up into functions so it can be more clear what the overall algorithm is?
-- from The Zen of Python, by Tim Peters
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.
Coming back to this (I told you I would get stalled). It's not clear to me how to break this up. Would it be clearer if it were written recursively instead of using a while loop? I can definitely add more comments explaining the algorithm and what it's checking for.
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.
Rather than comments, I'd look for loops which could be broken out into specific functions. Then the name of the functions can serve as the documentation.
The high-level algorithm here, as I understand it, is "when a continue or break is encountered, it's an error if that's not in a loop". So I might expect to find a function named
isInLoop()
. Then maybeCONTINUE()
might be something like:Also, the operation, "traverse the parents until finding a node matching some predicate" has become a pretty common operation, so that can be a function too. Just grepping for
parent
I find a few, the poorly namedgetParent
, and alsoon_conditional_branch()
.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.
That's roughly the algorithm (and more or less what I had in the first pass), but not completely correct. More accurate is "continue or break is only valid if it is in a loop, and if it a function or class definition, the loop must also be inside the function or class definition. And also for continue if it is in a finally block the loop must also be inside the finally block." I'm not sure if that can be written as a simple predicate (but I may just need to think about it harder).
I guess the algorithm itself is not too complicated (it's just, walk up the tree and note what you see first, a loop, a function definition, a class definition, or (for
continue
) a finally block). The complications are arising from the fact that for loops and finally blocks, the nodes have different subtrees, so just saying "thecontinue
is in theast.For
is not enough because it could be in theorelse
part. So maybe that's what really should be abstracted here.Thoughts?
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.
It also occurs to me that this algorithm is a rather inefficient way of dealing with this issue. When I see a loop, I don't know if I am in the loop or the orelse part, so I walk the whole tree to make sure I'm not in the orelse part. But I really ought to be keeping track of the previous node and checking that way. Fixing this may also make the whole algorithm clearer (and I also probably should comment the code a little more). Let me give it a try and you can see what you think.
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.
OK, the latest version should be much easier to read. I don't think it's necessary to split it into a function at this point.
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.
Plus the algorithm itself no longer uses the for/else construct that it was so concerned about, which is a plus, as few people understand how to read that correctly.