pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.25k stars 1.12k forks source link

False `unbalanced-tuple-unpacking` report #5671

Open tushar-deepsource opened 2 years ago

tushar-deepsource commented 2 years ago

Bug description

def f(x):
    l = []
    for i in x:
        l.append(i)
    return l

a, b = f('12')

Configuration

No response

Command used

pylint mytest.py | grep 0632

Pylint output

mytest.py:7:0: W0632: Possible unbalanced tuple unpacking with sequence defined at line 3: left side has 2 label(s), right side has 0 value(s) (unbalanced-tuple-unpacking)

Expected behavior

No issue raised.

Pylint version

pylint 2.11.1
astroid 2.8.6
Python 3.10.1 (main, Dec  6 2021, 22:25:40) [Clang 13.0.0 (clang-1300.0.29.3)]

OS / Environment

MacOS Monterey

Additional dependencies

No response

DanielNoord commented 2 years ago

This would require control flow inference I think as we would need to infer the value of x before we're sure what f() returns. I agree that the message currently is a bit weird (0 vs. 2), but I think the current false positive makes more sense than the false negative:

def f(x):
    l = []
    for i in x:
        l.append(i)
    return l

a, b = f('1')
tusharsadhwani commented 2 years ago

A simpler fix would be: if it can see that l's value is being changed later (seeing that there's an append or extend call in the code below for example) then it shouldn't raise the issue as it no longer knows for sure if this issue exists.

mbyrnepr2 commented 2 years ago

The warning isn't emitted with Pylint version 2.13. I can reproduce it after downgrading to 2.11 as in the report.

tusharsadhwani commented 2 years ago

That's awesome. Got any timeline for 2.13 release?

mbyrnepr2 commented 2 years ago

That's awesome. Got any timeline for 2.13 release?

Let's see if a maintainer has an answer for you on that.

Having looked at your example on 2.13 further, I've noticed a false-negative when assigning more than 2 items on the left-hand-side in the example; so that would need to be updated.

Update: Sorry for confusion @tushar-deepsource but to summarise: This issue is not fixed by 2.13; it is simply the case that 2.13 has a separate issue in the logic for unbalanced-tuple-unpacking which hides the false-positive in this report.

Pierre-Sassoulas commented 2 years ago

You can see what's still need to be done in order to release 2.13 in the milestones: https://github.com/PyCQA/pylint/milestone/49

mbyrnepr2 commented 2 years ago

A simpler fix would be: if it can see that l's value is being changed later (seeing that there's an append or extend call in the code below for example) then it shouldn't raise the issue as it no longer knows for sure if this issue exists.

I think the intention of the programmer would be to mutate the list later in the function otherwise they could use a tuple. Perhaps it is an idea to not emit the unbalanced-tuple-unpacking warning if it is a list as in this example.

tusharsadhwani commented 2 years ago

Yeah I can see that.

But still, if we're able to confidently infer that the length of the list is unchanged, I think it's still alright. The only false positives are when you reassign/append/extend/remove/pop on the list.

tushar-deepsource commented 2 years ago

Unfortunately, the current way the rule is implemented I don't see any way to do that.

We're inferring the value of the list object while unpacking it. To be able to detect modifications to the list we need to have access to the AST of the part that defines the list, not just the part that unpacks it.

The only viable solution right now would be to disable the rule on mutable sequences :(

saroad2 commented 2 years ago

The warning isn't emitted with Pylint version 2.13.

I can confirm that this bug is reproduced with pylint 2.13.7 Here's my code:

import random

def dummy_version():
    major, minor, patch = (
        random.randint(0, 10),
        random.randint(0, 10),
        random.randint(0, 10),
    )
    return f"{major}.{minor}.{patch}"

def dummy_versions(n: int):
    versions = []
    for _ in range(n):
        version = dummy_version()
        while version in versions:
            version = dummy_version()
        versions.append(version)
    return versions

version1, version2 = dummy_versions(2)
print(f"{version1=}")
print(f"{version2=}")

Running pylint on this file returns:

sandbox\__main__.py:23:0: W0632: Possible unbalanced tuple unpacking with sequence defined at line 14: left side has 2 label(s), right side has 0 value(s) (unbalanced-tuple-unpacking)
folmos-at-orange commented 3 months ago

While this issue is resolved, you may avoid the warning by using itertools.chain to build your output list.

# chain.py
import itertools

def f(x):
    l = []
    for i in x:
        l.append(i)
    return itertools.chain(l)

a, b = f('12')

pylint output

> pylint --disable=all --enable=W0632 chain.py

-------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 8.57/10, +1.43)
tusharsadhwani commented 3 months ago

you may avoid the warning by using itertools.chain to build your output list

More realistically it's probably just unable to infer the number of items returned by chain, and hence doesn't raise a warning.