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 for ternary conditionals in with items #4679

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ Release date: TBA
..
Put bug fixes that should not wait for a new minor version here

* Fix false-positive ``consider-using-with`` (R1732) if a ternary conditional is used together with ``with``

Closes #4676

* Fix false-positive ``deprecated-module`` when relative import uses deprecated module name.

Closes #4629
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 a ternary conditional is used together with ``with``

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

* Add type annotations to pyreverse dot files
Expand Down
18 changes: 17 additions & 1 deletion pylint/checkers/refactoring/refactoring_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,22 @@ def _is_inside_context_manager(node: astroid.Call) -> bool:
)


def _is_part_of_with_items(node: astroid.Call) -> bool:
"""
Checks if one of the node's parents is an ``astroid.With`` node and that the node itself is located
somewhere under its ``items``.
"""
frame = node.frame()
current = node
while current != frame:
if isinstance(current, astroid.With):
items_start = current.items[0][0].lineno
items_end = current.items[-1][0].tolineno
return items_start <= node.lineno <= items_end
current = current.parent
return False


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."""
Expand Down Expand Up @@ -1313,7 +1329,7 @@ def _check_consider_using_with(self, node: astroid.Call):
or (
# things like ``open("foo")`` which are not already inside a ``with`` statement
inferred.qname() in CALLS_RETURNING_CONTEXT_MANAGERS
and not isinstance(node.parent, astroid.With)
and not _is_part_of_with_items(node)
)
)
if could_be_used_in_with and not (
Expand Down
17 changes: 16 additions & 1 deletion tests/functional/c/consider/consider_using_with_open.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# pylint: disable=missing-function-docstring, missing-module-docstring, invalid-name, import-outside-toplevel
# pylint: disable=missing-class-docstring, too-few-public-methods, unused-variable
# pylint: disable=missing-class-docstring, too-few-public-methods, unused-variable, multiple-statements
"""
The functional test for the standard ``open()`` function has to be moved in a separate file,
because PyPy has to be excluded for the tests as the ``open()`` function is uninferable in PyPy.
Expand Down Expand Up @@ -49,3 +49,18 @@ def test_open_outside_assignment():
def test_open_inside_with_block():
with open("foo") as fh:
open("bar") # [consider-using-with]


def test_ternary_if_in_with_block(file1, file2, which):
"""Regression test for issue #4676 (false positive)"""
with (open(file1) if which else open(file2)) as input_file: # must not trigger
return input_file.read()


def test_single_line_with(file1):
with open(file1): return file1.read() # must not trigger


def test_multiline_with_items(file1, file2, which):
with (open(file1) if which
else open(file2)) as input_file: return input_file.read()