Added a new refactoring warning, 'simplifiable-if-statement'

This is used when an if statement could be reduced to a boolean evaluation
of its test, as seen in this example:

   if some_cond:
      return True
   else:
      return False

   could be reduced to `return bool(some_cond)`

Closes issue #698.

4 files changed, 211 insertions, 1 deletions

diff --git a/ChangeLog b/ChangeLog
index 63aaa04..e9b67fa 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,10 @@ ChangeLog for Pylint
 
 --
 
+    * Added a new refactoring warning, 'simplifiable-if-statement',
+      used when an if statement could be reduced to a boolean evaluation
+      of its test. Closes issue #698.
+         
     * Added a new refactoring warning, 'too-many-boolean-expressions',
       used when a if statement contains too many boolean expressions,
       which makes the code less maintainable and harder to understand.
diff --git a/pylint/checkers/base.py b/pylint/checkers/base.py
index fe2578e..001a70c 100644
--- a/pylint/checkers/base.py
+++ b/pylint/checkers/base.py
@@ -1468,7 +1468,7 @@ class RecommandationChecker(_BasicChecker):
                       'consider-using-enumerate',
                       'Emitted when code that iterates with range and len is '
                       'encountered. Such code can be simplified by using the '
-                      'enumerate builtin.')
+                      'enumerate builtin.'),
            }
 
     @staticmethod
@@ -1654,6 +1654,10 @@ class ElifChecker(BaseTokenChecker):
                       'Used when a function or a method has too many nested '
                       'blocks. This makes the code less understandable and '
                       'maintainable.'),
+            'R0102': ('The if statement can be reduced by %s',
+                      'simplifiable-if-statement',
+                      'Used when an if statement can be reduced to a boolean '
+                      'conversion of the statement\'s test. '),
            }
     options = (('max-nested-blocks',
                 {'default' : 5, 'type' : 'int', 'metavar' : '<int>',
@@ -1671,6 +1675,83 @@ class ElifChecker(BaseTokenChecker):
         self._if_counter = 0
         self._nested_blocks_msg = None
 
+    @staticmethod
+    def _is_bool_const(node):
+        return (isinstance(node.value, astroid.Const)
+                    and isinstance(node.value.value, bool))
+
+    def _is_actual_elif(self, node):
+        """Check if the given node is an actual elif
+
+        This is a problem we're having with the builtin ast module,
+        which splits `elif` branches into a separate if statement.
+        Unfortunately we need to know the exact type in certain
+        cases.
+        """
+
+        if isinstance(node.parent, astroid.If):
+            orelse = node.parent.orelse
+            # current if node must directly follow a "else"
+            if orelse and orelse == [node]:
+                if self._elifs[self._if_counter]:
+                    return True
+        return False
+
+    def _check_simplifiable_if(self, node):
+        """Check if the given if node can be simplified.
+
+        The if statement can be reduced to a boolean expression
+        in some cases. For instance, if there are two branches
+        and both of them return a boolean value that depends on
+        the result of the statement's test, then this can be reduced
+        to `bool(test)` without losing any functionality.
+        """
+
+        if self._is_actual_elif(node):
+            # Not interested in if statements with multiple branches.
+            return
+        if len(node.orelse) != 1 or len(node.body) != 1:
+            return
+
+        # Check if both branches can be reduced.
+        first_branch = node.body[0]
+        else_branch = node.orelse[0]
+        if isinstance(first_branch, astroid.Return):
+            if not isinstance(else_branch, astroid.Return):
+                return
+            first_branch_is_bool = self._is_bool_const(first_branch)
+            else_branch_is_bool = self._is_bool_const(else_branch)
+            reduced_to = "returning bool of test"
+        elif isinstance(first_branch, astroid.Assign):
+            if not isinstance(else_branch, astroid.Assign):
+                return
+            first_branch_is_bool = self._is_bool_const(first_branch)
+            else_branch_is_bool = self._is_bool_const(else_branch)
+            reduced_to = "assigning bool of test"
+        else:
+            return
+
+        if not first_branch_is_bool or not else_branch_is_bool:
+            return
+        if not first_branch.value.value:
+            # This is a case that can't be easily simplified and
+            # if it can be simplified, it will usually result in a
+            # code that's harder to understand and comprehend.
+            # Let's take for instance `arg and arg <= 3`. This could theoretically be
+            # reduced to `not arg or arg > 3`, but the net result is that now the
+            # condition is harder to understand, because it requires understanding of
+            # an extra clause:
+            #   * first, there is the negation of truthness with `not arg`
+            #   * the second clause is `arg > 3`, which occurs when arg has a
+            #     a truth value, but it implies that `arg > 3` is equivalent
+            #     with `arg and arg > 3`, which means that the user must
+            #     think about this assumption when evaluating `arg > 3`.
+            #     The original form is easier to grasp.
+            return
+
+        self.add_message('simplifiable-if-statement', node=node,
+                         args=(reduced_to, ))
+
     def process_tokens(self, tokens):
         # Process tokens and look for 'if' or 'elif'
         for _, token, _, _, _ in tokens:
@@ -1698,6 +1779,7 @@ class ElifChecker(BaseTokenChecker):
 
     @check_messages('too-many-nested-blocks')
     def visit_if(self, node):
+        self._check_simplifiable_if(node)
         self._check_nested_blocks(node)
         self._if_counter += 1
 
diff --git a/pylint/test/functional/simplifiable_if_statement.py b/pylint/test/functional/simplifiable_if_statement.py
new file mode 100644
index 0000000..48d6dbb
--- /dev/null
+++ b/pylint/test/functional/simplifiable_if_statement.py
@@ -0,0 +1,120 @@
+"""Test that some if statement tests can be simplified."""

+

+# pylint: disable=missing-docstring, invalid-name

+

+

+def test_simplifiable_1(arg):

+    # Simple test that can be replaced by bool(arg)

+    if arg: # [simplifiable-if-statement]

+        return True

+    else:

+        return False

+

+

+def test_simplifiable_2(arg, arg2):

+    # Can be reduced to bool(arg and not arg2)

+    if arg and not arg2: # [simplifiable-if-statement]

+        return True

+    else:

+        return False

+

+

+def test_simplifiable_3(arg, arg2):

+    # Can be reduced to bool(arg and not arg2)

+    if arg and not arg2: # [simplifiable-if-statement]

+        var = True

+    else:

+        var = False

+    return var

+

+

+def test_simplifiable_4(arg):

+    if arg:

+        var = True

+    else:

+        if arg == "arg1": # [simplifiable-if-statement]

+            return True

+        else:

+            return False

+    return var

+

+

+def test_not_necessarily_simplifiable_1(arg, arg2):

+    # Can be reduced to bool(not arg and not arg2) or to

+    # `not all(N)`, which is a bit harder to understand

+    # than `any(N)` when var should be False.

+    if arg or arg2:

+        var = False

+    else:

+        var = True

+    return var

+

+

+def test_not_necessarily_simplifiabile_2(arg):

+    # This could theoretically be reduced to `not arg or arg > 3`

+    # but the net result is that now the condition is harder to understand,

+    # because it requires understanding of an extra clause:

+    #   * first, there is the negation of truthness with `not arg`

+    #   * the second clause is `arg > 3`, which occurs when arg has a

+    #     a truth value, but it implies that `arg > 3` is equivalent

+    #     with `arg and arg > 3`, which means that the user must

+    #     think about this assumption when evaluating `arg > 3`.

+    #     The original form is easier to grasp.

+    if arg and arg <= 3:

+        return False

+    else:

+        return True

+

+

+def test_not_simplifiable_3(arg):

+    if arg:

+        test_not_necessarily_simplifiabile_2(arg)

+        test_not_necessarily_simplifiable_1(arg, arg)

+        return False

+    else:

+        if arg < 3:

+            test_simplifiable_3(arg, 42)

+        return True

+

+

+def test_not_simplifiable_4(arg):

+    # Not interested in multiple elifs

+    if arg == "any":

+        return True

+    elif test_not_simplifiable_3(arg) == arg:

+        return True

+    else:

+        return False

+

+

+def test_not_simplifiable_5(arg):

+    # Different actions in each branch

+    if arg == "any":

+        return True

+    else:

+        var = 42

+    return var

+

+

+def test_not_simplifiable_6(arg):

+    # Different actions in each branch

+    if arg == "any":

+        var = 42

+    else:

+        return True

+    return var

+

+def test_not_simplifiable_7(arg):

+    # Returning something different

+    if arg == "any":

+        return 4

+    else:

+        return 5

+

+

+def test_not_simplifiable_8(arg):

+    # Only one of the branch returns something boolean

+    if arg == "any":

+        return True

+    else:

+        return 0

diff --git a/pylint/test/functional/simplifiable_if_statement.txt b/pylint/test/functional/simplifiable_if_statement.txt
new file mode 100644
index 0000000..74bdd16
--- /dev/null
+++ b/pylint/test/functional/simplifiable_if_statement.txt
@@ -0,0 +1,4 @@
+simplifiable-if-statement:8:test_simplifiable_1:The if statement can be reduced by returning bool of test

+simplifiable-if-statement:16:test_simplifiable_2:The if statement can be reduced by returning bool of test

+simplifiable-if-statement:24:test_simplifiable_3:The if statement can be reduced by assigning bool of test

+simplifiable-if-statement:35:test_simplifiable_4:The if statement can be reduced by returning bool of test
\ No newline at end of file