False positive R1732 consider-using-with

[sam-s]

Steps to reproduce

Given a file a.py:

"test"
class C():
    "my context manager"
    def __init__(self):
        self.fd = None
    def __enter__(self):
        self.fd = open("foo")
    def __exit__(self, _exc_type, _exc_value, _traceback):
        self.fd.close()

Current behavior

Result of pylint a.py:

a.py:7: [R1732(consider-using-with), C.__enter__] Consider using 'with' for resource-allocating operations

Expected behavior

When resources are allocated in a context manager's __enter__ method, this warning should be suppressed.

Instead, a warning should be issued if the resource is not released in the __exit__ method.

pylint --version output

Result of pylint --version output:

pylint 2.8.2
astroid 2.5.6
Python 3.9.4 (default, Apr  4 2021, 19:38:44) 
[GCC 10.2.1 20210401]

[Pierre-Sassoulas]

I think it makes sense. @DudeNr33 do you want to handle this one ?

[DudeNr33]

Yes, I will take a look at it.
That is definitely not the intended behavior for this check.

[DudeNr33]

Suppressing the message if the resource is allocated inside a context manager is finished.

However I struggle a bit with the suggested warning that should trigger if the resource is not released in the __exit__ or after yield in a function decorated with @contextlib.contextmanager.

1. The current logic resides in the RefactoringChecker. The warning does not really fit there, but moving it somewhere else would essentially mean duplicating all the existing code to detect the resource-allocating calls.
2. Predicting the correct call that has to be present in the __exit__ or after the yield is also not trivial. I would try to implement a set-like class (ExpectedCalls or something like that) where if something like open() is found one could add an expected call for close(). If this object is not empty, every visited call is checked if it matches one of the expected calls. If so, this call gets removed from the set. In leave_functiondef() one would check if the set of expected calls is empty. If not, an appropriate message missing call to "close()" can be triggered. Then the set of expected calls is cleared. However, there are probably quite a few corner cases to consider, e.g., making sure that the method was called on the right object, making sure that it was called after the yieldor inside __exit__ and so on.

@Pierre-Sassoulas what is your opinion on this? Should we split this up in two separate tickets? Which checker should be responsible for emitting the warning?

[Pierre-Sassoulas]

Yes let's get rid of the false positive first then we can make another issue for the non trivial check in __exit__. I don't have an opinion on the best checker to use, but we can move the message in another checker and rename it with old_names if it makes more sense to put the check elsewhere.