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 the scope name prefix to prevent scope traversal in a tight loop #708

Merged
merged 3 commits into from
Jun 21, 2022
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 24 additions & 19 deletions libcst/metadata/scope_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,24 +217,12 @@ def _index(self) -> int:
return self.__index

def get_qualified_names_for(self, full_name: str) -> Set[QualifiedName]:
scope = self.scope
name_prefixes = []
while scope:
if isinstance(scope, ClassScope):
name_prefixes.append(scope.name)
elif isinstance(scope, FunctionScope):
name_prefixes.append(f"{scope.name}.<locals>")
elif isinstance(scope, ComprehensionScope):
name_prefixes.append("<comprehension>")
elif not isinstance(scope, (GlobalScope, BuiltinScope)):
raise Exception(f"Unexpected Scope: {scope}")

scope = scope.parent if scope.parent != scope else None

parts = [*reversed(name_prefixes)]
if full_name:
parts.append(full_name)
return {QualifiedName(".".join(parts), QualifiedNameSource.LOCAL)}
return {
QualifiedName(
self.scope._maybe_dotted_name(full_name),
QualifiedNameSource.LOCAL,
)
}


# even though we don't override the constructor.
Expand Down Expand Up @@ -409,6 +397,7 @@ class Scope(abc.ABC):
_assignment_count: int
_accesses_by_name: MutableMapping[str, Set[Access]]
_accesses_by_node: MutableMapping[cst.CSTNode, Set[Access]]
_name_prefix: str

def __init__(self, parent: "Scope") -> None:
super().__init__()
Expand All @@ -418,6 +407,7 @@ def __init__(self, parent: "Scope") -> None:
self._assignment_count = 0
self._accesses_by_name = defaultdict(set)
self._accesses_by_node = defaultdict(set)
self._name_prefix = ""

def record_assignment(self, name: str, node: cst.CSTNode) -> None:
target = self._find_assignment_target(name)
Expand Down Expand Up @@ -591,6 +581,11 @@ def accesses(self) -> Accesses:
"""Return an :class:`~libcst.metadata.Accesses` contains all accesses in current scope."""
return Accesses(self._accesses_by_name)

# makes a dot separated name but filters out empty strings
def _maybe_dotted_name(self, *args: Optional[str]) -> str:
# filter(None, ...) removes all falsey values (ie empty string)
return ".".join(filter(None, [self._name_prefix, *args]))


class BuiltinScope(Scope):
"""
Expand Down Expand Up @@ -667,6 +662,8 @@ def __init__(
self.name = name
self.node = node
self._scope_overwrites = {}
# pyre-fixme[4]: Attribute `_name_prefix` of class `LocalScope` has type `str` but no type is specified.
self._name_prefix = self._make_name_prefix()
Copy link
Contributor Author

@lpetre lpetre Jun 20, 2022

Choose a reason for hiding this comment

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

Pyre is unhappy with this line. Any suggestions @zsol

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a bug to me, might be worth raising with Pyre folks internally


def record_global_overwrite(self, name: str) -> None:
self._scope_overwrites[name] = self.globals
Expand Down Expand Up @@ -695,6 +692,9 @@ def __getitem__(self, name: str) -> Set[BaseAssignment]:
else:
return self.parent._getitem_from_self_or_parent(name)

def _make_name_prefix(self) -> str:
return self.parent._maybe_dotted_name(self.name, "<locals>")


# even though we don't override the constructor.
class FunctionScope(LocalScope):
Expand Down Expand Up @@ -741,6 +741,9 @@ def _contains_in_self_or_parent(self, name: str) -> bool:
"""
return self.parent._contains_in_self_or_parent(name)

def _make_name_prefix(self) -> str:
return self.parent._maybe_dotted_name(self.name)


# even though we don't override the constructor.
class ComprehensionScope(LocalScope):
Expand All @@ -755,7 +758,9 @@ class ComprehensionScope(LocalScope):
# TODO: Assignment expressions (Python 3.8) will complicate ComprehensionScopes,
# and will require us to handle such assignments as non-local.
# https://www.python.org/dev/peps/pep-0572/#scope-of-the-target
pass

def _make_name_prefix(self) -> str:
return self.parent._maybe_dotted_name("<comprehension>")


# Generates dotted names from an Attribute or Name node:
Expand Down