[contextmanager-generator-missing-cleanup][new feature] Warn about @contextlib.contextmanager without try/finally in generator functions #9133
Conversation
created new _BasicChecker inheriting class registered this new linter like the rest in __init__.py
original example case included a slightly modified positive case 2 negative cases where exception is handled properly
Checks that contextmanager decorated functions that are generators handle GeneratorExit cleanly
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thank you for your contribution, a lot of work went into this clearly. Would you mind adding a good/bad example in https://github.com/pylint-dev/pylint/tree/main/doc/data/messages too ? It's not enforced yet but we're close to being fully documented :)
Changed the detection logic of a bad generator function to test whether there was attempt at cleanup code
removed extra pylint disables from heading of test cases
output files got updated due to newlines above
This comment has been minimized.
This comment has been minimized.
jacobtylerwalls
added
the
Needs decision 🔒
jacobtylerwalls
added
Needs PR
…ng_cleanup' error
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9133 +/- ##
==========================================
+ Coverage 95.76% 95.83% +0.07%
==========================================
Files 173 174 +1
Lines 18665 18885 +220
==========================================
+ Hits 17874 18098 +224
+ Misses 791 787 -4
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
I have a bit of trouble trying to read the code as there is so much state involved.
It feels like this will only warn whenever the context manager is defined in the same module as the module it is used in, or if the module with the manager is linted before the using module. Shouldn't we be trying to safe_infer the decorator and seeing if it is indeed a "faulty" context manager? That would make it robust against imports as well.
We can still consider caching the result of the check using some state, but since safe_infer should be cached I'm not even sure if that is needed.
|
Ahh I didn't know about 'safe_infer'. That would be helpful because I can find the FunctionDef of the used item in the With. Note for later That should allow me to remove the odd state objects in the init. As for the warnings being on using generator was due to new background from the issue #2832. Using certain context-managers in a generator will cause context leaks which should be warned on the using function rather than the context-manager. |
doc/data/messages/c/contextmanager-generator-missing-cleanup/good.py
Outdated
Show resolved
Hide resolved
| 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)) |
There was a problem hiding this comment.
nit: probably don't need to list-ify this generator
There was a problem hiding this comment.
Maybe I'm missing something obvious, but it seemed on a first impression that list is more expensive than any in this instance. Or is that not so?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Does this need reviews or do you need to polish some things still @rhyn0 ? I would be glad to have this in 3.2.0 😄 |
|
From what I can tell it's very close:
Happy to give the whole thing a once-over when those are in 👍 |
This comment has been minimized.
This comment has been minimized.
|
Failing ReadTheDocs error seems to be the following Not sure if I need to change this here |
| 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 ``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. |
There was a problem hiding this comment.
Thanks, this is really great. A really subtle set of circumstances explained very clearly.
| 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 ``as ...`` or discard of the return value also implies whether the context needs cleanup or not. |
| @@ -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. | |||
There was a problem hiding this comment.
| A contextmanager must yield a non constant value and not handle cleanup for GeneratorExit. | |
| A contextmanager must yield a non-constant value and not handle cleanup for GeneratorExit. |
| @@ -0,0 +1,6 @@ | |||
| Checks for generators that use contextmanagers that don't handle cleanup properly. | |||
There was a problem hiding this comment.
(FYI, if you use the new_check suffix instead of feature it will be grouped a little bit more helpfully.)
| 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)) |
There was a problem hiding this comment.
Maybe I'm missing something obvious, but it seemed on a first impression that list is more expensive than any in this instance. Or is that not so?
| 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 |
There was a problem hiding this comment.
Would you mind reverting the unrelated formatting changes? As you can tell from the test method names, they're testing edge cases relating to oddly formatted code. We don't want to unintentionally degrade test coverage.
Good call, let's not block your work on that. If it hasn't been fixed on main yet, please open an issue! |
Type of Changes
Description
Add a checker for
contextlib.contextmanagerdecorated generator functions that don't have cleanup handling for GeneratorExit.Closes #2832