no-else-return / R1705 instructs users to use error-prone construction

[aes]

Bug description

This triggers warnings no-else-return and inconsistent-return-statements:

    def fuuu(thing):
        """Demo function"""
        if thing == "some":
            result = handle_the_something_case(...)
            return result
        elif thing == "boring":
            happen = "boring"
        elif thing in other:
            happen = set_us_up_the_bomb(...)
            return happen
        else:
            raise TypeError("Unknown case")

and after "fixing" it, this gives no warnings:

    def fuuu(thing):
        """Demo function"""
        if thing == "some":
            result = handle_the_something_case(...)
            return result
        if thing == "boring":
            happen = "boring"
        if thing in other:
            happen = set_us_up_the_bomb(...)
            return happen
    
        raise TypeError("Unknown case")

There are some problems with this. One is that it is no longer obvious that the if-statement should be total. (That is, that it should cover all cases.) Another is the (potentially unexpected) 'other' to 'boring' case fall-through.

Note that pylint explicitly tells users to express their code in the latter, DANGEROUS, way.

Configuration

No response

Command used

pylint demo.py

Pylint output

************* Module demo
demo.py:8:4: R1705: Unnecessary "elif" after "return", remove the leading "el" from "elif" (no-else-return)
demo.py:6:0: R1710: Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)

------------------------------------------------------------------
Your code has been rated at 9.13/10 (previous run: 8.46/10, +0.67)

Expected behavior

I would expect pylint to direct users towards safer ways of expressing their code.

Pylint version

pylint 2.15.6
astroid 2.13.5
Python 3.11.6 (main, Oct  8 2023, 05:06:43) [GCC 13.2.0]

OS / Environment

Debian GNU/Linux trixie/sid

Additional dependencies

No response

[nickdrozd]

Your example code has two elifs. Pylint only suggests getting rid of the first one, which comes after a return statement and can therefore be changed to if. The second elif does not come after a return and cannot be safely changed, and Pylint does not suggest changing it.

As for the inconsistent-return-statements warning, it's not obvious exactly how your example should be modified. As you pointed out, just cutting the else and unconditionally raising the error is bad. You could add in an unconditional return None at the end to make it clear that None will be returned if nothing else is. That's implicitly the case already (None is returned in the "boring" case), but Pylint is suggesting to make that explicit.

Following Pylint's suggestions might lead to this:

def fuuu(thing):
    """Demo function"""
    if thing == "some":
        result = handle_the_something_case()
        return result
    if thing == "boring":  # <-- elif changed to if
        happen = "boring"
    elif thing == "other": # <-- elif unchanged
        happen = set_us_up_the_bomb()
        return happen
    else:
        raise TypeError("Unknown case")

    return None  # <-- explicit return

Maybe the warning could be improved to make it clearer that only particular elif instances are flagged?

[Privat33r-dev]

There is an interesting article regarding this topic from Google. Documentation can be improved by suggestion to ignore the rule in certain cases (core responsibility) to improve readability.

I am not sure if removing this rule would be useful since it can help many starting developers to avoid excessive nesting which is a bigger evil.

[Pierre-Sassoulas]

Thank you for the article @Privat33r-dev

Not all scenarios will be clear-cut for which pattern to use; use your best judgment to choose between these two styles.

For a linter, it means we'll have false positives if we try to enforce google's vision automatically. We're probably better off to keep a consistency check and let those that want to use their best judgement disable the check entirely. Keeping this issue open in order to make the warning clearer as suggested in #9274 (comment)