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

Fix false-positive consider-using-with when using contextlib.ExitStack #4665

Merged

Conversation

DudeNr33
Copy link
Collaborator

@DudeNr33 DudeNr33 commented Jul 3, 2021

Steps

  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

The contextlib.ExitStack class provides a method to automatically call __exit__ on context managers added to its stack when leaving a with contextlib.ExitStack() as stack block.
If a callable that could be used in a with block was added to the stack, no R1732 consider-using-with message should be triggered:

with contextlib.ExitStack() as stack:
    _ = stack.enter_context(
        open("/sys/firmware/devicetree/base/hwid,location", "r")
    )  # must not trigger

While contextlib.AsyncExitStack should theoretically also be covered (as it can be used with synchronous context managers as well), it is currently not possible to implement this, as trying to infer the stack.enter_context returns Uninferable in this case...

I also noticed that the whole consider-using-with check does not check asynchronous context managers at all.
But I guess this would exceed the scope of this PR, and should probably get its own message (e.g. consider-using-async-with).

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #4654

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I juste have a minor comment about the documentation, thanks a lot for the fix and for the clean tests and implementation !

doc/whatsnew/2.9.rst Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 3, 2021

Coverage Status

Coverage decreased (-0.003%) to 92.038% when pulling 23b628e on DudeNr33:issue-4654-false-positive-r1732 into c026826 on PyCQA:main.

@DudeNr33 DudeNr33 marked this pull request as ready for review July 3, 2021 19:07
@Pierre-Sassoulas Pierre-Sassoulas added the False Positive 🦟 A message is emitted but nothing is wrong with the code label Jul 4, 2021
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Two small comments

@@ -62,6 +62,7 @@ New checkers
Other Changes
=============


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Empty line :)

"contextlib.ExitStack.enter_context", # necessary for Python 3.6 compatibility
)
)
parent = node.parent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it's just used two times, it's probably better to write out node.parent directly. That way you don't have to think about it.

@DudeNr33
Copy link
Collaborator Author

DudeNr33 commented Jul 4, 2021

Thank you both for the feedback, the latest commit should cover all requested changes!

@Pierre-Sassoulas Pierre-Sassoulas merged commit c2d03c6 into pylint-dev:main Jul 4, 2021
@Pierre-Sassoulas
Copy link
Member

Thank you for this fix, it will be released in 2.9.4 :)

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

Successfully merging this pull request may close these issues.

R1732(consider-using-with): Regression on contextlib.ExitStack
4 participants