Skip to content

Commit

Permalink
Fix false positive when testing for-loops for unbalanced unpacking (W…
Browse files Browse the repository at this point in the history 
…0644) (#8892)

* When checking for loops for unbalanced dict unpacking, compare the number of targets to the size of each value, instead of the number of values, since the for loop will iterate the values and unpack each in turn onto the targets.

Co-authored-by: Pierre Sassoulas <[email protected]>
Co-authored-by: Mark Byrne <[email protected]>
  • Loading branch information
@Neowizard @Pierre-Sassoulas @mbyrnepr2
3 people authored Sep 4, 2023
1 parent de56b55 commit 7b72c39
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 21 deletions.
5 changes: 5 additions & 0 deletions doc/whatsnew/fragments/8156.false_positive
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
When checking for unbalanced dict unpacking in for-loops, Pylint will now test whether the length of each value to be
unpacked matches the number of unpacking targets. Previously, Pylint would test the number of values for the loop
iteration, which would produce a false unbalanced-dict-unpacking warning.

Closes #8156
53 changes: 45 additions & 8 deletions pylint/checkers/variables.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import collections
import copy
import itertools
import math
import os
import re
from collections import defaultdict
Expand Down Expand Up @@ -1326,9 +1327,29 @@ def visit_for(self, node: nodes.For) -> None:
if any(isinstance(target, nodes.Starred) for target in targets):
return

if len(targets) != len(values):
details = _get_unpacking_extra_info(node, inferred)
self._report_unbalanced_unpacking(node, inferred, targets, values, details)
if isinstance(inferred, nodes.Dict):
if isinstance(node.iter, nodes.Name):
# If this a case of 'dict-items-missing-iter', we don't want to
# report it as an 'unbalanced-dict-unpacking' as well
# TODO (performance), merging both checks would streamline this
if len(targets) == 2:
return

else:
is_starred_targets = any(
isinstance(target, nodes.Starred) for target in targets
)
for value in values:
value_length = self._get_value_length(value)
is_valid_star_unpack = is_starred_targets and value_length >= len(
targets
)
if len(targets) != value_length and not is_valid_star_unpack:
details = _get_unpacking_extra_info(node, inferred)
self._report_unbalanced_unpacking(
node, inferred, targets, value_length, details
)
break

def leave_for(self, node: nodes.For) -> None:
self._store_type_annotation_names(node)
Expand Down Expand Up @@ -2957,16 +2978,32 @@ def _check_unpacking(
if values is not None:
if len(targets) != len(values):
self._report_unbalanced_unpacking(
node, inferred, targets, values, details
node, inferred, targets, len(values), details
)
# attempt to check unpacking may be possible (i.e. RHS is iterable)
elif not utils.is_iterable(inferred):
self._report_unpacking_non_sequence(node, details)

@staticmethod
def _get_value_length(value_node: nodes.NodeNG) -> int:
value_subnodes = VariablesChecker._nodes_to_unpack(value_node)
if value_subnodes is not None:
return len(value_subnodes)
if isinstance(value_node, nodes.Const) and isinstance(
value_node.value, (str, bytes)
):
return len(value_node.value)
if isinstance(value_node, nodes.Subscript):
step = value_node.slice.step or 1
splice_range = value_node.slice.upper.value - value_node.slice.lower.value
splice_length = int(math.ceil(splice_range / step))
return splice_length
return 1

@staticmethod
def _nodes_to_unpack(node: nodes.NodeNG) -> list[nodes.NodeNG] | None:
"""Return the list of values of the `Assign` node."""
if isinstance(node, (nodes.Tuple, nodes.List, *DICT_TYPES)):
if isinstance(node, (nodes.Tuple, nodes.List, nodes.Set, *DICT_TYPES)):
return node.itered() # type: ignore[no-any-return]
if isinstance(node, astroid.Instance) and any(
ancestor.qname() == "typing.NamedTuple" for ancestor in node.ancestors()
Expand All @@ -2979,15 +3016,15 @@ def _report_unbalanced_unpacking(
node: nodes.NodeNG,
inferred: InferenceResult,
targets: list[nodes.NodeNG],
values: list[nodes.NodeNG],
values_count: int,
details: str,
) -> None:
args = (
details,
len(targets),
"" if len(targets) == 1 else "s",
len(values),
"" if len(values) == 1 else "s",
values_count,
"" if values_count == 1 else "s",
)

symbol = (
Expand Down
35 changes: 34 additions & 1 deletion tests/functional/u/unbalanced/unbalanced_dict_unpacking.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ def all_dict():
for a, b, c, d, e, f, g in {1: 2}.items(): # [unbalanced-dict-unpacking]
pass

for key, value in {1: 2}: # [unbalanced-dict-unpacking]
# This is an instance of dict-items-missing-iter, so unbalanced-dict-unpacking is suppressed
for key, value in {1: 2}:
pass

for key, value in {1: 2}.keys(): # [unbalanced-dict-unpacking, consider-iterating-dictionary]
Expand Down Expand Up @@ -89,3 +90,35 @@ def all_dict():
_, *others = {1: 2, 3: 4, 5: 6}.items()
_, *others = {1: 2, 3: 4, 5: 6}.values()
_, others = {1: 2, 3: 4, 5: 6}.values() # [unbalanced-dict-unpacking]


for value1, value2 in {1: 2, 3: 4}.values(): # [unbalanced-dict-unpacking]
pass

for value1, value2 in {1: (2, 3), 4: 5}.values(): # [unbalanced-dict-unpacking]
pass

for value1, value2 in {1: 2, 3: (4, 5)}.values(): # [unbalanced-dict-unpacking]
pass

for value1, value2 in {1: (2, 3, 4), 5: (6, 7)}.values(): # [unbalanced-dict-unpacking]
pass

for value1, value2, value3 in {1: (2, 3, 4), 5: (6, 7)}.values(): # [unbalanced-dict-unpacking]
pass

# These should not raise unbalanced-dict since the there are the same number of targets and values in every expression
for value1, value2 in {1: (2, 3), 4: (5, 6)}.values():
pass

for value1, value2, value3 in {1: (2, 3, 4), 5: (6, 7, 8)}.values():
pass

for value1, value2 in {1: [2, 3], 4: {5, 6}}.values():
pass

for value1, value2 in {1: {2: 3, 4: 5}, 6: {7: 8, 9: 10}}.values():
pass

for value1, value2 in {1: [1, 2, 3][0:2]}.values():
pass
28 changes: 16 additions & 12 deletions tests/functional/u/unbalanced/unbalanced_dict_unpacking.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,19 @@ unbalanced-dict-unpacking:6:4:6:41:dict_vals:"Possible unbalanced dict unpacking
unbalanced-dict-unpacking:10:4:10:49:dict_keys:"Possible unbalanced dict unpacking with {1: 2, 'hi': 20}.keys(): left side has 7 labels, right side has 2 values":INFERENCE
unbalanced-dict-unpacking:16:4:16:63:dict_items:"Possible unbalanced dict unpacking with {1: 2, 'boo': 3}.items(): left side has 3 labels, right side has 2 values":INFERENCE
unbalanced-dict-unpacking:20:4:20:38:all_dict:"Possible unbalanced dict unpacking with {1: 2, 3: 4}: left side has 7 labels, right side has 2 values":INFERENCE
unbalanced-dict-unpacking:23:0:24:8::"Possible unbalanced dict unpacking with {1: 2}.items(): left side has 7 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:26:0:27:8::"Possible unbalanced dict unpacking with {1: 2}: left side has 2 labels, right side has 1 value":INFERENCE
consider-iterating-dictionary:29:18:29:31::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
unbalanced-dict-unpacking:29:0:30:8::"Possible unbalanced dict unpacking with {1: 2}.keys(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:32:0:33:8::"Possible unbalanced dict unpacking with {1: 2}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:49:0:49:28::"Possible unbalanced dict unpacking with populated.items(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:54:0:54:37::"Possible unbalanced dict unpacking with {1: 2, 3: 4, 5: 6}.items(): left side has 2 labels, right side has 3 values":INFERENCE
unbalanced-dict-unpacking:56:0:56:12::"Possible unbalanced dict unpacking with {}: left side has 3 labels, right side has 0 values":INFERENCE
unbalanced-dict-unpacking:67:0:68:14::"Possible unbalanced dict unpacking with {'key': 'value'}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:77:0:78:14::"Possible unbalanced dict unpacking with {'key': 'value', 1: 2, 20: 21}.values(): left side has 2 labels, right side has 3 values":INFERENCE
unbalanced-dict-unpacking:80:0:81:14::"Possible unbalanced dict unpacking with {'key': 'value', 1: 2, 20: 21}.values(): left side has 2 labels, right side has 3 values":INFERENCE
unbalanced-dict-unpacking:91:0:91:39::"Possible unbalanced dict unpacking with {1: 2, 3: 4, 5: 6}.values(): left side has 2 labels, right side has 3 values":INFERENCE
unbalanced-dict-unpacking:23:0:24:8::"Possible unbalanced dict unpacking with {1: 2}.items(): left side has 7 labels, right side has 2 values":INFERENCE
consider-iterating-dictionary:30:18:30:31::Consider iterating the dictionary directly instead of calling .keys():INFERENCE
unbalanced-dict-unpacking:30:0:31:8::"Possible unbalanced dict unpacking with {1: 2}.keys(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:33:0:34:8::"Possible unbalanced dict unpacking with {1: 2}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:50:0:50:28::"Possible unbalanced dict unpacking with populated.items(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:55:0:55:37::"Possible unbalanced dict unpacking with {1: 2, 3: 4, 5: 6}.items(): left side has 2 labels, right side has 3 values":INFERENCE
unbalanced-dict-unpacking:57:0:57:12::"Possible unbalanced dict unpacking with {}: left side has 3 labels, right side has 0 values":INFERENCE
unbalanced-dict-unpacking:68:0:69:14::"Possible unbalanced dict unpacking with {'key': 'value'}.values(): left side has 2 labels, right side has 5 values":INFERENCE
unbalanced-dict-unpacking:78:0:79:14::"Possible unbalanced dict unpacking with {'key': 'value', 1: 2, 20: 21}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:81:0:82:14::"Possible unbalanced dict unpacking with {'key': 'value', 1: 2, 20: 21}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:92:0:92:39::"Possible unbalanced dict unpacking with {1: 2, 3: 4, 5: 6}.values(): left side has 2 labels, right side has 3 values":INFERENCE
unbalanced-dict-unpacking:95:0:96:8::"Possible unbalanced dict unpacking with {1: 2, 3: 4}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:98:0:99:8::"Possible unbalanced dict unpacking with {1: (2, 3), 4: 5}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:101:0:102:8::"Possible unbalanced dict unpacking with {1: 2, 3: (4, 5)}.values(): left side has 2 labels, right side has 1 value":INFERENCE
unbalanced-dict-unpacking:104:0:105:8::"Possible unbalanced dict unpacking with {1: (2, 3, 4), 5: (6, 7)}.values(): left side has 2 labels, right side has 3 values":INFERENCE
unbalanced-dict-unpacking:107:0:108:8::"Possible unbalanced dict unpacking with {1: (2, 3, 4), 5: (6, 7)}.values(): left side has 3 labels, right side has 2 values":INFERENCE

0 comments on commit 7b72c39

Please sign in to comment.