False-positive consider-using-with when returning a file handler

[cdce8p]

Bug description

def func():
    return open("test.py")  # false-positive

text = func().read()
print(text)

(Configuration)

Using the following configuration:

Command used

test.py:2:11: R1732: Consider using 'with' for resource-allocating operations (consider-using-with)

(Expected behavior)

The example above might not be consider good code style, but it's enough to reproduce the issue. If the code is replaced by

def func():
    with open("test.py") as fp:
        return fp

text = func().read()
print(text)

Python will through an exception

ValueError: I/O operation on closed file.

(Version affected)

pylint 2.9.6-dev0
astroid 2.6.6-dev0
Python 3.10.0b4 (v3.10.0b4:2ba4b20854, Jul  9 2021, 21:56:21) [Clang 12.0.5 (clang-1205.0.22.9)]

(OS / Environment)

No response

(Additional dependencies)

No response

[cdce8p]

@DudeNr33 Would you mind taking a look at this one?

[DudeNr33]

Will do, but I don't really know if this should be considered a false positive, at least with the example you gave.

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 caller has to make sure he does not forget calling close(), which is actually missing in your example.
Will take a closer look this evening.

[cdce8p]

The reason I opened the issue is that I almost introduced a bug because of it. I didn't realize at the time that even after a return in a context manager the file is closed. It is pretty similar to

def func():
    with open("test.py") as fp:
        return fp.read()

which does work as expected.

You are right that it shifts the responsibility to the caller, but I think emitting the false-positive might cause more issues than not closing the file handler. Consider that a beginner might not now these details and will be surprised that code pylint suggested doesn't work. That still leaves the question though if a fix for this issue would even work with the current code. I haven't looked into that.

[DudeNr33]

I understand your concern. Trying to satisfy the linter should ideally not lead to erroneous code, even though this can not always be avoided - I've seen that more than once that fixing linter messages introduced new bugs.

The question is: should we rather
a) think about a clearer description for the check message
b) completely suppress the check when returning context managers
c) leave it as it is, and add a new feature which detects that an already "active" context manager is returned inside a with block - as you mentioned that is a bug itself, and will most certainly also be a bug for other context managers, not just the open() function

I had a quick look at the file changed by the commit where you mentioned this issue.
The "correct" (as in: just my opinion and a way that should satisfy the current Pylint implementation ;)) way to get rid of the warning and ensure that the resource is correctly released again would be to turn the load_data() function into a context manager:

from contextlib import contextmanager

@contextmanager
def load_data(...):
    try:
        # other code
        with open(filepath, "rb") as file_content:
            yield file_content
    except:
        # exception handling
        yield None

You can then use with load_data(...) as file_content inside the send_file method.

In my opinion that's a suitable solution and something that the consider-using-with message should encourage to refactor to. So options a) and maybe as a later addition c) would be the best approach.
Going for b) would be a quick fix, but we would lose functionality.
What do you think?

(Edited the code: there must only be one yield that executes, so indented the yield None to be in the except block. For the complete function there could be more lines where a yield None must be added, I think).

[DudeNr33]

Oh, or option d)

Actually do suppress consider-using-with on return statements, and instead issue a message along the lines of consider-using-contextmanager - maybe this would be the clearest solution?

[cdce8p]

Oh, or option d)

Actually do suppress consider-using-with on return statements, and instead issue a message along the lines of consider-using-contextmanager - maybe this would be the clearest solution?

I would like that!

[DudeNr33]

You can assign this issue to me if you like. :)