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 <pierre.sassoulas@gmail.com>

18 files changed, 293 insertions, 37 deletions

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