Skip to content

False negative - tuple unpacking #5708

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

Merged
Merged
Show file tree
Hide file tree
Changes from 3 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
4 changes: 4 additions & 0 deletions ChangeLog
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ Release date: TBA

Closes #5281

* Fixed false negative ``unpacking-non-sequence`` when value is an empty list.

Closes #5707

* Fixed false positive ``consider-using-dict-comprehension`` when creating a dict
using a list of tuples where key AND value vary depending on the same condition.

Expand Down
22 changes: 13 additions & 9 deletions pylint/checkers/variables.py
Original file line number Diff line number Diff line change
Expand Up @@ -2400,15 +2400,8 @@ def _check_unpacking(self, inferred, node, targets):
return

# Attempt to check unpacking is properly balanced
values: Optional[List] = None
if isinstance(inferred, (nodes.Tuple, nodes.List)):
values = inferred.itered()
elif isinstance(inferred, astroid.Instance) and any(
ancestor.qname() == "typing.NamedTuple" for ancestor in inferred.ancestors()
):
values = [i for i in inferred.values() if isinstance(i, nodes.AssignName)]

if values:
values = self._get_values_to_unpack(inferred)
if values is not None:
if len(targets) != len(values):
# Check if we have starred nodes.
if any(isinstance(target, nodes.Starred) for target in targets):
Expand All @@ -2430,6 +2423,17 @@ def _check_unpacking(self, inferred, node, targets):
args=(_get_unpacking_extra_info(node, inferred),),
)

@staticmethod
def _get_values_to_unpack(node: nodes.Assign) -> Optional[List]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add typing to the List? I guess:

Suggested change
def _get_values_to_unpack(node: nodes.Assign) -> Optional[List]:
def _get_values_to_unpack(node: nodes.Assign) -> Optional[List[nodes.AssignName]]:

Or:

Suggested change
def _get_values_to_unpack(node: nodes.Assign) -> Optional[List]:
def _get_values_to_unpack(node: nodes.Assign) -> Optional[List[nodes.NodeNG]]:

Copy link
Member Author

@mbyrnepr2 mbyrnepr2 Jan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point; I think it is more accurate to use Optional[List[nodes.NodeNG]] since the nodes which are unpacked won't necessarily be of type nodes.AssignName (they could be a list of node.Const nodes, for example in the case of x, y, z = 1, 2, 3).

Moreover we can make the function name more accurate (_nodes_to_unpack?) & the type of the parameter should be nodes.NodeNG since this value originates from the returned value of the safe_infer method.

"""Return the list of values of the `Assign` node"""
if isinstance(node, (nodes.Tuple, nodes.List)):
return node.itered()
if isinstance(node, astroid.Instance) and any(
ancestor.qname() == "typing.NamedTuple" for ancestor in node.ancestors()
):
return [i for i in node.values() if isinstance(i, nodes.AssignName)]
return None

def _check_module_attrs(self, node, module, module_names):
"""check that module_names (list of string) are accessible through the
given module
Expand Down
63 changes: 45 additions & 18 deletions tests/functional/u/unbalanced_tuple_unpacking.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,85 +5,99 @@

# pylint: disable=missing-class-docstring, missing-function-docstring, using-constant-test, useless-object-inheritance,import-outside-toplevel


def do_stuff():
"""This is not right."""
first, second = 1, 2, 3 # [unbalanced-tuple-unpacking]
first, second = 1, 2, 3 # [unbalanced-tuple-unpacking]
return first + second


def do_stuff1():
"""This is not right."""
first, second = [1, 2, 3] # [unbalanced-tuple-unpacking]
first, second = [1, 2, 3] # [unbalanced-tuple-unpacking]
return first + second


def do_stuff2():
"""This is not right."""
(first, second) = 1, 2, 3 # [unbalanced-tuple-unpacking]
(first, second) = 1, 2, 3 # [unbalanced-tuple-unpacking]
return first + second


def do_stuff3():
"""This is not right."""
first, second = range(100)
return first + second


def do_stuff4():
""" This is right """
"""This is right"""
first, second = 1, 2
return first + second


def do_stuff5():
""" This is also right """
"""This is also right"""
first, second = (1, 2)
return first + second


def do_stuff6():
""" This is right """
"""This is right"""
(first, second) = (1, 2)
return first + second


def temp():
""" This is not weird """
"""This is not weird"""
if True:
return [1, 2]
return [2, 3, 4]


def do_stuff7():
""" This is not right, but we're not sure """
"""This is not right, but we're not sure"""
first, second = temp()
return first + second


def temp2():
""" This is weird, but correct """
"""This is weird, but correct"""
if True:
return (1, 2)

if True:
return (2, 3)
return (4, 5)


def do_stuff8():
""" This is correct """
"""This is correct"""
first, second = temp2()
return first + second


def do_stuff9():
""" This is not correct """
first, second = unpack() # [unbalanced-tuple-unpacking]
"""This is not correct"""
first, second = unpack() # [unbalanced-tuple-unpacking]
return first + second


class UnbalancedUnpacking(object):
""" Test unbalanced tuple unpacking in instance attributes. """
"""Test unbalanced tuple unpacking in instance attributes."""

# pylint: disable=attribute-defined-outside-init, invalid-name, too-few-public-methods
def test(self):
""" unpacking in instance attributes """
"""unpacking in instance attributes"""
# we're not sure if temp() returns two or three values
# so we shouldn't emit an error
self.a, self.b = temp()
self.a, self.b = temp2()
self.a, self.b = unpack() # [unbalanced-tuple-unpacking]
self.a, self.b = unpack() # [unbalanced-tuple-unpacking]


def issue329(*args):
""" Don't emit unbalanced tuple unpacking if the
"""Don't emit unbalanced tuple unpacking if the
rhs of the assignment is a variable-length argument,
because we don't know the actual length of the tuple.
"""
Expand All @@ -97,6 +111,7 @@ def test_decimal():
See astroid https://bitbucket.org/logilab/astroid/issues/92/
"""
from decimal import Decimal

dec = Decimal(2)
first, second, third = dec.as_tuple()
return first, second, third
Expand All @@ -105,6 +120,7 @@ def test_decimal():
def test_issue_559():
"""Test that we don't have a false positive wrt to issue #559."""
from ctypes import c_int

root_x, root_y, win_x, win_y = [c_int()] * 4
return root_x, root_y, win_x, win_y

Expand All @@ -121,10 +137,21 @@ def my_sum(self):

def sum_unpack_3_into_4(self):
"""Attempt to unpack 3 variables into 4"""
first, second, third, fourth = self # [unbalanced-tuple-unpacking]
first, second, third, fourth = self # [unbalanced-tuple-unpacking]
return first + second + third + fourth

def sum_unpack_3_into_2(self):
"""Attempt to unpack 3 variables into 2"""
first, second = self # [unbalanced-tuple-unpacking]
first, second = self # [unbalanced-tuple-unpacking]
return first + second


def my_function(mystring):
"""The number of items on the right-hand-side of the assignment to this function is not known"""
mylist = []
for item in mystring:
mylist.append(item)
return mylist


a, b = my_function("12") # [unbalanced-tuple-unpacking]
15 changes: 8 additions & 7 deletions tests/functional/u/unbalanced_tuple_unpacking.txt
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
unbalanced-tuple-unpacking:10:4:10:27:do_stuff:"Possible unbalanced tuple unpacking with sequence (1, 2, 3): left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:15:4:15:29:do_stuff1:"Possible unbalanced tuple unpacking with sequence [1, 2, 3]: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:20:4:20:29:do_stuff2:"Possible unbalanced tuple unpacking with sequence (1, 2, 3): left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:70:4:70:28:do_stuff9:"Possible unbalanced tuple unpacking with sequence defined at line 7 of functional.u.unpacking: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:82:8:82:33:UnbalancedUnpacking.test:"Possible unbalanced tuple unpacking with sequence defined at line 7 of functional.u.unpacking: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:124:8:124:43:MyClass.sum_unpack_3_into_4:"Possible unbalanced tuple unpacking with sequence defined at line 112: left side has 4 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:129:8:129:28:MyClass.sum_unpack_3_into_2:"Possible unbalanced tuple unpacking with sequence defined at line 112: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:11:4:11:27:do_stuff:"Possible unbalanced tuple unpacking with sequence (1, 2, 3): left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:17:4:17:29:do_stuff1:"Possible unbalanced tuple unpacking with sequence [1, 2, 3]: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:23:4:23:29:do_stuff2:"Possible unbalanced tuple unpacking with sequence (1, 2, 3): left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:82:4:82:28:do_stuff9:"Possible unbalanced tuple unpacking with sequence defined at line 7 of functional.u.unpacking: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:96:8:96:33:UnbalancedUnpacking.test:"Possible unbalanced tuple unpacking with sequence defined at line 7 of functional.u.unpacking: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:140:8:140:43:MyClass.sum_unpack_3_into_4:"Possible unbalanced tuple unpacking with sequence defined at line 128: left side has 4 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:145:8:145:28:MyClass.sum_unpack_3_into_2:"Possible unbalanced tuple unpacking with sequence defined at line 128: left side has 2 label(s), right side has 3 value(s)":UNDEFINED
unbalanced-tuple-unpacking:157:0:157:24::"Possible unbalanced tuple unpacking with sequence defined at line 151: left side has 2 label(s), right side has 0 value(s)":UNDEFINED