From f61c616c6b5db5c703ca6940ba772054baad83ef Mon Sep 17 00:00:00 2001 From: David Liu Date: Mon, 2 Aug 2021 17:20:37 -0400 Subject: [PATCH 1/2] Fix false negative for used-before-assignment (ExceptHandler) Closes #626. --- ChangeLog | 5 ++ doc/whatsnew/2.10.rst | 5 ++ pylint/checkers/variables.py | 61 +++++++++++++------ tests/functional/u/unused/unused_variable.py | 3 +- tests/functional/u/unused/unused_variable.txt | 3 +- .../u/use/used_before_assignment_issue626.py | 44 +++++++++++++ .../u/use/used_before_assignment_issue626.txt | 5 ++ 7 files changed, 105 insertions(+), 21 deletions(-) create mode 100644 tests/functional/u/use/used_before_assignment_issue626.py create mode 100644 tests/functional/u/use/used_before_assignment_issue626.txt diff --git a/ChangeLog b/ChangeLog index aaefea2374..ffba88a13d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -68,6 +68,11 @@ Release date: TBA Closes #4681 +* Fix false negative for ``used-before-assignment`` when the variable is assigned + in an exception handler, but used outside of the handler. + + Closes #626 + What's New in Pylint 2.9.6? =========================== diff --git a/doc/whatsnew/2.10.rst b/doc/whatsnew/2.10.rst index 26b792a6ab..d352e41615 100644 --- a/doc/whatsnew/2.10.rst +++ b/doc/whatsnew/2.10.rst @@ -54,3 +54,8 @@ Other Changes Ref #1162 Closes #1990 Closes #4168 + +* Fix false negative for ``used-before-assignment`` when the variable is assigned + in an exception handler, but used outside of the handler. + + Closes #626 diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index 1dcdd8fc46..5aa402e2dc 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -545,36 +545,55 @@ def consumed(self): def scope_type(self): return self._atomic.scope_type - def mark_as_consumed(self, name, new_node): + def mark_as_consumed(self, name, consumed_nodes): """ - Mark the name as consumed and delete it from + Mark the given nodes as consumed for the name. + If all of the nodes for the name were consumed, delete the name from the to_consume dictionary """ - self.consumed[name] = new_node - del self.to_consume[name] + unconsumed = [n for n in self.to_consume[name] if n not in set(consumed_nodes)] + self.consumed[name] = consumed_nodes + + if unconsumed: + self.to_consume[name] = unconsumed + else: + del self.to_consume[name] def get_next_to_consume(self, node): - # Get the definition of `node` from this scope + """ + Return a list of the nodes that define `node` from this scope. + Return None to indicate a special case that needs to be handled by the caller. + """ name = node.name parent_node = node.parent - found_node = self.to_consume.get(name) + found_nodes = self.to_consume.get(name) if ( - found_node + found_nodes and isinstance(parent_node, astroid.Assign) - and parent_node == found_node[0].parent + and parent_node == found_nodes[0].parent ): - lhs = found_node[0].parent.targets[0] + lhs = found_nodes[0].parent.targets[0] if lhs.name == name: # this name is defined in this very statement - found_node = None + found_nodes = None if ( - found_node + found_nodes and isinstance(parent_node, astroid.For) and parent_node.iter == node - and parent_node.target in found_node + and parent_node.target in found_nodes ): - found_node = None - return found_node + found_nodes = None + + # Filter out assignments in ExceptHandlers that node is not contained in + if found_nodes: + found_nodes = [ + n + for n in found_nodes + if not isinstance(n.statement(), astroid.ExceptHandler) + or n.statement().parent_of(node) + ] + + return found_nodes # pylint: disable=too-many-public-methods @@ -943,6 +962,7 @@ def visit_assignname(self, node): def visit_delname(self, node): self.visit_name(node) + # pylint: disable=too-many-branches def visit_name(self, node): """Check that a name is defined in the current scope""" stmt = node.statement() @@ -1019,12 +1039,17 @@ def visit_name(self, node): self._loopvar_name(node, name) break - found_node = current_consumer.get_next_to_consume(node) - if found_node is None: + found_nodes = current_consumer.get_next_to_consume(node) + if found_nodes is None: continue # checks for use before assignment - defnode = utils.assign_parent(current_consumer.to_consume[name][0]) + if found_nodes: + defnode = utils.assign_parent(found_nodes[0]) + else: + defnode = None + if used_before_assignment_is_enabled: + self.add_message("used-before-assignment", args=name, node=node) if ( undefined_variable_is_enabled or used_before_assignment_is_enabled @@ -1156,7 +1181,7 @@ def visit_name(self, node): elif current_consumer.scope_type == "lambda": self.add_message("undefined-variable", node=node, args=name) - current_consumer.mark_as_consumed(name, found_node) + current_consumer.mark_as_consumed(name, found_nodes) # check it's not a loop variable used outside the loop self._loopvar_name(node, name) break diff --git a/tests/functional/u/unused/unused_variable.py b/tests/functional/u/unused/unused_variable.py index 11c4081147..887a355532 100644 --- a/tests/functional/u/unused/unused_variable.py +++ b/tests/functional/u/unused/unused_variable.py @@ -144,8 +144,7 @@ def func3(): print(f"{error}") try: 1 / 2 - except TypeError as error: - # TODO fix bug for not identifying unused variables in nested exceptions see issue #4391 + except TypeError as error: # [unused-variable] print("warning") def func4(): diff --git a/tests/functional/u/unused/unused_variable.txt b/tests/functional/u/unused/unused_variable.txt index 760d8b8da6..482ebdbc44 100644 --- a/tests/functional/u/unused/unused_variable.txt +++ b/tests/functional/u/unused/unused_variable.txt @@ -19,4 +19,5 @@ unused-import:104:4:test_global:Unused version imported from sys as VERSION unused-import:105:4:test_global:Unused import this unused-import:106:4:test_global:Unused re imported as RE unused-variable:110:4:function2:Unused variable 'unused' -unused-variable:154:4:func4:Unused variable 'error' +unused-variable:147:8:func3:Unused variable 'error' +unused-variable:153:4:func4:Unused variable 'error' diff --git a/tests/functional/u/use/used_before_assignment_issue626.py b/tests/functional/u/use/used_before_assignment_issue626.py new file mode 100644 index 0000000000..c98632b7f7 --- /dev/null +++ b/tests/functional/u/use/used_before_assignment_issue626.py @@ -0,0 +1,44 @@ +# pylint: disable=missing-docstring,invalid-name +def main1(): + try: + raise ValueError + except ValueError as e: # [unused-variable] + pass + + print(e) # [used-before-assignment] + + +def main2(): + try: + raise ValueError + except ValueError as e: + print(e) + + +def main3(): + try: + raise ValueError + except ValueError as e: # [unused-variable] + pass + + e = 10 + print(e) + + +def main4(): + try: + raise ValueError + except ValueError as e: # [unused-variable] + pass + + try: + raise ValueError + except ValueError as e: + pass + + try: + raise ValueError + except ValueError as e: + pass + + print(e) # [used-before-assignment] diff --git a/tests/functional/u/use/used_before_assignment_issue626.txt b/tests/functional/u/use/used_before_assignment_issue626.txt new file mode 100644 index 0000000000..e7e56c1959 --- /dev/null +++ b/tests/functional/u/use/used_before_assignment_issue626.txt @@ -0,0 +1,5 @@ +unused-variable:5:4:main1:Unused variable 'e' +used-before-assignment:8:10:main1:Using variable 'e' before assignment +unused-variable:21:4:main3:Unused variable 'e' +unused-variable:31:4:main4:Unused variable 'e' +used-before-assignment:44:10:main4:Using variable 'e' before assignment From 161a27199fce6c12411c61042212b7a669d04ee3 Mon Sep 17 00:00:00 2001 From: David Liu Date: Mon, 2 Aug 2021 22:20:23 -0400 Subject: [PATCH 2/2] Fix unused-variable check for exception variables --- pylint/checkers/variables.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index 5aa402e2dc..27ebbf4e6e 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -1741,6 +1741,12 @@ def _check_is_unused(self, name, node, stmt, global_names, nonlocal_names): if utils.is_overload_stub(node): return + # Special case for exception variable + if isinstance(stmt.parent, astroid.ExceptHandler) and any( + n.name == name for n in stmt.parent.nodes_of_class(astroid.Name) + ): + return + self.add_message(message_name, args=name, node=stmt) def _is_name_ignored(self, stmt, name):