Wemake-python-styleguide: Enforce consistent `return` statements

[sobolevn]

Rule request

Question

There are three ways to return None in python:

1. return
2. return None
3. function without a return

We need to ask developers what do they think about this inconsistency.
My opinion is that we should only keep 2 and possibly 3.

Why keeping 2? It is the most explicit notation out of the two options.

Why possibly 3? In my ideal world all functions should return something. Ideally in one place.
Yearly returns are also fine, but in this case all branches of this function must return something explicitly.

Thesis

After several hours of thinking about this problem and asking opinions of other developers, this is what I came to:

1. If there are no returns in a functions - that's fine
2. If not all code paths return - that's fine
3. If all return in a functions uses short form, that's fine
4. If any return in a function uses explicit form return something, it means that all returns must be explicit: return something

Related: https://eslint.org/docs/rules/consistent-return

Reasoning

This is done for consistency reasons.

[sobolevn]

Examples

Correct

def test(arg: int) -> int:
    if arg > 0:
        return arg
    return -arg

def test(arg: int) -> None:
    if arg > 0:
        return
    if arg < 10:
        return
    some_call(arg)

def test(arg: int) -> None:
    if arg > 0:
        return
    assert arg < 10  # no-return

Wrong

def test(arg: int) -> None:
    if arg > 0:
        return
    return None

def test(arg: int) -> None:
    if arg > 0:
        return None
    return

def test(arg: int) -> None:
    if arg > 0:
        return arg
    assert arg < 10  # no-return

[kxepal]

def test(arg: int) -> None:
    if arg > 0:
        return arg
    assert arg < 10  # no-return

Is that correct?

[sobolevn]

@kxepal You are right. This example is not correct. I have updated the description.

[kxepal]

:beer:
However, examples two and three quite simiar. May be improve second one with return None in both of cases?

[sobolevn]

No, that's redundant. When using early return pattern (all returns are used without any arguments) it is fine not to have an extra return in the end.

@kxepal maybe you want to help me with this one? I would appreciate your help!

[sobolevn]

Ok, after some initial attempts to handle this I have found the right explanation for this feature. It consists of several independent rules.

Consistent return values

We only check that if any return contains a value - all returns must contain a value. We do not check types or that all path returns. Only the value.

Last line extra return

It is possible to finish your function with a return statement. Like so:

def some():
      ...
      return

Or

def some():
      ...
      return some_value_or_None

The thing is: just return in the last line is not required at all. It is more readable not to write it.
But having return some_value_or_None is always possible.

[kxepal]

@sobolevn
Actually, I'm not sure how to start with all these asts. Could you recommend any good example around?

[sobolevn]

A good place to start is https://wemake-python-styleguide.readthedocs.io/en/latest/pages/api.html
Then head to https://wemake-python-styleguide.readthedocs.io/en/latest/pages/contributing.html and https://greentreesnakes.readthedocs.io/en/latest/

Then read about what visitors are: https://wemake-python-styleguide.readthedocs.io/en/latest/pages/visitors/base.html
Then about violations: https://wemake-python-styleguide.readthedocs.io/en/latest/pages/violations/base.html

You will need to create a new visitor in this file: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/visitors/ast/keywords.py
And a new violation here: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/violations/consistency.py

[kxepal]

Thanks! I'm going to grab some tea for this.

[sobolevn]

@kxepal how's your progress? Should I take this over or not?

[kxepal]

Better don't count on me here. But I'm still watching the changes to catch the wave.

[sobolevn]

pylint actually has this check: https://github.com/PyCQA/pylint/blob/5a86fddcb815ebd08fbc3ffcbaf7b11c2ba2e107/pylint/checkers/refactoring.py#L128

So, it may be easier to implement.

It is also covering the second return rule: https://github.com/PyCQA/pylint/blob/5a86fddcb815ebd08fbc3ffcbaf7b11c2ba2e107/pylint/checkers/refactoring.py#L136

[kxepal]

Wow, never saw that one.

[sobolevn]

@kxepal I guess consistent values rule is also the case with yield.

def function():
      if some:
           yield
     yield None
     yield 1

I see no difference with return in how values should be defined.