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 when testing for-loops for unbalanced unpacking (W0644) #8892

Merged
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
519c09a
When checking for loops for unbalanced dict unpacking, compare the nu…
Neowizard Jul 27, 2023
f028970
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 27, 2023
877afaf
Prevent unbalanced-dict-unpacking from shadowing dict-iter-missing-it…
Neowizard Jul 27, 2023
d8cbf40
Fixed support for Dict values when checking for unbalanced-dict-unpac…
Neowizard Jul 27, 2023
4c2cd8a
Added support for star-unpacking when checking for unbalanced-dict-un…
Neowizard Jul 27, 2023
c10f299
Merge branch 'false-positive-unbalanced-dict-unpacking' of https://gi…
Neowizard Jul 27, 2023
cfc4907
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 27, 2023
31bdca5
Added test case to cover subscripts/splices when checking for unbalan…
Neowizard Jul 27, 2023
71e54a8
Regenerated docs
Neowizard Jul 27, 2023
4892c97
Merge branch 'false-positive-unbalanced-dict-unpacking' of https://gi…
Neowizard Jul 27, 2023
e21bc39
Fixed but in how we test the length of a splice - was missing `Const.…
Neowizard Jul 30, 2023
181f07d
Reverted unintended changes to docs/user_guide
Neowizard Jul 30, 2023
07558f3
Added news fragment
Neowizard Jul 30, 2023
e29fea9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 30, 2023
de1206d
Merge branch 'pylint-dev:main' into false-positive-unbalanced-dict-un…
Neowizard Jul 30, 2023
ed8d7d1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 30, 2023
72eb2e6
Fix news fragment to match expected regex (capitalize "Closes")
Neowizard Jul 30, 2023
bc98d03
Work around mypy's definition that `math.ceil` returns floats
Neowizard Jul 30, 2023
8ac683e
Merge branch 'false-positive-unbalanced-dict-unpacking' of https://gi…
Neowizard Jul 30, 2023
28ad6fb
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 30, 2023
3151e25
Add trailing newline to 8156.false_positive as required by the pre_co…
Neowizard Jul 30, 2023
c98b889
Merge branch 'false-positive-unbalanced-dict-unpacking' of https://gi…
Neowizard Jul 30, 2023
197746c
Fix type'o in unbalanced_dict_unpacking.py test
Neowizard Jul 30, 2023
a0471d4
Merge branch 'pylint-dev:main' into false-positive-unbalanced-dict-un…
Neowizard Aug 2, 2023
bc65c3c
Merge branch 'pylint-dev:main' into false-positive-unbalanced-dict-un…
Neowizard Aug 6, 2023
f721da9
Merge remote-tracking branch 'upstream/main' into false-positive-unba…
Neowizard Aug 14, 2023
49888a1
Detect incidents of missing call to `items` during unbalanced-dict-un…
Neowizard Aug 14, 2023
51bf4bd
Rephrased comment about bypassing unbalanced-dict-unpacking for consi…
Neowizard Aug 15, 2023
f63349a
Fix spelling
Pierre-Sassoulas Aug 15, 2023
766d13b
Merge branch 'main' into false-positive-unbalanced-dict-unpacking
Neowizard Aug 23, 2023
bd3044d
Merge remote-tracking branch 'upstream/main' into false-positive-unba…
Neowizard Aug 30, 2023
f6becf1
Explicitly state the cases handled by the end of `visit_for` in `vari…
Neowizard Aug 30, 2023
3989730
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 30, 2023
fb2f253
Refactored conditions for readability when detecting ubnalanced-dict-…
Neowizard Aug 30, 2023
0fe5f2e
Fixed type'o in variable name variables.py
Neowizard Sep 3, 2023
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
5 changes: 5 additions & 0 deletions doc/whatsnew/fragments/8156.false_positive
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
47 changes: 39 additions & 8 deletions pylint/checkers/variables.py
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,23 @@ 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) and 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

is_stared_targets = any(isinstance(target, nodes.Starred) for target in targets)
Neowizard marked this conversation as resolved.
Show resolved Hide resolved
for value in values:
mbyrnepr2 marked this conversation as resolved.
Show resolved Hide resolved
value_length = self._get_value_length(value)
is_valid_star_unpack = is_stared_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 +2972,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)):
mbyrnepr2 marked this conversation as resolved.
Show resolved Hide resolved
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 +3010,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
32 changes: 32 additions & 0 deletions tests/functional/u/unbalanced/unbalanced_dict_unpacking.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,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
13 changes: 9 additions & 4 deletions tests/functional/u/unbalanced/unbalanced_dict_unpacking.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,20 @@ 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:23:0:24:8::"Possible unbalanced dict unpacking with {1: 2}.items(): left side has 7 labels, right side has 2 values":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:67:0:68:14::"Possible unbalanced dict unpacking with {'key': 'value'}.values(): left side has 2 labels, right side has 5 values":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 1 value":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 1 value":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:94:0:95: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:97:0:98: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:100:0:101: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:103:0:104: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:106:0:107: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