Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 32 commits into from
May 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
062373e
add: initial layout for new error
rhyn0 Oct 8, 2023
b6774f6
add: test cases for new checker
rhyn0 Oct 8, 2023
b0547eb
add: first pass at new checker
rhyn0 Oct 8, 2023
72068a2
add: towncrier fragment for changes
rhyn0 Oct 8, 2023
81c7ab6
add: suggestions, better logic for this warning
rhyn0 Oct 14, 2023
9be51c2
add: good and bad example under doc
rhyn0 Oct 14, 2023
314158b
update: functional tests to use new message
rhyn0 Oct 14, 2023
74841ab
update: other tests to exclude new warning
rhyn0 Oct 14, 2023
f0d9fad
change: keep new checker name under 'basic'
rhyn0 Oct 14, 2023
06204d7
change: update positive/negative examples to new spec for this feature
rhyn0 Mar 3, 2024
9928473
Fix other pylint rule violations in test data, update expected output
rhyn0 Mar 3, 2024
6c95035
change: new checker logic for refined 'contextmanager_generator_missi…
rhyn0 Mar 3, 2024
eef0ac0
update: test files with unused suppress flags, add suppress flags
rhyn0 Mar 3, 2024
8022d8d
fix: CI docstring spelling errors
rhyn0 Mar 3, 2024
3ec527d
add: test cases for code coverage, order of functions and various Wit…
rhyn0 Mar 3, 2024
f1b6eb9
disable wrong spelling for AsyncWith
rhyn0 Mar 4, 2024
7f3874d
add more test cases from Primer errors
rhyn0 Mar 4, 2024
eb40479
remove 'spelling mistakes' sections, handle Primer errors
rhyn0 Mar 4, 2024
1558bcd
remove unused suppressions from 'eef0ac00d'
rhyn0 Mar 4, 2024
4ac898a
add negative test case for decorated usage of contextmanager
rhyn0 Mar 24, 2024
5c50505
rework checker to use simpler logic from utils.safe_infer
rhyn0 Mar 24, 2024
de54fc4
reword feature fragment, explain purpose more in depth
rhyn0 Mar 24, 2024
f3f0876
fix comment in good examples
rhyn0 Mar 24, 2024
3c365ce
use inference to check for handler of GeneratorExit
rhyn0 Mar 24, 2024
8730bc7
don't raise for contextmanagers that handle base Exception or bare Ex…
rhyn0 Mar 24, 2024
1f3cb77
reword top level error message
rhyn0 May 5, 2024
03c5562
remove unneccessary disable comment, update expected output
rhyn0 May 5, 2024
d87ebac
add details and related links to error message documentation
rhyn0 May 5, 2024
4d36753
remove TODO from checker
rhyn0 May 5, 2024
db8db8f
revert test breaking formatting changes
rhyn0 May 11, 2024
b89e7df
update: documentation about new checker, grammar
rhyn0 May 11, 2024
3bd7132
fix: make checking for generators with try blocks cheaper
rhyn0 May 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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]
rhyn0 marked this conversation as resolved.
Show resolved Hide resolved
with cm() as context:
yield context * 2
Original file line number Diff line number Diff line change
@@ -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.
rhyn0 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- `Rationale <https://discuss.python.org/t/preventing-yield-inside-certain-context-managers/1091>`_
- `CPython Issue <https://github.com/python/cpython/issues/81924#issuecomment-1093830682>`_
3 changes: 3 additions & 0 deletions doc/user_guide/checkers/features.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions doc/user_guide/messages/messages_overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions doc/whatsnew/fragments/2832.new_check
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions pylint/checkers/base/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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))
135 changes: 135 additions & 0 deletions pylint/checkers/base/function_checker.py
Original file line number Diff line number Diff line change
@@ -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(
rhyn0 marked this conversation as resolved.
Show resolved Hide resolved
check_handles_generator_exceptions(try_node)
for try_node in try_with_yield_nodes
):
return False
return True
4 changes: 1 addition & 3 deletions tests/functional/c/consider/consider_using_with.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down
12 changes: 6 additions & 6 deletions tests/functional/c/consider/consider_using_with.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions tests/functional/c/consider/consider_using_with_open.py
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
14 changes: 7 additions & 7 deletions tests/functional/c/consider/consider_using_with_open.txt
Original file line number Diff line number Diff line change
@@ -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
Loading
Loading