Add checks for unconsistent returns #431
Conversation
Added in PEP 8 : """ Be consistent in return statements. Either all return statements in a function should return an expression, or none of them should. If any return statement returns an expression, any return statements where no value is returned should explicitly state this as return None, and an explicit return statement should be present at the end of the function (if reachable). """ Checking for unconsistent returns corresponds to implementing a check based on ast tree. Given an AST tree, one can easily collect return statements and check whether they return an explicit value or not. Also, from the AST tree, one can try to check whether the end of the function is reachable. Warning W740 is added for explicit inconsistent returns. Warning W741 is added for implicit inconsistent returns : when a functions explicitly returns a value at some point but does not at the (reachable) end of the function. If the end of a function is not reachable but warning is triggered, one might simply add a "return None" or an "assert False" : as one said : "Explicit is better than implicit.". Regarding the code : Implementation has been made as easy to understand as possible. The new check itself has been implemented in a new class UnconsistentReturns which uses yet another class FlowAnalysis which serves as an holder for various helper methods. Also, I took this chance to change a few details so that AST-tree checks fit more easily. This changes a few APIs. I don't know if anyone is relying on those. Regarding the tests : Adding the first AST-tree based check leads to most of the (incorrect) test code to be parsed which leads to many SyntaxError either related to a single version of Python or to all of them. A A new symbol has been to be able to convey the fact that an error is expecting only for such or such version of Python. I've fixed all issues related to SyntaxError so that they are all passing all right. I hope I haven't changed what is actually tested.
|
Btw this is for #399 |
|
It seems like a few things do no pass properly on all Python versions. Will work on it soon. |
|
That said, however this is implemented, it should start out in the |
Comment was made that checks should be ignored by default. This is now the case and code/documentation and tests have been updated. Also, Travis commented that a few things went wrong on pretty much all Python versions. I think I have fixed most of the issues (related to the fact that : - some AST elements have changed - sys.version_info is not a namedtuple on Python 2.6). Also, I realised that a test would fail as a SyntaxError only on Python 2.6. For that reason, I've made the version tag on error somewhat more generic. Now, it can be any prefix for a version : 2 corresponds to all Python 2 version as 2.6 correponds to 2.6 and so on...
|
I've updated the commit accordingly. |
SyntaxErrors were reported in a slightly different place making the tests fail. Removing error location for the 3 errors involved fixes the issue.
| return i | ||
|
|
||
|
|
||
| # W741:1:1 |
There was a problem hiding this comment.
Confusing comment which is the glitch of a temporary version. No warning is actually returned (missing colon in the convent). To be removed asap.
|
Any news regarding this pull-request ? |
|
Any news regarding this PR ? |
|
@SylvainDe: There are branch conflicts that need fixing. I'm here at PyCon Sprints in Portland, OR. I'll be happy to fix the conflicts and propose a new branch. |
|
I'm going to try to fix the branch conflicts. My plan is to make my own fork, cherry pick these commits to that fork, rebase PyCQA master, and fix any conflicts that come up. Then I'll come back to this thread and reference my PR with the fixes. |
|
So having tried to get this PR to not have merge requests for a couple of hours I think that either the PR needs to be rebased against PyCQA Master (that will be hard since the PR merges master) or I can take the big idea's behind @SylvainDe's code and make my own branch. I think I'd prefer to first since that gives @SylvainDe the commit credit. They most likely are going to have to do a If I haven't heard anything from them in the next couple of days I'll make me own branch and re-create their changes from a up to date fork. |
|
A mid way point could be to 'git commit --author=...' Where you could recreate the attribution manually. |
|
I like that option actually. I can do this tomorrow when I sprint. |
|
Hi guys, I'm happy for this change to be integrated, whoever performs the final steps and I am really happy to see some new interest in this PR. In any case, I won't be able to do so in the next couple of days so if you guys want to make the best out of PyCon sprints, go for it. If you haven't time to do so, please let me know and I'll try to handle this as soon as possible. Thanks in advance Additional notes:
|
|
@SylvainDe and @IanLee1521 I was told today by a friend at the PyCon sprints that using the AST in PyCodeStyle wasn't allowed. This change imports the AST. Is that policy statement true? If it is how should we approach this PR? |
|
@rawrgulmuffins As far as I am concerned, I wish I knew the answer. From my point of view, this policy was made explicit only after I created the pull request (and I think the exact text comes from the history you can find from this very page). My understanding is that the ast module will be actually used only if at least one check needs it so that you don't pay for what you don't need. Because the check for consistent return is not part of the default checks, I guess it's OK to have it rely on ast module because normal user won't see the performance impact. I'll let the official maintainer give you more details because that would much interest me (also I was wondering if there were any documentation about implementing ast checks as well). I hope the PyCon sprints are going well and I hope I could be there and enjoy it with you guys (and girls obviously). |
|
@SylvainDe Thanks for the quick reply. I've been having a lot of fun at the sprints this year. |
|
@IanLee1521 @sigmavirus24 Now that I have a bit more free time on my hands, do you reckon it is worth spending some more time on this PR to solve the merge issues ? |
|
This PR uses the word "unconsistent" in almost all places. As far as I'm concerned, this is not a word and "inconsistent" should be used. |
|
Oh indeed. I'll fix this asap. Thanks for spotting. |
|
@SylvainDe I intend to implement this check in Pylint, let me know if it's OK with you to reuse some of the code wrote by you in this PR. |
|
It's totally fine for me :-)
Le 18 janv. 2017 23:30, "Łukasz Rogalski" <notifications@github.com> a
écrit :
… @SylvainDe <https://github.com/SylvainDe> I intend to implement this
check in Pylint, let me know if it's OK with you to reuse some of the code
wrote by you in this PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#431 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABL0rbiVBKyO4W03_7QK1ETTSNHJzXX1ks5rTpKggaJpZM4FyAaF>
.
|
|
@rogalski Also, feel free to ask me if anything at all needs some explanation. |
|
The original PR is too big to be handled properly. I'll try to split into smaller, easier to integrate parts... |
Added in PEP 8 :
"""
Be consistent in return statements. Either all return statements in a
function should return an expression, or none of them should. If any
return statement returns an expression, any return statements where no
value is returned should explicitly state this as return None, and an
explicit return statement should be present at the end of the function
(if reachable).
"""
Checking for unconsistent returns corresponds to implementing a check
based on ast tree. Given an AST tree, one can easily collect return
statements and check whether they return an explicit value or not.
Also, from the AST tree, one can try to check whether the end of
the function is reachable.
Warning W740 is added for explicit inconsistent returns.
Warning W741 is added for implicit inconsistent returns : when a
functions explicitly returns a value at some point but does not
at the (reachable) end of the function. If the end of a function
is not reachable but warning is triggered, one might simply add a
"return None" or an "assert False" : as one said : "Explicit is
better than implicit.".
Regarding the code :
Implementation has been made as easy to understand as possible.
The new check itself has been implemented in a new class
UnconsistentReturns which uses yet another class FlowAnalysis
which serves as an holder for various helper methods.
Also, I took this chance to change a few details so that AST-tree
checks fit more easily. This changes a few APIs. I don't know if
anyone is relying on those.
Regarding the tests :
Adding the first AST-tree based check leads to most of the
(incorrect) test code to be parsed which leads to many SyntaxError
either related to a single version of Python or to all of them. A
A new symbol has been to be able to convey the fact that an error
is expecting only for such or such version of Python.
I've fixed all issues related to SyntaxError so that they are all
passing all right. I hope I haven't changed what is actually tested.