Adding a check for inconsistent-return-statements inside function or methods. (#1641)

Close #1267

Author: hippo91

ChangeLog

@@ -102,6 +102,9 @@ What's New in Pylint 1.8?
 
       Close #1336
 
+    * Added a new Python check for inconsistent return statements inside method or function.
+      Close #1267
+
     * Fix ``superfluous-parens`` false positive related to handling logical statements
       involving ``in`` operator.
 

doc/whatsnew/1.8.rst

@@ -219,6 +219,34 @@ New checkers
      var = "123"
      logging.log(logging.INFO, "Var: %s", var)
 
+* A new Python checker was added to warn about ``inconsistent-return-statements``. A function or a method
+  has inconsistent return statements if it returns both explicit and implicit values :
+
+  .. code-block:: python
+
+    def mix_implicit_explicit_returns(arg):
+        if arg < 10:
+            return True
+        elif arg < 20:
+            return
+
+  According to PEP8_, if any return statement returns an expression, 
+  any return statements where no value is returned should explicitly state this as return None,
+  and an explicit return statement should be present at the end of the function (if reachable).
+  Thus, the previous function should be written:
+
+  .. code-block:: python
+
+    def mix_implicit_explicit_returns(arg):
+        if arg < 10:
+            return True
+        elif arg < 20:
+            return None
+
+  Close #1267
+
+ .. _PEP8: https://www.python.org/dev/peps/pep-0008
+
 Other Changes
 =============
 

pylint/checkers/classes.py

@@ -298,14 +298,14 @@ def _safe_infer_call_result(node, caller, context=None):
         inferit = node.infer_call_result(caller, context=context)
         value = next(inferit)
     except astroid.InferenceError:
-        return  # inference failed
+        return None  # inference failed
     except StopIteration:
-        return  # no values infered
+        return None  # no values infered
     try:
         next(inferit)
-        return  # there is ambiguity on the inferred node
+        return None  # there is ambiguity on the inferred node
     except astroid.InferenceError:
-        return  # there is some kind of ambiguity
+        return None  # there is some kind of ambiguity
     except StopIteration:
         return value
 

pylint/checkers/format.py

@@ -1064,6 +1064,7 @@ def check_indent_level(self, string, expected, line_num):
             self.add_message('bad-indentation', line=line_num,
                              args=(level * unit_size + len(suppl), i_type,
                                    expected * unit_size))
+        return None
 
 
 def register(linter):

pylint/checkers/imports.py

@@ -97,6 +97,7 @@ def _get_first_import(node, context, name, base, level, alias):
                     break
     if found and not are_exclusive(first, node):
         return first
+    return None
 
 
 def _ignore_import_failure(node, modname, ignored_modules):
@@ -629,7 +630,7 @@ def _check_relative_import(self, modnode, importnode, importedmodnode,
         the imported module name.
         """
         if not self.linter.is_message_enabled('relative-import'):
-            return
+            return None
         if importedmodnode.file is None:
             return False # built-in module
         if modnode is importedmodnode:
@@ -641,6 +642,8 @@ def _check_relative_import(self, modnode, importnode, importedmodnode,
             self.add_message('relative-import',
                              args=(importedasname, importedmodnode.name),
                              node=importnode)
+            return None
+        return None
 
     def _add_imported_module(self, node, importedmodname):
         """notify an imported module, used to analyze dependencies"""

pylint/checkers/python3.py

@@ -36,6 +36,7 @@ def _is_old_octal(literal):
         except ValueError:
             return False
         return True
+    return None
 
 
 def _check_dict_node(node):
@@ -892,6 +893,7 @@ def _check_raise_value(self, node, expr):
             if isinstance(value, str):
                 self.add_message('raising-string', node=node)
                 return True
+        return None
 
 
 class Python3TokenChecker(checkers.BaseTokenChecker):

pylint/checkers/refactoring.py

@@ -27,6 +27,45 @@ 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
+    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
+        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)
+        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:
@@ -111,6 +150,14 @@ class RefactoringChecker(checkers.BaseTokenChecker):
                   'a generator may lead to hard to find bugs. This PEP specify that '
                   'raise StopIteration has to be replaced by a simple return statement',
                   {'minversion': (3, 0)}),
+        'R1710': ('Either all return statements in a function should return an expression, '
+                  'or none of them should.',
+                  'inconsistent-return-statements',
+                  'According to PEP8, if any return statement returns an expression, '
+                  'any return statements where no value is returned should explicitly '
+                  'state this as return None, and an explicit return statement '
+                  'should be present at the end of the function (if reachable)'
+                 ),
     }
     options = (('max-nested-blocks',
                 {'default': 5, 'type': 'int', 'metavar': '<int>',
@@ -122,6 +169,7 @@ class RefactoringChecker(checkers.BaseTokenChecker):
 
     def __init__(self, linter=None):
         checkers.BaseTokenChecker.__init__(self, linter)
+        self._return_nodes = {}
         self._init()
 
     def _init(self):
@@ -309,12 +357,15 @@ def visit_if(self, node):
         self._check_nested_blocks(node)
         self._check_superfluous_else_return(node)
 
-    @utils.check_messages('too-many-nested-blocks')
-    def leave_functiondef(self, _):
+    @utils.check_messages('too-many-nested-blocks', 'inconsistent-return-statements')
+    def leave_functiondef(self, node):
         # check left-over nested blocks stack
         self._emit_nested_blocks_message_if_needed(self._nested_blocks)
         # new scope = reinitialize the stack of nested blocks
         self._nested_blocks = []
+        # check consistent return statements
+        self._check_consistent_returns(node)
+        self._return_nodes[node.name] = []
 
     def visit_raise(self, node):
         self._check_stop_iteration_inside_generator(node)
@@ -491,6 +542,33 @@ def _seq_based_ternary_params(node):
         condition = node.slice.value
         return condition, true_value, false_value
 
+    def visit_functiondef(self, node):
+        self._return_nodes[node.name] = []
+        return_nodes = node.nodes_of_class(astroid.Return)
+        self._return_nodes[node.name] = [_rnode for _rnode in return_nodes
+                                         if _rnode.frame() == node.frame()]
+
+    def _check_consistent_returns(self, node):
+        """Check that all return statements inside a function are consistent.
+
+        Return statements are consistent if:
+            - all returns are explicit and if there is no implicit return;
+            - all returns are empty and if there is, possibly, an implicit return.
+
+        Args:
+            node (astroid.FunctionDef): the function holding the return statements.
+
+        """
+        # explicit return statements are those with a not None value
+        explicit_returns = [_node for _node in self._return_nodes[node.name]
+                            if _node.value is not None]
+        if not explicit_returns:
+            return
+        if (len(explicit_returns) == len(self._return_nodes[node.name])
+                and _is_node_return_ended(node)):
+            return
+        self.add_message('inconsistent-return-statements', node=node)
+
 
 class RecommandationChecker(checkers.BaseChecker):
     __implements__ = (interfaces.IAstroidChecker,)

pylint/checkers/typecheck.py

@@ -1002,15 +1002,15 @@ def visit_extslice(self, node):
     @check_messages('invalid-sequence-index')
     def visit_index(self, node):
         if not node.parent or not hasattr(node.parent, "value"):
-            return
+            return None
         # Look for index operations where the parent is a sequence type.
         # If the types can be determined, only allow indices to be int,
         # slice or instances with __index__.
         parent_type = safe_infer(node.parent.value)
         if not isinstance(parent_type, (astroid.ClassDef, astroid.Instance)):
-            return
+            return None
         if not has_known_bases(parent_type):
-            return
+            return None
 
         # Determine what method on the parent this index will use
         # The parent of this node will be a Subscript, and the parent of that
@@ -1029,20 +1029,20 @@ def visit_index(self, node):
         try:
             methods = dunder_lookup.lookup(parent_type, methodname)
             if methods is astroid.YES:
-                return
+                return None
             itemmethod = methods[0]
         except (exceptions.NotFoundError,
                 exceptions.AttributeInferenceError,
                 IndexError):
-            return
+            return None
         if not isinstance(itemmethod, astroid.FunctionDef):
-            return
+            return None
         if itemmethod.root().name != BUILTINS:
-            return
+            return None
         if not itemmethod.parent:
-            return
+            return None
         if itemmethod.parent.name not in SEQUENCE_TYPES:
-            return
+            return None
 
         # For ExtSlice objects coming from visit_extslice, no further
         # inference is necessary, since if we got this far the ExtSlice
@@ -1052,18 +1052,18 @@ def visit_index(self, node):
         else:
             index_type = safe_infer(node)
         if index_type is None or index_type is astroid.YES:
-            return
+            return None
         # Constants must be of type int
         if isinstance(index_type, astroid.Const):
             if isinstance(index_type.value, int):
-                return
+                return None
         # Instance values must be int, slice, or have an __index__ method
         elif isinstance(index_type, astroid.Instance):
             if index_type.pytype() in (BUILTINS + '.int', BUILTINS + '.slice'):
-                return
+                return None
             try:
                 index_type.getattr('__index__')
-                return
+                return None
             except exceptions.NotFoundError:
                 pass
         elif isinstance(index_type, astroid.Slice):
@@ -1074,6 +1074,7 @@ def visit_index(self, node):
 
         # Anything else is an error
         self.add_message('invalid-sequence-index', node=node)
+        return None
 
     @check_messages('invalid-slice-index')
     def visit_slice(self, node):

pylint/checkers/utils.py

@@ -505,6 +505,7 @@ def _is_property_decorator(decorator):
             for ancestor in infered.ancestors():
                 if ancestor.name == 'property' and ancestor.root().name == BUILTINS_NAME:
                     return True
+    return None
 
 
 def decorated_with(func, qnames):
@@ -610,21 +611,38 @@ def is_from_fallback_block(node):
 
 
 def _except_handlers_ignores_exception(handlers, exception):
-    func = functools.partial(error_of_type,
-                             error_type=(exception, ))
+    func = functools.partial(error_of_type, error_type=(exception, ))
     return any(map(func, handlers))
 
 
-def node_ignores_exception(node, exception):
-    """Check if the node is in a TryExcept which handles the given exception."""
+def get_exception_handlers(node, exception):
+    """Return the collections of handlers handling the exception in arguments.
+
+    Args:
+        node (astroid.Raise): the node raising the exception.
+        exception (builtin.Exception or str): exception or name of the exception.
+
+    Returns:
+        generator: the collection of handlers that are handling the exception or None.
+
+    """
     current = node
     ignores = (astroid.ExceptHandler, astroid.TryExcept)
     while current and not isinstance(current.parent, ignores):
         current = current.parent
 
     if current and isinstance(current.parent, astroid.TryExcept):
-        return _except_handlers_ignores_exception(current.parent.handlers, exception)
-    return False
+        return (_handler for _handler in current.parent.handlers
+                if error_of_type(_handler, exception))
+    return None
+
+
+def node_ignores_exception(node, exception):
+    """Check if the node is in a TryExcept which handles the given exception."""
+    managing_handlers = get_exception_handlers(node, exception)
+    if not managing_handlers:
+        return False
+    return any(managing_handlers)
 
 
 def class_is_abstract(node):
@@ -774,12 +792,12 @@ def safe_infer(node, context=None):
         inferit = node.infer(context=context)
         value = next(inferit)
     except astroid.InferenceError:
-        return
+        return None
     try:
         next(inferit)
-        return # None if there is ambiguity on the inferred node
+        return None # None if there is ambiguity on the inferred node
     except astroid.InferenceError:
-        return # there is some kind of ambiguity
+        return None # there is some kind of ambiguity
     except StopIteration:
         return value
 
@@ -824,9 +842,9 @@ def node_type(node):
                 continue
             types.add(var_type)
             if len(types) > 1:
-                return
+                return None
     except astroid.InferenceError:
-        return
+        return None
     return types.pop() if types else None