Skip to content
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

Dynamic __getattr__ still leads to no-member if inference is ambiguous #9833

Open
abaumfalk opened this issue Jul 25, 2024 · 5 comments
Open
Assignees
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Good first issue Friendly and approachable by new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@abaumfalk
Copy link

Bug description

"""
The following code should lint without errors, but pylint shows:
test.py:28:6: E1101: Instance of 'A' has no 'any' member (no-member)
"""
# pylint: disable=too-few-public-methods


class A:
    """class without attributes"""


class B:
    """class with any attribute"""
    def __getattr__(self, name):
        return None
    # uncomment to remove the error
    # any = 1


def a_or_b(p) -> A | B:
    """function, which may return an instance of A or B"""
    if p == "a":
        return A()
    return B()


x: B = a_or_b("b")  # we have annotated that x is an instance of B
print(x.any)  # pylint shows E1101 (no-member)

# this does not cause an error in any case
y = B()
print(y.any)

Configuration

No response

Command used

pylint test.py

Pylint output

************* Module test
test.py:28:6: E1101: Instance of 'A' has no 'any' member (no-member)

Expected behavior

no error

Pylint version

pylint 3.2.6
astroid 3.2.4
Python 3.10.12

OS / Environment

$ uname -a
Linux xxx 5.15.0-112-generic #122-Ubuntu SMP Thu May 23 07:48:21 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux

Additional dependencies

No response

@abaumfalk abaumfalk added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jul 25, 2024
@Pierre-Sassoulas
Copy link
Member

Let's hear from another maintainer, but I feel this is too dynamic for static analysis and should be handled with the 'generated-member' option. I think we'll have to realistically closes as 'won't fix'.

@Pierre-Sassoulas Pierre-Sassoulas added Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jul 25, 2024
@jacobtylerwalls
Copy link
Member

There are a couple things going on here:

  • the type annotation x: B
  • the control flow that could determine from == "a" that only B() instances are returned

The first one is not in scope for pylint: we're not going to start trusting user type annotations. pylint is the tool you want when you want to test your annotations.

The second one is in scope, but will need a new astroid constraint. If you don't mind, I'll refocus this issue on that.

@jacobtylerwalls jacobtylerwalls changed the title false positive: explicit type annotation not working in combination with __getattr__ pylint inference lacks a constraint on if equality (if a == b:) Jul 26, 2024
@jacobtylerwalls jacobtylerwalls added Needs astroid update Needs an astroid update (probably a release too) before being mergable Needs PR This issue is accepted, sufficiently specified and now needs an implementation Needs astroid constraint Enhancement ✨ Improvement to a component Astroid Related to astroid and removed Needs decision 🔒 Needs a decision before implemention or rejection Needs astroid update Needs an astroid update (probably a release too) before being mergable labels Jul 26, 2024
@abaumfalk
Copy link
Author

There are a couple things going on here:

* the type annotation `x: B`

* the control flow that could determine from `== "a"` that only B() instances are returned

The first one is not in scope for pylint: we're not going to start trusting user type annotations. pylint is the tool you want when you want to test your annotations.

The second one is in scope, but will need a new astroid constraint. If you don't mind, I'll refocus this issue on that.

Thank you for your replies.

I disagree with refocusing on the second option, though. My example might be a bit oversimplified. In my real-world case I have a class which generates its attributes from a dynamic configuration; there is no realistic way to determine the control flow there. (But I won't deny that it might be helpful in other cases)

Concerning the first option, I might have misunderstood why pylint shows an error at all - adding an annotation seems not to change its behaviour at all, which might be ok.

But from my point of view there is an inconsistency in pylint's behaviour, which I do not understand (yet?).

Please consider the following set of examples, where I eliminated the possibility to determine the code flow.
Can you explain, why x.any and y.any lint ok, but z.any does not?

"""
************* Module test
test.py:47:6: E1101: Instance of 'A' has no 'any' member (no-member)
"""
# pylint: disable=too-few-public-methods
from time import time


class A:
    """class without attributes"""


class B:
    """class with 'any' attribute"""
    any = 1


def a_or_b() -> A | B:
    """function, which may return an instance of A or B"""
    if int(time()) % 2:
        return A()
    return B()


x = a_or_b()
print(x.any)  # no pylint error


class C:
    """class with any attribute"""
    def __getattr__(self, attr):
        return None


y = C()
print(y.any)  # no pylint error


def a_or_c() -> A | C:
    """function, which may return an instance of A or C"""
    if int(time()) % 2:
        return A()
    return C()


z = a_or_c()
print(z.any)  # E1101: Instance of 'A' has no 'any' member (no-member)

@jacobtylerwalls
Copy link
Member

Thanks for the follow-up. I'm happy to refocus on that last issue with __getattr__(). Once pylint determines there is a dynamic __getattr__ here:

if owner.has_dynamic_getattr():

... it then proceeds to ignore that inference result (C), rather than bailing out of the check entirely. So no message is emitted for C only, but for A or C, it's as if C never happened, so an error is emitted for A.

I suggest moving that has_dynamic_getattr() higher in the stack so we can return earlier. Would you like to prepare a PR?

@jacobtylerwalls jacobtylerwalls added Good first issue Friendly and approachable by new contributors False Positive 🦟 A message is emitted but nothing is wrong with the code and removed Enhancement ✨ Improvement to a component Astroid Related to astroid Needs astroid constraint labels Jul 26, 2024
@jacobtylerwalls jacobtylerwalls changed the title pylint inference lacks a constraint on if equality (if a == b:) Dynamic __getattr__ still leads to no-member if inference is ambiguous Jul 26, 2024
@abaumfalk
Copy link
Author

I had a short peek into the code base and tests and found everything very well organized and readable!
So, yes - I would like to give it a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Good first issue Friendly and approachable by new contributors Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

No branches or pull requests

3 participants