From 922f38969c326826344e2d25af499cf8c5f80d8c Mon Sep 17 00:00:00 2001 From: DudeNr33 <3929834+DudeNr33@users.noreply.github.com> Date: Fri, 23 Apr 2021 20:31:21 +0200 Subject: Enhancement #3413 ``consider-using-with`` (#4372) * Implement consider-using-with check * Fix or disable consider-using-with in codebase * Fix ticket number in ChangeLog * Move functional test for ``open()`` into separate testfile and exclude this test from running with PyPy Co-authored-by: Pierre Sassoulas --- ChangeLog | 6 + doc/whatsnew/2.8.rst | 4 + pylint/checkers/refactoring/refactoring_checker.py | 51 ++++++++ pylint/checkers/spelling.py | 4 +- pylint/epylint.py | 41 +++--- pylint/graph.py | 5 +- pylint/lint/parallel.py | 4 +- pylint/pyreverse/writer.py | 2 +- pylint/testutils/lint_module_test.py | 2 + tests/checkers/unittest_format.py | 4 +- tests/functional/c/consider/consider_using_with.py | 138 +++++++++++++++++++++ .../functional/c/consider/consider_using_with.txt | 22 ++++ .../c/consider/consider_using_with_open.py | 15 +++ .../c/consider/consider_using_with_open.rc | 2 + .../c/consider/consider_using_with_open.txt | 1 + tests/functional/d/disabled_msgid_in_pylintrc.rc | 2 +- tests/functional/n/non/non_iterator_returned.py | 19 ++- tests/functional/n/non/non_iterator_returned.txt | 8 +- 18 files changed, 293 insertions(+), 37 deletions(-) create mode 100644 tests/functional/c/consider/consider_using_with.py create mode 100644 tests/functional/c/consider/consider_using_with.txt create mode 100644 tests/functional/c/consider/consider_using_with_open.py create mode 100644 tests/functional/c/consider/consider_using_with_open.rc create mode 100644 tests/functional/c/consider/consider_using_with_open.txt diff --git a/ChangeLog b/ChangeLog index 3a2759a98..200656717 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,12 @@ Release date: Undefined .. Put new features here and also in 'doc/whatsnew/2.8.rst' +* New refactoring message ``consider-using-with``. This message is emitted if resource-allocating functions or methods of the + standard library (like ``open()`` or ``threading.Lock.acquire()``) that can be used as a context manager are called without + a ``with`` block. + + Closes #3413 + * Resolve false positives on unused variables in decorator functions Closes #4252 diff --git a/doc/whatsnew/2.8.rst b/doc/whatsnew/2.8.rst index 11bce8c36..60c24e4ec 100644 --- a/doc/whatsnew/2.8.rst +++ b/doc/whatsnew/2.8.rst @@ -12,6 +12,10 @@ Summary -- Release highlights New checkers ============ +* New refactoring message ``consider-using-with``. This message is emitted if resource-allocating functions or methods of the + standard library (like ``open()`` or ``threading.Lock.acquire()``) that can be used as a context manager are called without + a ``with`` block. + * Add ``deprecated-argument`` check for deprecated arguments. * Add new extension ``ConfusingConsecutiveElifChecker``. This optional checker emits a refactoring message (R5601 ``confusing-consecutive-elif``) diff --git a/pylint/checkers/refactoring/refactoring_checker.py b/pylint/checkers/refactoring/refactoring_checker.py index e63386186..982b0126b 100644 --- a/pylint/checkers/refactoring/refactoring_checker.py +++ b/pylint/checkers/refactoring/refactoring_checker.py @@ -17,6 +17,35 @@ from pylint.checkers.utils import node_frame_class KNOWN_INFINITE_ITERATORS = {"itertools.count"} BUILTIN_EXIT_FUNCS = frozenset(("quit", "exit")) +CALLS_THAT_COULD_BE_REPLACED_BY_WITH = frozenset( + ( + "threading.lock.acquire", + "threading._RLock.acquire", + "threading.Semaphore.acquire", + "multiprocessing.managers.BaseManager.start", + "multiprocessing.managers.SyncManager.start", + ) +) +CALLS_RETURNING_CONTEXT_MANAGERS = frozenset( + ( + "_io.open", # regular 'open()' call + "codecs.open", + "urllib.request.urlopen", + "tempfile.NamedTemporaryFile", + "tempfile.SpooledTemporaryFile", + "tempfile.TemporaryDirectory", + "zipfile.ZipFile", + "zipfile.PyZipFile", + "zipfile.ZipFile.open", + "zipfile.PyZipFile.open", + "tarfile.TarFile", + "tarfile.TarFile.open", + "multiprocessing.context.BaseContext.Pool", + "concurrent.futures.thread.ThreadPoolExecutor", + "concurrent.futures.process.ProcessPoolExecutor", + "subprocess.Popen", + ) +) def _if_statement_is_always_returning(if_node, returning_node_class) -> bool: @@ -301,6 +330,12 @@ class RefactoringChecker(checkers.BaseTokenChecker): "consider-using-max-builtin", "Using the max builtin instead of a conditional improves readability and conciseness.", ), + "R1732": ( + "Consider using 'with' for resource-allocating operations", + "consider-using-with", + "Emitted if a resource-allocating assignment or call may be replaced by a 'with' block. " + "By using 'with' the release of the allocated resources is ensured even in the case of an exception.", + ), } options = ( ( @@ -819,6 +854,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): "consider-using-sys-exit", "super-with-arguments", "consider-using-generator", + "consider-using-with", ) def visit_call(self, node): self._check_raising_stopiteration_in_generator_next_call(node) @@ -826,6 +862,7 @@ class RefactoringChecker(checkers.BaseTokenChecker): self._check_quit_exit_call(node) self._check_super_with_arguments(node) self._check_consider_using_generator(node) + self._check_consider_using_with_instead_call(node) @staticmethod def _has_exit_in_scope(scope): @@ -1196,9 +1233,11 @@ class RefactoringChecker(checkers.BaseTokenChecker): "simplify-boolean-expression", "consider-using-ternary", "consider-swap-variables", + "consider-using-with", ) def visit_assign(self, node): self._check_swap_variables(node) + self._check_consider_using_with_instead_assign(node) if self._is_and_or_ternary(node.value): cond, truth_value, false_value = self._and_or_ternary_arguments(node.value) else: @@ -1229,6 +1268,18 @@ class RefactoringChecker(checkers.BaseTokenChecker): visit_return = visit_assign + def _check_consider_using_with_instead_assign(self, node: astroid.Assign): + assigned = node.value + if isinstance(assigned, astroid.Call): + inferred = utils.safe_infer(assigned.func) + if inferred and inferred.qname() in CALLS_RETURNING_CONTEXT_MANAGERS: + self.add_message("consider-using-with", node=node) + + def _check_consider_using_with_instead_call(self, node: astroid.Call): + inferred = utils.safe_infer(node.func) + if inferred and inferred.qname() in CALLS_THAT_COULD_BE_REPLACED_BY_WITH: + self.add_message("consider-using-with", node=node) + def _check_consider_using_join(self, aug_assign): """ We start with the augmented assignment and work our way upwards. diff --git a/pylint/checkers/spelling.py b/pylint/checkers/spelling.py index 3f32d4762..49c511990 100644 --- a/pylint/checkers/spelling.py +++ b/pylint/checkers/spelling.py @@ -314,7 +314,9 @@ class SpellingChecker(BaseTokenChecker): self.spelling_dict = enchant.DictWithPWL( dict_name, self.config.spelling_private_dict_file ) - self.private_dict_file = open(self.config.spelling_private_dict_file, "a") + self.private_dict_file = open( # pylint: disable=consider-using-with + self.config.spelling_private_dict_file, "a" + ) else: self.spelling_dict = enchant.Dict(dict_name) diff --git a/pylint/epylint.py b/pylint/epylint.py index a969bf0b1..9d3dbb54c 100755 --- a/pylint/epylint.py +++ b/pylint/epylint.py @@ -111,23 +111,24 @@ def lint(filename, options=()): ] + list(options) ) - process = Popen( + + with Popen( cmd, stdout=PIPE, cwd=parent_path, env=_get_env(), universal_newlines=True - ) + ) as process: - for line in process.stdout: - # remove pylintrc warning - if line.startswith("No config file found"): - continue + for line in process.stdout: + # remove pylintrc warning + if line.startswith("No config file found"): + continue - # modify the file name thats output to reverse the path traversal we made - parts = line.split(":") - if parts and parts[0] == child_path: - line = ":".join([filename] + parts[1:]) - print(line, end=" ") + # modify the file name thats output to reverse the path traversal we made + parts = line.split(":") + if parts and parts[0] == child_path: + line = ":".join([filename] + parts[1:]) + print(line, end=" ") - process.wait() - return process.returncode + process.wait() + return process.returncode def py_run(command_options="", return_std=False, stdout=None, stderr=None): @@ -171,19 +172,19 @@ def py_run(command_options="", return_std=False, stdout=None, stderr=None): else: stderr = sys.stderr # Call pylint in a subprocess - process = Popen( + with Popen( cli, shell=False, stdout=stdout, stderr=stderr, env=_get_env(), universal_newlines=True, - ) - proc_stdout, proc_stderr = process.communicate() - # Return standard output and error - if return_std: - return StringIO(proc_stdout), StringIO(proc_stderr) - return None + ) as process: + proc_stdout, proc_stderr = process.communicate() + # Return standard output and error + if return_std: + return StringIO(proc_stdout), StringIO(proc_stderr) + return None def Run(): diff --git a/pylint/graph.py b/pylint/graph.py index 88095cb0d..3c4bbcef1 100644 --- a/pylint/graph.py +++ b/pylint/graph.py @@ -105,9 +105,8 @@ class DotBackend: os.close(pdot) else: dot_sourcepath = outputfile - pdot = codecs.open(dot_sourcepath, "w", encoding="utf8") # type: ignore - pdot.write(self.source) # type: ignore - pdot.close() # type: ignore + with codecs.open(dot_sourcepath, "w", encoding="utf8") as pdot: # type: ignore + pdot.write(self.source) # type: ignore if target not in graphviz_extensions: if shutil.which(self.renderer) is None: raise RuntimeError( diff --git a/pylint/lint/parallel.py b/pylint/lint/parallel.py index 29fc2c1d0..9b666f06c 100644 --- a/pylint/lint/parallel.py +++ b/pylint/lint/parallel.py @@ -122,7 +122,9 @@ def check_parallel(linter, jobs, files, arguments=None): # 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) - pool = multiprocessing.Pool(jobs, initializer=initializer, initargs=[linter]) + pool = multiprocessing.Pool( # pylint: disable=consider-using-with + jobs, initializer=initializer, initargs=[linter] + ) # ..and now when the workers have inherited the linter, the actual reporter # can be set back here on the parent process so that results get stored into # correct reporter diff --git a/pylint/pyreverse/writer.py b/pylint/pyreverse/writer.py index 5feb1eb68..3102fdda6 100644 --- a/pylint/pyreverse/writer.py +++ b/pylint/pyreverse/writer.py @@ -167,7 +167,7 @@ class VCGWriter(DiagramWriter): def set_printer(self, file_name, basename): """initialize VCGWriter for a UML graph""" - self.graph_file = open(file_name, "w+") + self.graph_file = open(file_name, "w+") # pylint: disable=consider-using-with self.printer = VCGPrinter(self.graph_file) self.printer.open_graph( title=basename, diff --git a/pylint/testutils/lint_module_test.py b/pylint/testutils/lint_module_test.py index 1d9396bff..e3580946d 100644 --- a/pylint/testutils/lint_module_test.py +++ b/pylint/testutils/lint_module_test.py @@ -132,12 +132,14 @@ class LintModuleTest: unexpected[key] = -value return missing, unexpected + # pylint: disable=consider-using-with def _open_expected_file(self): try: return open(self._test_file.expected_output) except FileNotFoundError: return StringIO("") + # pylint: disable=consider-using-with def _open_source_file(self): if self._test_file.base == "invalid_encoded_data": return open(self._test_file.source) diff --git a/tests/checkers/unittest_format.py b/tests/checkers/unittest_format.py index 80d654f98..a2aa4687f 100644 --- a/tests/checkers/unittest_format.py +++ b/tests/checkers/unittest_format.py @@ -252,7 +252,9 @@ def test_disable_global_option_end_of_line(): Test for issue with disabling tokenizer messages that extend beyond the scope of the ast tokens """ - file_ = tempfile.NamedTemporaryFile("w", delete=False) + file_ = tempfile.NamedTemporaryFile( # pylint: disable=consider-using-with + "w", delete=False + ) with file_: file_.write( """ diff --git a/tests/functional/c/consider/consider_using_with.py b/tests/functional/c/consider/consider_using_with.py new file mode 100644 index 000000000..234c6093d --- /dev/null +++ b/tests/functional/c/consider/consider_using_with.py @@ -0,0 +1,138 @@ +# pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel +import codecs +import multiprocessing +import subprocess +import tarfile +import tempfile +import threading +import urllib +import zipfile +from concurrent import futures + + +def test_codecs_open(): + fh = codecs.open("test.txt", "utf8") # [consider-using-with] + fh.close() + + +def test_urlopen(): + _ = urllib.request.urlopen("http://www.python.org") # [consider-using-with] + + +def test_temporary_file(): + _ = tempfile.TemporaryFile("r") # [consider-using-with] + + +def test_named_temporary_file(): + _ = tempfile.NamedTemporaryFile("r") # [consider-using-with] + + +def test_spooled_temporary_file(): + _ = tempfile.SpooledTemporaryFile("r") # [consider-using-with] + + +def test_temporary_directory(): + _ = tempfile.TemporaryDirectory() # [consider-using-with] + + +def test_zipfile(): + myzip = zipfile.ZipFile("spam.zip", "w") # [consider-using-with] + _ = myzip.open("eggs.txt") # [consider-using-with] + + +def test_pyzipfile(): + myzip = zipfile.PyZipFile("spam.zip", "w") # [consider-using-with] + + with zipfile.PyZipFile("spam.zip", "w"): # must not trigger + pass + + _ = myzip.open("eggs.txt") # [consider-using-with] + + with myzip.open("eggs.txt"): # must not trigger + pass + + +def test_tarfile(): + tf = tarfile.open("/tmp/test.tar", "w") # [consider-using-with] + tf.close() + + with tarfile.open("/tmp/test.tar", "w"): # must not trigger + pass + + tf = tarfile.TarFile("/tmp/test2.tar", "w") # [consider-using-with] + tf.close() + + with tarfile.TarFile("/tmp/test2.tar", "w"): # must not trigger + pass + + +def test_lock_acquisition(): + lock = threading.Lock() + lock.acquire() # [consider-using-with] + lock.release() + + with lock: # must not trigger + pass + + rlock = threading.RLock() + rlock.acquire() # [consider-using-with] + rlock.release() + + with rlock: # must not trigger + pass + + # Not working currently + # condition = threading.Condition() + # condition.acquire() + # condition.release() + + # with condition: # must not trigger + # pass + + sema = threading.Semaphore() + sema.acquire() # [consider-using-with] + sema.release() + + with sema: # must not trigger + pass + + bounded_sema = threading.BoundedSemaphore() + bounded_sema.acquire() # [consider-using-with] + bounded_sema.release() + + with bounded_sema: # must not trigger + pass + + +def test_multiprocessing(): + # the different Locks provided by multiprocessing would be candidates + # for consider-using-with as well, but they lead to InferenceErrors. + _ = multiprocessing.Pool() # [consider-using-with] + with multiprocessing.Pool(): + pass + + manager = multiprocessing.managers.BaseManager() + manager.start() # [consider-using-with] + with multiprocessing.managers.BaseManager(): + pass + + manager = multiprocessing.managers.SyncManager() + manager.start() # [consider-using-with] + with multiprocessing.managers.SyncManager(): + pass + + +def test_futures(): + _ = futures.ThreadPoolExecutor() # [consider-using-with] + with futures.ThreadPoolExecutor(): + pass + + _ = futures.ProcessPoolExecutor() # [consider-using-with] + with futures.ProcessPoolExecutor(): + pass + + +def test_popen(): + _ = subprocess.Popen("sh") # [consider-using-with] + with subprocess.Popen("sh"): + pass diff --git a/tests/functional/c/consider/consider_using_with.txt b/tests/functional/c/consider/consider_using_with.txt new file mode 100644 index 000000000..43ea7f752 --- /dev/null +++ b/tests/functional/c/consider/consider_using_with.txt @@ -0,0 +1,22 @@ +consider-using-with:14:4:test_codecs_open:Consider using 'with' for resource-allocating operations +consider-using-with:19:4:test_urlopen:Consider using 'with' for resource-allocating operations +consider-using-with:23:4:test_temporary_file:Consider using 'with' for resource-allocating operations +consider-using-with:27:4:test_named_temporary_file:Consider using 'with' for resource-allocating operations +consider-using-with:31:4:test_spooled_temporary_file:Consider using 'with' for resource-allocating operations +consider-using-with:35:4:test_temporary_directory:Consider using 'with' for resource-allocating operations +consider-using-with:39:4:test_zipfile:Consider using 'with' for resource-allocating operations +consider-using-with:40:4:test_zipfile:Consider using 'with' for resource-allocating operations +consider-using-with:44:4:test_pyzipfile:Consider using 'with' for resource-allocating operations +consider-using-with:49:4:test_pyzipfile:Consider using 'with' for resource-allocating operations +consider-using-with:56:4:test_tarfile:Consider using 'with' for resource-allocating operations +consider-using-with:62:4:test_tarfile:Consider using 'with' for resource-allocating operations +consider-using-with:71:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations +consider-using-with:78:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations +consider-using-with:93:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations +consider-using-with:100:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations +consider-using-with:110:4:test_multiprocessing:Consider using 'with' for resource-allocating operations +consider-using-with:115:4:test_multiprocessing:Consider using 'with' for resource-allocating operations +consider-using-with:120:4:test_multiprocessing:Consider using 'with' for resource-allocating operations +consider-using-with:126:4:test_futures:Consider using 'with' for resource-allocating operations +consider-using-with:130:4:test_futures:Consider using 'with' for resource-allocating operations +consider-using-with:136:4:test_popen:Consider using 'with' for resource-allocating operations diff --git a/tests/functional/c/consider/consider_using_with_open.py b/tests/functional/c/consider/consider_using_with_open.py new file mode 100644 index 000000000..6e7cb04bd --- /dev/null +++ b/tests/functional/c/consider/consider_using_with_open.py @@ -0,0 +1,15 @@ +# pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel +""" +The functional test for the standard ``open()`` function has to be moved in a separate file, +because PyPy has to be excluded for the tests as the ``open()`` function is uninferable in PyPy. +However, all remaining checks for consider-using-with work in PyPy, so we do not want to exclude +PyPy from ALL functional tests. +""" + + +def test_open(): + fh = open("test.txt") # [consider-using-with] + fh.close() + + with open("test.txt") as fh: # must not trigger + fh.read() diff --git a/tests/functional/c/consider/consider_using_with_open.rc b/tests/functional/c/consider/consider_using_with_open.rc new file mode 100644 index 000000000..b47a74525 --- /dev/null +++ b/tests/functional/c/consider/consider_using_with_open.rc @@ -0,0 +1,2 @@ +[testoptions] +except_implementations=PyPy diff --git a/tests/functional/c/consider/consider_using_with_open.txt b/tests/functional/c/consider/consider_using_with_open.txt new file mode 100644 index 000000000..8e7c03e39 --- /dev/null +++ b/tests/functional/c/consider/consider_using_with_open.txt @@ -0,0 +1 @@ +consider-using-with:11:4:test_open:Consider using 'with' for resource-allocating operations diff --git a/tests/functional/d/disabled_msgid_in_pylintrc.rc b/tests/functional/d/disabled_msgid_in_pylintrc.rc index 2964930df..6a584cb99 100644 --- a/tests/functional/d/disabled_msgid_in_pylintrc.rc +++ b/tests/functional/d/disabled_msgid_in_pylintrc.rc @@ -1,4 +1,4 @@ [MESSAGES CONTROL] disable= - C0111,C0326,W0703 + C0111,C0326,W0703,R1732 diff --git a/tests/functional/n/non/non_iterator_returned.py b/tests/functional/n/non/non_iterator_returned.py index 89bcdb7d9..94c3601f5 100644 --- a/tests/functional/n/non/non_iterator_returned.py +++ b/tests/functional/n/non/non_iterator_returned.py @@ -1,6 +1,7 @@ """Check non-iterators returned by __iter__ """ -# pylint: disable=too-few-public-methods, missing-docstring, no-self-use, useless-object-inheritance +# pylint: disable=too-few-public-methods, missing-docstring, no-self-use, useless-object-inheritance, consider-using-with + class FirstGoodIterator(object): """ yields in iterator. """ @@ -9,6 +10,7 @@ class FirstGoodIterator(object): for index in range(10): yield index + class SecondGoodIterator(object): """ __iter__ and next """ @@ -23,12 +25,14 @@ class SecondGoodIterator(object): """Same as __next__, but for Python 2.""" return 1 + class ThirdGoodIterator(object): """ Returns other iterator, not the current instance """ def __iter__(self): return SecondGoodIterator() + class FourthGoodIterator(object): """ __iter__ returns iter(...) """ @@ -50,9 +54,11 @@ class IteratorClass(object, metaclass=IteratorMetaclass): class FifthGoodIterator(object): """__iter__ returns a class which uses an iterator-metaclass.""" + def __iter__(self): return IteratorClass + class FileBasedIterator(object): def __init__(self, path): self.path = path @@ -70,23 +76,26 @@ class FileBasedIterator(object): class FirstBadIterator(object): """ __iter__ returns a list """ - def __iter__(self): # [non-iterator-returned] + def __iter__(self): # [non-iterator-returned] return [] + class SecondBadIterator(object): """ __iter__ without next """ - def __iter__(self): # [non-iterator-returned] + def __iter__(self): # [non-iterator-returned] return self + class ThirdBadIterator(object): """ __iter__ returns an instance of another non-iterator """ - def __iter__(self): # [non-iterator-returned] + def __iter__(self): # [non-iterator-returned] return SecondBadIterator() + class FourthBadIterator(object): """__iter__ returns a class.""" - def __iter__(self): # [non-iterator-returned] + def __iter__(self): # [non-iterator-returned] return ThirdBadIterator diff --git a/tests/functional/n/non/non_iterator_returned.txt b/tests/functional/n/non/non_iterator_returned.txt index ab9d69e7e..a78948236 100644 --- a/tests/functional/n/non/non_iterator_returned.txt +++ b/tests/functional/n/non/non_iterator_returned.txt @@ -1,4 +1,4 @@ -non-iterator-returned:73:4:FirstBadIterator.__iter__:__iter__ returns non-iterator -non-iterator-returned:79:4:SecondBadIterator.__iter__:__iter__ returns non-iterator -non-iterator-returned:85:4:ThirdBadIterator.__iter__:__iter__ returns non-iterator -non-iterator-returned:91:4:FourthBadIterator.__iter__:__iter__ returns non-iterator +non-iterator-returned:79:4:FirstBadIterator.__iter__:__iter__ returns non-iterator +non-iterator-returned:86:4:SecondBadIterator.__iter__:__iter__ returns non-iterator +non-iterator-returned:93:4:ThirdBadIterator.__iter__:__iter__ returns non-iterator +non-iterator-returned:100:4:FourthBadIterator.__iter__:__iter__ returns non-iterator -- cgit v1.2.1