-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Warn about @contextlib.contextmanager without try/finally in generator functions #2832
Comments
Thank you @apnewberry This makes sense. |
Perhaps optionally a stricter version of this check: any @contextlib.contextmanager
def f():
...
try:
yield
finally:
... regardless of whether it's used in a generator, since the client code that uses the context manager in a generator may not be using pylint, and the context manager should provide the right behavior for such callers. |
If there is desire to get this moving, I am available and just have some clarifying questions. Same code as supplied by issue opener yields the following in Python 3.11. $ python3 --version
Python 3.11.5
$ python3 test.py
cm enter
stepping
<weakref at 0x103076f70; dead> Any guesses on the error name to use? And I'm guessing this should be added into 'extensions' for a bad builtin? Finally, it seems like we should only raise the error if a |
Thank you for specifying the design, clearly this one was not PR ready. And thank you for contributing to pylint :) Regarding the naming, we have a message named Admittedly the message is hard to name succinctly (there's generator, context manager and unhandled exception, 5 concepts when we generally have 4 words messages names or less) but maybe one of the following ?
This can be changed with a search&replace before merging if someone has a great name. About putting it in an extension, I'm not sure. Code that is implicitly not run is at least warning level, and warnings are generally valuable enough to be put in the basic checker. (Similar to return-in-finally for example). This can also changed easily, please create a new file like https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/base/pass_checker.py so it can be moved easily if we simply change
And I think it should also contains a yield (is a generator) |
I like the sound of
This sounds good
Yep that is my bad for typo - will see what I can get going on a first pass. |
@Pierre-Sassoulas I think this works for the given cases, but there are examples where it raises the error when I think it should probably not. Example: import contextlib
@contextlib.contextmanager
def test(): # [contextmanager-generator-missing-cleanup]
yield 2 In simple functions where there is no cleanup, this checker will still raise the Error. But want to check in and make sure the direction is right. |
I haven't seen this issue before, but I agree. This code is fine and I don't really see what this message is trying to achieve. The original code is just wrong: from __future__ import generator_stop
import contextlib
import sys
import weakref
@contextlib.contextmanager
def cm():
print("cm enter")
a = yield
print("cm exit")
def genfunc():
with cm():
print("stepping")
yield
def main():
gen = genfunc()
ref = weakref.ref(gen, print)
list(iter(gen))
if __name__ == "__main__":
main() Changing the |
Do you think there should be a warning for this at all then? Unclear why OP necessarily needed their code to look like that, but is it good to still include this error as a catch for people who might not know of this side effect? I could see myself writing something similar to this using a DBAPI connection and cursor if I tried. I think there is value in warning for original case |
I don't think it is feasible to warn for the original code without creating too many false positives. |
My thinking would be that a context manager decorated generator function that has cleanup code may or may not have cleanup properly executed. The problem for me is that the distinguishing is as you said, whether the user of the function knows which iterator consumables to use. |
Apparently, even if the suggestion of the proposed checker is implemented, you still will have "cleanup code" executed at the wrong time if you use the context manager as a decorator instead of as a This suggests to me that "cleanup code" in a context manager that needs to consider this If we want to continue with this check, maybe we can limit it to just:
That will hopefully reduce the messages detected in the draft PR primer run. @rhyn0 what do you think? |
A python discussion about how this (yielding out of a context manager managing a resource) is a bad pattern, and could possibly lead to a PEP at some point, but in the meantime, could a static checker help out? So I think doing something here would be nice. cc/ @DanielNoord |
After reading the linked discussion about Trio's use case, I see a better direction for this warning. Having warnings on the generator, when it yields non constant values, is probably best path forward. If we want to incorporate checks for code usage of these generators at |
Okay, let's move forward with the more specific checks! |
@jacobtylerwalls @DanielNoord pushed some changes but wanted to come back to this to make sure the Good/Bad examples make sense as to what we laid out. # good.py
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 "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 # bad.py
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 I think this hits all the bullet points we said earlier, but if there are any misses on this please let me know |
Overall looks good. I'd add some good cases with decorators e.g. I think the one question I have is about the difference between |
Ah, I see in the PR are you allowing yielding constants (as opposed to names) in addition to None as legal yields. So looking good, I'd just add a test case to good.py for it. |
If I have code that is decorated with This check seems to indicate that I should explicitly wrap it in try/finally with cleanup code consisting of "pass"? |
In #9625 we're discussing the case you bring up, where there is no code after yield. |
Is your feature request related to a problem?
If you decorate a generator function with
@contextlib.contextmanager
and use the context managercm
inside a generator function, the context manager's__exit__
will not be always be called because the generator raisesGeneratorExit
, which ends the generator that definescm
.Notice that
cm exit
never appears, ascm
's__exit__
is never called.Output:
Describe the solution you'd like
Generator functions decorated with
@contextlib.contextmanager
should not be used in generators unless the context manager hasfinally:
or specifically handles
GeneratorExit
:Additional context
Add any other context about the feature request here.
The text was updated successfully, but these errors were encountered: