From bc9f15fda2e85facb230b3372cddb9ae40b4f3d5 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Mon, 12 Dec 2022 03:23:25 -0500 Subject: [PATCH 1/9] Prevent `used-before-assignment` in pattern matching with a guard (#7922) --- doc/whatsnew/fragments/5327.false_positive | 4 ++++ pylint/checkers/variables.py | 3 +++ tests/functional/u/used/used_before_assignment_py310.py | 7 +++++++ tests/functional/u/used/used_before_assignment_py310.rc | 2 ++ 4 files changed, 16 insertions(+) create mode 100644 doc/whatsnew/fragments/5327.false_positive create mode 100644 tests/functional/u/used/used_before_assignment_py310.py create mode 100644 tests/functional/u/used/used_before_assignment_py310.rc diff --git a/doc/whatsnew/fragments/5327.false_positive b/doc/whatsnew/fragments/5327.false_positive new file mode 100644 index 0000000000..0cac649f80 --- /dev/null +++ b/doc/whatsnew/fragments/5327.false_positive @@ -0,0 +1,4 @@ +Fix false-positive for ``used-before-assignment`` in pattern matching +with a guard. + +Closes #5327 diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index 58a2216c0b..602dd54141 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -2139,6 +2139,7 @@ def _is_variable_violation( nodes.AugAssign, nodes.Expr, nodes.Return, + nodes.Match, ), ) and VariablesChecker._maybe_used_and_assigned_at_once(defstmt) @@ -2239,6 +2240,8 @@ def _maybe_used_and_assigned_at_once(defstmt: nodes.Statement) -> bool: """Check if `defstmt` has the potential to use and assign a name in the same statement. """ + if isinstance(defstmt, nodes.Match): + return any(case.guard for case in defstmt.cases) if isinstance(defstmt.value, nodes.BaseContainer) and defstmt.value.elts: # The assignment must happen as part of the first element # e.g. "assert (x:= True), x" diff --git a/tests/functional/u/used/used_before_assignment_py310.py b/tests/functional/u/used/used_before_assignment_py310.py new file mode 100644 index 0000000000..14f46b61e9 --- /dev/null +++ b/tests/functional/u/used/used_before_assignment_py310.py @@ -0,0 +1,7 @@ +"""Tests for used-before-assignment with python 3.10's pattern matching""" + +match ("example", "one"): + case (x, y) if x == "example": + print("x used to cause used-before-assignment!") + case _: + print("good thing it doesn't now!") diff --git a/tests/functional/u/used/used_before_assignment_py310.rc b/tests/functional/u/used/used_before_assignment_py310.rc new file mode 100644 index 0000000000..68a8c8ef15 --- /dev/null +++ b/tests/functional/u/used/used_before_assignment_py310.rc @@ -0,0 +1,2 @@ +[testoptions] +min_pyver=3.10 From e1c350db02980e356b6acc23dff4e71a6a1eb6aa Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 Dec 2022 13:27:06 +0000 Subject: [PATCH 2/9] Bump actions/setup-python from 4.3.0 to 4.3.1 (#7924) Bumps [actions/setup-python](https://github.com/actions/setup-python) from 4.3.0 to 4.3.1. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](https://github.com/actions/setup-python/compare/v4.3.0...v4.3.1) --- updated-dependencies: - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/changelog.yml | 2 +- .github/workflows/checks.yaml | 8 ++++---- .github/workflows/primer-test.yaml | 6 +++--- .github/workflows/primer_comment.yaml | 2 +- .github/workflows/primer_run_main.yaml | 2 +- .github/workflows/primer_run_pr.yaml | 2 +- .github/workflows/release.yml | 2 +- .github/workflows/tests.yaml | 12 ++++++------ 8 files changed, 18 insertions(+), 18 deletions(-) diff --git a/.github/workflows/changelog.yml b/.github/workflows/changelog.yml index aa59fdec52..a35925c079 100644 --- a/.github/workflows/changelog.yml +++ b/.github/workflows/changelog.yml @@ -28,7 +28,7 @@ jobs: fetch-depth: 0 - name: Set up Python ${{ env.DEFAULT_PYTHON }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ env.DEFAULT_PYTHON }} check-latest: true diff --git a/.github/workflows/checks.yaml b/.github/workflows/checks.yaml index 30dd1b1e3b..a469742ff1 100644 --- a/.github/workflows/checks.yaml +++ b/.github/workflows/checks.yaml @@ -36,7 +36,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ env.DEFAULT_PYTHON }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ env.DEFAULT_PYTHON }} check-latest: true @@ -90,7 +90,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ env.DEFAULT_PYTHON }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ env.DEFAULT_PYTHON }} check-latest: true @@ -139,7 +139,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ env.DEFAULT_PYTHON }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ env.DEFAULT_PYTHON }} check-latest: true @@ -171,7 +171,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ env.DEFAULT_PYTHON }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ env.DEFAULT_PYTHON }} check-latest: true diff --git a/.github/workflows/primer-test.yaml b/.github/workflows/primer-test.yaml index e85ac39fb5..df4fc1b798 100644 --- a/.github/workflows/primer-test.yaml +++ b/.github/workflows/primer-test.yaml @@ -38,7 +38,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true @@ -77,7 +77,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true @@ -113,7 +113,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true diff --git a/.github/workflows/primer_comment.yaml b/.github/workflows/primer_comment.yaml index 3f901278fb..338e5c228e 100644 --- a/.github/workflows/primer_comment.yaml +++ b/.github/workflows/primer_comment.yaml @@ -37,7 +37,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ env.DEFAULT_PYTHON }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ env.DEFAULT_PYTHON }} check-latest: true diff --git a/.github/workflows/primer_run_main.yaml b/.github/workflows/primer_run_main.yaml index e8e8f983c1..a86e2a2e39 100644 --- a/.github/workflows/primer_run_main.yaml +++ b/.github/workflows/primer_run_main.yaml @@ -35,7 +35,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true diff --git a/.github/workflows/primer_run_pr.yaml b/.github/workflows/primer_run_pr.yaml index b8f0815dd6..afc45cf043 100644 --- a/.github/workflows/primer_run_pr.yaml +++ b/.github/workflows/primer_run_pr.yaml @@ -46,7 +46,7 @@ jobs: fetch-depth: 0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6dbe0562f5..76c761041d 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -23,7 +23,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ env.DEFAULT_PYTHON }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ env.DEFAULT_PYTHON }} check-latest: true diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 6652130b9b..9a259e6413 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -35,7 +35,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true @@ -91,7 +91,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true @@ -136,7 +136,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true @@ -194,7 +194,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true @@ -240,7 +240,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true @@ -284,7 +284,7 @@ jobs: uses: actions/checkout@v3.1.0 - name: Set up Python ${{ matrix.python-version }} id: python - uses: actions/setup-python@v4.3.0 + uses: actions/setup-python@v4.3.1 with: python-version: ${{ matrix.python-version }} check-latest: true From 56f1357119baefafc347a5c52424a2dcbbbdace2 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 Dec 2022 13:27:45 +0000 Subject: [PATCH 3/9] Bump furo from 2022.9.29 to 2022.12.7 (#7926) Bumps [furo](https://github.com/pradyunsg/furo) from 2022.9.29 to 2022.12.7. - [Release notes](https://github.com/pradyunsg/furo/releases) - [Changelog](https://github.com/pradyunsg/furo/blob/main/docs/changelog.md) - [Commits](https://github.com/pradyunsg/furo/compare/2022.09.29...2022.12.07) --- updated-dependencies: - dependency-name: furo dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- doc/requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/requirements.txt b/doc/requirements.txt index e9c849eb7a..1c3a146f28 100644 --- a/doc/requirements.txt +++ b/doc/requirements.txt @@ -2,5 +2,5 @@ Sphinx==5.3.0 sphinx-reredirects<1 myst-parser~=0.18 towncrier~=22.8 -furo==2022.9.29 +furo==2022.12.7 -e . From b94fdb2944e5b94c33dc67eb385abbdf1f2e7b3f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 Dec 2022 13:58:09 +0000 Subject: [PATCH 4/9] Bump flake8-bugbear from 22.10.27 to 22.12.6 (#7927) * Bump flake8-bugbear from 22.10.27 to 22.12.6 Bumps [flake8-bugbear](https://github.com/PyCQA/flake8-bugbear) from 22.10.27 to 22.12.6. - [Release notes](https://github.com/PyCQA/flake8-bugbear/releases) - [Commits](https://github.com/PyCQA/flake8-bugbear/compare/22.10.27...22.12.6) --- updated-dependencies: - dependency-name: flake8-bugbear dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pierre Sassoulas --- .pre-commit-config.yaml | 2 +- requirements_test_pre_commit.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 022cfe4bec..24c0dd7534 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -58,7 +58,7 @@ repos: hooks: - id: flake8 additional_dependencies: - [flake8-bugbear==22.10.27, flake8-typing-imports==1.14.0] + [flake8-bugbear==22.12.6, flake8-typing-imports==1.14.0] exclude: *fixtures - repo: local hooks: diff --git a/requirements_test_pre_commit.txt b/requirements_test_pre_commit.txt index c5d1fcb921..ccc4241a9d 100644 --- a/requirements_test_pre_commit.txt +++ b/requirements_test_pre_commit.txt @@ -3,7 +3,7 @@ bandit==1.7.4 black==22.10.0 flake8==6.0.0 -flake8-bugbear==22.10.27 +flake8-bugbear==22.12.6 flake8-typing-imports==1.14.0 isort==5.10.1 mypy==0.991 From ca5b4aff168ff65fc9b9896265ecf0e58404427f Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Mon, 12 Dec 2022 09:02:07 -0500 Subject: [PATCH 5/9] Fix unreleased false positives with `used-before-assignment` (#7921) Cases with NamedExpr and Starred relating to definitions under "always false" conditions added in https://github.com/PyCQA/pylint/pull/6677 --- pylint/checkers/variables.py | 17 ++++++----------- .../u/undefined/undefined_variable_py38.py | 4 ++++ .../functional/u/used/used_before_assignment.py | 10 ++++++++++ 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index 602dd54141..29911c4717 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -900,21 +900,16 @@ def _defines_name_raises_or_returns(name: str, node: nodes.NodeNG) -> bool: if isinstance(node, nodes.Assign): for target in node.targets: for elt in utils.get_all_elements(target): + if isinstance(elt, nodes.Starred): + elt = elt.value if isinstance(elt, nodes.AssignName) and elt.name == name: return True if isinstance(node, nodes.If): - # Check for assignments inside the test - if isinstance(node.test, nodes.NamedExpr) and node.test.target.name == name: + if any( + child_named_expr.target.name == name + for child_named_expr in node.nodes_of_class(nodes.NamedExpr) + ): return True - if isinstance(node.test, nodes.Call): - for arg_or_kwarg in node.test.args + [ - kw.value for kw in node.test.keywords - ]: - if ( - isinstance(arg_or_kwarg, nodes.NamedExpr) - and arg_or_kwarg.target.name == name - ): - return True return False @staticmethod diff --git a/tests/functional/u/undefined/undefined_variable_py38.py b/tests/functional/u/undefined/undefined_variable_py38.py index 8afb5eaf9e..2612e535f9 100644 --- a/tests/functional/u/undefined/undefined_variable_py38.py +++ b/tests/functional/u/undefined/undefined_variable_py38.py @@ -186,3 +186,7 @@ def expression_in_ternary_operator_inside_container_wrong_position(): NEVER_DEFINED = 1 print(defined) print(NEVER_DEFINED) # [used-before-assignment] + +if (still_defined := False) == 1: + NEVER_DEFINED_EITHER = 1 +print(still_defined) diff --git a/tests/functional/u/used/used_before_assignment.py b/tests/functional/u/used/used_before_assignment.py index f8ed651b51..d36b2fd8d5 100644 --- a/tests/functional/u/used/used_before_assignment.py +++ b/tests/functional/u/used/used_before_assignment.py @@ -106,3 +106,13 @@ def redefine_time_import_with_global(): if VAR11: VAR12 = False print(VAR12) + +def turn_on2(**kwargs): + """https://github.com/PyCQA/pylint/issues/7873""" + if "brightness" in kwargs: + brightness = kwargs["brightness"] + var, *args = (1, "set_dimmer_state", brightness) + else: + var, *args = (1, "restore_dimmer_state") + + print(var, *args) From b9b77c2f3f550a50bb8fec4b842ed3ff648966b4 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 Dec 2022 14:05:08 +0000 Subject: [PATCH 6/9] Bump black from 22.10.0 to 22.12.0 (#7925) * Bump black from 22.10.0 to 22.12.0 Bumps [black](https://github.com/psf/black) from 22.10.0 to 22.12.0. - [Release notes](https://github.com/psf/black/releases) - [Changelog](https://github.com/psf/black/blob/main/CHANGES.md) - [Commits](https://github.com/psf/black/compare/22.10.0...22.12.0) --- updated-dependencies: - dependency-name: black dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pierre Sassoulas --- .pre-commit-config.yaml | 2 +- requirements_test_pre_commit.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 24c0dd7534..7f927b96dc 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -44,7 +44,7 @@ repos: - id: isort exclude: doc/data/messages/(r/reimported|w/wrong-import-order|u/ungrouped-imports|m/misplaced-future|m/multiple-imports)/bad.py - repo: https://github.com/psf/black - rev: 22.10.0 + rev: 22.12.0 hooks: - id: black args: [--safe, --quiet] diff --git a/requirements_test_pre_commit.txt b/requirements_test_pre_commit.txt index ccc4241a9d..a84a5f50ab 100644 --- a/requirements_test_pre_commit.txt +++ b/requirements_test_pre_commit.txt @@ -1,7 +1,7 @@ # Everything in this file should reflect the pre-commit configuration # in .pre-commit-config.yaml bandit==1.7.4 -black==22.10.0 +black==22.12.0 flake8==6.0.0 flake8-bugbear==22.12.6 flake8-typing-imports==1.14.0 From 1b3922b3d34dbd526e1a182bfb705aedda6ea3d1 Mon Sep 17 00:00:00 2001 From: Dani Alcala <112832187+clavedeluna@users.noreply.github.com> Date: Mon, 12 Dec 2022 15:00:30 -0300 Subject: [PATCH 7/9] Fix ``no-else-return false Negative for try/except/else pattern (#7888) Co-authored-by: Pierre Sassoulas --- doc/whatsnew/fragments/7788.false_negative | 3 + .../refactoring/refactoring_checker.py | 43 ++++++++-- pylint/lint/pylinter.py | 4 +- tests/functional/n/no/no_else_break.txt | 14 ++-- tests/functional/n/no/no_else_continue.txt | 14 ++-- tests/functional/n/no/no_else_raise.txt | 14 ++-- tests/functional/n/no/no_else_return.py | 84 +++++++++++++++++++ tests/functional/n/no/no_else_return.txt | 21 +++-- tests/functional/r/raise_missing_from.py | 2 +- 9 files changed, 161 insertions(+), 38 deletions(-) create mode 100644 doc/whatsnew/fragments/7788.false_negative diff --git a/doc/whatsnew/fragments/7788.false_negative b/doc/whatsnew/fragments/7788.false_negative new file mode 100644 index 0000000000..f04311a6bc --- /dev/null +++ b/doc/whatsnew/fragments/7788.false_negative @@ -0,0 +1,3 @@ +``no-else-return`` or ``no-else-raise`` will be emitted if ``except`` block always returns or raises. + +Closes #7788 diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 6a79ce55a7..3612385921 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -73,6 +73,16 @@ def _if_statement_is_always_returning( return any(isinstance(node, returning_node_class) for node in if_node.body) +def _except_statement_is_always_returning( + node: nodes.TryExcept, returning_node_class: nodes.NodeNG +) -> bool: + """Detect if all except statements return.""" + return all( + any(isinstance(child, returning_node_class) for child in handler.body) + for handler in node.handlers + ) + + def _is_trailing_comma(tokens: list[tokenize.TokenInfo], index: int) -> bool: """Check if the given token is a trailing comma. @@ -531,7 +541,7 @@ def _is_bool_const(node: nodes.Return | nodes.Assign) -> bool: node.value.value, bool ) - def _is_actual_elif(self, node: nodes.If) -> bool: + def _is_actual_elif(self, node: nodes.If | nodes.TryExcept) -> bool: """Check if the given node is an actual elif. This is a problem we're having with the builtin ast module, @@ -642,10 +652,14 @@ def leave_module(self, _: nodes.Module) -> None: ) self._init() - @utils.only_required_for_messages("too-many-nested-blocks") - def visit_tryexcept(self, node: nodes.TryExcept) -> None: + @utils.only_required_for_messages("too-many-nested-blocks", "no-else-return") + def visit_tryexcept(self, node: nodes.TryExcept | nodes.TryFinally) -> None: self._check_nested_blocks(node) + if isinstance(node, nodes.TryExcept): + self._check_superfluous_else_return(node) + self._check_superfluous_else_raise(node) + visit_tryfinally = visit_tryexcept visit_while = visit_tryexcept @@ -707,23 +721,38 @@ def visit_with(self, node: nodes.With) -> None: self._check_redefined_argument_from_local(name) def _check_superfluous_else( - self, node: nodes.If, msg_id: str, returning_node_class: nodes.NodeNG + self, + node: nodes.If | nodes.TryExcept, + msg_id: str, + returning_node_class: nodes.NodeNG, ) -> None: + if isinstance(node, nodes.TryExcept) and isinstance( + node.parent, nodes.TryFinally + ): + # Not interested in try/except/else/finally statements. + return + if not node.orelse: - # Not interested in if statements without else. + # Not interested in if/try statements without else. return if self._is_actual_elif(node): # Not interested in elif nodes; only if return - if _if_statement_is_always_returning(node, returning_node_class): + if ( + isinstance(node, nodes.If) + and _if_statement_is_always_returning(node, returning_node_class) + ) or ( + isinstance(node, nodes.TryExcept) + and _except_statement_is_always_returning(node, returning_node_class) + ): orelse = node.orelse[0] if (orelse.lineno, orelse.col_offset) in self._elifs: args = ("elif", 'remove the leading "el" from "elif"') else: args = ("else", 'remove the "else" and de-indent the code inside it') - self.add_message(msg_id, node=node, args=args) + self.add_message(msg_id, node=node, args=args, confidence=HIGH) def _check_superfluous_else_return(self, node: nodes.If) -> None: return self._check_superfluous_else( diff --git a/pylint/lint/pylinter.py b/pylint/lint/pylinter.py index 70f27c7c02..2f96a9fda1 100644 --- a/pylint/lint/pylinter.py +++ b/pylint/lint/pylinter.py @@ -454,8 +454,8 @@ def _load_reporter_by_name(self, reporter_name: str) -> reporters.BaseReporter: reporter_class = _load_reporter_by_class(reporter_name) except (ImportError, AttributeError, AssertionError) as e: raise exceptions.InvalidReporterError(name) from e - else: - return reporter_class() + + return reporter_class() def set_reporter( self, reporter: reporters.BaseReporter | reporters.MultiReporter diff --git a/tests/functional/n/no/no_else_break.txt b/tests/functional/n/no/no_else_break.txt index cf045f367b..1e5bee1eb2 100644 --- a/tests/functional/n/no/no_else_break.txt +++ b/tests/functional/n/no/no_else_break.txt @@ -1,7 +1,7 @@ -no-else-break:8:8:11:17:foo1:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:16:8:21:17:foo2:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-break:28:12:33:21:foo3:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:41:8:48:17:foo4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:54:8:63:17:foo5:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-break:70:12:74:21:foo6:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-break:110:8:116:21:bar4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-break:8:8:11:17:foo1:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:16:8:21:17:foo2:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":HIGH +no-else-break:28:12:33:21:foo3:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:41:8:48:17:foo4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:54:8:63:17:foo5:"Unnecessary ""elif"" after ""break"", remove the leading ""el"" from ""elif""":HIGH +no-else-break:70:12:74:21:foo6:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-break:110:8:116:21:bar4:"Unnecessary ""else"" after ""break"", remove the ""else"" and de-indent the code inside it":HIGH diff --git a/tests/functional/n/no/no_else_continue.txt b/tests/functional/n/no/no_else_continue.txt index f0e813e405..7b0dde4e9b 100644 --- a/tests/functional/n/no/no_else_continue.txt +++ b/tests/functional/n/no/no_else_continue.txt @@ -1,7 +1,7 @@ -no-else-continue:8:8:11:17:foo1:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:16:8:21:17:foo2:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-continue:28:12:33:24:foo3:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:41:8:48:17:foo4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:54:8:63:17:foo5:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-continue:70:12:74:21:foo6:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-continue:110:8:116:24:bar4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-continue:8:8:11:17:foo1:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:16:8:21:17:foo2:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":HIGH +no-else-continue:28:12:33:24:foo3:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:41:8:48:17:foo4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:54:8:63:17:foo5:"Unnecessary ""elif"" after ""continue"", remove the leading ""el"" from ""elif""":HIGH +no-else-continue:70:12:74:21:foo6:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-continue:110:8:116:24:bar4:"Unnecessary ""else"" after ""continue"", remove the ""else"" and de-indent the code inside it":HIGH diff --git a/tests/functional/n/no/no_else_raise.txt b/tests/functional/n/no/no_else_raise.txt index 72f8f83766..9e22f9a43b 100644 --- a/tests/functional/n/no/no_else_raise.txt +++ b/tests/functional/n/no/no_else_raise.txt @@ -1,7 +1,7 @@ -no-else-raise:6:4:11:26:foo1:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:15:4:23:26:foo2:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-raise:29:8:34:30:foo3:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:41:4:48:13:foo4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:53:4:62:13:foo5:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-raise:68:8:72:17:foo6:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-raise:104:4:110:33:bar4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-raise:6:4:11:26:foo1:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:15:4:23:26:foo2:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":HIGH +no-else-raise:29:8:34:30:foo3:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:41:4:48:13:foo4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:53:4:62:13:foo5:"Unnecessary ""elif"" after ""raise"", remove the leading ""el"" from ""elif""":HIGH +no-else-raise:68:8:72:17:foo6:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:104:4:110:33:bar4:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH diff --git a/tests/functional/n/no/no_else_return.py b/tests/functional/n/no/no_else_return.py index 0a87551faa..eb4b05d87b 100644 --- a/tests/functional/n/no/no_else_return.py +++ b/tests/functional/n/no/no_else_return.py @@ -108,3 +108,87 @@ def bar4(x): return False except ValueError: return None + +# pylint: disable = bare-except +def try_one_except() -> bool: + try: # [no-else-return] + print('asdf') + except: + print("bad") + return False + else: + return True + + +def try_multiple_except() -> bool: + try: # [no-else-return] + print('asdf') + except TypeError: + print("bad") + return False + except ValueError: + print("bad second") + return False + else: + return True + +def try_not_all_except_return() -> bool: # [inconsistent-return-statements] + try: + print('asdf') + except TypeError: + print("bad") + return False + except ValueError: + val = "something" + else: + return True + +# pylint: disable = raise-missing-from +def try_except_raises() -> bool: + try: # [no-else-raise] + print('asdf') + except: + raise ValueError + else: + return True + +def try_except_raises2() -> bool: + try: # [no-else-raise] + print('asdf') + except TypeError: + raise ValueError + except ValueError: + raise TypeError + else: + return True + +def test() -> bool: # [inconsistent-return-statements] + try: + print('asdf') + except RuntimeError: + return False + finally: + print("in finally") + + +def try_finally_return() -> bool: # [inconsistent-return-statements] + try: + print('asdf') + except RuntimeError: + return False + else: + print("inside else") + finally: + print("in finally") + +def try_finally_raise(): + current_tags = {} + try: + yield current_tags + except Exception: + current_tags["result"] = "failure" + raise + else: + current_tags["result"] = "success" + finally: + print("inside finally") diff --git a/tests/functional/n/no/no_else_return.txt b/tests/functional/n/no/no_else_return.txt index c73b177da5..a2c64f73c3 100644 --- a/tests/functional/n/no/no_else_return.txt +++ b/tests/functional/n/no/no_else_return.txt @@ -1,7 +1,14 @@ -no-else-return:6:4:11:16:foo1:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:15:4:23:16:foo2:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-return:29:8:34:20:foo3:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:41:4:48:13:foo4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:53:4:62:13:foo5:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":UNDEFINED -no-else-return:68:8:72:17:foo6:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED -no-else-return:104:4:110:23:bar4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":UNDEFINED +no-else-return:6:4:11:16:foo1:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:15:4:23:16:foo2:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":HIGH +no-else-return:29:8:34:20:foo3:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:41:4:48:13:foo4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:53:4:62:13:foo5:"Unnecessary ""elif"" after ""return"", remove the leading ""el"" from ""elif""":HIGH +no-else-return:68:8:72:17:foo6:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:104:4:110:23:bar4:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:114:4:120:19:try_one_except:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-return:124:4:133:19:try_multiple_except:"Unnecessary ""else"" after ""return"", remove the ""else"" and de-indent the code inside it":HIGH +inconsistent-return-statements:135:0:135:29:try_not_all_except_return:Either all return statements in a function should return an expression, or none of them should.:UNDEFINED +no-else-raise:148:4:153:19:try_except_raises:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +no-else-raise:156:4:163:19:try_except_raises2:"Unnecessary ""else"" after ""raise"", remove the ""else"" and de-indent the code inside it":HIGH +inconsistent-return-statements:165:0:165:8:test:Either all return statements in a function should return an expression, or none of them should.:UNDEFINED +inconsistent-return-statements:174:0:174:22:try_finally_return:Either all return statements in a function should return an expression, or none of them should.:UNDEFINED diff --git a/tests/functional/r/raise_missing_from.py b/tests/functional/r/raise_missing_from.py index 897a0e9c4f..3b66609c8e 100644 --- a/tests/functional/r/raise_missing_from.py +++ b/tests/functional/r/raise_missing_from.py @@ -1,5 +1,5 @@ # pylint:disable=missing-docstring, unreachable, using-constant-test, invalid-name, bare-except -# pylint:disable=try-except-raise, undefined-variable, too-few-public-methods, superfluous-parens +# pylint:disable=try-except-raise, undefined-variable, too-few-public-methods, superfluous-parens, no-else-raise try: 1 / 0 From 5eca8ec5947a0d45e588bded0e910a986d4dd77b Mon Sep 17 00:00:00 2001 From: Daniel Date: Mon, 12 Dec 2022 23:34:46 +0100 Subject: [PATCH 8/9] Avoid hanging forever after a parallel job was killed (#7834) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Replace multiprocessing.pool with concurrent.futures.ProcessPoolExecutor to avoid deadlocks. In a multiprocessing.pool, if a process terminates in a non-clean fashion (for example, due to OOM or a segmentation fault), the pool will silently replace said process, but the work that the process was supposed to do will never be done, causing pylint to hang indefinitely. The concurrent.futures.ProcessPoolExecutor will raise a BrokenProcessPool exception in that case, avoiding the hang. Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- doc/whatsnew/fragments/3899.bugfix | 4 ++++ pylint/lint/parallel.py | 21 +++++++++--------- pylint/lint/run.py | 9 ++++++-- tests/test_check_parallel.py | 35 +++++++++++++++++++++++++----- 4 files changed, 52 insertions(+), 17 deletions(-) create mode 100644 doc/whatsnew/fragments/3899.bugfix diff --git a/doc/whatsnew/fragments/3899.bugfix b/doc/whatsnew/fragments/3899.bugfix new file mode 100644 index 0000000000..ed0ad38594 --- /dev/null +++ b/doc/whatsnew/fragments/3899.bugfix @@ -0,0 +1,4 @@ +Pylint will no longer deadlock if a parallel job is killed but fail +immediately instead. + +Closes #3899 diff --git a/pylint/lint/parallel.py b/pylint/lint/parallel.py index 8d0053108b..9730914b75 100644 --- a/pylint/lint/parallel.py +++ b/pylint/lint/parallel.py @@ -23,10 +23,15 @@ except ImportError: multiprocessing = None # type: ignore[assignment] +try: + from concurrent.futures import ProcessPoolExecutor +except ImportError: + ProcessPoolExecutor = None # type: ignore[assignment,misc] + if TYPE_CHECKING: from pylint.lint import PyLinter -# PyLinter object used by worker processes when checking files using multiprocessing +# PyLinter object used by worker processes when checking files using parallel mode # should only be used by the worker processes _worker_linter: PyLinter | None = None @@ -34,8 +39,7 @@ def _worker_initialize( linter: bytes, arguments: None | str | Sequence[str] = None ) -> None: - """Function called to initialize a worker for a Process within a multiprocessing - Pool. + """Function called to initialize a worker for a Process within a concurrent Pool. :param linter: A linter-class (PyLinter) instance pickled with dill :param arguments: File or module name(s) to lint and to be added to sys.path @@ -137,9 +141,9 @@ def check_parallel( # is identical to the linter object here. This is required so that # a custom PyLinter object can be used. initializer = functools.partial(_worker_initialize, arguments=arguments) - with multiprocessing.Pool( - jobs, initializer=initializer, initargs=[dill.dumps(linter)] - ) as pool: + with ProcessPoolExecutor( + max_workers=jobs, initializer=initializer, initargs=(dill.dumps(linter),) + ) as executor: linter.open() all_stats = [] all_mapreduce_data: defaultdict[ @@ -158,7 +162,7 @@ def check_parallel( stats, msg_status, mapreduce_data, - ) in pool.imap_unordered(_worker_check_single_file, files): + ) in executor.map(_worker_check_single_file, files): linter.file_state.base_name = base_name linter.file_state._is_base_filestate = False linter.set_current_module(module, file_path) @@ -168,8 +172,5 @@ def check_parallel( all_mapreduce_data[worker_idx].append(mapreduce_data) linter.msg_status |= msg_status - pool.close() - pool.join() - _merge_mapreduce_data(linter, all_mapreduce_data) linter.stats = merge_stats([linter.stats] + all_stats) diff --git a/pylint/lint/run.py b/pylint/lint/run.py index c4b4834a07..1f54670ef7 100644 --- a/pylint/lint/run.py +++ b/pylint/lint/run.py @@ -30,6 +30,11 @@ except ImportError: multiprocessing = None # type: ignore[assignment] +try: + from concurrent.futures import ProcessPoolExecutor +except ImportError: + ProcessPoolExecutor = None # type: ignore[assignment,misc] + def _query_cpu() -> int | None: """Try to determine number of CPUs allotted in a docker container. @@ -185,9 +190,9 @@ def __init__( ) sys.exit(32) if linter.config.jobs > 1 or linter.config.jobs == 0: - if multiprocessing is None: + if ProcessPoolExecutor is None: print( - "Multiprocessing library is missing, fallback to single process", + "concurrent.futures module is missing, fallback to single process", file=sys.stderr, ) linter.set_option("jobs", 1) diff --git a/tests/test_check_parallel.py b/tests/test_check_parallel.py index 15b59304c0..96e67517ec 100644 --- a/tests/test_check_parallel.py +++ b/tests/test_check_parallel.py @@ -9,8 +9,9 @@ from __future__ import annotations import argparse -import multiprocessing import os +from concurrent.futures import ProcessPoolExecutor +from concurrent.futures.process import BrokenProcessPool from pickle import PickleError import dill @@ -182,10 +183,10 @@ def test_worker_initialize_pickling(self) -> None: """ linter = PyLinter(reporter=Reporter()) linter.attribute = argparse.ArgumentParser() # type: ignore[attr-defined] - with multiprocessing.Pool( - 2, initializer=worker_initialize, initargs=[dill.dumps(linter)] - ) as pool: - pool.imap_unordered(print, [1, 2]) + with ProcessPoolExecutor( + max_workers=2, initializer=worker_initialize, initargs=(dill.dumps(linter),) + ) as executor: + executor.map(print, [1, 2]) def test_worker_check_single_file_uninitialised(self) -> None: pylint.lint.parallel._worker_linter = None @@ -577,3 +578,27 @@ def test_map_reduce(self, num_files: int, num_jobs: int, num_checkers: int) -> N assert str(stats_single_proc.by_msg) == str( stats_check_parallel.by_msg ), "Single-proc and check_parallel() should return the same thing" + + @pytest.mark.timeout(5) + def test_no_deadlock_due_to_initializer_error(self) -> None: + """Tests that an error in the initializer for the parallel jobs doesn't + lead to a deadlock. + """ + linter = PyLinter(reporter=Reporter()) + + linter.register_checker(SequentialTestChecker(linter)) + + # Create a dummy file, the actual contents of which will be ignored by the + # register test checkers, but it will trigger at least a single-job to be run. + single_file_container = _gen_file_datas(count=1) + + # The error in the initializer should trigger a BrokenProcessPool exception + with pytest.raises(BrokenProcessPool): + check_parallel( + linter, + jobs=1, + files=iter(single_file_container), + # This will trigger an exception in the initializer for the parallel jobs + # because arguments has to be an Iterable. + arguments=1, # type: ignore[arg-type] + ) From c61bc76806f7b389db4b7b484174dbc286fa2c11 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 13 Dec 2022 06:35:41 +0100 Subject: [PATCH 9/9] [pre-commit.ci] pre-commit autoupdate (#7932) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updates: - [github.com/PyCQA/isort: 5.10.1 → 5.11.1](https://github.com/PyCQA/isort/compare/5.10.1...5.11.1) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> --- .pre-commit-config.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 7f927b96dc..9f2db3209a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -39,7 +39,7 @@ repos: args: [--py37-plus] exclude: *fixtures - repo: https://github.com/PyCQA/isort - rev: 5.10.1 + rev: 5.11.1 hooks: - id: isort exclude: doc/data/messages/(r/reimported|w/wrong-import-order|u/ungrouped-imports|m/misplaced-future|m/multiple-imports)/bad.py