[contextmanager-generator-missing-cleanup][new feature] Warn about @contextlib.contextmanager without try/finally in generator functions

doc/data/messages/c/contextmanager-generator-missing-cleanup/bad.py

@@ -0,0 +1,14 @@
+import contextlib
+
+
+@contextlib.contextmanager
+def cm():
+    contextvar = "acquired context"
+    print("cm enter")
+    yield contextvar
+    print("cm exit")
+
+
+def genfunc_with_cm():  # [contextmanager-generator-missing-cleanup]
+    with cm() as context:
+        yield context * 2

doc/data/messages/c/contextmanager-generator-missing-cleanup/good.py

@@ -0,0 +1,49 @@
+import contextlib
+
+
+@contextlib.contextmanager
+def good_cm_except():
+    contextvar = "acquired context"
+    print("good cm enter")
+    try:
+        yield contextvar
+    except GeneratorExit:
+        print("good cm exit")
+
+
+def genfunc_with_cm():
+    with good_cm_except() as context:
+        yield context * 2
+
+
+def genfunc_with_discard():
+    with good_cm_except():
+        yield "discarded"
+
+
+@contextlib.contextmanager
+def good_cm_yield_none():
+    print("good cm enter")
+    yield
+    print("good cm exit")
+
+
+def genfunc_with_none_yield():
+    with good_cm_yield_none() as var:
+        print(var)
+        yield "constant yield"
+
+
+@contextlib.contextmanager
+def good_cm_finally():
+    contextvar = "acquired context"
+    print("good cm enter")
+    try:
+        yield contextvar
+    finally:
+        print("good cm exit")
+
+
+def good_cm_finally_genfunc():
+    with good_cm_finally() as context:
+        yield context * 2

doc/whatsnew/fragments/2832.feature

@@ -0,0 +1,6 @@
+Checks for generators that use contextmanagers that don't handle cleanup properly.
+Is meant to raise visibilty on the case that a generator is not fully exhausted and the contextmanager is not cleaned up properly.
+A contextmanager must yield a non constant value and not handle cleanup for GeneratorExit.
+The using generator must attempt to use the yielded context value `with x() as y` and not just `with x()`.
+
+Closes #2832

pylint/checkers/base/__init__.py

@@ -23,6 +23,7 @@
 from pylint.checkers.base.basic_error_checker import BasicErrorChecker
 from pylint.checkers.base.comparison_checker import ComparisonChecker
 from pylint.checkers.base.docstring_checker import DocStringChecker
+from pylint.checkers.base.function_checker import FunctionChecker
 from pylint.checkers.base.name_checker import (
     KNOWN_NAME_TYPES_WITH_STYLE,
     AnyStyle,
@@ -46,3 +47,4 @@ def register(linter: PyLinter) -> None:
     linter.register_checker(DocStringChecker(linter))
     linter.register_checker(PassChecker(linter))
     linter.register_checker(ComparisonChecker(linter))
+    linter.register_checker(FunctionChecker(linter))

pylint/checkers/base/function_checker.py

@@ -0,0 +1,137 @@
+# Licensed under the GPL: https://www.gnu.org/licenses/old-licenses/gpl-2.0.html
+# For details: https://github.com/pylint-dev/pylint/blob/main/LICENSE
+# Copyright (c) https://github.com/pylint-dev/pylint/blob/main/CONTRIBUTORS.txt
+
+"""Function checker for Python code."""
+
+from __future__ import annotations
+
+from itertools import chain
+
+from astroid import nodes
+
+from pylint.checkers import utils
+from pylint.checkers.base.basic_checker import _BasicChecker
+
+
+class FunctionChecker(_BasicChecker):
+    """Check if a function definition handles possible side effects."""
+
+    msgs = {
+        "W0135": (
+            "The code after line %s will never be executed ('GeneratorExit' needs to be handled)",
+            "contextmanager-generator-missing-cleanup",
+            "Used when a generator is used in a contextmanager"
+            " and the cleanup is not handled.",
+        )
+    }
+
+    @utils.only_required_for_messages("contextmanager-generator-missing-cleanup")
+    def visit_functiondef(self, node: nodes.FunctionDef) -> None:
+        self._check_contextmanager_generator_missing_cleanup(node)
+
+    @utils.only_required_for_messages("contextmanager-generator-missing-cleanup")
+    def visit_asyncfunctiondef(self, node: nodes.AsyncFunctionDef) -> None:
+        self._check_contextmanager_generator_missing_cleanup(node)
+
+    def _check_contextmanager_generator_missing_cleanup(
+        self, node: nodes.FunctionDef
+    ) -> None:
+        """Check a FunctionDef to find if it is a generator
+        that uses a contextmanager internally.
+
+        If it is, check if the contextmanager is properly cleaned up. Otherwise, add message.
+
+        :param node: FunctionDef node to check
+        :type node: nodes.FunctionDef
+        """
+        # if function does not use a Yield statement, it cant be a generator
+        # TODO: AsyncWith also captured here?
+        with_nodes = list(node.nodes_of_class(nodes.With))
+        if not with_nodes:
+            return
+        # check for Yield inside the With statement
+        # TODO: should this also capture YieldFrom?
+        yield_nodes = list(
+            chain.from_iterable(
+                with_node.nodes_of_class(nodes.Yield) for with_node in with_nodes
+            )
+        )
+        if not yield_nodes:
+            return
+
+        # infer the call that yields a value, and check if it is a contextmanager
+        for with_node in with_nodes:
+            for call, held in with_node.items:
+                if held is None:
+                    # if we discard the value, then we can skip checking it
+                    continue
+
+                # safe infer is a generator
+                inferred_node = getattr(utils.safe_infer(call), "parent", None)
+                if not isinstance(inferred_node, nodes.FunctionDef):
+                    continue
+                if self._node_fails_contextmanager_cleanup(inferred_node, yield_nodes):
+                    self.add_message(
+                        "contextmanager-generator-missing-cleanup",
+                        node=node,
+                        args=(node.lineno,),
+                    )
+
+    @staticmethod
+    def _node_fails_contextmanager_cleanup(
+        node: nodes.FunctionDef, yield_nodes: list[nodes.Yield]
+    ) -> bool:
+        """Check if a node fails contextmanager cleanup.
+
+        Current checks for a contextmanager:
+            - only if the context manager yields a non-constant value
+            - only if the context manager lacks a finally, or does not catch GeneratorExit
+
+        :param node: Node to check
+        :type node: nodes.FunctionDef
+        :return: True if fails, False otherwise
+        :param yield_nodes: List of Yield nodes in the function body
+        :type yield_nodes: list[nodes.Yield]
+        :rtype: bool
+        """
+
+        def check_handles_generator_exceptions(try_node: nodes.Try) -> bool:
+            # needs to handle either GeneratorExit, Exception, or bare except
+            for handler in try_node.handlers:
+                if handler.type is None:
+                    # handles all exceptions (bare except)
+                    return True
+                inferred = utils.safe_infer(handler.type)
+                if inferred and inferred.qname() in {
+                    "builtins.GeneratorExit",
+                    "builtins.Exception",
+                }:
+                    return True
+            return False
+
+        # if context manager yields a non-constant value, then continue checking
+        if any(
+            yield_node.value is None or isinstance(yield_node.value, nodes.Const)
+            for yield_node in yield_nodes
+        ):
+            return False
+        # if function body has multiple Try, filter down to the ones that have a yield node
+        try_with_yield_nodes = [
+            try_node
+            for try_node in node.nodes_of_class(nodes.Try)
+            if list(try_node.nodes_of_class(nodes.Yield))
+        ]
+        if not try_with_yield_nodes:
+            # no try blocks at all, so checks after this line do not apply
+            return True
+        # if the contextmanager has a finally block, then it is fine
+        if all(try_node.finalbody for try_node in try_with_yield_nodes):
+            return False
+        # if the contextmanager catches GeneratorExit, then it is fine
+        if all(
+            check_handles_generator_exceptions(try_node)
+            for try_node in try_with_yield_nodes
+        ):
+            return False
+        return True

tests/functional/c/consider/consider_using_with.py

@@ -1,4 +1,5 @@
 # pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel
+# pylint: disable=contextmanager-generator-missing-cleanup
 import codecs
 import contextlib
 import multiprocessing
@@ -186,9 +187,7 @@ def test_futures():
     pass
 
 
-global_pool = (
-    multiprocessing.Pool()
-)  # must not trigger, will be used in nested scope
+global_pool = multiprocessing.Pool()  # must not trigger, will be used in nested scope
 
 
 def my_nested_function():

tests/functional/c/consider/consider_using_with.txt

@@ -1,28 +1,28 @@
-consider-using-with:17:8:17:49:test_pathlib_open:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:18:8:18:41:test_pathlib_open:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:20:8:20:34:test_pathlib_open:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:26:9:26:40:test_codecs_open:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:31:8:31:55:test_urlopen:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:39:8:39:40:test_named_temporary_file:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:43:8:43:42:test_spooled_temporary_file:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:47:8:47:37:test_temporary_directory:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:51:12:51:44:test_zipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:52:8:52:30:test_zipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:56:12:56:46:test_pyzipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:61:8:61:30:test_pyzipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:68:9:68:43:test_tarfile:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:74:9:74:47:test_tarfile:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:83:4:83:18:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:90:4:90:19:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:97:4:97:18:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:104:4:104:26:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:140:8:140:30:test_multiprocessing:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:145:4:145:19:test_multiprocessing:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:150:4:150:19:test_multiprocessing:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:156:8:156:30:test_popen:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:18:8:18:49:test_pathlib_open:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:19:8:19:41:test_pathlib_open:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:21:8:21:34:test_pathlib_open:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:27:9:27:40:test_codecs_open:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:32:8:32:55:test_urlopen:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:40:8:40:40:test_named_temporary_file:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:44:8:44:42:test_spooled_temporary_file:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:48:8:48:37:test_temporary_directory:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:52:12:52:44:test_zipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:53:8:53:30:test_zipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:57:12:57:46:test_pyzipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:62:8:62:30:test_pyzipfile:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:69:9:69:43:test_tarfile:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:75:9:75:47:test_tarfile:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:84:4:84:18:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:91:4:91:19:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:98:4:98:18:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:105:4:105:26:test_lock_acquisition:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:141:8:141:30:test_multiprocessing:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:146:4:146:19:test_multiprocessing:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:151:4:151:19:test_multiprocessing:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:157:8:157:30:test_popen:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:211:4:211:26::Consider using 'with' for resource-allocating operations:UNDEFINED
 consider-using-with:212:4:212:26::Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:213:4:213:26::Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:218:4:218:26::Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:224:4:224:26::Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:240:18:240:40:test_subscript_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:242:24:242:46:test_subscript_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:217:4:217:26::Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:223:4:223:26::Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:239:18:239:40:test_subscript_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:241:24:241:46:test_subscript_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED

tests/functional/c/consider/consider_using_with_open.py

@@ -1,5 +1,6 @@
 # pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel
 # pylint: disable=missing-class-docstring, too-few-public-methods, unused-variable, multiple-statements, line-too-long
+# pylint: disable=contextmanager-generator-missing-cleanup
 """
 Previously, open was uninferable on PyPy so we moved all functional tests
 to a separate file. This is no longer the case but the files remain split.
@@ -26,7 +27,9 @@ def __init__(self):
             self.file_handle = None
 
         def __enter__(self):
-            self.file_handle = open("foo.txt", "w", encoding="utf-8")  # must not trigger
+            self.file_handle = open(
+                "foo.txt", "w", encoding="utf-8"
+            )  # must not trigger
 
         def __exit__(self, exc_type, exc_value, traceback):
             self.file_handle.close()
@@ -52,17 +55,22 @@ def test_open_inside_with_block():
 
 def test_ternary_if_in_with_block(file1, file2, which):
     """Regression test for issue #4676 (false positive)"""
-    with (open(file1, encoding="utf-8") if which else open(file2, encoding="utf-8")) as input_file:  # must not trigger
+    with (
+        open(file1, encoding="utf-8") if which else open(file2, encoding="utf-8")
+    ) as input_file:  # must not trigger
         return input_file.read()
 
 
 def test_single_line_with(file1):
-    with open(file1, encoding="utf-8"): return file1.read()  # must not trigger
+    with open(file1, encoding="utf-8"):
+        return file1.read()  # must not trigger
 
 
 def test_multiline_with_items(file1, file2, which):
-    with (open(file1, encoding="utf-8") if which
-        else open(file2, encoding="utf-8")) as input_file: return input_file.read()
+    with (
+        open(file1, encoding="utf-8") if which else open(file2, encoding="utf-8")
+    ) as input_file:
+        return input_file.read()
 
 
 def test_suppress_on_return():
@@ -115,9 +123,7 @@ 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 = open("bar", encoding="utf8")  # [consider-using-with]
         file_handle = None
         return file_handle
 
@@ -136,13 +142,15 @@ def test_defined_in_try_and_finally(self):
             Path("foo").touch()
         finally:
             # +1: [used-before-assignment]
-            file_handle.open("foo", encoding="utf")  # must not trigger consider-using-with
+            file_handle.open(
+                "foo", encoding="utf"
+            )  # must not trigger consider-using-with
         with file_handle:
             return file_handle.read()
 
     def test_defined_in_different_except_handlers(self, a, b):
         try:
-            result = a/b
+            result = a / b
         except ZeroDivisionError:
             logfile = open("math_errors.txt", encoding="utf8")  # must not trigger
             result = "Can't divide by zero"

tests/functional/c/consider/consider_using_with_open.txt

@@ -1,7 +1,7 @@
-consider-using-with:10:9:10:43::Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:14:9:14:43:test_open:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:44:4:44:33:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:45:14:45:43:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:50:8:50:37:test_open_inside_with_block:Consider using 'with' for resource-allocating operations:UNDEFINED
-consider-using-with:118:26:120:13:TestControlFlow.test_triggers_if_reassigned_after_if_else:Consider using 'with' for resource-allocating operations:UNDEFINED
-used-before-assignment:139:12:139:23:TestControlFlow.test_defined_in_try_and_finally:Using variable 'file_handle' before assignment:CONTROL_FLOW
+consider-using-with:11:9:11:43::Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:15:9:15:43:test_open:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:47:4:47:33:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:48:14:48:43:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:53:8:53:37:test_open_inside_with_block:Consider using 'with' for resource-allocating operations:UNDEFINED
+consider-using-with:126:26:126:54:TestControlFlow.test_triggers_if_reassigned_after_if_else:Consider using 'with' for resource-allocating operations:UNDEFINED
+used-before-assignment:145:12:145:23:TestControlFlow.test_defined_in_try_and_finally:Using variable 'file_handle' before assignment:CONTROL_FLOW