Add checker for consistency of return statement

[rogalski]

Steps to reproduce

Consider PEP8:
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).

Yes:

def foo(x):
    if x >= 0:
        return math.sqrt(x)
    else:
        return None

def bar(x):
    if x < 0:
        return None
    return math.sqrt(x)

No:

def foo(x):
    if x >= 0:
        return math.sqrt(x)

def bar(x):
    if x < 0:
        return
    return math.sqrt(x)

Current behavior

R: 2, 4: Unnecessary "else" after "return" (no-else-return) for foo method of Yes section.

Expected behavior

inconsistent-returns emitted for samples in No: section. Message should be emitted for function and not individual returns (as inconsistency occurs in function). Accepted functions are those that have (only returns with explicit value and no implicit return) or (only empty returns and implicit return at the end).

Also, no-else-return seems more like a nitpick and less like a valuable check.

pylint --version output

master

Since PyCQA/pycodestyle#431 seems to be rejected due to 'no AST' policy, it likely should be covered by pylint.

[PCManticore]

I personally like no-else-return, but we can move it to a less important stage in 2.0.

Regarding the issue itself, sounds okay, although, now that we are talking about no-else-return, I would put it in the same stage / tier, meaning a kind of nitpick. For example, the refactoring checker has some checks that are pretty useful, that can make the code more readable, while these two are just visually pleasant.

[rogalski]

I disagree about it being just a 'visually pleasant' - it may be an indicator of messy interface. If function sometimes returns value, sometimes returns early "without a value", what does return value represent? If return is meant to be None, explicit is better than implicit, and this explicitness should be expressed in code by using None literal.

[PCManticore]

I am sorry but let's agree to disagree. There was no problem with having no return until the PEP 8 specifically mandated it.

[joeflack4]

I'm getting Unnecessary "else" after "return" (no-else-return) for the following.

` def to_json(self, pretty=False):
"""Get the JSON representation of raw ODK form.

    Args:
        pretty (bool): Activates prettification, involving insertion of
            several kinds of whitespace for readability.

    Returns:
        json: A full JSON representation of the XLSForm.
    """
    import json
    raw_survey = []
    header = self.metadata['raw_data']['survey'][0]
    for i, row in enumerate(self.metadata['raw_data']['survey']):
        if i == 0:
            continue
        raw_survey.append({str(k): str(v) for k, v in zip(header, row)})

    if pretty:
        return json.dumps(raw_survey, indent=2)
    else:
        return json.dumps(raw_survey)`

Any explanation as to why I'm seeing this lint error? Doesn't seem consistent with the explanation given in this thread.

[moylop260]

Try to use the following changes:

...
if pretty:
    return json.dumps(raw_survey, indent=2)
- else:
-    return json.dumps(raw_survey)`
+ return json.dumps(raw_survey)`

if the sentence if is used so a return will be used and the last sentence won't be executed.
If the sentence if is not used so the last return will be used.

I means, you don't need a else because the if-return force that all next code is a else section without a explicit else

More info about: http://eslint.org/docs/rules/no-else-return

[joeflack4]

Makes quite a lot of sense! Thanks.

[AWhetter]

To any beginner contributors, this will involve implementing a new check, likely on the FormatChecker.

One idea for an implementation might be to create a stack of the return statements in each functiondef by defining visit_functiondef and leave_functiondef on the checker, similarly to what the variables checker does (https://github.com/PyCQA/pylint/blob/30f0ff9178f75385c63ee0c445223641494a95aa/pylint/checkers/variables.py#L563), except that you would be storing a list of the return nodes in the function, and checking a visited return node against the return nodes in the stack.
This is just an idea. How you implement the check is ultimately up to you.

[hippo91]

I'm working on it. Could you please add a "Work in progress" label?