diff --git a/tests/fixtures/noqa/noqa.py b/tests/fixtures/noqa/noqa.py index a35d7d0b6..f2c8ee77b 100644 --- a/tests/fixtures/noqa/noqa.py +++ b/tests/fixtures/noqa/noqa.py @@ -531,7 +531,7 @@ def useless_returning_else(): def multiple_return_path(): - try: # noqa: WPS419 + try: # noqa: WPS419, WPS503 return 1 except Exception: return 2 diff --git a/tests/test_checker/test_noqa.py b/tests/test_checker/test_noqa.py index 30601e372..9ef12b3c8 100644 --- a/tests/test_checker/test_noqa.py +++ b/tests/test_checker/test_noqa.py @@ -250,7 +250,7 @@ 'WPS500': 1, 'WPS501': 1, 'WPS502': 2, - 'WPS503': 1, + 'WPS503': 2, 'WPS504': 1, 'WPS505': 1, 'WPS506': 1, diff --git a/tests/test_visitors/test_ast/test_conditions/test_returning_else/test_useful_else.py b/tests/test_visitors/test_ast/test_conditions/test_returning_else/test_useful_else.py deleted file mode 100644 index 3b5caf247..000000000 --- a/tests/test_visitors/test_ast/test_conditions/test_returning_else/test_useful_else.py +++ /dev/null @@ -1,163 +0,0 @@ -import pytest - -from wemake_python_styleguide.visitors.ast.conditions import IfStatementVisitor - -correct_example1 = """ -def function(): - if some_condition: - return None - return None -""" - -correct_example2 = """ -def function(): - if some_condition: - return None -""" - -correct_example3 = """ -def function(): - if some_condition: - return None - elif other_condition: - return None - return None -""" - -correct_example4 = """ -def function(): - if some_condition: - ... - else: - return None -""" - -correct_example4 = """ -if some_condition: - ... -else: - raise ValueError() -""" - -correct_example5 = """ -if some_condition: - if other: - raise TypeError() -else: - raise ValueError() -""" - -correct_example6 = """ -def function(): - if some_condition: - if other: - raise TypeError() - elif other is None: - ... - else: - ... - else: - raise ValueError() -""" - -correct_example7 = """ -def function(): - if some_condition: - with open() as file: - return file - else: - raise ValueError() -""" - -correct_example8 = """ -def function(): - if some_condition: - ... - elif other_condition: - ... - else: - return None -""" - -correct_example9 = """ -def function(): - if some_condition: - return None - elif other_condition: - return None - else: - print(1) - print(2) - if nested: - return 1 -""" - -correct_example10 = """ -def function(): - if some_condition: - return None - elif other_condition: - return None - raise ValueError() -""" - -correct_example11 = """ -def function(): - if some_condition: - return None - elif other_condition: - ... - else: - raise ValueError() -""" - -correct_example12 = """ -def function(): - if some_condition: - ... - elif other_condition: - return None - else: - raise ValueError() -""" - -correct_example13 = """ -def function(): - for test in some: - if some_condition: - continue - elif other_condition: - break - raise ValueError() -""" - - -@pytest.mark.parametrize('code', [ - correct_example1, - correct_example2, - correct_example3, - correct_example4, - correct_example5, - correct_example6, - correct_example7, - correct_example8, - correct_example9, - correct_example10, - correct_example11, - correct_example12, - correct_example13, -]) -def test_else_that_can_not_be_removed( - assert_errors, - parse_ast_tree, - code, - default_options, - mode, -): - """Testing that extra ``else`` blocks cannot be removed.""" - tree = parse_ast_tree(mode(code)) - - visitor = IfStatementVisitor(default_options, tree=tree) - visitor.run() - - assert_errors(visitor, []) diff --git a/tests/test_visitors/test_ast/test_conditions/test_simplifiable_if_statements.py b/tests/test_visitors/test_ast/test_conditions/test_simplifiable_if_statements.py index 0f630ee26..54cd131bb 100644 --- a/tests/test_visitors/test_ast/test_conditions/test_simplifiable_if_statements.py +++ b/tests/test_visitors/test_ast/test_conditions/test_simplifiable_if_statements.py @@ -2,7 +2,6 @@ from wemake_python_styleguide.violations.refactoring import ( SimplifiableReturningIfViolation, - UselessReturningElseViolation, ) from wemake_python_styleguide.visitors.ast.conditions import IfStatementVisitor @@ -15,13 +14,6 @@ def some_function(): return {1} """ -simple_early_returning_if = """ -def some_function(): - if some_condition: - return {0} - return {1} -""" - complex_early_returning_if_inside = """ def some_function(): if some_condition: @@ -54,7 +46,29 @@ def some_function(): return {0} """ +simple_early_returning_if = """ +def some_function(): + if some_condition: + return {0} + return {1} +""" +early_returning_if_else = """ +def some_function(): + if some_condition: + return {0} + else: + return {1} +""" + + +@pytest.mark.parametrize('code', [ + simple_early_returning_if, + early_returning_if_else, + + complex_early_returning_if_inside, + complex_early_returning_if_outside, +]) @pytest.mark.parametrize('comparators', [ ('variable', '"test"'), ('12', 'variable.call()'), @@ -64,11 +78,12 @@ def some_function(): def test_complex_early_returning_if( assert_errors, parse_ast_tree, + code, comparators, default_options, ): """These early returning ifs can not be simplified.""" - tree = parse_ast_tree(simple_early_returning_if.format(*comparators)) + tree = parse_ast_tree(code.format(*comparators)) visitor = IfStatementVisitor( default_options, @@ -103,6 +118,10 @@ def test_complex_early_returning_if_inside( assert_errors(visitor, []) +@pytest.mark.parametrize('code', [ + simple_early_returning_if, + early_returning_if_else, +]) @pytest.mark.parametrize('comparators', [ ('True', 'False'), ('False', 'True'), @@ -111,15 +130,13 @@ def test_simplifiable_early_returning_if( assert_errors, parse_ast_tree, comparators, + code, default_options, ): """These early returning ifs are simplifiable.""" - tree = parse_ast_tree(simple_early_returning_if.format(*comparators)) + tree = parse_ast_tree(code.format(*comparators)) - visitor = IfStatementVisitor( - default_options, - tree=tree, - ) + visitor = IfStatementVisitor(default_options, tree=tree) visitor.run() assert_errors(visitor, [SimplifiableReturningIfViolation]) @@ -141,7 +158,7 @@ def test_complex_else( visitor = IfStatementVisitor(default_options, tree=tree) visitor.run() - assert_errors(visitor, [UselessReturningElseViolation]) + assert_errors(visitor, []) @pytest.mark.parametrize('comparators', [ @@ -160,7 +177,7 @@ def test_not_simplifiable_elif( visitor = IfStatementVisitor(default_options, tree=tree) visitor.run() - assert_errors(visitor, [UselessReturningElseViolation]) + assert_errors(visitor, []) @pytest.mark.parametrize('comparators', [ diff --git a/tests/test_visitors/test_ast/test_conditions/test_returning_else/test_redundant_returning_else.py b/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_if_else.py similarity index 55% rename from tests/test_visitors/test_ast/test_conditions/test_returning_else/test_redundant_returning_else.py rename to tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_if_else.py index 8b633a3d4..6bd6f706f 100644 --- a/tests/test_visitors/test_ast/test_conditions/test_returning_else/test_redundant_returning_else.py +++ b/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_if_else.py @@ -1,10 +1,174 @@ import pytest from wemake_python_styleguide.violations.refactoring import ( - SimplifiableReturningIfViolation, UselessReturningElseViolation, ) -from wemake_python_styleguide.visitors.ast.conditions import IfStatementVisitor +from wemake_python_styleguide.visitors.ast.conditions import UselessElseVisitor + +# Correct: + +correct_example1 = """ +def function(): + if some_condition: + return None + return None +""" + +correct_example2 = """ +def function(): + if some_condition: + return None +""" + +correct_example3 = """ +def function(): + if some_condition: + return None + elif other_condition: + return None + return None +""" + +correct_example4 = """ +def function(): + if some_condition: + ... + else: + return None +""" + +correct_example4 = """ +if some_condition: + ... +else: + raise ValueError() +""" + +correct_example5 = """ +if some_condition: + if other: + raise TypeError() +else: + raise ValueError() +""" + +correct_example6 = """ +def function(): + if some_condition: + if other: + raise TypeError() + elif other is None: + ... + else: + ... + else: + raise ValueError() +""" + +correct_example7 = """ +def function(): + if some_condition: + with open() as file: + return file + else: + raise ValueError() +""" + +correct_example8 = """ +def function(): + if some_condition: + ... + elif other_condition: + ... + else: + return None +""" + +correct_example9 = """ +def function(): + if some_condition: + return None + elif other_condition: + return None + else: + print(1) + print(2) + if nested: + return 1 +""" + +correct_example10 = """ +def function(): + if some_condition: + return None + elif other_condition: + return None + raise ValueError() +""" + +correct_example11 = """ +def function(): + if some_condition: + return None + elif other_condition: + ... + else: + raise ValueError() +""" + +correct_example12 = """ +def function(): + if some_condition: + ... + elif other_condition: + return None + else: + raise ValueError() +""" + +correct_example13 = """ +def function(): + for test in some: + if some_condition: + continue + elif other_condition: + break + raise ValueError() +""" + + +@pytest.mark.parametrize('code', [ + correct_example1, + correct_example2, + correct_example3, + correct_example4, + correct_example5, + correct_example6, + correct_example7, + correct_example8, + correct_example9, + correct_example10, + correct_example11, + correct_example12, + correct_example13, +]) +def test_else_that_can_not_be_removed1( + assert_errors, + parse_ast_tree, + code, + default_options, + mode, +): + """Testing that extra ``else`` blocks cannot be removed.""" + tree = parse_ast_tree(mode(code)) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) + + +# Wrong: function_level_condition = """ def function(): @@ -82,7 +246,7 @@ def test_else_that_can_be_removed_in_function( """Testing that extra ``else`` blocks can be removed.""" tree = parse_ast_tree(mode(template.format(code, code))) - visitor = IfStatementVisitor(default_options, tree=tree) + visitor = UselessElseVisitor(default_options, tree=tree) visitor.run() assert_errors(visitor, [UselessReturningElseViolation]) @@ -107,7 +271,7 @@ def test_else_that_can_be_removed_and_complex_if( """Testing that extra ``else`` blocks can be removed.""" tree = parse_ast_tree(mode(template.format(code, code))) - visitor = IfStatementVisitor(default_options, tree=tree) + visitor = UselessElseVisitor(default_options, tree=tree) visitor.run() assert_errors(visitor, [UselessReturningElseViolation]) @@ -132,13 +296,10 @@ def test_else_can_be_removed_and_simplifiable_if( """Extra ``else`` blocks can be removed, plus the ``if`` is simplifiable.""" tree = parse_ast_tree(mode(template.format(code, code))) - visitor = IfStatementVisitor(default_options, tree=tree) + visitor = UselessElseVisitor(default_options, tree=tree) visitor.run() - assert_errors(visitor, [ - UselessReturningElseViolation, - SimplifiableReturningIfViolation, - ]) + assert_errors(visitor, [UselessReturningElseViolation]) @pytest.mark.parametrize('template', [ @@ -161,7 +322,7 @@ def test_else_that_can_be_removed_in_loop( """Testing that extra ``else`` blocks can be removed.""" tree = parse_ast_tree(mode(template.format(code, code))) - visitor = IfStatementVisitor(default_options, tree=tree) + visitor = UselessElseVisitor(default_options, tree=tree) visitor.run() assert_errors(visitor, [UselessReturningElseViolation]) @@ -183,7 +344,7 @@ def test_else_that_can_be_removed_in_module( """Testing that extra ``else`` blocks can be removed.""" tree = parse_ast_tree(template.format(code, code)) - visitor = IfStatementVisitor(default_options, tree=tree) + visitor = UselessElseVisitor(default_options, tree=tree) visitor.run() assert_errors(visitor, [UselessReturningElseViolation]) @@ -201,7 +362,7 @@ def test_else_that_can_be_removed_in_module( 'print()', 'new_var = 1', ]) -def test_else_that_can_not_be_removed( +def test_else_that_can_not_be_removed2( assert_errors, parse_ast_tree, template, @@ -212,7 +373,7 @@ def test_else_that_can_not_be_removed( """Testing that extra ``else`` blocks cannot be removed.""" tree = parse_ast_tree(mode(template.format(code, 'raise ValueError()'))) - visitor = IfStatementVisitor(default_options, tree=tree) + visitor = UselessElseVisitor(default_options, tree=tree) visitor.run() assert_errors(visitor, []) diff --git a/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_loop_else.py b/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_loop_else.py new file mode 100644 index 000000000..f2c60bbef --- /dev/null +++ b/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_loop_else.py @@ -0,0 +1,215 @@ +import pytest + +from wemake_python_styleguide.violations.refactoring import ( + UselessReturningElseViolation, +) +from wemake_python_styleguide.visitors.ast.conditions import UselessElseVisitor + +# Correct: + +correct_example1 = """ +def wrapper(): + for x in ...: + raise ... +""" + +correct_example2 = """ +while ...: + raise ... +""" + +correct_example3 = """ +def function(): + for x in ...: + ... + raise ... +""" + +correct_example4 = """ +while ...: + ... +raise ... +""" + +correct_example4 = """ +def function(): + for x in ...: + ... + else: + raise ... +""" + +correct_example5 = """ +def function(): + while ...: + ... + else: + raise ... +""" + +correct_example6 = """ +def function(): + for x in ...: + return + else: + ... + return +""" + +correct_example7 = """ +def function(): + while ...: + return + else: + ... + return +""" + +correct_example8 = """ +def function(): + for x in ...: + if ...: + return 2 + return 1 +""" + + +@pytest.mark.parametrize('code', [ + correct_example1, + correct_example2, + correct_example3, + correct_example4, + correct_example5, + correct_example6, + correct_example7, + correct_example8, +]) +def test_else_that_can_not_be_removed( + assert_errors, + parse_ast_tree, + code, + default_options, +): + """Testing that extra ``else`` blocks cannot be removed.""" + tree = parse_ast_tree(code) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) + + +# Templates: + +while_loop = """ +def wrapper(): + while ...: + if ...: # nested + {0} + else: + {1} +""" + +for_loop = """ +def wrapper(): + for x in ...: + ... + {0} + ... + else: + {1} +""" + + +@pytest.mark.parametrize('template', [ + while_loop, + for_loop, +]) +@pytest.mark.parametrize('code1', [ + 'return', + 'raise ValueError()', + 'break', + 'continue', +]) +@pytest.mark.parametrize('code2', [ + 'return', + 'raise ValueError()', +]) +def test_else_that_can_be_removed( + assert_errors, + parse_ast_tree, + code1, + code2, + template, + default_options, + mode, +): + """Testing that extra ``else`` blocks can be removed.""" + tree = parse_ast_tree(mode(template.format(code1, code2))) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [UselessReturningElseViolation]) + + +@pytest.mark.parametrize('template', [ + while_loop, + for_loop, +]) +@pytest.mark.parametrize('code', [ + 'print()', + 'new_var = 1', +]) +@pytest.mark.parametrize('returning', [ + 'return', + 'raise ValueError()', + 'break', + 'continue', +]) +def test_else_that_cannot_be_removed1( + assert_errors, + parse_ast_tree, + code, + returning, + template, + default_options, + mode, +): + """Testing that extra ``else`` blocks can be removed.""" + tree = parse_ast_tree(mode(template.format(returning, code))) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) + + +@pytest.mark.parametrize('template', [ + while_loop, + for_loop, +]) +@pytest.mark.parametrize('code', [ + 'print()', + 'new_var = 1', +]) +@pytest.mark.parametrize('returning', [ + 'return', + 'raise ValueError()', +]) +def test_else_that_cannot_be_removed2( + assert_errors, + parse_ast_tree, + code, + returning, + template, + default_options, + mode, +): + """Testing that extra ``else`` blocks can be removed.""" + tree = parse_ast_tree(mode(template.format(code, returning))) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) diff --git a/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_try_else.py b/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_try_else.py new file mode 100644 index 000000000..539628054 --- /dev/null +++ b/tests/test_visitors/test_ast/test_conditions/test_useless_else/test_useless_try_else.py @@ -0,0 +1,293 @@ +import pytest + +from wemake_python_styleguide.violations.refactoring import ( + UselessReturningElseViolation, +) +from wemake_python_styleguide.visitors.ast.conditions import UselessElseVisitor + +# Correct: + +correct_example1 = """ +def function(): + try: + ... + except ...: + return None + return None +""" + +correct_example2 = """ +def function(): + try: + ... + except ...: + ... + return None +""" + +correct_example3 = """ +def function(): + try: + ... + except ...: + return None + except: + ... + return None +""" + +correct_example4 = """ +def function(): + try: + ... + except ...: + return None + else: + ... +""" + +correct_example4 = """ +def function(): + try: + ... + except ...: + ... + else: + return None +""" + +correct_example5 = """ +def function(): + try: + ... + except ...: + ... + else: + return None + return other +""" + +correct_example6 = """ +def function(): + try: + ... + except ...: + return None + else: + return None + finally: + ... +""" + +correct_example7 = """ +def function(): + try: + ... + except ...: + return None + else: + ... + finally: + ... + return None +""" + +correct_example8 = """ +def function(): + try: + ... + finally: + ... + return None +""" + +correct_example9 = """ +try: + ... +except ...: + raise ... +else: + ... +raise ... +""" + +correct_example10 = """ +try: + ... +except ...: + raise ... +raise ... +""" + + +@pytest.mark.parametrize('code', [ + correct_example1, + correct_example2, + correct_example3, + correct_example4, + correct_example5, + correct_example6, + correct_example7, + correct_example8, + correct_example9, + correct_example10, +]) +def test_else_that_can_not_be_removed1( + assert_errors, + parse_ast_tree, + code, + default_options, +): + """Testing that extra ``else`` blocks cannot be removed.""" + tree = parse_ast_tree(code) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) + + +# Templates: + +try_except_else = """ +def function(): + try: + ... + except: + {0} + else: + {1} +""" + +try_multiple_except_else = """ +def function(): + try: + ... + except ...: + {0} + except ...: + {0} + except: + {0} + else: + {1} +""" + +try_except_else_loop = """ +while ...: + try: + ... + except: + {0} + else: + {1} +""" + +try_except_else_module = """ +try: + ... +except: + {0} +else: + {1} +""" + + +@pytest.mark.parametrize('template', [ + try_except_else, + try_multiple_except_else, +]) +@pytest.mark.parametrize('code', [ + 'return', + 'raise ValueError()', +]) +def test_else_that_can_be_removed_in_function( + assert_errors, + parse_ast_tree, + code, + template, + default_options, +): + """Testing that extra ``else`` blocks can be removed.""" + tree = parse_ast_tree(template.format(code, code)) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [UselessReturningElseViolation]) + + +@pytest.mark.parametrize('template', [ + try_except_else_loop, +]) +@pytest.mark.parametrize('code', [ + 'break', + 'raise ValueError()', + 'continue', +]) +def test_else_that_can_be_removed_in_loop( + assert_errors, + parse_ast_tree, + template, + code, + default_options, +): + """Testing that extra ``else`` blocks can be removed.""" + tree = parse_ast_tree(template.format(code, code)) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [UselessReturningElseViolation]) + + +@pytest.mark.parametrize('template', [ + try_except_else_module, +]) +@pytest.mark.parametrize('code', [ + 'raise ValueError()', +]) +def test_else_that_can_be_removed_in_module( + assert_errors, + parse_ast_tree, + template, + code, + default_options, +): + """Testing that extra ``else`` blocks can be removed.""" + tree = parse_ast_tree(template.format(code, code)) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [UselessReturningElseViolation]) + + +@pytest.mark.parametrize('template', [ + try_except_else, + try_multiple_except_else, + try_except_else_loop, + try_except_else_module, +]) +@pytest.mark.parametrize('code', [ + 'print()', + 'new_var = 1', +]) +@pytest.mark.parametrize('returning', [ + 'raise ValueError()', +]) +def test_else_that_can_not_be_removed2( + assert_errors, + parse_ast_tree, + template, + code, + returning, + default_options, +): + """Testing that extra ``else`` blocks cannot be removed.""" + tree = parse_ast_tree(template.format(code, returning)) + + visitor = UselessElseVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) diff --git a/wemake_python_styleguide/logic/naming/alphabet.py b/wemake_python_styleguide/logic/naming/alphabet.py index 359c8ec9e..b1e5dd8cb 100644 --- a/wemake_python_styleguide/logic/naming/alphabet.py +++ b/wemake_python_styleguide/logic/naming/alphabet.py @@ -100,8 +100,7 @@ def does_contain_unicode(name: str) -> bool: name.encode('ascii') except UnicodeEncodeError: return True - else: - return False + return False def get_unreadable_characters( diff --git a/wemake_python_styleguide/logic/nodes.py b/wemake_python_styleguide/logic/nodes.py index eaba05515..feb10dfbc 100644 --- a/wemake_python_styleguide/logic/nodes.py +++ b/wemake_python_styleguide/logic/nodes.py @@ -15,8 +15,7 @@ def is_literal(node: ast.AST) -> bool: ast.literal_eval(node) except ValueError: return False - else: - return True + return True def get_parent(node: ast.AST) -> Optional[ast.AST]: diff --git a/wemake_python_styleguide/presets/types/tree.py b/wemake_python_styleguide/presets/types/tree.py index d9c4898a6..90262ea94 100644 --- a/wemake_python_styleguide/presets/types/tree.py +++ b/wemake_python_styleguide/presets/types/tree.py @@ -84,6 +84,7 @@ conditions.IfStatementVisitor, conditions.BooleanConditionVisitor, conditions.ImplicitBoolPatternsVisitor, + conditions.UselessElseVisitor, iterables.IterableUnpackingVisitor, diff --git a/wemake_python_styleguide/violations/refactoring.py b/wemake_python_styleguide/violations/refactoring.py index ab4a75c04..811fa9929 100644 --- a/wemake_python_styleguide/violations/refactoring.py +++ b/wemake_python_styleguide/violations/refactoring.py @@ -220,9 +220,10 @@ class UselessReturningElseViolation(ASTViolation): """ Forbid useless ``else`` cases in returning functions. - We check single ``if`` statements that all contain - ``return`` or ``raise`` or ``break`` statements with this rule. - We do not check ``if`` statements with ``elif`` cases. + We check single ``if``, ``for``, ``while``, and ``try`` + statements that all contain + ``return``, ``raise``, ``continue``, or ``break`` + statements with this rule. Reasoning: Using extra ``else`` creates a situation when @@ -250,6 +251,7 @@ def some_function(): .. versionadded:: 0.7.0 .. versionchanged:: 0.11.0 + .. versionchanged:: 0.15.1 """ diff --git a/wemake_python_styleguide/visitors/ast/conditions.py b/wemake_python_styleguide/visitors/ast/conditions.py index e04e646b5..a50d55742 100644 --- a/wemake_python_styleguide/visitors/ast/conditions.py +++ b/wemake_python_styleguide/visitors/ast/conditions.py @@ -5,11 +5,11 @@ from typing_extensions import final -from wemake_python_styleguide.logic import source +from wemake_python_styleguide.logic import source, walk from wemake_python_styleguide.logic.tree import ifs, keywords, operators from wemake_python_styleguide.logic.tree.compares import CompareBounds from wemake_python_styleguide.logic.tree.functions import given_function_called -from wemake_python_styleguide.types import AnyIf, AnyNodes +from wemake_python_styleguide.types import AnyIf, AnyLoop, AnyNodes from wemake_python_styleguide.violations.best_practices import ( SameElementsInConditionViolation, ) @@ -31,6 +31,7 @@ _OperatorPairs = Mapping[Type[ast.boolop], Type[ast.cmpop]] +# TODO: move to logic def _duplicated_isinstance_call(node: ast.BoolOp) -> List[str]: counter: DefaultDict[str, int] = defaultdict(int) @@ -51,7 +52,8 @@ def _duplicated_isinstance_call(node: ast.BoolOp) -> List[str]: ] -def _get_duplicate_names(variables: List[Set[str]]): +# TODO: move to logic +def _get_duplicate_names(variables: List[Set[str]]) -> Set[str]: return reduce( lambda acc, element: acc.intersection(element), variables, @@ -66,35 +68,11 @@ def _get_duplicate_names(variables: List[Set[str]]): class IfStatementVisitor(BaseNodeVisitor): """Checks single and consecutive ``if`` statement nodes.""" - #: Nodes that break or return the execution flow. - _returning_nodes: ClassVar[AnyNodes] = ( - ast.Break, - ast.Raise, - ast.Return, - ast.Continue, - ) - - def __init__(self, *args, **kwargs) -> None: - """We need to store visited ``if`` not to duplicate violations.""" - super().__init__(*args, **kwargs) - self._visited_ifs: Set[ast.If] = set() - def visit_any_if(self, node: ast.If) -> None: - """ - Checks ``if`` nodes and expressions. - - Raises: - UselessReturningElseViolation - NegatedConditionsViolation - MultilineConditionsViolation - UselessLenCompareViolation - SimplifiableReturningIfViolation - - """ + """Checks ``if`` nodes and expressions.""" self._check_negated_conditions(node) self._check_useless_len(node) if isinstance(node, ast.If): - self._check_useless_else(node) self._check_multiline_conditions(node) self._check_simplifiable_returning_if(node) self.generic_visit(node) @@ -119,7 +97,64 @@ def _check_multiline_conditions(self, node: ast.If) -> None: self.add_violation(MultilineConditionsViolation(node)) break - def _check_useless_else(self, node: ast.If) -> None: + def _check_useless_len(self, node: AnyIf) -> None: + if isinstance(node.test, ast.Call): + if given_function_called(node.test, {'len'}): + self.add_violation(UselessLenCompareViolation(node)) + + def _check_simplifiable_returning_if(self, node: ast.If) -> None: + body = node.body + simple_if_and_root = not (ifs.has_elif(node) or ifs.is_elif(node)) + if keywords.is_simple_return(body) and simple_if_and_root: + if ifs.has_else(node): + else_body = node.orelse + if keywords.is_simple_return(else_body): + self.add_violation(SimplifiableReturningIfViolation(node)) + return + body = getattr(node, 'wps_parent').body # TODO: refactor + next_index_in_parent = body.index(node) + 1 + if keywords.next_node_returns_bool(body, next_index_in_parent): + self.add_violation(SimplifiableReturningIfViolation(node)) + + +@final +@alias('visit_any_loop', ( + 'visit_For', + 'visit_AsyncFor', + 'visit_While', +)) +class UselessElseVisitor(BaseNodeVisitor): + """Ensures that ``else`` is used correctly for different nodes.""" + + #: Nodes that break or return the execution flow. + _returning_nodes: ClassVar[AnyNodes] = ( + ast.Break, + ast.Raise, + ast.Return, + ast.Continue, + ) + + def __init__(self, *args, **kwargs) -> None: + """We need to store visited ``if`` not to duplicate violations.""" + super().__init__(*args, **kwargs) + self._visited_ifs: Set[ast.If] = set() + + def visit_If(self, node: ast.If) -> None: + """Checks ``if`` statements.""" + self._check_useless_if_else(node) + self.generic_visit(node) + + def visit_Try(self, node: ast.Try) -> None: + """Checks exception handling.""" + self._check_useless_try_else(node) + self.generic_visit(node) + + def visit_any_loop(self, node: AnyLoop) -> None: + """Checks any loops.""" + self._check_useless_loop_else(node) + self.generic_visit(node) + + def _check_useless_if_else(self, node: ast.If) -> None: real_ifs = [] for chained_if in ifs.chain(node): if isinstance(chained_if, ast.If): @@ -131,10 +166,7 @@ def _check_useless_else(self, node: ast.If) -> None: continue previous_has_returns = all( - ifs.has_nodes( - self._returning_nodes, - real_if.body, - ) + ifs.has_nodes(self._returning_nodes, real_if.body) for real_if in real_ifs ) current_has_returns = ifs.has_nodes( @@ -146,24 +178,39 @@ def _check_useless_else(self, node: ast.If) -> None: UselessReturningElseViolation(chained_if[0]), ) - def _check_useless_len(self, node: AnyIf) -> None: - if isinstance(node.test, ast.Call): - if given_function_called(node.test, {'len'}): - self.add_violation(UselessLenCompareViolation(node)) + def _check_useless_try_else(self, node: ast.Try) -> None: + if not node.orelse or node.finalbody: + # `finally` cancels this rule. + # Because refactoring `try` with `else` and `finally` + # by moving `else` body after `finally` will change + # the execution order. + return - def _check_simplifiable_returning_if(self, node: ast.If) -> None: - body = node.body - simple_if_and_root = not (ifs.has_elif(node) or ifs.is_elif(node)) - if keywords.is_simple_return(body) and simple_if_and_root: - if ifs.has_else(node): - else_body = node.orelse - if keywords.is_simple_return(else_body): - self.add_violation(SimplifiableReturningIfViolation(node)) - return - body = getattr(node, 'wps_parent').body - next_index_in_parent = body.index(node) + 1 - if keywords.next_node_returns_bool(body, next_index_in_parent): - self.add_violation(SimplifiableReturningIfViolation(node)) + all_except_returning = all( + walk.is_contained(except_, self._returning_nodes) + for except_ in node.handlers + ) + else_returning = any( + walk.is_contained(sub, self._returning_nodes) + for sub in node.orelse + ) + if all_except_returning and else_returning: + self.add_violation(UselessReturningElseViolation(node)) + + def _check_useless_loop_else(self, node: AnyLoop) -> None: + if not node.orelse: + return + + body_returning = any( + walk.is_contained(sub, self._returning_nodes) + for sub in node.body + ) + else_returning = any( + walk.is_contained(sub, self._returning_nodes) + for sub in node.orelse + ) + if body_returning and else_returning: + self.add_violation(UselessReturningElseViolation(node)) @final @@ -177,14 +224,7 @@ def __init__(self, *args, **kwargs) -> None: self._isinstance_calls: List[ast.BoolOp] = [] def visit_BoolOp(self, node: ast.BoolOp) -> None: - """ - Checks that ``and`` and ``or`` conditions are correct. - - Raises: - SameElementsInConditionViolation - UnmergedIsinstanceCallsViolation - - """ + """Checks that ``and`` and ``or`` conditions are correct.""" self._check_same_elements(node) self._check_isinstance_calls(node) self.generic_visit(node) @@ -237,14 +277,7 @@ class ImplicitBoolPatternsVisitor(BaseNodeVisitor): } def visit_BoolOp(self, node: ast.BoolOp) -> None: - """ - Checks that ``and`` and ``or`` do not form implicit anti-patterns. - - Raises: - ImplicitComplexCompareViolation - ImplicitInConditionViolation - - """ + """Checks ``and`` and ``or`` don't form implicit anti-patterns.""" self._check_implicit_in(node) self._check_implicit_complex_compare(node) self.generic_visit(node)