Skip to content
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

False positive consider-using-with in if/else block #4751

Closed
DudeNr33 opened this issue Jul 26, 2021 · 3 comments · Fixed by #4929
Closed

False positive consider-using-with in if/else block #4751

DudeNr33 opened this issue Jul 26, 2021 · 3 comments · Fixed by #4929
Labels
Bug 🪲 False Positive 🦟 A message is emitted but nothing is wrong with the code

Comments

@DudeNr33
Copy link
Collaborator

Bug description

Given a file a.py:

# pylint: disable=missing-docstring, invalid-name
a = True
if a:
    file_handle = open("foo")
else:
    file_handle = open("bar")
with file_handle:
    print(file_handle.read())

(Configuration)

No response

Command used

Result of pylint a.py:

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

------------------------------------------------------------------
Your code has been rated at 8.33/10 (previous run: 8.33/10, +0.00)

(Expected behavior)

No message consider-using-with, as file_handle is used in a subsequent with block.
#4721 implemented changes that already suppress this message for things like:

file_handle = open("foo")
with file_handle:
    print(file_handle.read())

So naturally the example code above should also not trigger the message.

(Version affected)

pylint 2.9.5
astroid 2.6.5
Python 3.9.6 (default, Jun 28 2021, 19:24:41) 
[Clang 12.0.5 (clang-1205.0.22.9)]

(OS / Environment)

macOS BigSur 11.4

(Additional dependencies)

No response

@DudeNr33 DudeNr33 added Bug 🪲 Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jul 26, 2021
@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jul 26, 2021
@timmartin
Copy link
Contributor

The issue seems to be that when the second assignment is encountered:

a = True
if a:
    file_handle = open("foo")
else:
    file_handle = open("bar")  # <--here

the checker thinks it's being reassigned, despite the fact that the program can't possibly hit both assignments in the same run. I've had a quick look at the existing checkers and I'm not sure if this is something that can be addressed easily. It doesn't seem like Pylint / Astroid as it stands has any logic for dealing with the case that something is conditionally defined in one arm of an if block.

For example:

if random.random() < 0.5:
    b = 6
else:
    pass

print(b)

doesn't report (possibly) uninitialised variable. Is this correct? If so, then this isn't really specific to the consider-using-with checker but will be a much wider change that would need to be made.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Aug 6, 2021

You are definitely correct that the Problem is in the else clause. I have an idea how to solve it for this check, but that would not solve the general problem you described.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Aug 6, 2021

The problem with the consider-using-with check is, that it intends to check if a variable containing a context manager is redefined before it is used in a with block, as you already mentioned.

Now for consider-using-with I would extend the logic of the check to see if the assignment is made inside an else block and if so double check if the previous assignment happened in the corresponding if block.

But that is just a solution to this check and not for others.
I tried to see how pylint would handle a normal case of a variable that is redefined before it is used (as e.g. PyCharm does it), but apparently this is not checked by pylint at all:

a = 1
a = 2  # <-- no message here
print(a)

Edit: might benefit from results of #4795.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants