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

Fix inference cycle in getattr when a base is an astroid.Attribute #934

Conversation

nelfin
Copy link
Contributor

@nelfin nelfin commented Apr 7, 2021

Steps

  • For new features or bug fixes, add a ChangeLog entry describing what your PR does.
  • Write a good description on what the PR does.

Description

Ref #904. getattr attempts to find values on self.ancestors, which
infers each node listed in self.bases. Removing or resetting the
context passed to ancestors allows it to infer the nodes, but opens up
the possibility of cycles through recursive definitions. We should be
able to infer the value of the base node correctly as a ClassDef. The
root of this issue stems from the fact that infer_attribute attempts
to temporarily set context.boundnode, but when unwrapping its changes
it sets the boundnode to None instead of its previous value, which
loses state and causes the inference of the Attribute node to have a
duplicate key: once when looking up the class definition (which should
have boundnode = None), and once when inferring the (other) attribute
value (which should not have boundnode = None)

Type of Changes

Type
🐛 Bug fix

Related Issue

Closes #904

@nelfin nelfin force-pushed the fix/904-inference-error-on-metaclass-attribute branch 2 times, most recently from eb2e14d to fdf69e0 Compare April 8, 2021 23:40
@nelfin nelfin force-pushed the fix/904-inference-error-on-metaclass-attribute branch from fdf69e0 to e402894 Compare May 12, 2021 21:52
@hippo91 hippo91 self-assigned this May 13, 2021
Copy link
Contributor

@hippo91 hippo91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @nelfin ! Smart and very local change covered with many tests!
I have only one remark regarding one unit test. I let you discover it.
Maybe we should test these MR against pylint master? @nelfin @cdce8p what do you think about it?

tests/unittest_inference.py Show resolved Hide resolved
Ref pylint-dev#904. `getattr` attempts to find values on `self.ancestors`, which
infers each node listed in `self.bases`. Removing or resetting the
context passed to ancestors allows it to infer the nodes, but opens up
the possibility of cycles through recursive definitions. We should be
able to infer the value of the base node correctly as a `ClassDef`. The
root of this issue stems from the fact that `infer_attribute` attempts
to temporarily set `context.boundnode`, but when unwrapping its changes
it sets the `boundnode` to `None` instead of its previous value, which
loses state and causes the inference of the `Attribute` node to have a
duplicate key: once when looking up the class definition (which should
have `boundnode = None`), and once when inferring the (other) attribute
value (which should not have `boundnode = None`)
@nelfin nelfin force-pushed the fix/904-inference-error-on-metaclass-attribute branch from e402894 to fa3731b Compare May 22, 2021 23:21
Copy link
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should test these MR against pylint master?

@hippo91 I did some tests and didn't notice anything out of the ordinary. Good to merge once the discussions are resolved.

@cdce8p cdce8p added the pylint-tested PRs that don't cause major regressions with pylint label May 23, 2021
@cdce8p cdce8p added this to the 2.5.7 milestone May 23, 2021
@hippo91
Copy link
Contributor

hippo91 commented May 24, 2021

Maybe we should test these MR against pylint master?

@hippo91 I did some tests and didn't notice anything out of the ordinary. Good to merge once the discussions are resolved.

OK. Many thanks @cdce8p. The new label pylint-tested is really a good idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pylint-tested PRs that don't cause major regressions with pylint Waiting on author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error during inference of class inheriting from another with mod.Type format
3 participants