diff --git a/doc/data/messages/c/contextmanager-generator-missing-cleanup/bad.py b/doc/data/messages/c/contextmanager-generator-missing-cleanup/bad.py new file mode 100644 index 0000000000..e65906a4fb --- /dev/null +++ b/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 diff --git a/doc/data/messages/c/contextmanager-generator-missing-cleanup/details.rst b/doc/data/messages/c/contextmanager-generator-missing-cleanup/details.rst new file mode 100644 index 0000000000..88860d279a --- /dev/null +++ b/doc/data/messages/c/contextmanager-generator-missing-cleanup/details.rst @@ -0,0 +1,10 @@ +Instantiating and using a contextmanager inside a generator function can +result in unexpected behavior if there is an expectation that the context is only +available for the generator function. In the case that the generator is not closed or destroyed +then the context manager is held suspended as is. + +This message warns on the generator function instead of the contextmanager function +because the ways to use a contextmanager are many. +A contextmanager can be used as a decorator (which immediately has ``__enter__``/``__exit__`` applied) +and the use of ``as ...`` or discard of the return value also implies whether the context needs cleanup or not. +So for this message, warning the invoker of the contextmanager is important. diff --git a/doc/data/messages/c/contextmanager-generator-missing-cleanup/good.py b/doc/data/messages/c/contextmanager-generator-missing-cleanup/good.py new file mode 100644 index 0000000000..406d984529 --- /dev/null +++ b/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 diff --git a/doc/data/messages/c/contextmanager-generator-missing-cleanup/related.rst b/doc/data/messages/c/contextmanager-generator-missing-cleanup/related.rst new file mode 100644 index 0000000000..aacc968cd5 --- /dev/null +++ b/doc/data/messages/c/contextmanager-generator-missing-cleanup/related.rst @@ -0,0 +1,2 @@ +- `Rationale `_ +- `CPython Issue `_ diff --git a/doc/user_guide/checkers/features.rst b/doc/user_guide/checkers/features.rst index 73b9bcdc0b..d07a2a0a29 100644 --- a/doc/user_guide/checkers/features.rst +++ b/doc/user_guide/checkers/features.rst @@ -166,6 +166,9 @@ Basic checker Messages This is a particular case of W0104 with its own message so you can easily disable it if you're using those strings as documentation, instead of comments. +:contextmanager-generator-missing-cleanup (W0135): *The context used in function %r will not be exited.* + Used when a contextmanager is used inside a generator function and the + cleanup is not handled. :unnecessary-pass (W0107): *Unnecessary pass statement* Used when a "pass" statement can be removed without affecting the behaviour of the code. diff --git a/doc/user_guide/messages/messages_overview.rst b/doc/user_guide/messages/messages_overview.rst index cd63d3f85c..a9ca2eb48c 100644 --- a/doc/user_guide/messages/messages_overview.rst +++ b/doc/user_guide/messages/messages_overview.rst @@ -227,6 +227,7 @@ All messages in the warning category: warning/comparison-with-callable warning/confusing-with-statement warning/consider-ternary-expression + warning/contextmanager-generator-missing-cleanup warning/dangerous-default-value warning/deprecated-argument warning/deprecated-class diff --git a/doc/whatsnew/fragments/2832.new_check b/doc/whatsnew/fragments/2832.new_check new file mode 100644 index 0000000000..64b38be2e9 --- /dev/null +++ b/doc/whatsnew/fragments/2832.new_check @@ -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 diff --git a/pylint/checkers/base/__init__.py b/pylint/checkers/base/__init__.py index c9067b405e..a3e6071c4c 100644 --- a/pylint/checkers/base/__init__.py +++ b/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)) diff --git a/pylint/checkers/base/function_checker.py b/pylint/checkers/base/function_checker.py new file mode 100644 index 0000000000..bf85747119 --- /dev/null +++ b/pylint/checkers/base/function_checker.py @@ -0,0 +1,135 @@ +# 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 context used in function %r will not be exited.", + "contextmanager-generator-missing-cleanup", + "Used when a contextmanager is used inside a generator function" + " 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 + with_nodes = list(node.nodes_of_class(nodes.With)) + if not with_nodes: + return + # check for Yield inside the With statement + 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.name,), + ) + + @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 any(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 diff --git a/tests/functional/c/consider/consider_using_with.py b/tests/functional/c/consider/consider_using_with.py index e8e1623374..9ff70e08b2 100644 --- a/tests/functional/c/consider/consider_using_with.py +++ b/tests/functional/c/consider/consider_using_with.py @@ -186,9 +186,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(): diff --git a/tests/functional/c/consider/consider_using_with.txt b/tests/functional/c/consider/consider_using_with.txt index 455762f57d..864a0784c1 100644 --- a/tests/functional/c/consider/consider_using_with.txt +++ b/tests/functional/c/consider/consider_using_with.txt @@ -20,9 +20,9 @@ consider-using-with:140:8:140:30:test_multiprocessing:Consider using 'with' for 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: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:210:4:210:26::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:216:4:216:26::Consider using 'with' for resource-allocating operations:UNDEFINED +consider-using-with:222:4:222:26::Consider using 'with' for resource-allocating operations:UNDEFINED +consider-using-with:238:18:238:40:test_subscript_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED +consider-using-with:240:24:240:46:test_subscript_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED diff --git a/tests/functional/c/consider/consider_using_with_open.py b/tests/functional/c/consider/consider_using_with_open.py index dd58426879..b76765cf89 100644 --- a/tests/functional/c/consider/consider_using_with_open.py +++ b/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. diff --git a/tests/functional/c/consider/consider_using_with_open.txt b/tests/functional/c/consider/consider_using_with_open.txt index 3819e266dd..57aaff736b 100644 --- a/tests/functional/c/consider/consider_using_with_open.txt +++ b/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:45:4:45:33:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED +consider-using-with:46:14:46:43:test_open_outside_assignment:Consider using 'with' for resource-allocating operations:UNDEFINED +consider-using-with:51:8:51:37:test_open_inside_with_block:Consider using 'with' for resource-allocating operations:UNDEFINED +consider-using-with:119:26:121:13:TestControlFlow.test_triggers_if_reassigned_after_if_else:Consider using 'with' for resource-allocating operations:UNDEFINED +used-before-assignment:140:12:140:23:TestControlFlow.test_defined_in_try_and_finally:Using variable 'file_handle' before assignment:CONTROL_FLOW diff --git a/tests/functional/c/contextmanager_generator_missing_cleanup.py b/tests/functional/c/contextmanager_generator_missing_cleanup.py new file mode 100644 index 0000000000..ff7f274e09 --- /dev/null +++ b/tests/functional/c/contextmanager_generator_missing_cleanup.py @@ -0,0 +1,177 @@ +# pylint: disable = missing-docstring, unused-variable, bare-except, broad-exception-caught +from collections import namedtuple +import contextlib +from contextlib import contextmanager + +# Positive + + +@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 + + +@contextmanager +def name_cm(): + contextvar = "acquired context" + print("cm enter") + yield contextvar + print("cm exit") + + +def genfunc_with_name_cm(): # [contextmanager-generator-missing-cleanup] + with name_cm() as context: + yield context * 2 + + +def genfunc_with_cm_after(): # [contextmanager-generator-missing-cleanup] + with after_cm() as context: + yield context * 2 + + +@contextlib.contextmanager +def after_cm(): + contextvar = "acquired context" + print("cm enter") + yield contextvar + print("cm exit") + + +@contextmanager +def cm_with_improper_handling(): + contextvar = "acquired context" + print("cm enter") + try: + yield contextvar + except ValueError: + pass + print("cm exit") + + +def genfunc_with_cm_improper(): # [contextmanager-generator-missing-cleanup] + with cm_with_improper_handling() as context: + yield context * 2 + + +# Negative + + +class Enterable: + def __enter__(self): + print("enter") + return self + + def __exit__(self, *args): + print("exit") + + +def genfunc_with_enterable(): + enter = Enterable() + with enter as context: + yield context * 2 + + +def genfunc_with_enterable_attr(): + EnterableTuple = namedtuple("EnterableTuple", ["attr"]) + t = EnterableTuple(Enterable()) + with t.attr as context: + yield context.attr * 2 + + +@contextlib.contextmanager +def good_cm_except(): + contextvar = "acquired context" + print("good cm enter") + try: + yield contextvar + except GeneratorExit: + print("good cm exit") + + +def good_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 "discarded" + + +@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 + + +def genfunc_with_cm_finally_odd_body(): + with good_cm_finally() as context: + if context: + yield context * 2 + else: + yield context * 3 + + +@cm_with_improper_handling +def genfunc_wrapped(): + yield "wrapped" + + +@contextmanager +def cm_bare_handler(): + contextvar = "acquired context" + print("cm enter") + try: + yield contextvar + except: + print("cm exit") + + +@contextmanager +def cm_base_exception_handler(): + contextvar = "acquired context" + print("cm enter") + try: + yield contextvar + except Exception: + print("cm exit") + + +def genfunc_with_cm_bare_handler(): + with cm_bare_handler() as context: + yield context * 2 + + +def genfunc_with_cm_base_exception_handler(): + with cm_base_exception_handler() as context: + yield context * 2 diff --git a/tests/functional/c/contextmanager_generator_missing_cleanup.txt b/tests/functional/c/contextmanager_generator_missing_cleanup.txt new file mode 100644 index 0000000000..ca18ed4d9a --- /dev/null +++ b/tests/functional/c/contextmanager_generator_missing_cleanup.txt @@ -0,0 +1,4 @@ +contextmanager-generator-missing-cleanup:17:0:17:19:genfunc_with_cm:The context used in function 'genfunc_with_cm' will not be exited.:UNDEFINED +contextmanager-generator-missing-cleanup:30:0:30:24:genfunc_with_name_cm:The context used in function 'genfunc_with_name_cm' will not be exited.:UNDEFINED +contextmanager-generator-missing-cleanup:35:0:35:25:genfunc_with_cm_after:The context used in function 'genfunc_with_cm_after' will not be exited.:UNDEFINED +contextmanager-generator-missing-cleanup:59:0:59:28:genfunc_with_cm_improper:The context used in function 'genfunc_with_cm_improper' will not be exited.:UNDEFINED diff --git a/tests/functional/m/missing/missing_kwoa.py b/tests/functional/m/missing/missing_kwoa.py index 15df710bfd..15943254c4 100644 --- a/tests/functional/m/missing/missing_kwoa.py +++ b/tests/functional/m/missing/missing_kwoa.py @@ -2,6 +2,7 @@ import contextlib import typing + def target(pos, *, keyword): return pos + keyword @@ -13,18 +14,19 @@ def forwarding_kwds(pos, **kwds): def forwarding_args(*args, keyword): target(*args, keyword=keyword) + def forwarding_conversion(*args, **kwargs): target(*args, **dict(kwargs)) def not_forwarding_kwargs(*args, **kwargs): - target(*args) # [missing-kwoa] + target(*args) # [missing-kwoa] target(1, keyword=2) PARAM = 1 -target(2, PARAM) # [too-many-function-args, missing-kwoa] +target(2, PARAM) # [too-many-function-args, missing-kwoa] def some_function(*, param): @@ -39,9 +41,8 @@ def other_function(**kwargs): class Parent: - @typing.overload - def __init__( self, *, first, second, third): + def __init__(self, *, first, second, third): pass @typing.overload @@ -53,22 +54,19 @@ def __init__(self, *, first): pass def __init__( - self, - *, - first, - second: typing.Optional[str] = None, - third: typing.Optional[str] = None): + self, + *, + first, + second: typing.Optional[str] = None, + third: typing.Optional[str] = None, + ): self._first = first self._second = second self._third = third class Child(Parent): - def __init__( - self, - *, - first, - second): + def __init__(self, *, first, second): super().__init__(first=first, second=second) self._first = first + second @@ -77,6 +75,7 @@ def __init__( def run(*, a): yield + def test_context_managers(**kw): run(**kw) @@ -89,4 +88,5 @@ def test_context_managers(**kw): with run(**kw), run(): # [missing-kwoa] pass + test_context_managers(a=1) diff --git a/tests/functional/m/missing/missing_kwoa.txt b/tests/functional/m/missing/missing_kwoa.txt index fc1694ed20..e31249cfb8 100644 --- a/tests/functional/m/missing/missing_kwoa.txt +++ b/tests/functional/m/missing/missing_kwoa.txt @@ -1,4 +1,4 @@ -missing-kwoa:21:4:21:17:not_forwarding_kwargs:Missing mandatory keyword argument 'keyword' in function call:INFERENCE -missing-kwoa:27:0:27:16::Missing mandatory keyword argument 'keyword' in function call:INFERENCE -too-many-function-args:27:0:27:16::Too many positional arguments for function call:UNDEFINED -missing-kwoa:89:20:89:25:test_context_managers:Missing mandatory keyword argument 'a' in function call:INFERENCE +missing-kwoa:23:4:23:17:not_forwarding_kwargs:Missing mandatory keyword argument 'keyword' in function call:INFERENCE +missing-kwoa:29:0:29:16::Missing mandatory keyword argument 'keyword' in function call:INFERENCE +too-many-function-args:29:0:29:16::Too many positional arguments for function call:UNDEFINED +missing-kwoa:88:20:88:25:test_context_managers:Missing mandatory keyword argument 'a' in function call:INFERENCE