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

Cache ancestors #1120

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions astroid/nodes/scoped_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import io
import itertools
import typing
from collections import OrderedDict
from typing import List, Optional

from astroid import bases
Expand Down Expand Up @@ -1962,6 +1963,9 @@ def my_meth(self, arg):
# a dictionary of class instances attributes
_astroid_fields = ("decorators", "bases", "keywords", "body") # name

all_ancestors = {}
direct_ancestors = {}

decorators = None
"""The decorators that are applied to this class.

Expand Down Expand Up @@ -2331,13 +2335,26 @@ def ancestors(self, recurs=True, context=None):
:returns: The base classes
:rtype: iterable(NodeNG)
"""
if recurs and context in self.all_ancestors:
yield from self.all_ancestors[context].keys()
elif not recurs and context in self.direct_ancestors:
yield from self.direct_ancestors[context].keys()

# FIXME: should be possible to choose the resolution order
# FIXME: inference make infinite loops possible here
yielded = {self}
yielded = OrderedDict()
Copy link
Author

Choose a reason for hiding this comment

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

I think this is necessary for Python 3.6 to preserve the order of yielded ancestors: a plain dict achieves the same thing from Python 3.7+

yielded[self] = None
if context is None:
context = contextmod.InferenceContext()
if not self.bases and self.qname() != "builtins.object":
yield builtin_lookup("object")[1][0]
result = builtin_lookup("object")[1][0]
yielded[result] = None
del yielded[self]
if recurs:
self.all_ancestors[context] = yielded
else:
self.direct_ancestors[context] = yielded
yield result
return

for stmt in self.bases:
Expand All @@ -2352,7 +2369,7 @@ def ancestors(self, recurs=True, context=None):
if not baseobj.hide:
if baseobj in yielded:
continue
yielded.add(baseobj)
yielded[baseobj] = None
yield baseobj
if not recurs:
continue
Expand All @@ -2362,11 +2379,17 @@ def ancestors(self, recurs=True, context=None):
break
if grandpa in yielded:
continue
yielded.add(grandpa)
yielded[grandpa] = None
yield grandpa
except InferenceError:
continue

del yielded[self]
if recurs:
self.all_ancestors[context] = yielded
else:
self.direct_ancestors[context] = yielded

def local_attr_ancestors(self, name, context=None):
"""Iterate over the parents that define the given name.

Expand Down
2 changes: 1 addition & 1 deletion tests/unittest_scoped_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,7 @@ class Past(Present):
astroid = builder.parse(data)
past = astroid["Past"]
attr = past.getattr("attr")
self.assertEqual(len(attr), 1)
self.assertEqual(len(attr), 1, attr)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertEqual(len(attr), 1, attr)
assert len(attr) == 1

I think when using pytest this is possible (?)

Copy link
Author

Choose a reason for hiding this comment

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

Any Ideas why this might have started failing though? I had a read through getattr but since it doesn't call ancestors directly I'm going to need to dig a little more.

Copy link
Author

Choose a reason for hiding this comment

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

on main, attr is...

[<AssignName.attr l.3 at 0x[...]>]

with my changes this becomes...

[<AssignName.attr l.3 at 0x[...]>, <Const.int l.3 at 0x[...]>]

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

I suspect that using ClassDefs and InferenceContexts as keys was a bit naive: they aren't Singletons and don't implement __hash__ and __eq__ so I am only caching specific instances by object id instead of detecting when the ancestors or contexts are equivalent.

attr1 = attr[0]
self.assertIsInstance(attr1, nodes.AssignName)
self.assertEqual(attr1.name, "attr")
Expand Down