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
5 changes: 5 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ Release date: TBA
..
Put bug fixes that should not wait for a new minor version here


* Fix false-positive ``consider-using-with`` (R1732) if ``contextlib.ExitStack`` takes care of calling the ``__exit__`` method

Closes #4654

* Fix a false positive for ``unused-private-member`` when mutating a private attribute
with ``cls``

Expand Down
2 changes: 2 additions & 0 deletions doc/whatsnew/2.9.rst
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ New checkers
Other Changes
=============

* Fix false-positive ``consider-using-with`` (R1732) if ``contextlib.ExitStack`` takes care of calling the ``__exit__`` method

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 :)

* Add type annotations to pyreverse dot files

* Pylint's tags are now the standard form ``vX.Y.Z`` and not ``pylint-X.Y.Z`` anymore.
Expand Down
23 changes: 21 additions & 2 deletions pylint/checkers/refactoring/refactoring_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ def get_curline_index_start():
return False


def _is_inside_context_manager(node):
def _is_inside_context_manager(node: astroid.Call) -> bool:
frame = node.frame()
if not isinstance(
frame, (astroid.FunctionDef, astroid.BoundMethod, astroid.UnboundMethod)
Expand All @@ -114,6 +114,23 @@ def _is_inside_context_manager(node):
)


def _will_be_released_automatically(node: astroid.Call) -> bool:
"""Checks if a call that could be used in a ``with`` statement is used in an alternative
construct which would ensure that its __exit__ method is called."""
callables_taking_care_of_exit = frozenset(
(
"contextlib._BaseExitStack.enter_context",
"contextlib.ExitStack.enter_context", # necessary for Python 3.6 compatibility
)
)
if not isinstance(node.parent, astroid.Call):
return False
func = utils.safe_infer(node.parent.func)
if not func:
return False
return func.qname() in callables_taking_care_of_exit


class RefactoringChecker(checkers.BaseTokenChecker):
"""Looks for code which can be refactored

Expand Down Expand Up @@ -1299,7 +1316,9 @@ def _check_consider_using_with(self, node: astroid.Call):
and not isinstance(node.parent, astroid.With)
)
)
if could_be_used_in_with and not _is_inside_context_manager(node):
if could_be_used_in_with and not (
_is_inside_context_manager(node) or _will_be_released_automatically(node)
):
self.add_message("consider-using-with", node=node)

def _check_consider_using_join(self, aug_assign):
Expand Down
8 changes: 8 additions & 0 deletions tests/functional/c/consider/consider_using_with.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,11 @@ def test_popen():
_ = subprocess.Popen("sh") # [consider-using-with]
with subprocess.Popen("sh"):
pass


def test_suppress_in_exit_stack():
"""Regression test for issue #4654 (false positive)"""
with contextlib.ExitStack() as stack:
_ = stack.enter_context(
open("/sys/firmware/devicetree/base/hwid,location", "r")
) # must not trigger