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: pylint-dev/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: @property members defined in metaclasses of a base class are not correctly inferred #940
   Open MR: Inferring property fields in a class context when metaclass is present #941
   Status: Inferring property fields in a class context when metaclass is present #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 Inferring property fields in a class context when metaclass is present #941 to fix the issue with Enum.__members__ and added a MR to pylint for the corresponding tests
   Update regression tests for PyCQA/astroid#940 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: Delayed attribute assignment to object() may cause incorrect inference of instance attributes #945
   Open MR: Update _can_assign_attr to return False for builtins.object #946
   Open MR pylint: Add regression tests of instance attribute inference on builtins.object pylint#4348
   Fixed pylint issues: Regression with not-callable pylint#3595, not-callable: false positive due to conflit between "next" user-function and "collections.abc" pylint#3970, False positive E1102 when adding a non-callable attribute to object pylint#4221, False positive on E1102 (not-callable) for methods called prev or next 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 infer_stmts cannot infer multiple uses of the same AssignName #926 and Update _can_assign_attr to return False for builtins.object #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 Fix strong references to mutable objects in context.clone #927 and Update _can_assign_attr to return False for builtins.object #946
   Open Issue: member lookup with Generic base class in 2.5.3  #942
   MR for test cases: Add regression test for no-member with generic base class pylint#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): Add regression tests for inference bug repros pylint#4387

PRs with fixed test cases

#992
pylint-dev/pylint#4325
pylint-dev/pylint#4348
pylint-dev/pylint#4471
pylint-dev/pylint#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
pylint-dev/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 Fix strong references to mutable objects in context.clone #927 and Inferring property fields in a class context when metaclass is present #941? I think the ChangeLog entries need to be moved as well.
• @hippo91 Could you take a look at Inferring property fields in a class context when metaclass is present #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 (pylint-dev/pylint#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.