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: O(n³) runtime fix #185

Open
wants to merge 13 commits into
base: various_fixes
Choose a base branch
from
Open
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
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@
"@semantic-release/exec": "^6.0.3",
"@semantic-release/git": "^10.0.1",
"conventional-changelog-conventionalcommits": "^8.0.0",
"semantic-release": "^24.1.2"
"semantic-release": "^24.2.0"
}
}
206 changes: 111 additions & 95 deletions poetry.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ griffe = ">=0.47.0,<0.49"

[tool.poetry.group.dev.dependencies]
pytest = ">=7.4.3,<9.0.0"
pytest-cov = ">=4,<6"
pytest-cov = ">=4,<7"
syrupy = "^4.6.0"

[tool.poetry.group.docs.dependencies]
Expand Down
16 changes: 4 additions & 12 deletions src/safeds_stubgen/api_analyzer/_ast_visitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -1453,18 +1453,12 @@ def _check_publicity_in_reexports(self, name: str, qname: str, parent: Module |
continue

# If the whole module was reexported we have to check if the name or alias is intern
if module_is_reexported:
if module_is_reexported and not_internal and (isinstance(parent, Module) or parent.is_public):

# Check the wildcard imports of the source
for wildcard_import in reexport_source.wildcard_imports:
if (
(
(is_from_same_package and wildcard_import.module_name == module_name)
or (is_from_another_package and wildcard_import.module_name == module_qname)
)
and not_internal
and (isinstance(parent, Module) or parent.is_public)
):
if ((is_from_same_package and wildcard_import.module_name == module_name)
or (is_from_another_package and wildcard_import.module_name == module_qname)):
return True

# Check the qualified imports of the source
Expand All @@ -1475,11 +1469,9 @@ def _check_publicity_in_reexports(self, name: str, qname: str, parent: Module |
if (
qualified_import.qualified_name in {module_name, module_qname}
and (
(qualified_import.alias is None and not_internal)
qualified_import.alias is None
or (qualified_import.alias is not None and not is_internal(qualified_import.alias))
)
and not_internal
and (isinstance(parent, Module) or parent.is_public)
):
# If the module name or alias is not internal, check if the parent is public
return True
Expand Down
118 changes: 48 additions & 70 deletions src/safeds_stubgen/docstring_parsing/_docstring_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
import logging
from typing import TYPE_CHECKING, Literal

from _griffe import models as griffe_models
from griffe import load
from griffe.dataclasses import Docstring
from griffe.docstrings.dataclasses import DocstringAttribute, DocstringParameter
from griffe.docstrings.utils import parse_annotation
from griffe.enumerations import DocstringSectionKind, Parser
Expand All @@ -24,35 +24,49 @@

if TYPE_CHECKING:
from pathlib import Path

from griffe.dataclasses import Object
from mypy import nodes


class DocstringParser(AbstractDocstringParser):
def __init__(self, parser: Parser, package_path: Path):
self.parser = parser

while True:
# If a package has no __init__.py file Griffe can't parse it, therefore we check the parent
try:
self.griffe_build = load(package_path, docstring_parser=parser)
griffe_build = load(package_path, docstring_parser=parser)
break
except KeyError:
package_path = package_path.parent

self.parser = parser
self.__cached_node: str | None = None
self.__cached_docstring: Docstring | None = None
self.griffe_index: dict[str, griffe_models.Object] = {}
self._recursive_griffe_indexer(griffe_build)

def _recursive_griffe_indexer(self, griffe_build: griffe_models.Object | griffe_models.Alias) -> None:
for member in griffe_build.all_members.values():
if isinstance(
member,
griffe_models.Class | griffe_models.Function | griffe_models.Attribute | griffe_models.Alias
):
self.griffe_index[member.path] = member

if isinstance(member, griffe_models.Module | griffe_models.Class):
self._recursive_griffe_indexer(member)

def get_class_documentation(self, class_node: nodes.ClassDef) -> ClassDocstring:
griffe_node = self._get_griffe_node(class_node.fullname)
griffe_node = self.griffe_index[class_node.fullname] if class_node.fullname in self.griffe_index else None

if griffe_node is None: # pragma: no cover
raise TypeError(f"Expected a griffe node for {class_node.fullname}, got None.")
msg = (
f"Something went wrong while searching for the docstring for {class_node.fullname}. Please make sure"
" that all directories with python files have an __init__.py file.",
)
logging.warning(msg)

description = ""
docstring = ""
examples = []
if griffe_node.docstring is not None:
if griffe_node is not None and griffe_node.docstring is not None:
docstring = griffe_node.docstring.value.strip("\n")

try:
Expand All @@ -76,7 +90,7 @@ def get_function_documentation(self, function_node: nodes.FuncDef) -> FunctionDo
docstring = ""
description = ""
examples = []
griffe_docstring = self.__get_cached_docstring(function_node.fullname)
griffe_docstring = self._get_griffe_docstring(function_node.fullname)
if griffe_docstring is not None:
docstring = griffe_docstring.value.strip("\n")

Expand Down Expand Up @@ -110,9 +124,9 @@ def get_parameter_documentation(
# For constructors (__init__ functions) the parameters are described on the class
if function_name == "__init__" and parent_class_qname:
parent_qname = parent_class_qname.replace("/", ".")
griffe_docstring = self.__get_cached_docstring(parent_qname)
griffe_docstring = self._get_griffe_docstring(parent_qname)
else:
griffe_docstring = self.__get_cached_docstring(function_qname)
griffe_docstring = self._get_griffe_docstring(function_qname)

# Find matching parameter docstrings
matching_parameters = []
Expand All @@ -123,7 +137,7 @@ def get_parameter_documentation(
# https://github.com/Safe-DS/Library-Analyzer/issues/10)
if self.parser == Parser.numpy and len(matching_parameters) == 0 and function_name == "__init__":
# Get constructor docstring & find matching parameter docstrings
constructor_docstring = self.__get_cached_docstring(function_qname)
constructor_docstring = self._get_griffe_docstring(function_qname)
if constructor_docstring is not None:
matching_parameters = self._get_matching_docstrings(constructor_docstring, parameter_name, "param")

Expand All @@ -136,7 +150,7 @@ def get_parameter_documentation(
raise TypeError(f"Expected parameter docstring, got {type(last_parameter)}.")

if griffe_docstring is None: # pragma: no cover
griffe_docstring = Docstring("")
griffe_docstring = griffe_models.Docstring("")

annotation = last_parameter.annotation
if annotation is None:
Expand All @@ -154,27 +168,23 @@ def get_parameter_documentation(
description=last_parameter.description.strip("\n") or "",
)

def get_attribute_documentation(
self,
parent_class_qname: str,
attribute_name: str,
) -> AttributeDocstring:
def get_attribute_documentation(self, parent_class_qname: str, attribute_name: str) -> AttributeDocstring:
parent_class_qname = parent_class_qname.replace("/", ".")

# Find matching attribute docstrings
parent_qname = parent_class_qname
griffe_docstring = self.__get_cached_docstring(parent_qname)
griffe_docstring = self._get_griffe_docstring(parent_qname)
if griffe_docstring is None:
matching_attributes = []
griffe_docstring = Docstring("")
griffe_docstring = griffe_models.Docstring("")
else:
matching_attributes = self._get_matching_docstrings(griffe_docstring, attribute_name, "attr")

# For Numpydoc, if the class has a constructor we have to check both the class and then the constructor
# (see issue https://github.com/Safe-DS/Library-Analyzer/issues/10)
if self.parser == Parser.numpy and len(matching_attributes) == 0:
constructor_qname = f"{parent_class_qname}.__init__"
constructor_docstring = self.__get_cached_docstring(constructor_qname)
constructor_docstring = self._get_griffe_docstring(constructor_qname)

# Find matching parameter docstrings
if constructor_docstring is not None:
Expand All @@ -198,7 +208,7 @@ def get_attribute_documentation(

def get_result_documentation(self, function_qname: str) -> list[ResultDocstring]:
# Find matching parameter docstrings
griffe_docstring = self.__get_cached_docstring(function_qname)
griffe_docstring = self._get_griffe_docstring(function_qname)

if griffe_docstring is None:
return []
Expand Down Expand Up @@ -251,7 +261,7 @@ def get_result_documentation(self, function_qname: str) -> list[ResultDocstring]

@staticmethod
def _get_matching_docstrings(
function_doc: Docstring,
function_doc: griffe_models.Docstring,
name: str,
type_: Literal["attr", "param"],
) -> list[DocstringAttribute | DocstringParameter]:
Expand All @@ -278,7 +288,7 @@ def _get_matching_docstrings(
def _griffe_annotation_to_api_type(
self,
annotation: Expr | str,
docstring: Docstring,
docstring: griffe_models.Docstring,
) -> sds_types.AbstractType | None:
if isinstance(annotation, ExprName | ExprAttribute):
if annotation.canonical_path == "typing.Any":
Expand All @@ -291,6 +301,8 @@ def _griffe_annotation_to_api_type(
return sds_types.NamedType(name="float", qname="builtins.float")
elif annotation.canonical_path == "str":
return sds_types.NamedType(name="str", qname="builtins.str")
elif annotation.canonical_path == "bytes":
return sds_types.NamedType(name="bytes", qname="builtins.bytes")
elif annotation.canonical_path == "list":
return sds_types.ListType(types=[])
elif annotation.canonical_path == "tuple":
Expand Down Expand Up @@ -403,49 +415,15 @@ def _remove_default_from_griffe_annotation(self, annotation: str) -> str:
return annotation.split(", default")[0]
return annotation

def _get_griffe_node(self, qname: str) -> Object | None:
node_qname_parts = qname.split(".")
griffe_node = self.griffe_build
for part in node_qname_parts:
if part in griffe_node.modules:
griffe_node = griffe_node.modules[part]
elif part in griffe_node.classes:
griffe_node = griffe_node.classes[part]
elif part in griffe_node.functions:
griffe_node = griffe_node.functions[part]
elif part in griffe_node.attributes:
griffe_node = griffe_node.attributes[part]
elif part == "__init__" and griffe_node.is_class:
return None
elif griffe_node.name == part:
continue
else: # pragma: no cover
msg = (
f"Something went wrong while searching for the docstring for {qname}. Please make sure"
" that all directories with python files have an __init__.py file.",
)
logging.warning(msg)

return griffe_node

def __get_cached_docstring(self, qname: str) -> Docstring | None:
"""
Return the Docstring for the given function node.
def _get_griffe_docstring(self, qname: str) -> griffe_models.Docstring | None:
griffe_node = self.griffe_index[qname] if qname in self.griffe_index else None

It is only recomputed when the function node differs from the previous one that was passed to this function.
This avoids reparsing the docstring for the function itself and all of its parameters.
if griffe_node is not None:
return griffe_node.docstring

On Lars's system this caused a significant performance improvement: Previously, 8.382s were spent inside the
function get_parameter_documentation when parsing sklearn. Afterward, it was only 2.113s.
"""
if self.__cached_node != qname or qname.endswith("__init__"):
self.__cached_node = qname

griffe_node = self._get_griffe_node(qname)
if griffe_node is not None:
griffe_docstring = griffe_node.docstring
self.__cached_docstring = griffe_docstring
else:
self.__cached_docstring = None

return self.__cached_docstring
msg = (
f"Something went wrong while searching for the docstring for {qname}. Please make sure"
" that all directories with python files have an __init__.py file.",
)
logging.warning(msg)
return None
Loading