False positive inconsistent-return-statements with never returning functions

[hippo91]

Steps to reproduce

Given a file a.py:

import sys
def parser_error(msg):
    sys.exit(1)

def test(s):
    try:
      n = int(s)
      if n < 1:
        raise ValueError()
      return n
    except ValueError:
        parser_error('parser error') # calls system exit

Current behavior

Results of pylint a.py --disable=all --enable=inconsistent-return-statements:

************* Module a
a.py:5: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.09/10 (previous run: 9.09/10, +0.00)

Expected behavior

No message

pylint --version output

Result of pylint --version output:

pylint 2.8.0-dev1
astroid 2.6.0-dev1
Python 3.9.1 (default, Dec 30 2020, 11:18:33) 
[GCC 7.5.0]

See also #3468.

The message is not emitted if return is added in front of parser_error call.

[hippo91]

I don't know what to think about this issue.
First i tried to handle it with such a function:

    def _node_never_returns(self, node:astroid.node_classes.NodeNG) -> bool:
        """
        """
        if isinstance(node, astroid.FunctionDef):
            if self._is_function_def_never_returning(node):
                return True
            if node.body:
                return all(self._node_never_returns(c_node) for c_node in node.body)
            return False
        if isinstance(node, astroid.Expr):
            return self._node_never_returns(node.value)
        if isinstance(node, astroid.Call):
            try:
                funcdef_node = node.func.inferred()[0]
                if self._node_never_returns(funcdef_node):
                    return True
            except astroid.InferenceError:
                pass
        return False

to detect if a function never returns.
For simple function as parser_error in the snippet it does the job. But taking into account more complex function needs
to have control_flow capabilities. pylint does not have it yet.
It could be possible to do as i did for _is_node_return_ended but it is quite painful and will probably rise up the execution time due to the recursive approach.
I wonder if this kind of issue should just be handled through configuration file (never-returning-functions parameter already exists)?
@same-id @Pierre-Sassoulas @cdce8p what do you think about it?

[cdce8p]

Although analyzing the control_flow might work, it will undoubtably introduce complexity that will be hard to maintain in the long term.

I don't really know how common this issue is but I really like the idea from #4122 to use typing.NoReturn.

[same-id]

Hi,

First, I agree with @cdce8p.
typing.NoReturn is the right tool for that and when I'm writing my own code I tend to add this to functions that do not return (throwing an exception / using sys.exit() (which is throwing an exception :-)).

When I created my own _my_parser_error() functions that simply wraps argparse.parse_error() and adds a NoReturn type hint - it still didn't work, and that's when I decided to comment on the issue here, since I assumed pylint should catch this explicit hint no matter what (#4122).

Besides typing.NoReturn, I think that this is a best effort game.

argparse.parse_error() in python37 (maybe even latest, didn't check) is not marked with typing.NoReturn - it should be - but until that happens we can either add it to a "list of known functions" or simply use the simplistic _node_never_returns and if it catches it, just use it - and it will probably catch other things. (maybe worth renaming _node_never_returns with a name that suggests that this is a best effort type of thing and not guaranteed to work in all cases)

So to summarize, I think that the best approach is:

• Mandatory NoReturn type hint check
• Best effort in all other cases

Also, thanks for looking into this.