False positive consider-using-with for class member variable

[alwaysmpe]

Bug description

# pylint: disable=missing-docstring
class dataWrapper:
    def __init__(self, file_name):
        self.file_handle = open(file_name)
    def __del__(self):
        self.file_handle.close()

    def __next__(self):
        return self.file_handle.__next__()

    def __iter__(self):
        self.file_handle.__iter__()
        return self

Configuration

No response

Command used

pylint a.py

Pylint output

************* Module a
a.py:4:24: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)

Expected behavior

In the case of a member variable where the lifetime of the class is intended to match the lifetime of the associated resources (RAII), I think this warning is unnecessary and incorrect.

The case I've given is trivial. I'm using a temporary folder to do some processing, output a temporary file, stream the file as a server response, then close and delete both file and folder.

My personal use case is similar to the above, but this warning also triggers when writing a context manager (ie with enter/exit functions). I've currently completely disabled this warning, ideally I'd like to either disable for classes or on member variables (I've not found that this is possible).

Based on discussion in #4748, in particular that "The intention of this message is to inform the user that he is doing something which shifts the responsibility of freeing the allocated resources to somebody else", in this case the responsibility isn't being shifted.

Pylint version

pylint 2.9.6
astroid 2.6.6
Python 3.9.6 (default, Jul  9 2021, 09:44:29) 
[GCC 8.3.0]

OS / Environment

Debian

Additional dependencies

No response

[DudeNr33]

Can you give an example where it triggers for a context manager?
This should not be the case.

As for the original question: that may be just because you gave a simplified example, and not as straightforward for your real code, but in this case I would propose the following code:

class dataWrapper:
    def __init__(self, file_name):
        self.file_name = file_name

    def __iter__(self):
        with open(self.file_name, encoding="utf8") as file_handle:
            yield from file_handle

It's shorter, the resource is only open as long as necessary for reading, and closing the resource is not bound to the garbage collector and how the end user uses the dataWrapper class. Consider that __del__ will only be called when either done explicitly by using del(...) or when the last (strong) reference to the object is gone.

[alwaysmpe]

So, to give a more accurate version that uses a temp directory and some processed input data:

from tempfile import TemporaryDirectory
import subprocess

class dataWrapper:
    def __init__(self, input_data):
        self.temp_directory = TemporaryDirectory()
        in_path = self.temp_directory.name + '/temp.in'
        with open(in_path) as temp_file;
            temp_file.write(input_data)
        out_path = self.temp_directory.name + '/temp.out'
        subprocess.run(['file_processor', in_path, out_path], check=True)
        self.file_handle = open(out_path)
    def __del__(self):
        self.file_handle.close()
        self.temp_directory.cleanup()
    def __next__(self):
        return self.file_handle.__next__()
    def __iter__(self):
        self.file_handle.__iter__()
        return self

This can be written using 2 nested withs and a yeild in the iter function, but (to me) feels cleaner when done in the init. Obviously the linter disagrees. One argument in favour of this is that the processing may fail, which should probably be handled in the init rather than iter.

I'm aware that this won't be cleaned up until the last strong reference is gone, (in my application) this file is a server response, so will be a very short lived file.

RE context manager, I believe it's a detail of implementation, rather than a detail of encapsulation.

[DudeNr33]

Thanks for the more detailed example.
I see that using with blocks here is a bit more unwieldy as you now have two things that would trigger the message (TemporaryDirectory and open).
Implementing an iterable context manager would be another option that should satisfy the check and have the file (and tempdir) only open as long as necessary - but in your case I understand it's not really an issue.

The thing is that I don't really know how we should detect this (I would argue) rather special case without disabling this check for class members completely. Which would mean losing more than we would gain.

Ultimately the refactoring messages will always be a bit opinionated, even if they try to promote things which are commonly seen as "best practice". There may be cases where one simply disagrees with this definition, or has a concrete example where following this best practice is just not practicable. In this case I think it is totally fine to disable a check for a specific class (or completely if necessary).

This does not mean I am not open to suggestions. If you have an idea how we could fix this "false-positive" while still keeping the check intact for the intended use cases, I am happy to hear it.
The discussion you quoted in the original post gives (almost) good example why I would not want to disable it for class members completely: If the caller has to know that he must call another method explicitly to free a resource that was allocated internally by the class, I want this check to trigger, as this would require the caller to know a lot about the internals of the class.

[alwaysmpe]

Thank you for your answer/thoughts.

I'll leave it for now, as you're right it's a relatively small use case. I've ignored the specific warnings in the necessary location.