Pylint: False positive inconsistent-return-statements with last ressort "assert False"

[mhooreman]

Steps to reproduce

Lint the following module:

"""qdlkdm"""


def f(x):
    """lkfmlskfmlk"""
    if x != 0:
        return x > 0
    assert False

Current behavior

inconsistent-return-statements(R1710): false_positive.py:4:0: f: Either all return statements in a function should return an expression, or none of them should.

Expected behavior

No issue

Remark

No issue if the assert is before the chain of if ... but it is not doable in every case

pylint --version output

pylint 2.3.1
astroid 2.2.5
Python 3.7.3 (default, Mar 27 2019, 09:40:28) 
[GCC 7.3.0]

[PCManticore]

@mhooreman Thanks for the report. This particular example feels like an artificial use case, that I doubt it's going to be found in the wild. Why not move the assert before the if statement? I doubt we need to increase the complexity of this check just to account for this kind of examples.

[mhooreman]

@PCManticore Thanks for your support. The example I gave is simplified, in order to make it obvious.
The problem happens in real world. Here's my example code:

    def _mustShowDetails(self, severity):  # pylint: disable=R1710
        # R1710: disable=inconsistent-return-statements:
        # https://github.com/PyCQA/pylint/issues/2908
        if self.details == DetailsLevel.none:
            return False
        if self.details == DetailsLevel.everything:
            return True
        if self.details == DetailsLevel.failed:
            return severity.severity > 0
        assert False, "Code should not come here"

According to your suggestion, I should make a if on the various possibilities at the beginning in order to test for unexpected details values, and than make the detailed if again, which will lead to copy-paste and spaghetti code.

Another option would be an Exception, but that's really here about checking assumptions of the code itself, so an assertion is better fitted.

So, in my opinion, this is not an "invalid" case, and I'd suggest to re-open it.

Unless there is a better way to see this, which I have not foreseen?

Thanks a lot and best regards.

[mhooreman]

@PCManticore
I'm sorry, but have you seen my last comment?
Thanks

[hippo91]

@mhooreman thanks again for your remark.

In fact this is not the presence of an assert at the end of the function that leads to emission of inconsistent-return-statement but instead the lack of a last return statement.
For example, the following code doesn't emit the inconsistent-return-statement:

def _mustShowDetails(self, severity):
    # https://github.com/PyCQA/pylint/issues/2908
    if self.details == DetailsLevel.none:
        return False
    if self.details == DetailsLevel.everything:
        return True
    if self.details == DetailsLevel.failed:
        return severity.severity > 0
    assert False, "Code should not come here"
    return None

Even if the last return could never be reached, i think it make sense to add it.

[mhooreman]

@hippo91 thanks a lot. Indeed, my understanding was incorrect. Returning None fixes the R1710.

[prauscher]

Sorry to re-comment, but if you combine pylint with coverage, adding return None as last resort will introduce uncheckable code, leading to lack of coverage. As far as i can see, this could be resolved in different matters:

1. add pylint: disable=inconsistent-return-statement before return None
2. add exception to .coveragerc
3. fixing pylint to accept assert False as last resort
4. replace assert False with raise AssertionError, as this would be detected by pylint

I'm not actually too much into option 3, but curious what would you consider the cleanest way possible.

[PCManticore]

Not sure why you're not raising an exception instead? The assert will also be removed when running with -O flag in which case it's dead code anyway. An exception, eventually a custom one, could safely propagate the intent without having to use assert. I don't think this is an issue that needs to be fixed in pylint.

[mhooreman]

@PCManticore I don't know if the last message addressed to me, but I agreed with you on 18 Jun. Best regards.

[mthuurne]

An assert is a way of documenting "this condition should always be true, and if it's not (because people make mistakes), that is a bug in the code that the assert is part of". With an exception it is not explicit whose fault it is when it is raised. For example when I see ValueError being raised, my first suspect is an invalid argument being passed, not an internal error.

As @prauscher said, there is also the issue of coverage: for assert False it's clear that it is not a problem if there is no test coverage, since it should be unreachable. For a raise statement, in general you do want test coverage. The same would apply to a static code checker looking for dead code: it is obvious that assert False as dead code is fine, but for a raise statement that's not trivial. In fact, if a static code checker would be able to find a way to trigger an assertion, it could report that as a bug, while it couldn't do that with a raise statements.

So in my opinion, replacing assert False with raise makes the code harder to interpret for both human and machine readers.