methods that return a sequence yield unpacking-non-sequence

[mathiasertl]

If you unpack a sequence return value of a method, pylint falsely reports unpacking-non-sequence. It seems to work correctly with functions.

The issue is introduced with pylint==2.9.0 and happens with all later versions, including the current pylint==3.0.0a4. It does not happen with pylint==2.8.3, which is why I think this is a different issue from the similar looking #4137 (which mentions 2.7.0).

Steps to reproduce

The below example shows a minimal showcase of the false positive for methods and also shows that the error does not happen with functions.

# pylint: disable=missing-module-docstring,missing-function-docstring,unused-argument,missing-class-docstring
import typing


@typing.overload
def func_overload(arg: str) -> typing.Tuple[str, str]:
    ...


@typing.overload
def func_overload(arg: bytes) -> typing.Tuple[bytes, bytes]:
    ...


def func_overload(arg: typing.Union[str, bytes]) -> typing.Tuple[str, str]:
    return "x", "y"


class Test:
    @typing.overload
    def meth_overload(self, arg: str) -> typing.Tuple[str, str]:
        ...

    @typing.overload
    def meth_overload(self, arg: bytes) -> typing.Tuple[bytes, bytes]:
        ...

    def meth_overload(self, arg: typing.Union[str, bytes]) -> typing.Tuple[str, str]:
        return "x", "y"


X, Y = func_overload("foo")  # OK
A, B = Test().meth_overload("foo")  # "E0633: Attempting to unpack a non-sequence (unpacking-non-sequence)"

When you invoke pylint on this file, the second line will show the false positive (and not the first one):

$ pylint test.py
************* Module test
test.py:33:0: E0633: Attempting to unpack a non-sequence (unpacking-non-sequence)

------------------------------------------------------------------
Your code has been rated at 6.88/10 (previous run: 6.88/10, +0.00)

[timmartin]

From my investigation so far, this doesn't seem to be related to typing.overload. If you have the following class:

class Test:
    def meth_overload(self):
        return None

    def meth_overload(self):
        return 42

then astroid will infer that the meth_overload method returns None, not int. This seems to happen because there are multiple candidates for the attribute lookup on class Test and astroid picks the first one, not the last one.

It seems that the immediate cause of astroid picking the wrong declaration is the code in nodes/scoped_nodes.py, in ClassDef.igetattr:

        attributes = self.getattr(name, context, class_context=class_context)
        # If we have more than one attribute, make sure that those starting from
        # the second one are from the same scope. This is to account for modifications
        # to the attribute happening *after* the attribute's definition (e.g. AugAssigns on lists)
        if len(attributes) > 1:
            first_attr, attributes = attributes[0], attributes[1:]
            first_scope = first_attr.scope()
            attributes = [first_attr] + [
                attr
                for attr in attributes
                if attr.parent and attr.parent.scope() == first_scope
            ]

The two member function definitions have different scopes, so the subsequent ones are discarded and only the first (incorrect) one is kept.

This scope code was added in this commit, to deal with a case where a class member is defined and then later AugAssigned.

[timmartin]

I've made some progress on this, but my approach is hitting a lot of problems. The obvious solution seems to be to change it to:

        first_attr, attributes = attributes[0], attributes[1:]
        first_scope = first_attr.parent.scope()  # Instead of first_attr.scope()
        attributes = [first_attr] + [
            attr
            for attr in attributes
            if attr.parent and attr.parent.scope() == first_scope
        ]

so that we're comparing attr1.parent.scope() with attr2.parent.scope() rather than with attr2.scope(). This seems to make much more sense of the case where there are multiple functions with the same name. Unfortunately this doesn't work in general, because there's a bunch of code that is relying on this behaviour. For example, Astroid doesn't seem to do proper inference of what @property.setter does, rather it relies on the fact that in something like:

class Foo:
    @property
    def bar():
        #...

    @bar.setter
    def bar(self, value):
        # ...

Only the first defintion is seen. When the setter is accessed via fset there's some ad hoc code that searches for the setter value without using igetattr.

I can see how this might be addressed, but it seems like a major piece of work to chase down all these other issues. Does anyone with more astroid experience want to weigh in on whether this is a good idea?

[jacobtylerwalls]

Thanks for the writeup. Will handle in pylint-dev/astroid#1015