False Positive on contextmanager-generator-missing-cleanup / W0135 When Using With Expression in the ContextManager

[ancantus]

Bug description

The contextmanager-generator-missing-cleanup warning triggers a false positive (by my understanding of the warning) when a yield expression is fully cleaned up by a surrounding with statement (or multiple with statements).

Below is a simplified example of the issue:

import contextlib


@contextlib.contextmanager
def cm():
    with open("/tmp/test", "wb+") as contextvar:
        yield contextvar.fileno()


def genfunc_with_cm():  # [contextmanager-generator-missing-cleanup]
    with cm() as context:
        yield context * 2

The warning can be silenced with the guidance from https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/contextmanager-generator-missing-cleanup.html . But by my understanding this is an unnecessary warning in this case. The with statement in cm() does the 'right thing' when yield throws GeneratorExit and closes the file.

Configuration

No response

Command used

pylint ./test.py

Pylint output

************* Module test
W  test.py:10: The context used in function 'genfunc_with_cm' will not be exited. (contextmanager-generator-missing-cleanup | W0135)

Expected behavior

I would expect the contextmanager-generator-missing-cleanup to only warn when there is extra cleanup required in a contextmanager outside of with statements.

I think you see the intention of these cases being supported in the good_cm_yield_none() example in https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/contextmanager-generator-missing-cleanup.html

If there's nothing to clean up: the warning should not be generated.

Pylint version

pylint 3.2.0
astroid 3.2.0
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0]

OS / Environment

Ubuntu 22.04 LTS

Additional dependencies

No response

[DanielNoord]

Yes I agree, this seems like a false positive. Opinions @Pierre-Sassoulas @rhyn0?

[rhyn0]

Yah that seems correct.

Wondering if the way to check for this is by recursively tracing with calls and then we just have a white list of safe context managers such as open. Have to imagine we could have a context manager function using another and so on so forth

Worried about how a poorly written opener could break a whitelist containing open but that is probably a rare edge case compared to the example above.

[Pierre-Sassoulas]

Worried about how a poorly written opener could break a whitelist containing open but that is probably a rare edge case compared to the example above.

We have astroid inference to check that it's an actual open and not something else. We generally don't because shadowing builtin is really bad practice in general and the perf impact to everyone to prevent an edge case that should never happen is not negligible.

[cdce8p]

My current understanding is that as long as there is no additional code after the yield, it's fine. Should be as simple as checking that the yield statement is the last on in the current block and recursively upwards until the current scope is reached.

Am I missing something?

[ancantus]

that as long as there is no additional code after the yield, it's fine

This is my mental model of the situation as well. What orginally triggered this in my code was not the open() call: but a custom written context manager. The example with open() is just the smallest reproducable example I could come up with.

[eli-schwartz]

Similar case: If I have code that is decorated with @contextmanager, creates with TemporaryDirectory() and does some prep work, then exits the function by yielding a file in the temporary directory -- there is no expected code to run after yield but I do want the parent tempdir cleanup.

Explicitly wrapping it in try/finally with cleanup code consisting of "pass", feels wrong because that's just a pinky promise that cleanup is done.

mesonbuild/compilers/compilers.py:858:4: W0135: The context used in function 'cached_compile' will not be exited. (contextmanager-generator-missing-cleanup)

https://github.com/mesonbuild/meson/blob/128f0e828e425793203f3112c23cb31f959b4b3e/mesonbuild/compilers/compilers.py#L862-L874

[cdce8p]

Opened #9654 which should at least address the most common false-positive case with a singular yield.

@eli-schwartz I looked at your example. The fix won't work there unfortunately. Checking all if branches would add additional complexity. It's probably simpler to add a pylint: disable in that case.

[eli-schwartz]

@eli-schwartz I looked at your example. The fix won't work there unfortunately. Checking all if branches would add additional complexity. It's probably simpler to add a pylint: disable in that case.

Thanks for looking. I ended up adding it to pylintrc instead.