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

Regression no-member with inherited AnnAssign nodes #7631

Closed
cdce8p opened this issue Oct 16, 2022 · 7 comments · Fixed by #7632
Closed

Regression no-member with inherited AnnAssign nodes #7631

cdce8p opened this issue Oct 16, 2022 · 7 comments · Fixed by #7632
Assignees
Labels
Astroid Related to astroid Regression
Milestone

Comments

@cdce8p
Copy link
Member

cdce8p commented Oct 16, 2022

Regression from #7509
The false-positive is actually caused by an issue in astroid. In particular with https://github.com/PyCQA/astroid/blob/v2.12.11/astroid/nodes/scoped_nodes/scoped_nodes.py#L2570-L2577

Opened this so I don't forget to add the example as regression test once the issue is fixed.

from __future__ import annotations

class Base:
    attr: int | None = None

class Parent(Base):
    attr: int

class Child(Parent):
    attr = 2

    def __init__(self):
        self.attr = self.attr | 4  # false-positive
test.py:13:20: E1101: Instance of 'Child' has no 'attr' member (no-member)
@cdce8p cdce8p added Astroid Related to astroid Regression labels Oct 16, 2022
@cdce8p cdce8p added this to the 2.16.0 milestone Oct 16, 2022
@cdce8p cdce8p self-assigned this Oct 16, 2022
@cdce8p cdce8p changed the title Regression no-member with inherited AnnAssign nodes Regression no-member with inherited AnnAssign nodes Oct 16, 2022
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.0, 2.15.5 Oct 17, 2022
@mbyrnepr2
Copy link
Member

Thanks for your work. I thought I would share this discussion because it sounds like we are moving away from adding these tests to Pylint which relate to astroid fixes; I could have misunderstood of course!

@cdce8p
Copy link
Member Author

cdce8p commented Oct 17, 2022

I thought I would share this discussion because it sounds like we are moving away from adding these tests to Pylint which relate to astroid fixes; I could have misunderstood of course!

Thanks for the link. Tbh though I don't fully agree. Test runtime isn't a good argument when the primer tests take 45min 😄 Whereas in astroid we only test a specific functionality, the pylint tests are much boarder. If we have a good library of acceptable code, we can much more easily prevent errors when changing other stuff.

Just to give one example, I only found the metaclass issue from pylint-dev/astroid#1836 because of the pylint test suite. Astroid tests for the original PR were green.

@Pierre-Sassoulas
Copy link
Member

As I said in the discussion you linked, we need to think about it each time imo. If the pipeline time is such a problem we can separate a lot of things and test them in another repository (pylint.testutil, for example is useless for pylint direct users and the first one I would do.) Splitting primers in 3 instead of 2 would also work as it's the bottleneck right now.

@cdce8p
Copy link
Member Author

cdce8p commented Oct 17, 2022

If the pipeline time is such a problem we can separate a lot of things and test them in another repository (pylint.testutil, for example is useless for pylint direct users and the first one I would do.) Splitting primers in 3 instead of 2 would also work as it's the bottleneck right now.

I don't think it's an issue. ~5min is completely fine. We could also use something like https://pypi.org/project/pytest-xdist/ to speed them up if it ever becomes a problem.

@DanielNoord
Copy link
Collaborator

I was mainly referring to local environments. It is not so much about speeding up CI, but speeding up fixing small regressions locally which you want to test before pushing to Github.
Imo, it is annoying to wait on tests locally that are already tested in the library that actually causes them.

@Pierre-Sassoulas
Copy link
Member

Imo, it is annoying to wait on tests locally that are already tested in the library that actually causes them.

Sure, we just need to consider the amount of code that uses the astroid fix in pylint. Sometime there is nothing so a test in astroid is enough, but other time there's a quite a lot of checker code and plumbing that also need to be tested.

@Pierre-Sassoulas
Copy link
Member

Look like this test was indeed required because it's failing on Windows in pylint and this wasn't caught in astroid :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Astroid Related to astroid Regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants