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

Add checks for unconsistent returns #431

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

SylvainDe
Copy link

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.

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.
@SylvainDe
Copy link
Author

Btw this is for #399

@SylvainDe
Copy link
Author

It seems like a few things do no pass properly on all Python versions. Will work on it soon.

@sigmavirus24
Copy link
Member

pep8 is intended to be as fast as possible. Using the ast module defeats that purpose. This is why pep8-naming is a separate plugin. To avoid including ast parsing in pep8.

That said, however this is implemented, it should start out in the DEFAULT_IGNORE list.

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...
@SylvainDe
Copy link
Author

I've updated the commit accordingly.
There is still a failure on Travis corresponding to test failing on pypy because of SyntaxError. I'll have to find the right way to fix this asap.

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

Choose a reason for hiding this comment

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

Confusing comment which is the glitch of a temporary version. No warning is actually returned (missing colon in the convent). To be removed asap.

@SylvainDe
Copy link
Author

Any news regarding this pull-request ?

doismellburning added a commit to doismellburning/pep8 that referenced this pull request Nov 8, 2015
@SylvainDe
Copy link
Author

Any news regarding this PR ?

@ArcTanSusan
Copy link
Contributor

@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.

@rawrgulmuffins
Copy link

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.

@rawrgulmuffins
Copy link

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 git reset --hard on their last two commits, fetch PyCQA:master, and then rebase.

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.

@IanLee1521
Copy link
Member

IanLee1521 commented Jun 3, 2016

A mid way point could be to 'git commit --author=...' Where you could recreate the attribution manually.

@rawrgulmuffins
Copy link

I like that option actually. I can do this tomorrow when I sprint.

@SylvainDe
Copy link
Author

SylvainDe commented Jun 3, 2016

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:

  • my commits/PR may contain changes which are not very relevant and can probably be discarded/pushed in a different commit (I was trying to wrap my head around the integration of AST checks at the time).
  • probably an irrelevant detail but in [DRAFT] Add support for checks on unconsistent returns #527 , the sentence "The original author is initially SylvainDe in Add checks for unconsistent returns #431, which is a PR that has been inactive for over a year" is just not true. If you look at the history, you have both commits and messages less than a year ago and until quite recently (?), this PR status was ready to be merged.

@rawrgulmuffins
Copy link

@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?

@SylvainDe
Copy link
Author

@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).

@rawrgulmuffins
Copy link

@SylvainDe Thanks for the quick reply. I've been having a lot of fun at the sprints this year.

@SylvainDe
Copy link
Author

@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 ?

@FichteFoll
Copy link
Contributor

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.

@SylvainDe
Copy link
Author

Oh indeed. I'll fix this asap. Thanks for spotting.

@rogalski
Copy link
Contributor

@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.

@SylvainDe
Copy link
Author

SylvainDe commented Jan 19, 2017 via email

@SylvainDe
Copy link
Author

@rogalski Also, feel free to ask me if anything at all needs some explanation.

@SylvainDe
Copy link
Author

The original PR is too big to be handled properly. I'll try to split into smaller, easier to integrate parts...

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.

7 participants