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 a false positive for simplify-boolean-expression when multiple values inferred #7627

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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/whatsnew/fragments/7626.false_positive
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Fix a false positive for ``simplify-boolean-expression`` when multiple values
are inferred for a constant.

Adds a keyword-only ``compare_constants`` argument to ``safe_infer``.
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

Closes #7626
11 changes: 5 additions & 6 deletions pylint/checkers/refactoring/refactoring_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
from pylint import checkers
from pylint.checkers import utils
from pylint.checkers.utils import node_frame_class
from pylint.interfaces import HIGH
from pylint.interfaces import HIGH, INFERENCE

if TYPE_CHECKING:
from pylint.lint import PyLinter
Expand Down Expand Up @@ -1495,19 +1495,18 @@ def visit_return(self, node: nodes.Return | nodes.Assign) -> None:
):
return

inferred_truth_value = utils.safe_infer(truth_value)
inferred_truth_value = utils.safe_infer(truth_value, compare_constants=True)
if inferred_truth_value is None or inferred_truth_value == astroid.Uninferable:
truth_boolean_value = True
else:
truth_boolean_value = inferred_truth_value.bool_value()
return
truth_boolean_value = inferred_truth_value.bool_value()

if truth_boolean_value is False:
message = "simplify-boolean-expression"
suggestion = false_value.as_string()
else:
message = "consider-using-ternary"
suggestion = f"{truth_value.as_string()} if {cond.as_string()} else {false_value.as_string()}"
self.add_message(message, node=node, args=(suggestion,))
self.add_message(message, node=node, args=(suggestion,), confidence=INFERENCE)
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

def _append_context_managers_to_stack(self, node: nodes.Assign) -> None:
if _is_inside_context_manager(node):
Expand Down
18 changes: 17 additions & 1 deletion pylint/checkers/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1343,12 +1343,18 @@ def _get_python_type_of_node(node: nodes.NodeNG) -> str | None:

@lru_cache(maxsize=1024)
def safe_infer(
node: nodes.NodeNG, context: InferenceContext | None = None
node: nodes.NodeNG,
context: InferenceContext | None = None,
*,
compare_constants: bool = False,
) -> InferenceResult | None:
"""Return the inferred value for the given node.

Return None if inference failed or if there is some ambiguity (more than
one node has been inferred of different types).

If compare_constants is True and if multiple constants are inferred,
unequal inferred values are also considered ambiguous and return None.
"""
inferred_types: set[str | None] = set()
try:
Expand All @@ -1362,11 +1368,21 @@ def safe_infer(
if value is not astroid.Uninferable:
inferred_types.add(_get_python_type_of_node(value))

first_constant_value = None
if compare_constants and isinstance(value, nodes.Const):
first_constant_value = value.value

try:
for inferred in infer_gen:
inferred_type = _get_python_type_of_node(inferred)
if inferred_type not in inferred_types:
return None # If there is ambiguity on the inferred node.
if (
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
compare_constants
and isinstance(inferred, nodes.Const)
and inferred.value != first_constant_value
):
return None
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved
if (
isinstance(inferred, nodes.FunctionDef)
and inferred.args.args is not None
Expand Down
30 changes: 23 additions & 7 deletions tests/functional/t/ternary.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,21 @@
"""Test for old ternary constructs"""
from UNINFERABLE import condition, true_value, false_value, some_callable # pylint: disable=import-error
from UNINFERABLE import condition, some_callable # pylint: disable=import-error
jacobtylerwalls marked this conversation as resolved.
Show resolved Hide resolved

SOME_VALUE1 = true_value if condition else false_value
SOME_VALUE2 = condition and true_value or false_value # [consider-using-ternary]
TRUE_VALUE = True
FALSE_VALUE = False

SOME_VALUE1 = TRUE_VALUE if condition else FALSE_VALUE
SOME_VALUE2 = condition and TRUE_VALUE or FALSE_VALUE # [consider-using-ternary]
SOME_VALUE3 = condition

def func1():
"""Ternary return value correct"""
return true_value if condition else false_value
return TRUE_VALUE if condition else FALSE_VALUE


def func2():
"""Ternary return value incorrect"""
return condition and true_value or false_value # [consider-using-ternary]
return condition and TRUE_VALUE or FALSE_VALUE # [consider-using-ternary]


SOME_VALUE4 = some_callable(condition) and 'ERROR' or 'SUCCESS' # [consider-using-ternary]
Expand All @@ -30,10 +33,23 @@ def func2():
def func4():
""""Using a Name as a condition but still emits"""
truth_value = 42
return condition and truth_value or false_value # [consider-using-ternary]
return condition and truth_value or FALSE_VALUE # [consider-using-ternary]


def func5():
""""Using a Name that infers to False as a condition does not emit"""
falsy_value = False
return condition and falsy_value or false_value # [simplify-boolean-expression]
return condition and falsy_value or FALSE_VALUE # [simplify-boolean-expression]


def func_control_flow():
"""Redefining variables should invalidate simplify-boolean-expression."""
flag_a = False
flag_b = False
for num in range(2):
if num == 1:
flag_a = True
else:
flag_b = True
multiple = (flag_a and flag_b) or func5()
return multiple
16 changes: 8 additions & 8 deletions tests/functional/t/ternary.txt
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
consider-using-ternary:5:0:5:53::Consider using ternary (true_value if condition else false_value):UNDEFINED
consider-using-ternary:15:4:15:50:func2:Consider using ternary (true_value if condition else false_value):UNDEFINED
consider-using-ternary:18:0:18:63::Consider using ternary ('ERROR' if some_callable(condition) else 'SUCCESS'):UNDEFINED
consider-using-ternary:19:0:19:60::Consider using ternary ('greater' if SOME_VALUE1 > 3 else 'not greater'):UNDEFINED
consider-using-ternary:20:0:20:67::Consider using ternary ('both' if SOME_VALUE2 > 4 and SOME_VALUE3 else 'not'):UNDEFINED
simplify-boolean-expression:23:0:23:50::Boolean expression may be simplified to SOME_VALUE2:UNDEFINED
consider-using-ternary:33:4:33:51:func4:Consider using ternary (truth_value if condition else false_value):UNDEFINED
simplify-boolean-expression:39:4:39:51:func5:Boolean expression may be simplified to false_value:UNDEFINED
consider-using-ternary:8:0:8:53::Consider using ternary (TRUE_VALUE if condition else FALSE_VALUE):INFERENCE
consider-using-ternary:18:4:18:50:func2:Consider using ternary (TRUE_VALUE if condition else FALSE_VALUE):INFERENCE
consider-using-ternary:21:0:21:63::Consider using ternary ('ERROR' if some_callable(condition) else 'SUCCESS'):INFERENCE
consider-using-ternary:22:0:22:60::Consider using ternary ('greater' if SOME_VALUE1 > 3 else 'not greater'):INFERENCE
consider-using-ternary:23:0:23:67::Consider using ternary ('both' if SOME_VALUE2 > 4 and SOME_VALUE3 else 'not'):INFERENCE
simplify-boolean-expression:26:0:26:50::Boolean expression may be simplified to SOME_VALUE2:INFERENCE
consider-using-ternary:36:4:36:51:func4:Consider using ternary (truth_value if condition else FALSE_VALUE):INFERENCE
simplify-boolean-expression:42:4:42:51:func5:Boolean expression may be simplified to FALSE_VALUE:INFERENCE