From 770dd9e70b7235e6268fb4581f1db23ab18c6cd7 Mon Sep 17 00:00:00 2001 From: Claudiu Popa Date: Sat, 21 Nov 2015 17:21:11 +0200 Subject: 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. --- ChangeLog | 4 + pylint/checkers/base.py | 84 ++++++++++++++- .../test/functional/simplifiable_if_statement.py | 120 +++++++++++++++++++++ .../test/functional/simplifiable_if_statement.txt | 4 + 4 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 pylint/test/functional/simplifiable_if_statement.py create mode 100644 pylint/test/functional/simplifiable_if_statement.txt 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' : '', @@ -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 -- cgit v1.2.1