Merge pull request #1827 from hippo91/1.8

Backport of PR #1777

6 files changed, 121 insertions, 56 deletions

diff --git a/ChangeLog b/ChangeLog
index 7dfabcbd9..15a989688 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -34,6 +34,11 @@ Release data: |TBA|
 
       Close #1731
 
+    * Fix false positive ``inconsistent-return-statements`` message when never
+      returning functions are used (i.e sys.exit for example).
+
+      Close #1771
+
 What's New in Pylint 1.8.1?
 ===========================
 
diff --git a/doc/whatsnew/1.8.rst b/doc/whatsnew/1.8.rst
index 4ec35b804..e92b9d143 100644
--- a/doc/whatsnew/1.8.rst
+++ b/doc/whatsnew/1.8.rst
@@ -375,3 +375,6 @@ Other Changes
 
 * Fix unused-argument false positives with overshadowed variable in dictionary comprehension.
   (backport from 2.0)
+
+* Fixing false positive ``inconsistent-return-statements`` when
+  never returning functions are used (i.e such as sys.exit). (backport from 2.0)
diff --git a/pylint/checkers/refactoring.py b/pylint/checkers/refactoring.py
index db7877572..4032b6230 100644
--- a/pylint/checkers/refactoring.py
+++ b/pylint/checkers/refactoring.py
@@ -35,53 +35,6 @@ def _all_elements_are_true(gen):
     return values and all(values)
 
 
-def _is_node_return_ended(node):
-    """Check if the node ends with an explicit return statement.
-
-    Args:
-        node (astroid.NodeNG): node to be checked.
-
-    Returns:
-        bool: True if the node ends with an explicit statement, False otherwise.
-
-    """
-    # Recursion base case
-    if isinstance(node, astroid.Return):
-        return True
-    # Avoid the check inside while loop as we don't know
-    # if they will be completed
-    if isinstance(node, astroid.While):
-        return True
-    if isinstance(node, astroid.Raise):
-        # a Raise statement doesn't need to end with a return statement
-        # but if the exception raised is handled, then the handler has to
-        # ends with a return statement
-        if not node.exc:
-            # Ignore bare raises
-            return True
-        exc = utils.safe_infer(node.exc)
-        if exc is None or exc is astroid.Uninferable:
-            return False
-        exc_name = exc.pytype().split('.')[-1]
-        handlers = utils.get_exception_handlers(node, exc_name)
-        handlers = list(handlers) if handlers is not None else []
-        if handlers:
-            # among all the handlers handling the exception at least one
-            # must end with a return statement
-            return any(_is_node_return_ended(_handler) for _handler in handlers)
-        # if no handlers handle the exception then it's ok
-        return True
-    if isinstance(node, astroid.If):
-        # if statement is returning if there are exactly two return statements in its
-        # children : one for the body part, the other for the orelse part
-        return_stmts = [_is_node_return_ended(_child) for _child in node.get_children()]
-        return sum(return_stmts) == 2
-    # recurses on the children of the node except for those which are except handler
-    # because one cannot be sure that the handler will really be used
-    return any(_is_node_return_ended(_child) for _child in node.get_children()
-               if not isinstance(_child, astroid.ExceptHandler))
-
-
 def _if_statement_is_always_returning(if_node):
     def _has_return_node(elems, scope):
         for node in elems:
@@ -179,6 +132,14 @@ class RefactoringChecker(checkers.BaseTokenChecker):
                 {'default': 5, 'type': 'int', 'metavar': '<int>',
                  'help': 'Maximum number of nested blocks for function / '
                          'method body'}
+               ),
+               ('never-returning-functions',
+                {'default': ('optparse.Values', 'sys.exit',),
+                 'type': 'csv',
+                 'help': 'Complete name of functions that never returns. When checking '
+                         'for inconsistent-return-statements if a never returning function is '
+                         'called then it will be considered as an explicit return statement '
+                         'and no message will be printed.'}
                ),)
 
     priority = 0
@@ -187,12 +148,17 @@ class RefactoringChecker(checkers.BaseTokenChecker):
         checkers.BaseTokenChecker.__init__(self, linter)
         self._return_nodes = {}
         self._init()
+        self._never_returning_functions = None
 
     def _init(self):
         self._nested_blocks = []
         self._elifs = []
         self._nested_blocks_msg = None
 
+    def open(self):
+        # do this in open since config not fully initialized in __init__
+        self._never_returning_functions = set(self.config.never_returning_functions)
+
     @decorators.cachedproperty
     def _dummy_rgx(self):
         return lint_utils.get_global_option(
@@ -567,10 +533,77 @@ class RefactoringChecker(checkers.BaseTokenChecker):
         if not explicit_returns:
             return
         if (len(explicit_returns) == len(self._return_nodes[node.name])
-                and _is_node_return_ended(node)):
+                and self._is_node_return_ended(node)):
             return
         self.add_message('inconsistent-return-statements', node=node)
 
+    def _is_node_return_ended(self, node):
+        """Check if the node ends with an explicit return statement.
+
+        Args:
+            node (astroid.NodeNG): node to be checked.
+
+        Returns:
+            bool: True if the node ends with an explicit statement, False otherwise.
+
+        """
+        # Recursion base case
+        if isinstance(node, astroid.Return):
+            return True
+        if isinstance(node, astroid.Call):
+            try:
+                funcdef_node = node.func.infered()[0]
+                if self._is_function_def_never_returning(funcdef_node):
+                    return True
+            except astroid.InferenceError:
+                pass
+        # Avoid the check inside while loop as we don't know
+        # if they will be completed
+        if isinstance(node, astroid.While):
+            return True
+        if isinstance(node, astroid.Raise):
+            # a Raise statement doesn't need to end with a return statement
+            # but if the exception raised is handled, then the handler has to
+            # ends with a return statement
+            if not node.exc:
+                # Ignore bare raises
+                return True
+            exc = utils.safe_infer(node.exc)
+            if exc is None or exc is astroid.Uninferable:
+                return False
+            exc_name = exc.pytype().split('.')[-1]
+            handlers = utils.get_exception_handlers(node, exc_name)
+            handlers = list(handlers) if handlers is not None else []
+            if handlers:
+                # among all the handlers handling the exception at least one
+                # must end with a return statement
+                return any(self._is_node_return_ended(_handler) for _handler in handlers)
+            # if no handlers handle the exception then it's ok
+            return True
+        if isinstance(node, astroid.If):
+            # if statement is returning if there are exactly two return statements in its
+            # children : one for the body part, the other for the orelse part
+            return_stmts = [self._is_node_return_ended(_child) for _child in node.get_children()]
+            return sum(return_stmts) == 2
+        # recurses on the children of the node except for those which are except handler
+        # because one cannot be sure that the handler will really be used
+        return any(self._is_node_return_ended(_child) for _child in node.get_children()
+                   if not isinstance(_child, astroid.ExceptHandler))
+
+    def _is_function_def_never_returning(self, node):
+        """Return True if the function never returns. False otherwise.
+
+        Args:
+            node (astroid.FunctionDef): function definition node to be analyzed.
+
+        Returns:
+            bool: True if the function never returns, False otherwise.
+        """
+        try:
+            return node.qname() in self._never_returning_functions
+        except TypeError:
+            return False
+
 
 class RecommandationChecker(checkers.BaseChecker):
     __implements__ = (interfaces.IAstroidChecker,)
diff --git a/pylint/test/functional/inconsistent_returns.py b/pylint/test/functional/inconsistent_returns.py
index 8532c22de..e8c04f0e9 100644
--- a/pylint/test/functional/inconsistent_returns.py
+++ b/pylint/test/functional/inconsistent_returns.py
@@ -1,6 +1,7 @@
 #pylint: disable=missing-docstring, no-else-return, invalid-name, unused-variable, superfluous-parens
 """Testing inconsistent returns"""
 import math
+import sys
 
 # These ones are consistent
 def explicit_returns(var):
@@ -106,6 +107,20 @@ def bug_1772():
         if counter == 100:
             return 7
 
+def bug_1771(var):
+    if var == 1:
+        sys.exit(1)
+    else:
+        return var * 2
+
+def bug_1771_with_user_config(var):
+    # sys.getdefaultencoding is considered as a never
+    # returning function in the inconsistent_returns.rc file.
+    if var == 1:
+        sys.getdefaultencoding()
+    else:
+        return var * 2
+
 # Next ones are not consistent
 def explicit_implicit_returns(var): # [inconsistent-return-statements]
     if var >= 0:
@@ -178,3 +193,9 @@ def bug_1772_counter_example(): # [inconsistent-return-statements]
             counter += 1
             if counter == 100:
                 return 7
+
+def bug_1771_counter_example(var): # [inconsistent-return-statements]
+    if var == 1:
+        inconsistent_returns_in_nested_function()
+    else:
+        return var * 2
diff --git a/pylint/test/functional/inconsistent_returns.rc b/pylint/test/functional/inconsistent_returns.rc
new file mode 100644
index 000000000..dd6cbb598
--- /dev/null
+++ b/pylint/test/functional/inconsistent_returns.rc
@@ -0,0 +1,2 @@
+[REFACTORING]
+never-returning-functions=sys.exit,sys.getdefaultencoding
diff --git a/pylint/test/functional/inconsistent_returns.txt b/pylint/test/functional/inconsistent_returns.txt
index 893c00b29..6ce725665 100644
--- a/pylint/test/functional/inconsistent_returns.txt
+++ b/pylint/test/functional/inconsistent_returns.txt
@@ -1,8 +1,9 @@
-inconsistent-return-statements:110:explicit_implicit_returns:Either all return statements in a function should return an expression, or none of them should.
-inconsistent-return-statements:114:empty_explicit_returns:Either all return statements in a function should return an expression, or none of them should.
-inconsistent-return-statements:119:explicit_implicit_returns2:Either all return statements in a function should return an expression, or none of them should.
-inconsistent-return-statements:127:explicit_implicit_returns3:Either all return statements in a function should return an expression, or none of them should.
-inconsistent-return-statements:135:returns_missing_in_catched_exceptions:Either all return statements in a function should return an expression, or none of them should.
-inconsistent-return-statements:145:complex_func:Either all return statements in a function should return an expression, or none of them should.
-inconsistent-return-statements:153:inconsistent_returns_in_nested_function.not_consistent_returns_inner:Either all return statements in a function should return an expression, or none of them should.
-inconsistent-return-statements:174:bug_1772_counter_example:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:125:explicit_implicit_returns:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:129:empty_explicit_returns:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:134:explicit_implicit_returns2:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:142:explicit_implicit_returns3:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:150:returns_missing_in_catched_exceptions:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:160:complex_func:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:168:inconsistent_returns_in_nested_function.not_consistent_returns_inner:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:189:bug_1772_counter_example:Either all return statements in a function should return an expression, or none of them should.
+inconsistent-return-statements:197:bug_1771_counter_example:Either all return statements in a function should return an expression, or none of them should.