diff --git a/CHANGELOG.md b/CHANGELOG.md index 40dc28a78..279f59006 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,8 @@ Semantic versioning in our case means: - Adds baseline information for all complexity violation messages: `x > baseline` - Changes how cognitive complexity is calculated - Adds support for positional arguments in different checks +- Forbids to use `continue` and `break` in `finally`. It is a terrible practice, because + `finally` is implicitly called and can cause damage to logic with its implicitness. ### Bugfixes diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 822db3e6e..dea588fc6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,11 +47,15 @@ you will have to do several things: 2. Add docs about the error code to the `pages/usage/violations/index.rst` 3. Add a test that the plugin is working to `tests/test_plugins.py` - ## One magic command Run `make test` to run everything we have! +#### Building on Windows + +- Building directly in Windows does not work. +- Instead, use a Windows Subsystem for Linux (WSL) such as Ubuntu 18.04 LTS that you can get from the Microsoft Store. +- Clone the project to a part of the WSL where Windows does not overwrite permissions, for example _directly to the home of the WSL_ (do `cd` and then `git clone`). That problem looks like [this](https://github.com/wemake-services/wemake-python-styleguide/issues/1007#issuecomment-562719702) and you can read more about why changing the permissons does not work [here](https://github.com/Microsoft/WSL/issues/81). ## Tests diff --git a/docs/pages/usage/setup.rst b/docs/pages/usage/setup.rst index 615fd3dd4..08943f5cf 100644 --- a/docs/pages/usage/setup.rst +++ b/docs/pages/usage/setup.rst @@ -16,7 +16,7 @@ Installation We also recommend to use `poetry `_ instead of a default ``pip``. -You might want to also intsall optional tools +You might want to also install optional tools that pairs nicely with ``wemake-python-styleguide``: - :ref:`flakehell` for easy integration into a **legacy** codebase diff --git a/poetry.lock b/poetry.lock index 6fdfe1f5f..b5cd91e1e 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1758,10 +1758,6 @@ flake8-eradicate = [ {file = "flake8-eradicate-0.2.4.tar.gz", hash = "sha256:b693e9dfe6da42dbc7fb75af8486495b9414d1ab0372d15efcf85a2ac85fd368"}, {file = "flake8_eradicate-0.2.4-py3-none-any.whl", hash = "sha256:b0bcdbb70a489fb799f9ee11fefc57bd0d3251e1ea9bdc5bf454443cccfd620c"}, ] -flake8-executable = [ - {file = "flake8-executable-2.0.3.tar.gz", hash = "sha256:a636ff78b14b63b1245d1c4d509db2f6ea0f2e27a86ee7eb848f3827bef7e16d"}, - {file = "flake8_executable-2.0.3-py3-none-any.whl", hash = "sha256:968618c475a23a538ced9b957a741b818d37610838f99f6abcea249e4de7c9ec"}, -] flake8-isort = [ {file = "flake8-isort-2.8.0.tar.gz", hash = "sha256:64454d1f154a303cfe23ee715aca37271d4f1d299b2f2663f45b73bff14e36a9"}, {file = "flake8_isort-2.8.0-py2.py3-none-any.whl", hash = "sha256:aa0c4d004e6be47e74f122f5b7f36554d0d78ad8bf99b497a460dedccaa7cce9"}, diff --git a/tests/fixtures/noqa/noqa.py b/tests/fixtures/noqa/noqa.py index ad58a7d06..dbcd33d12 100644 --- a/tests/fixtures/noqa/noqa.py +++ b/tests/fixtures/noqa/noqa.py @@ -1,4 +1,4 @@ -#!/usr/bin/env pytho # noqa: WPS452 WPS323 +#!/usr/bin/env pytho # noqa: WPS453 WPS323 # -*- coding: utf-8 -*- """ This file contains all possible violations. @@ -693,3 +693,14 @@ def consecutive_yields(): 'wrong', ] *numbers, = [4, 7] # noqa: WPS356 + +for element in range(10): + try: # noqa: WPS452 + my_print(1) + except AnyError: + my_print('nope') + finally: + # See: + # https://github.com/wemake-services/wemake-python-styleguide/issues/1082 + break + my_print(4) diff --git a/tests/fixtures/noqa/noqa38.py b/tests/fixtures/noqa/noqa38.py index ef0bca8d3..be526304b 100644 --- a/tests/fixtures/noqa/noqa38.py +++ b/tests/fixtures/noqa/noqa38.py @@ -49,3 +49,15 @@ def wrong_comprehension2(): def positional_only(first, /, second): # noqa: WPS451 anti_wps428 = 1 + + +for element in range(10): + try: # noqa: WPS452 + my_print(1) + except AnyError: + my_print('nope') + finally: + # See: + # https://github.com/wemake-services/wemake-python-styleguide/issues/1082 + continue + my_print(4) diff --git a/tests/test_checker/test_noqa.py b/tests/test_checker/test_noqa.py index c5a1d0393..d17499d0f 100644 --- a/tests/test_checker/test_noqa.py +++ b/tests/test_checker/test_noqa.py @@ -47,6 +47,7 @@ 'WPS416': int(not PY38), # only works for `< python3.8` 'WPS451': int(PY38), # only works for `>= python3.8` + 'WPS452': int(PY38), # only works for `>= python3.8` 'WPS602': 2, }) @@ -217,7 +218,8 @@ 'WPS449': 1, 'WPS450': 1, 'WPS451': 0, # defined in version specific table. - 'WPS452': 2, + 'WPS452': 1, # also defined in version specific table. + 'WPS453': 2, 'WPS500': 1, 'WPS501': 1, diff --git a/tests/test_visitors/test_ast/test_exceptions/test_finally_in_loops.py b/tests/test_visitors/test_ast/test_exceptions/test_finally_in_loops.py new file mode 100644 index 000000000..7534ef9aa --- /dev/null +++ b/tests/test_visitors/test_ast/test_exceptions/test_finally_in_loops.py @@ -0,0 +1,174 @@ +# -*- coding: utf-8 -*- + +import pytest + +from wemake_python_styleguide.compat.constants import PY38 +from wemake_python_styleguide.violations.best_practices import ( + LoopControlFinallyViolation, +) +from wemake_python_styleguide.visitors.ast.exceptions import ( + WrongTryExceptVisitor, +) + +right_try_example_with_for = """ +def function(): + for element in range(10): + try: + ... + except: + ... + finally: + ... +""" + +right_try_example_with_while = """ +def function(): + while first_element < second_element: + try: + ... + except: + ... + finally: + ... +""" + +right_example_with_for = """ +def function(): + def some(): + for elem in range(10): + if elem > 5: + {0} +""" + +right_example_with_while = """ +def function(): + def some(): + while a < b: + if a == 5: + {0} +""" + +wrong_try_example = """ +def function(): + for element in range(10): + try: + ... + except: + ... + finally: + {0} +""" + +wrong_try_example_with_while = """ +def function(): + while first_element < second_element: + try: + ... + except: + ... + finally: + {0} +""" + + +@pytest.mark.parametrize('statement', [ + 'break', + 'continue', +]) +@pytest.mark.parametrize('code', [ + right_example_with_for, + right_example_with_while, + right_try_example_with_for, + right_try_example_with_while, +]) +def test_right_finally( + assert_errors, + parse_ast_tree, + code, + statement, + default_options, + mode, +): + """Testing that regular loops and loops with `try` are allowed.""" + tree = parse_ast_tree(mode(code.format(statement))) + + visitor = WrongTryExceptVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, []) + + +@pytest.mark.parametrize('statement', [ + 'break', +]) +@pytest.mark.parametrize('code', [ + wrong_try_example, + wrong_try_example_with_while, +]) +def test_finally_with_break( + assert_errors, + parse_ast_tree, + code, + statement, + default_options, + mode, +): + """Testing that `break` keyword is not allowed in `finally`.""" + tree = parse_ast_tree(mode(code.format(statement))) + + visitor = WrongTryExceptVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [LoopControlFinallyViolation]) + + +@pytest.mark.skipif( + not PY38, + reason='continue in finally works only since python3.8', +) +@pytest.mark.parametrize('statement', [ + 'continue', +]) +@pytest.mark.parametrize('code', [ + wrong_try_example, + wrong_try_example_with_while, +]) +def test_finally_with_continue( + assert_errors, + parse_ast_tree, + code, + statement, + default_options, + mode, +): + """Testing that `continue` keyword is not allowed in `finally`.""" + tree = parse_ast_tree(mode(code.format(statement))) + + visitor = WrongTryExceptVisitor(default_options, tree=tree) + visitor.run() + + assert_errors(visitor, [LoopControlFinallyViolation]) + + +@pytest.mark.skipif( + PY38, + reason='continue in finally does not work till since python3.8', +) +@pytest.mark.parametrize('statement', [ + 'continue', +]) +@pytest.mark.parametrize('code', [ + wrong_try_example, + wrong_try_example_with_while, +]) +def test_finally_with_continue_and_exception( + assert_errors, + parse_ast_tree, + code, + statement, + default_options, + mode, +): + """Testing using `continue` keyword in python < 3.8 raises an exception.""" + with pytest.raises(SyntaxError): + parse_ast_tree(mode(code.format(statement))) diff --git a/tests/test_visitors/test_ast/test_exceptions/test_finally_return_path.py b/tests/test_visitors/test_ast/test_exceptions/test_finally_return_path.py index ab8376377..e58fb6dd7 100644 --- a/tests/test_visitors/test_ast/test_exceptions/test_finally_return_path.py +++ b/tests/test_visitors/test_ast/test_exceptions/test_finally_return_path.py @@ -16,7 +16,7 @@ right_outside1 = """ def function(): # we need function to use ``return`` - for _ in range(10): # we need ``for`` to use ``break`` + for _ in range(10): try: ... except: @@ -363,7 +363,6 @@ def function(): 'return', 'return None', 'return 1', - 'break', 'raise ValueError', 'raise ValueError()', 'raise TypeError(1)', @@ -408,7 +407,6 @@ def test_wrong_return_in_else_or_finally( 'return', 'return None', 'return 1', - 'break', 'raise ValueError', 'raise ValueError()', 'raise TypeError(1)', @@ -455,33 +453,33 @@ def test_correct_return_path_in_try_except( ('raise ValueError(1)', '...', '...', 'return 1', '...'), ('raise ValueError(1)', '...', '...', 'break', '...'), - ('...', '...', '...', 'raise ValueError', 'break'), + ('...', '...', '...', 'raise ValueError', 'return 0'), ('...', '...', '...', 'raise ValueError', 'return None'), ('...', '...', '...', 'break', 'return'), ('...', '...', '...', 'break', 'raise ValueError'), - ('...', '...', '...', 'return', 'break'), + ('...', '...', '...', 'return', 'return 1'), ('...', '...', '...', 'return', 'raise ValueError()'), ('break', '...', '...', '...', 'raise ValueError'), ('break', '...', '...', '...', 'return'), - ('return', '...', '...', '...', 'break'), + ('return', '...', '...', '...', 'raise ValueError(1)'), ('return 0', '...', '...', '...', 'raise ValueError'), ('raise ValueError(1)', '...', '...', '...', 'return 1'), - ('raise ValueError(1)', '...', '...', '...', 'break'), + ('raise ValueError(1)', '...', '...', '...', 'return'), ('...', 'break', '...', '...', 'raise ValueError'), ('...', 'break', '...', '...', 'return'), - ('...', 'return', '...', '...', 'break'), + ('...', 'return', '...', '...', 'raise ValueError(1)'), ('...', 'return 0', '...', '...', 'raise ValueError'), ('...', 'raise ValueError(1)', '...', '...', 'return 1'), - ('...', 'raise ValueError(1)', '...', '...', 'break'), + ('...', 'raise ValueError(1)', '...', '...', 'return 0'), ('...', '...', 'break', '...', 'raise ValueError'), ('...', '...', 'break', '...', 'return'), - ('...', '...', 'return', '...', 'break'), + ('...', '...', 'return', '...', 'raise ValueError'), ('...', '...', 'return 0', '...', 'raise ValueError'), ('...', '...', 'raise ValueError(1)', '...', 'return 1'), - ('...', '...', 'raise ValueError(1)', '...', 'break'), + ('...', '...', 'raise ValueError(1)', '...', 'return 0'), ]) def test_different_nodes_trigger_violation( assert_errors, diff --git a/wemake_python_styleguide/violations/best_practices.py b/wemake_python_styleguide/violations/best_practices.py index a964f7357..fd25cbbb1 100644 --- a/wemake_python_styleguide/violations/best_practices.py +++ b/wemake_python_styleguide/violations/best_practices.py @@ -71,6 +71,7 @@ FloatKeyViolation ProtectedModuleMemberViolation PositionalOnlyArgumentsViolation + LoopControlFinallyViolation ExecutableMismatchViolation Best practices @@ -128,6 +129,7 @@ .. autoclass:: FloatKeyViolation .. autoclass:: ProtectedModuleMemberViolation .. autoclass:: PositionalOnlyArgumentsViolation +.. autoclass:: LoopControlFinallyViolation .. autoclass:: ExecutableMismatchViolation """ @@ -2024,6 +2026,49 @@ def my_function(first, /, second): code = 451 +class LoopControlFinallyViolation(ASTViolation): + """ + Forbids to use ``break`` and ``continue`` in ``finally`` case. + + Related to :class:`~TryExceptMultipleReturnPathViolation`. + + Reasoning: + Putting any control statements in `finally` is a + terrible practice, because `finally` is implicitly + called and can cause damage to your logic with + its implicitness. + We should not allow it. + + Solution: + Remove ``break`` and ``continue`` from ``finally`` blocks. + + Example:: + + # Correct: + try: + ... + finally: + ... + + # Wrong: + try: + ... + finally: + break + + try: + ... + finally: + continue + + .. versionadded:: 0.14.0 + + """ + + error_template = 'Found `break` or `continue` in `finally` block' + code = 452 + + @final class ExecutableMismatchViolation(TokenizeViolation): """ @@ -2050,4 +2095,4 @@ class ExecutableMismatchViolation(TokenizeViolation): """ error_template = 'Found executable mismatch: {0}' - code = 452 + code = 453 diff --git a/wemake_python_styleguide/visitors/ast/exceptions.py b/wemake_python_styleguide/visitors/ast/exceptions.py index a00c35865..c8a597fd6 100644 --- a/wemake_python_styleguide/visitors/ast/exceptions.py +++ b/wemake_python_styleguide/visitors/ast/exceptions.py @@ -15,6 +15,7 @@ BaseExceptionViolation, DuplicateExceptionViolation, IncorrectExceptOrderViolation, + LoopControlFinallyViolation, TryExceptMultipleReturnPathViolation, ) from wemake_python_styleguide.violations.consistency import ( @@ -69,12 +70,14 @@ def visit_Try(self, node: ast.Try) -> None: DuplicateExceptionViolation TryExceptMultipleReturnPathViolation IncorrectExceptOrderViolation + LoopControlFinallyViolation """ self._check_if_needs_except(node) self._check_duplicate_exceptions(node) self._check_return_path(node) self._check_exception_order(node) + self._check_break_or_continue_in_finally(node) self.generic_visit(node) def _check_if_needs_except(self, node: ast.Try) -> None: @@ -123,6 +126,15 @@ def _check_exception_order(self, node: ast.Try) -> None: else: seen.add(exception) + def _check_break_or_continue_in_finally(self, node: ast.Try) -> None: + has_wrong_nodes = any( + is_contained(line, (ast.Break, ast.Continue)) + for line in node.finalbody + ) + + if has_wrong_nodes: + self.add_violation(LoopControlFinallyViolation(node)) + @final class NestedTryBlocksVisitor(BaseNodeVisitor):