diff --git a/ChangeLog b/ChangeLog index df0a7313ba..5ed27c8e78 100644 --- a/ChangeLog +++ b/ChangeLog @@ -39,6 +39,10 @@ Release date: TBA Closes #4045 +* Fixed false negative for ``used-before-assignment`` in finally blocks + if an except handler did not define the assignment that might have failed + in the try block. + * Fixed extremely long processing of long lines with comma's. Closes #5483 diff --git a/doc/whatsnew/2.13.rst b/doc/whatsnew/2.13.rst index 10970a1dd0..b971a746ac 100644 --- a/doc/whatsnew/2.13.rst +++ b/doc/whatsnew/2.13.rst @@ -90,6 +90,10 @@ Other Changes Closes #4045 +* Fixed false negative for ``used-before-assignment`` in finally blocks + if an except handler did not define the assignment that might have failed + in the try block. + * Fix a false positive for ``assigning-non-slot`` when the slotted class defined ``__setattr__``. diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py index f01866276b..c06f69b700 100644 --- a/pylint/checkers/variables.py +++ b/pylint/checkers/variables.py @@ -652,23 +652,15 @@ def get_next_to_consume(self, node): # If this node is in a Finally block of a Try/Finally, # filter out assignments in the try portion, assuming they may fail - if ( - found_nodes - and isinstance(node_statement.parent, nodes.TryFinally) - and node_statement in node_statement.parent.finalbody - ): - filtered_nodes = [ - n - for n in found_nodes - if not ( - n.statement(future=True).parent is node_statement.parent - and n.statement(future=True) in n.statement(future=True).parent.body + if found_nodes: + uncertain_nodes = ( + self._uncertain_nodes_in_try_blocks_when_evaluating_finally_blocks( + found_nodes, node_statement ) - ] - filtered_nodes_set = set(filtered_nodes) - difference = [n for n in found_nodes if n not in filtered_nodes_set] - self.consumed_uncertain[node.name] += difference - found_nodes = filtered_nodes + ) + self.consumed_uncertain[node.name] += uncertain_nodes + uncertain_nodes_set = set(uncertain_nodes) + found_nodes = [n for n in found_nodes if n not in uncertain_nodes_set] # If this node is in an ExceptHandler, # filter out assignments in the try portion, assuming they may fail @@ -779,6 +771,43 @@ def _uncertain_nodes_in_try_blocks_when_evaluating_except_blocks( uncertain_nodes.append(other_node) return uncertain_nodes + @staticmethod + def _uncertain_nodes_in_try_blocks_when_evaluating_finally_blocks( + found_nodes: List[nodes.NodeNG], node_statement: nodes.Statement + ) -> List[nodes.NodeNG]: + uncertain_nodes: List[nodes.NodeNG] = [] + ( + closest_try_finally_ancestor, + child_of_closest_try_finally_ancestor, + ) = utils.get_node_first_ancestor_of_type_and_its_child( + node_statement, nodes.TryFinally + ) + if closest_try_finally_ancestor is None: + return uncertain_nodes + if ( + child_of_closest_try_finally_ancestor + not in closest_try_finally_ancestor.finalbody + ): + return uncertain_nodes + for other_node in found_nodes: + other_node_statement = other_node.statement(future=True) + ( + other_node_try_finally_ancestor, + child_of_other_node_try_finally_ancestor, + ) = utils.get_node_first_ancestor_of_type_and_its_child( + other_node_statement, nodes.TryFinally + ) + if other_node_try_finally_ancestor is not closest_try_finally_ancestor: + continue + if ( + child_of_other_node_try_finally_ancestor + not in other_node_try_finally_ancestor.body + ): + continue + # Passed all tests for uncertain execution + uncertain_nodes.append(other_node) + return uncertain_nodes + # pylint: disable=too-many-public-methods class VariablesChecker(BaseChecker): diff --git a/tests/functional/c/consider/consider_using_with_open.py b/tests/functional/c/consider/consider_using_with_open.py index 7c584d8f53..ec56762333 100644 --- a/tests/functional/c/consider/consider_using_with_open.py +++ b/tests/functional/c/consider/consider_using_with_open.py @@ -137,7 +137,7 @@ def test_defined_in_try_and_finally(self): except FileNotFoundError: Path("foo").touch() finally: - file_handle.open("foo", encoding="utf") # must not trigger + file_handle.open("foo", encoding="utf") # [used-before-assignment] # must not trigger with file_handle: return file_handle.read() diff --git a/tests/functional/c/consider/consider_using_with_open.txt b/tests/functional/c/consider/consider_using_with_open.txt index b52b661af1..bf9d5dc16a 100644 --- a/tests/functional/c/consider/consider_using_with_open.txt +++ b/tests/functional/c/consider/consider_using_with_open.txt @@ -4,3 +4,4 @@ consider-using-with:46:4:46:33:test_open_outside_assignment:Consider using 'with consider-using-with:47:14:47:43:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED consider-using-with:52:8:52:37:test_open_inside_with_block:Consider using 'with' for resource-allocating operations:UNDEFINED consider-using-with:120:26:122:13:TestControlFlow.test_triggers_if_reassigned_after_if_else:Consider using 'with' for resource-allocating operations:UNDEFINED +used-before-assignment:140:12:140:23:TestControlFlow.test_defined_in_try_and_finally:Using variable 'file_handle' before assignment:UNDEFINED diff --git a/tests/functional/u/use/used_before_assignment_issue85.py b/tests/functional/u/use/used_before_assignment_issue85.py index 58d8e38d85..499d243e6d 100644 --- a/tests/functional/u/use/used_before_assignment_issue85.py +++ b/tests/functional/u/use/used_before_assignment_issue85.py @@ -7,3 +7,61 @@ def main(): finally: print(res) # [used-before-assignment] print(res) + + +def try_except_finally(): + """When evaluating finally blocks, assume try statements fail.""" + try: + res = 1 / 0 + res = 42 + except ZeroDivisionError: + print() + finally: + print(res) # [used-before-assignment] + print(res) + + +def try_except_finally_assignment_in_final_block(): + """Assignment of the name in the final block does not warn.""" + try: + res = 1 / 0 + res = 42 + except ZeroDivisionError: + print() + finally: + res = 999 + print(res) + print(res) + + +def try_except_finally_nested_try_finally_in_try(): + """Don't confuse assignments in different finally statements where + one is nested inside a try. + """ + try: + try: + res = 1 / 0 + finally: + print(res) # [used-before-assignment] + print(1 / 0) + except ZeroDivisionError: + print() + finally: + res = 999 # this assignment could be confused for that above + print(res) + print(res) + + +def try_except_finally_nested_try_finally_in_finally(): + """Don't confuse assignments in different finally statements where + one is nested inside a finally. + """ + try: + pass + finally: + try: + times = 1 + except TypeError: + pass + # Don't emit: only assume the outer try failed + print(times) diff --git a/tests/functional/u/use/used_before_assignment_issue85.txt b/tests/functional/u/use/used_before_assignment_issue85.txt index ce6e4b9d06..58e9bde719 100644 --- a/tests/functional/u/use/used_before_assignment_issue85.txt +++ b/tests/functional/u/use/used_before_assignment_issue85.txt @@ -1 +1,3 @@ used-before-assignment:8:14:8:17:main:Using variable 'res' before assignment:UNDEFINED +used-before-assignment:20:14:20:17:try_except_finally:Using variable 'res' before assignment:UNDEFINED +used-before-assignment:45:18:45:21:try_except_finally_nested_try_finally_in_try:Using variable 'res' before assignment:UNDEFINED