diff options
| -rw-r--r-- | ChangeLog | 4 | ||||
| -rw-r--r-- | doc/whatsnew/2.10.rst | 3 | ||||
| -rw-r--r-- | pylint/checkers/refactoring/refactoring_checker.py | 7 | ||||
| -rw-r--r-- | tests/functional/c/consider/consider_using_with_open.py | 93 | ||||
| -rw-r--r-- | tests/functional/c/consider/consider_using_with_open.txt | 11 |
5 files changed, 111 insertions, 7 deletions
@@ -19,6 +19,10 @@ What's New in Pylint 2.10.3? ============================ Release date: TBA + * Fix false positive for ``consider-using-with`` if a context manager is assigned to a + variable in different paths of control flow (e. g. if-else clause). + + Closes #4751 What's New in Pylint 2.10.2? diff --git a/doc/whatsnew/2.10.rst b/doc/whatsnew/2.10.rst index 85cba1c3f..09fc52870 100644 --- a/doc/whatsnew/2.10.rst +++ b/doc/whatsnew/2.10.rst @@ -82,6 +82,9 @@ Other Changes * ``consider-using-with`` is no longer triggered if a context manager is returned from a function. +* Fix false positive for ``consider-using-with`` if a context manager is assigned to a + variable in different paths of control flow (e. g. if-else clause). + * pylint does not crash with a traceback anymore when a file is problematic. It creates a template text file for opening an issue on the bug tracker instead. The linting can go on for other non problematic files instead of being impossible. diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index 7a3048ab4..9ce42e395 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -1442,10 +1442,15 @@ class RefactoringChecker(checkers.BaseTokenChecker): else assignee.attrname ) if varname in stack: + existing_node = stack[varname] + if astroid.are_exclusive(node, existing_node): + # only one of the two assignments can be executed at runtime, thus it is fine + stack[varname] = value + continue # variable was redefined before it was used in a ``with`` block self.add_message( "consider-using-with", - node=stack[varname], + node=existing_node, ) stack[varname] = value diff --git a/tests/functional/c/consider/consider_using_with_open.py b/tests/functional/c/consider/consider_using_with_open.py index ca807459e..7c584d8f5 100644 --- a/tests/functional/c/consider/consider_using_with_open.py +++ b/tests/functional/c/consider/consider_using_with_open.py @@ -1,4 +1,4 @@ -# pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel +# pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel, no-self-use # pylint: disable=missing-class-docstring, too-few-public-methods, unused-variable, multiple-statements, line-too-long """ The functional test for the standard ``open()`` function has to be moved in a separate file, @@ -7,6 +7,7 @@ However, all remaining checks for consider-using-with work in PyPy, so we do not PyPy from ALL functional tests. """ from contextlib import contextmanager +from pathlib import Path myfile = open("test.txt", encoding="utf-8") # [consider-using-with] @@ -68,3 +69,93 @@ def test_multiline_with_items(file1, file2, which): def test_suppress_on_return(): return open("foo", encoding="utf8") # must not trigger + + +class TestControlFlow: + """ + The message is triggered if a context manager is assigned to a variable, which name is later + reassigned without the variable being used inside a ``with`` first. + E.g. the following would trigger the message: + + a = open("foo") # <-- would trigger here + a = "something new" + + But it must not happen that the logic which checks if the same variable is assigned multiple + times in different code branches where only one of those assign statements is hit at runtime. + For example, the variable could be assigned in an if-else construct. + + These tests check that the message is not triggered in those circumstances. + """ + + def test_defined_in_if_and_else(self, predicate): + if predicate: + file_handle = open("foo", encoding="utf8") # must not trigger + else: + file_handle = open("bar", encoding="utf8") # must not trigger + with file_handle: + return file_handle.read() + + def test_defined_in_else_only(self, predicate): + if predicate: + result = "shiny watermelon" + else: + file_handle = open("foo", encoding="utf8") # must not trigger + with file_handle: + result = file_handle.read() + return result + + def test_defined_in_if_only(self, predicate): + if predicate: + file_handle = open("foo", encoding="utf8") # must not trigger + with file_handle: + result = file_handle.read() + else: + result = "shiny watermelon" + return result + + def test_triggers_if_reassigned_after_if_else(self, predicate): + if predicate: + file_handle = open("foo", encoding="utf8") + else: + file_handle = open( # [consider-using-with] + "bar", encoding="utf8" + ) + file_handle = None + return file_handle + + def test_defined_in_try_and_except(self): + try: + file_handle = open("foo", encoding="utf8") # must not trigger + except FileNotFoundError: + file_handle = open("bar", encoding="utf8") # must not trigger + with file_handle: + return file_handle.read() + + def test_defined_in_try_and_finally(self): + try: + file_handle = open("foo", encoding="utf8") # must not trigger + except FileNotFoundError: + Path("foo").touch() + finally: + file_handle.open("foo", encoding="utf") # must not trigger + with file_handle: + return file_handle.read() + + def test_defined_in_different_except_handlers(self, a, b): + try: + result = a/b + except ZeroDivisionError: + logfile = open("math_errors.txt", encoding="utf8") # must not trigger + result = "Can't divide by zero" + except TypeError: + logfile = open("type_errors.txt", encoding="utf8") # must not trigger + result = "Wrong types" + else: + logfile = open("results.txt", encoding="utf8") # must not trigger + with logfile: + logfile.write(result) + + def test_multiple_return_statements(self, predicate): + if predicate: + return open("foo", encoding="utf8") # must not trigger + return open("bar", encoding="utf8") # must not trigger diff --git a/tests/functional/c/consider/consider_using_with_open.txt b/tests/functional/c/consider/consider_using_with_open.txt index ea3503123..cf3fdc1c8 100644 --- a/tests/functional/c/consider/consider_using_with_open.txt +++ b/tests/functional/c/consider/consider_using_with_open.txt @@ -1,5 +1,6 @@ -consider-using-with:11:9::Consider using 'with' for resource-allocating operations -consider-using-with:15:9:test_open:Consider using 'with' for resource-allocating operations -consider-using-with:45:4:test_open_outside_assignment:Consider using 'with' for resource-allocating operations -consider-using-with:46:14:test_open_outside_assignment:Consider using 'with' for resource-allocating operations -consider-using-with:51:8:test_open_inside_with_block:Consider using 'with' for resource-allocating operations +consider-using-with:12:9::Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:16:9:test_open:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:46:4:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:47:14:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:52:8:test_open_inside_with_block:Consider using 'with' for resource-allocating operations:HIGH +consider-using-with:120:26:TestControlFlow.test_triggers_if_reassigned_after_if_else:Consider using 'with' for resource-allocating operations:HIGH |