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

False reports on W0644 unbalanced-dict-unpacking in pylint 2.16.0 #8156

Closed
Spindel opened this issue Feb 1, 2023 · 5 comments · Fixed by #8892 or #9093
Closed

False reports on W0644 unbalanced-dict-unpacking in pylint 2.16.0 #8156

Spindel opened this issue Feb 1, 2023 · 5 comments · Fixed by #8892 or #9093
Assignees
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@Spindel
Copy link

Spindel commented Feb 1, 2023

Bug description

When unpacking while iterating over dict.values() we seem to get a spurious warning about unbalanced dict unpacking.

Here's a test-case, result vs. result1 may require a few changes to the types, but the original code was doing a lot more dynamic things with callbacks and more.

from typing import Dict, Tuple, Callable

Inner = Callable[[int], Dict[str, int]]

def returns_dict(a:int, b:int) -> Dict[str, Tuple[int, int, int]]:
    result1 = {
        "abcdef": returns_things(a, b, "abcdef"),
        "cdef": returns_other_things(a,b, "cdef"),
    }
    result = {
        "abc": (1, 2, 3),
        "cde": (1, 2, 3),
    }
    return result

def returns_things(a:int, b:int, c:str) -> Tuple[str, str, Inner]:
    def inner(other: int) -> Dict[str, int]:
        return {"called": other}
    return f"{a}{b}", c, inner


def returns_other_things(a:int, b:int, c:str)  -> Tuple[str, str, Inner]:
    def inner(other: int) -> Dict[str, int]:
        return {"called": other}
    return f"{1}{c}", c, inner


def thing(a:int, b:int) -> None:
    bar = returns_dict(a, b)
    [print(x) for x in bar.values()]
    for x, y, _z in bar.values():
        assert x
        assert y

if __name__ == "__main__":
    thing(1, 2)

Configuration

No response

Command used

pylint pylint_testcase.py

Pylint output

************* Module pylint_testcase
pylint_testcase.py:32:4: W0644: Possible unbalanced dict unpacking with bar.values(): left side has 3 labels, right side has 2 values (unbalanced-dict-unpacking)

Expected behavior

No warning should happen

Pylint version

pylint 2.16.0
astroid 2.14.1
Python 3.11.1 (main, Jan  6 2023, 00:00:00) [GCC 12.2.1 20221121 (Red Hat 12.2.1-4)]

OS / Environment

Fedora 37, x86_64

Additional dependencies

No response

@Spindel Spindel added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Feb 1, 2023
@mbyrnepr2 mbyrnepr2 added this to the 2.16.1 milestone Feb 1, 2023
@mbyrnepr2
Copy link
Member

Thank you @Spindel. Indeed it is falling over when values() is used and there are 3 or more items to unpack:

for x, y, z in {1: ("a", "b", "c"), 2: ("d", "e", "f")}.values():
    print(x)

@mbyrnepr2 mbyrnepr2 added False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Feb 1, 2023
@Spindel
Copy link
Author

Spindel commented Feb 1, 2023

Thank you @Spindel. Indeed it is falling over when values() is used and there are 3 or more items to unpack:

for x, y, z in {1: ("a", "b", "c"), 2: ("d", "e", "f")}.values():
    print(x)

That's a much simpler reproducer, indeed.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.1, 2.16.2 Feb 2, 2023
@clavedeluna clavedeluna self-assigned this Feb 8, 2023
lukpueh added a commit to secure-systems-lab/securesystemslib that referenced this issue Mar 2, 2023
- Remove unnecessary else after raise
- Remove unnecessary else after return
- Disable unbalanced-dict-unpacking false positive
  see pylint-dev/pylint#8156

Signed-off-by: Lukas Puehringer <[email protected]>
lukpueh added a commit to secure-systems-lab/securesystemslib that referenced this issue Mar 2, 2023
- Remove unnecessary else after raise
- Remove unnecessary else after return
- Disable unbalanced-dict-unpacking false positive
  see pylint-dev/pylint#8156

Signed-off-by: Lukas Puehringer <[email protected]>
@Neowizard
Copy link
Contributor

Neowizard commented Jul 27, 2023

The following code shows the false-positive isn't caused by how many values are in the tuple, but by how many elements are in the dictionary:

# The unpacking error in this for-loop is intentional to show the warning doen't point to the actual error
for value1, value2 in {1: 'a', 2: 'b', 3: 'c'}.values():
    pass
# lint_warn.py:2:0: W0644: Possible unbalanced dict unpacking with {1: 'a', 2: 'b', 3: 'c'}.values(): left side has 2 labels, right side has 3 values (unbalanced-dict-unpacking)

The warning tells me I should have 3 values unpacked, instead of 1, despite this raising a runtime error.

for value1, value2 in {1: ('a', 'b'), 2: ('c', 'd'), 3: ('e', 'f')}.values():
    pass
#lint_warn.py:6:0: W0644: Possible unbalanced dict unpacking with {1: ('a', 'b'), 2: ('c', 'd'), 3: ('e', 'f')}.values(): left side has 2 labels, right side has 3 values (unbalanced-dict-unpacking)

The warning tells me to unpack 3 elements instead of 2.

# This unpacking error here is intentional to show there's no warning in this case
for value1, value2 in {1: ('a', 'b', 'c'), 2: ('d', 'e', 'f')}.values():
    pass

Pylint raises no warning here, despite the unpacking error.

for value1, value2 in {1: ('a', 'b'), 2: ('c', 'd'), 3: ('e', 'f'), 4: ('g', 'h')}.values():
    pass
#lint_warn.py:15:0: W0644: Possible unbalanced dict unpacking with {1: ('a', 'b'), 2: ('c', 'd'), 3: ('e', 'f'), 4: ('g', 'h')}.values(): left side has 2 labels, right side has 4 values (unbalanced-dict-unpacking)

The warning tells me to unpack 4 elements instead of 2.

It appears that pylint expects dict.values() to return a list of elements each of length equal the dict length (instead of the element's length).

To drive the point home, here's pylint warning me about W0644 when the dictionary value can't exist:

for value1, value2 in {1: does_not_exist(1), 2: does_not_exist(2), 3: does_not_exist(3)}.values():
    pass
#lint_warn.py:19:0: W0644: Possible unbalanced dict unpacking with {1: does_not_exist(1), 2: does_not_exist(2), 3: does_not_exist(3)}.values(): left side has 2 labels, right side has 3 values (unbalanced-dict-unpacking)
#lint_warn.py:19:26: E0602: Undefined variable 'does_not_exist' (undefined-variable)
#lint_warn.py:19:48: E0602: Undefined variable 'does_not_exist' (undefined-variable)
#lint_warn.py:19:70: E0602: Undefined variable 'does_not_exist' (undefined-variable)

@mbyrnepr2
Copy link
Member

mbyrnepr2 commented Jul 27, 2023

Thanks for that @Neowizard. Essentially the tuple values of the dict are treated as if they were non iterable values. It just isn’t handled at all at the moment.
Reading your comment I realise my previous comment applies to the ‘.items()’ case, not ‘values()’; I think I scanned the source too quickly at the time.

@Neowizard
Copy link
Contributor

Neowizard commented Jul 27, 2023

@mbyrnepr2 no worries. I created PR #8892 that I believe fixes the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
5 participants