Issue 4430 false positive consider-using-with R1732 (#4453)

* Suppress consider-using-with if used inside context manager

* Added ChangeLog entry

Author: DudeNr33

ChangeLog

@@ -11,6 +11,10 @@ Release date: 2021-04-26
 ..
   Put new features and bugfixes here and also in 'doc/whatsnew/2.9.rst'
 
+* Suppress ``consider-using-with`` inside context managers.
+
+  Closes #4430
+
 * Added ``--fail-on`` option to return non-zero exit codes regardless of ``--fail-under`` value.
 
 * numversion tuple contains integers again to fix multiple pylint's plugins that relied on it

pylint/checkers/refactoring/refactoring_checker.py

@@ -103,6 +103,17 @@ def get_curline_index_start():
     return False
 
 
+def _is_inside_context_manager(node):
+    frame = node.frame()
+    if not isinstance(
+        frame, (astroid.FunctionDef, astroid.BoundMethod, astroid.UnboundMethod)
+    ):
+        return False
+    return frame.name == "__enter__" or utils.decorated_with(
+        frame, "contextlib.contextmanager"
+    )
+
+
 class RefactoringChecker(checkers.BaseTokenChecker):
     """Looks for code which can be refactored
 
@@ -1272,12 +1283,20 @@ 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:
+            if (
+                inferred
+                and inferred.qname() in CALLS_RETURNING_CONTEXT_MANAGERS
+                and not _is_inside_context_manager(node)
+            ):
                 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:
+        if (
+            inferred
+            and inferred.qname() in CALLS_THAT_COULD_BE_REPLACED_BY_WITH
+            and not _is_inside_context_manager(node)
+        ):
             self.add_message("consider-using-with", node=node)
 
     def _check_consider_using_join(self, aug_assign):

tests/functional/c/consider/consider_using_with.py

@@ -1,5 +1,6 @@
 # pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel
 import codecs
+import contextlib
 import multiprocessing
 import subprocess
 import tarfile
@@ -81,14 +82,6 @@ def test_lock_acquisition():
     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()
@@ -104,6 +97,31 @@ def test_lock_acquisition():
         pass
 
 
+@contextlib.contextmanager
+def test_lock_acquisition_in_context_manager1():
+    """
+    The message must not be triggered if the resource allocation is done inside a context manager.
+    """
+    lock = threading.Lock()
+    lock.acquire()  # must not trigger
+    yield
+    lock.release()
+
+
+class MyLockContext:
+    """
+    The message must not be triggered if the resource allocation is done inside a context manager.
+    """
+    def __init__(self):
+        self.lock = threading.Lock()
+
+    def __enter__(self):
+        self.lock.acquire()  # must not trigger
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.lock.release()
+
+
 def test_multiprocessing():
     # the different Locks provided by multiprocessing would be candidates
     # for consider-using-with as well, but they lead to InferenceErrors.

tests/functional/c/consider/consider_using_with.txt

@@ -1,22 +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:15:4:test_codecs_open:Consider using 'with' for resource-allocating operations
+consider-using-with:20:4:test_urlopen:Consider using 'with' for resource-allocating operations
+consider-using-with:24:4:test_temporary_file:Consider using 'with' for resource-allocating operations
+consider-using-with:28:4:test_named_temporary_file:Consider using 'with' for resource-allocating operations
+consider-using-with:32:4:test_spooled_temporary_file:Consider using 'with' for resource-allocating operations
+consider-using-with:36:4:test_temporary_directory: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:41:4:test_zipfile:Consider using 'with' for resource-allocating operations
+consider-using-with:45:4:test_pyzipfile:Consider using 'with' for resource-allocating operations
+consider-using-with:50:4:test_pyzipfile:Consider using 'with' for resource-allocating operations
+consider-using-with:57:4:test_tarfile:Consider using 'with' for resource-allocating operations
+consider-using-with:63:4:test_tarfile:Consider using 'with' for resource-allocating operations
+consider-using-with:72:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations
+consider-using-with:79:4:test_lock_acquisition:Consider using 'with' for resource-allocating operations
+consider-using-with:86: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
+consider-using-with:128:4:test_multiprocessing:Consider using 'with' for resource-allocating operations
+consider-using-with:133:4:test_multiprocessing:Consider using 'with' for resource-allocating operations
+consider-using-with:138:4:test_multiprocessing:Consider using 'with' for resource-allocating operations
+consider-using-with:144:4:test_futures:Consider using 'with' for resource-allocating operations
+consider-using-with:148:4:test_futures:Consider using 'with' for resource-allocating operations
+consider-using-with:154:4:test_popen:Consider using 'with' for resource-allocating operations

tests/functional/c/consider/consider_using_with_open.py

@@ -1,10 +1,15 @@
 # pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel
+# pylint: disable=missing-class-docstring, too-few-public-methods, unused-variable
 """
 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.
 """
+from contextlib import contextmanager
+
+
+myfile = open("test.txt")  # [consider-using-with]
 
 
 def test_open():
@@ -13,3 +18,24 @@ def test_open():
 
     with open("test.txt") as fh:  # must not trigger
         fh.read()
+
+
+def test_open_in_enter():
+    """Message must not trigger if the resource is allocated in a context manager."""
+    class MyContextManager:
+        def __init__(self):
+            self.file_handle = None
+
+        def __enter__(self):
+            self.file_handle = open("foo.txt", "w")  # must not trigger
+
+        def __exit__(self, exc_type, exc_value, traceback):
+            self.file_handle.close()
+
+
+@contextmanager
+def test_open_in_with_contextlib():
+    """Message must not trigger if the resource is allocated in a context manager."""
+    file_handle = open("foo.txt", "w")  # must not trigger
+    yield file_handle
+    file_handle.close()

tests/functional/c/consider/consider_using_with_open.txt

@@ -1 +1,2 @@
-consider-using-with:11:4:test_open:Consider using 'with' for resource-allocating operations
+consider-using-with:12:0::Consider using 'with' for resource-allocating operations
+consider-using-with:16:4:test_open:Consider using 'with' for resource-allocating operations