Progress ``context.clone``

[cdce8p]

I was looking at the MR to fix context.clone and noticed that I didn't really knew anymore what the current status is, which bugs exist, and which have been fixed.

Lets try to gather the information here. Ideally that will make reviewing and working on the remaining issues a lot easier. /CC: @hippo91, @nelfin, @Pierre-Sassoulas

---

Original MR

Fix strong references to mutable objects in context.clone #927 Idea: Fix cycles in context.path Fixes: #926 pylint MR to update tests: PyCQA/pylint#4325

Issues

1. unsupported-membership-test => Fixed Reason: property members defined on a metaclass were not inferred as data descriptors in a class context on derived classes (e.g. EnumMeta -> Enum -> ExampleEnum(Enum)) Ideas: related to brain_namedtuple_enum? Open Issue: #940 Open MR: #941 Status: #941 fixes the underlying issue, but in the specific case of Enum classes, the __members__ property reverts to being l Uninferable. This means that unsupported-membership-test will not be raised, but also any checks on the actual content of __members__ for subclass-Enum class definitions are not otherwise useful. I have a wip fix for brain_namedtuple_enum at https://github.com/nelfin/astroid/tree/fix/XXX-enum-class-members-brain but I hadn't progressed any further than that @nelfin: I've updated #941 to fix the issue with Enum.__members__ and added a MR to pylint for the corresponding tests PyCQA/pylint#4466 Assigned: -

from enum import Enum

class MyEnum(Enum):
    CONST1 = "const"

def name_in_enum(name):
    # false-positive: unsupported-membership-test
    if name in MyEnum.__members__:
        return

2. not-callable (1) => Fixed Reason: delayed_assattr allowed setting attributes on the bultins.object class. Incorrect inference of a type in the collections.OrderedDict.pop method (imported by typing, hence some observations) meant that object.prev = None and object.next = None had been "set". (see the sentinel __marker = object() on OrderedDict and its usage in pop and __delitem__) Open Issue: #945 Open MR: #946 Open MR pylint: PyCQA/pylint#4348 Fixed pylint issues: PyCQA/pylint#3595, PyCQA/pylint#3970, PyCQA/pylint#4221, PyCQA/pylint#4232 Status: Fixed Assigned: @nelfin

class Example:
    def func(self):
        pass

whatthe = object()
whatthe.func = None

ex = Example()
ex.func()   # false-positive: not-callable

3. not-callable (2) Not a regression with #926 and #946, that test case fails on current master without those changes. Reason: data['abc'] is inferred as None. The reason seems to be that the name lookup does only find the initial assignment. Thus the change to lambda: ... isn't know during the check. A fix would probably require changing the infer_name and lookup methods. Status: ? Assigned: -

data = {
    'abc': None,
}
data['abc'] = lambda: print("Callback called")
data['abc']()  # false-positive `not-callable`

CONST = "my_constant"

4. invalid-sequence-index Reason: instance_getitem only returns the first call result. Since pylint safe_infer only sees one type it assumes that it can go ahead with the invalid-sequence-index check Status: No fix yet, need to discuss. I assume that we should "fix"/update instance_getitem to return all call results or Uninferable if there are multiple types? Should instance_getitem just return method.infer_call_result(...) instead of next(method.infer_call_result(...)). Then pylint _check_sequence_index should be updated to check that all inferred types are sequences (a la no-member checking if any inferred type has that member) Assigned: -

class DynamicGetitem:
    def __getitem__(self, key):
        if key == 'attributes':
            return []
        return {'world': 123}

ex = DynamicGetitem()
ex['hello']['world']
#  E1126: Sequence index is not an int, slice, or instance with __index__ (invalid-sequence-index)

5. no-member => Fixed Reason: Regression noticed after change in inference of typing.Generic Status: Fixed with #927 and #946 Open Issue: #942 MR for test cases: https://github.com/PyCQA/pylint/pull/4471 Assigned: -

from abc import ABC
from typing import Generic, TypeVar

Anything = TypeVar("Anything")
MoreSpecific = TypeVar("MoreSpecific", str, int)

class A(ABC, Generic[Anything]):
    def a_method(self) -> None:
        print("hello")

class B(A[MoreSpecific]):
    pass
class C(B[str]):
    pass

c = C()
c.a_method()  # false-positive: no-member

Related

• pylint MR with regression test cases (not yet fixed): https://github.com/PyCQA/pylint/pull/4387

PRs with fixed test cases

https://github.com/PyCQA/astroid/pull/992 https://github.com/PyCQA/pylint/pull/4325 https://github.com/PyCQA/pylint/pull/4348 https://github.com/PyCQA/pylint/pull/4471 https://github.com/PyCQA/pylint/pull/4473

[nelfin]

I was thinking the same thing. Suggested edits:

unsupported-membership-test

Reason: property members defined on a metaclass were not inferred as data descriptors in a class context on derived classes (e.g. EnumMeta -> Enum -> ExampleEnum(Enum)) Ideas: related to brain_namedtuple_enum? Open Issue: #940 Open MR: #941 Status: #941 fixes the underlying issue, but in the specific case of Enum classes, the __members__ property reverts to being l Uninferable. This means that unsupported-membership-test will not be raised, but also any checks on the actual content of __members__ for subclass-Enum class definitions are not otherwise useful. I have a wip fix for brain_namedtuple_enum at https://github.com/nelfin/astroid/tree/fix/XXX-enum-class-members-brain but I hadn't progressed any further than that

not-callable (1)

Reason: delayed_assattr allowed setting attributes on the bultins.object class. Incorrect inference of a type in the collections.OrderedDict.pop method (imported by typing, hence some observations) meant that object.prev = None and object.next = None had been "set". (see the sentinel __marker = object() on OrderedDict and its usage in pop and __delitem__)

invalid-sequence-index

Reason: instance_getitem only returns the first call result. Since pylint safe_infer only sees one type it assumes that it can go ahead with the invalid-sequence-index check Status: No fix yet, need to discuss. I assume that we should "fix"/update instance_getitem to return all call results or Uninferable if there are multiple types? Should instance_getitem just return method.infer_call_result(...) instead of next(method.infer_call_result(...)). Then pylint _check_sequence_index should be updated to check that all inferred types are sequences (a la no-member checking if any inferred type has that member)

[nelfin]

Another suggested edit:

not-callable (2)

~Regression with #926 and #946, possibly pylint error?~ Not a regression with #926 and #946, that test case fails on current master without those changes.

[nelfin]

unsupported-membership-test

Status: I've updated #941 to fix the issue with Enum.__members__ and added a MR to pylint for the corresponding tests PyCQA/pylint#4466

[cdce8p]

@nelfin @hippo91 @Pierre-Sassoulas I was thinking again about this issue. At the moment we don't really get anywhere with it. There is #927 which itself is a good change, but it results in previously undetected errors becoming visible. @nelfin You have done a fantastic job already finding the original issue and debugging / fixing the new ones. Thanks for that!

IMO opinion we should decide how to best continue now. The way I see it, it's unlikely that we find solutions for all of these issue (at least not in the next few days). Since #927 itself is correct, I suggest to merge it and accept the errors we'll introduce.

After that #941 is probably a good followup as it fixes the unsupported-membership-test error for enums. That just leaves the not-callable (2) and invalid-sequence-index which we can probably tolerate for now.

What do you guys think?

-- If we do continue that route:

• @nelfin Would you mind updating / resolving the merge conflicts for #927 and #941? I think the ChangeLog entries need to be moved as well.
• @hippo91 Could you take a look at #941? I plan to do the same in the next days.

[nelfin]

@cdce8p: I was/have been fine to wait on sorting the underlying issues before #927, but if you're keen to merge it then I'm happy to put in some extra effort around testing and debugging before release. Re: #927 and #941, I've updated them, but the merge conflicts were all in the ChangeLog anyway

[cdce8p]

@cdce8p: I was/have been fine to wait on sorting the underlying issues before #927, but if you're keen to merge it then I'm happy to put in some extra effort around testing and debugging before release. Re: #927 and #941, I've updated them, but the merge conflicts were all in the ChangeLog anyway

Just to be clear, I don't think we need to merge them. It's just that every time I come back to it, I have lost the overview about the current status 😅 Merging what's already done might help, since I would only need to keep track of what doesn't work currently.

Tbh I would even be fine releasing astroid / pylint with the two remaining errors. I wouldn't prefer it, but sometimes we can't fix everything 🤷🏻‍♂️

[Pierre-Sassoulas]

Yeah, the new release don't need to be perfect, only better than the old one (often it's slower because we added checks). We have a blocker on pylint right now (https://github.com/PyCQA/pylint/issues/4412), but releasing astroid is possible I guess.

[hippo91]

@Pierre-Sassoulas @cdce8p @nelfin i totally agree with you. Let's merge #927. Thanks for the hard work. @cdce8p i will try to review #941 in the next few days.

[cdce8p]

Update for not-callable (2)

data = {
    'abc': None,
}
data['abc'] = lambda: print("Callback called")
data['abc']()  # false-positive `not-callable`

data['abc'] is inferred as None. The reason seems to be that the name lookup does only find the initial assignment. Thus the change to lambda: ... isn't know during the check. A fix would probably require changing the infer_name and lookup methods. However, I feel like that is out of my league, at least for now.