pylint is confusing ExitStack() and AsyncExitStack() when both are used

[kriek]

Bug description

pylint is confusing ExitStack() and AsyncExitStack() when analyzing two different modules using each.

Consider as a minimized example, the following two modules.

sync.py

from contextlib import ExitStack
def sync_stack_use():
    stack = ExitStack()
    stack.pop_all().close()

async.py

from contextlib import AsyncExitStack
def async_stack_use():
    stack = AsyncExitStack()
    stack.pop_all().aclose()

Configuration

No response

Command used

1. pylint --disable=all --enable=no-member sync.py
2. pylint --disable=all --enable=no-member async.py
3. pylint --disable=all --enable=no-member sync.py async.py
4. pylint --disable=all --enable=no-member async.py sync.py

Pylint output

1. no finding
2. no finding
3. async.py:4:4: E1101: Instance of 'ExitStack' has no 'aclose' member; maybe 'close'? (no-member)
4. sync.py:4:4: E1101: Instance of 'AsyncExitStack' has no 'close' member; maybe 'aclose'? (no-member)

Expected behavior

1. no finding
2. no finding
3. no finding
4. no finding

Pylint version

pylint 2.15.2
astroid 2.12.9
Python 3.8.5 (tags/v3.8.5:580fbb0, Jul 20 2020, 15:57:54) [MSC v.1924 64 bit (AMD64)]

OS / Environment

Windows (reproduced also on Ubuntu 20.04 LTS).

Additional dependencies

No response

[Pierre-Sassoulas]

Thank you for opening the issue. It look like it could be somewhat related to #4053

[kriek]

I don't see the relation: here, the modules are fully different (path and content).

[kriek]

The problem is around pop_all() method. In contextlib.py, both ExitStack and AsyncExitStack are inheriting their pop_all() method from _BaseExitStack whose implementation looks like:

class _BaseExitStack:
    def pop_all(self):
        # simplified
        return type(self)()

It seems pylint is inferring the type of pop_all() return type once and then re-using the same type again whatever the derived class is.
My understanding is incomplete because I failed to reproduce the issue by creating a file reproducing the inheritance diagram and type(self)() usage. Doing so, pylint always infers the base class type, unlike with (Async)ExitStack.

[Pierre-Sassoulas]

Thank you for investigating, this root cause look like something that could fix a bunch of other bugs too.

[kriek]

You are welcome, I'm glad to give back a bit to pylint.

I've pushed my investigation further and was finally able to reproduce the problem out of contextlib usage.

Consider the following two files, pylint finds 3 no-member issues in them:

• the first one is the one originally reported
• the two others are the outcome I experienced in my previous comment (pylint inferring the base type)

other.py

class Base:
    def pop_all(self):
        return type(self)()

unexpected_no_member_findings.py

import other


class A(other.Base):
    def f_a(self):
        pass


class B(other.Base):
    def f_b(self):
        pass


def f_a():
    obj = A()
    result = obj.pop_all()
    result.f_a()  # ok


def f_b():
    obj = B()
    result = obj.pop_all()
    result.f_b()  # no-member: pylint thinks result type is A
    # To workaround, comment "result.f_a()" above.


# other.Base and Base are the exact same but the pylint outcome is different
# when Base definition is local to the module using it.
class Base:
    def pop_all(self):
        return type(self)()


class AnotherA(Base):
    def f_a(self):
        pass


class AnotherB(Base):
    def f_b(self):
        pass


def f_local_a():
    obj = AnotherA()
    result = obj.pop_all()
    result.f_a()  # no-member: pylint thinks result type is Base


def f_local_b():
    obj = AnotherB()
    result = obj.pop_all()
    result.f_b()  # no-member: pylint thinks result type is Base

[Pierre-Sassoulas]

Do you want to go further and try to fix this ? I can help you. I would start by adding a functional test with the content of your example in https://github.com/PyCQA/pylint/tree/main/tests/functional/n, then launch the test with pytest -k name_of_the_file.

[kriek]

I'd like to ! Thanks for your support offer, I'll start as you recommended.

[DanielNoord]

I would start looking into the astroid internals. I think our support for type() calls is minimal as it is hard to determine what the result is based on a passed argument that can change depending on the caller.

import astroid
result1, result2  = astroid.extract_node("""
class Base:
    def return_type(self):
        return type(self)()

class A(other.Base):
    def method(self):
        return self.return_type()

class B(other.Base):
    def method(self):
        return self.return_type()

A().method()  #@
B().method()  #@
"""
)

I would probably start looking here. Both of these currently return Uninferable.

[kriek]

I was not able to investigate much yet but at least, I created the functional test demonstrating the observed issue: ce2a9f9
Should we somehow mark that functional test as xfail? I don't know if this is possible. For now, I set the buggy no-member outcome as the expected one.

My next step is to look at astroid internals.

[Pierre-Sassoulas]

It's possible to add the functional test with the problem and add a comment like "# TODO false negative / false positive see link to issue 1234" as creating the test is at least half the work 😄

[kriek]

Thanks for the hint, I amended the commit to insert such a TODO comment: 811d2c1

[kriek]

@DanielNoord It seems the type() support is not that minimal: in your code snipped, Uninferable is returned because there is an other.Base vs Base typo. After fixing it, .A instances are inferred.

The bug seems located in inference_tip.py cache. If we disable it in _inference_tip_cached() implementation, the false negatives are fixed and the types are correctly inferred for all provided code samples.

The cache key (func + node) seems incomplete.

[kriek]

Do we need that inference_tip.py cache at all? I see the InferenceContext class in context.py has its own cache which seem more context aware.

What kind of benchmark should I run to evaluate inference_tip.py cache usefulness?

[DanielNoord]

Hm, that is a good question. We should probably move that discussion to the astroid repository. A basic test would just be running pylint over a fairly large repository and seeing whether the timings are different. That's very crude but might be a good start.

[kriek]

Discussion moved to pylint-dev/astroid#1828.