From dda690b06de1716096a6b5b6fcc6a484b5fac1d7 Mon Sep 17 00:00:00 2001 From: Jeremy Maitin-Shepard Date: Fri, 2 Aug 2024 15:33:38 -0700 Subject: [PATCH] python/apigen: Fix support for pybind11 overloaded functions This was accidentally broken by https://github.com/jbms/sphinx-immaterial/pull/357. --- sphinx_immaterial/apidoc/python/apigen.py | 87 ++++++++++++------- .../sphinx_immaterial_pybind11_issue_134.cpp | 18 ++++ tests/python_apigen_test.py | 38 ++++++++ .../Example.bar_bool.json | 7 ++ .../Example.bar_int.json | 7 ++ .../Example.foo_bool.json | 9 ++ .../Example.foo_int.json | 9 ++ 7 files changed, 146 insertions(+), 29 deletions(-) create mode 100644 tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_bool.json create mode 100644 tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_int.json create mode 100644 tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_bool.json create mode 100644 tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_int.json diff --git a/sphinx_immaterial/apidoc/python/apigen.py b/sphinx_immaterial/apidoc/python/apigen.py index 83009d4ee..65748ad04 100644 --- a/sphinx_immaterial/apidoc/python/apigen.py +++ b/sphinx_immaterial/apidoc/python/apigen.py @@ -119,10 +119,10 @@ class ParsedOverload(NamedTuple): overload_id: Optional[str] = None """Overload id specified in the docstring. - If there is just a single overload, will be `None`. Otherwise, if no overload - id is specified, a warning is produced and the index of the overload, - i.e. "1", "2", etc., is used as the id. - """ + If there is just a single overload, will be `None`. Otherwise, if no overload + id is specified, a warning is produced and the index of the overload, + i.e. "1", "2", etc., is used as the id. + """ def _extract_field(doc: str, field: str) -> Tuple[str, Optional[str]]: @@ -1613,6 +1613,49 @@ def _summarize_rst_content(content: List[str]) -> List[str]: return content[:i] +class _SiblingMatchKey(NamedTuple): + """Lookup key for finding siblings of a `_MemberDocumenterEntry`. + + Normally siblings are determined based on the identity of the associated + Python object. + + However, in the case of pybind11-overloaded functions, since all overloads + share the same Python object, the overload_id must also be taken into + account. + """ + + obj: Any + overload_id: Optional[str] + + def __hash__(self): + return hash((id(self.obj), self.overload_id)) + + def __eq__(self, other): + if not isinstance(other, _SiblingMatchKey): + return False + return self.obj is other.obj and self.overload_id == other.overload_id + + +def _get_sibling_match_key(entry: _MemberDocumenterEntry) -> Optional[_SiblingMatchKey]: + obj = None + if not isinstance( + entry.documenter, + ( + sphinx.ext.autodoc.FunctionDocumenter, + sphinx.ext.autodoc.MethodDocumenter, + sphinx.ext.autodoc.PropertyDocumenter, + ), + ): + return None + obj = entry.documenter.object + overload_id: Optional[str] = None + overload = entry.overload + if overload is not None: + overload_id = overload.overload_id + + return _SiblingMatchKey(obj, overload_id) + + class _ApiEntityCollector: def __init__( self, @@ -1717,7 +1760,10 @@ def document_members(*args, **kwargs): ) ] else: - signatures = entry.documenter.format_signature().split("\n") + if primary_entity is None: + signatures = entry.documenter.format_signature().split("\n") + else: + signatures = primary_entity.signatures overload_id: Optional[str] = None if entry.overload is not None: @@ -1762,34 +1808,17 @@ def collect_documenter_members( ) -> List[_ApiEntityMemberReference]: members: List[_ApiEntityMemberReference] = [] - object_to_api_entity_member_map: Dict[ - int, Tuple[Any, _ApiEntityMemberReference] - ] = {} + sibling_match_map: dict[_SiblingMatchKey, _ApiEntityMemberReference] = {} for entry in _get_documenter_members( self.app, documenter, canonical_full_name=canonical_object_name ): - obj = None - if isinstance( - entry.documenter, - ( - sphinx.ext.autodoc.FunctionDocumenter, - sphinx.ext.autodoc.MethodDocumenter, - sphinx.ext.autodoc.PropertyDocumenter, - ), - ): - obj = entry.documenter.object - obj_and_primary_sibling_member: Optional[ - Tuple[Any, _ApiEntityMemberReference] - ] = None + sibling_match_key = _get_sibling_match_key(entry) primary_sibling_entity: Optional[_ApiEntity] = None primary_sibling_member: Optional[_ApiEntityMemberReference] = None - if obj is not None: - obj_and_primary_sibling_member = object_to_api_entity_member_map.get( - id(obj) - ) - if obj_and_primary_sibling_member is not None: - primary_sibling_member = obj_and_primary_sibling_member[1] + if (sibling_match_key := _get_sibling_match_key(entry)) is not None: + primary_sibling_member = sibling_match_map.get(sibling_match_key) + if primary_sibling_member is not None: primary_sibling_entity = self.entities[ primary_sibling_member.canonical_object_name ] @@ -1816,8 +1845,8 @@ def collect_documenter_members( member_canonical_object_name, True ) else: - if obj is not None: - object_to_api_entity_member_map[id(obj)] = (obj, member) + if sibling_match_key is not None: + sibling_match_map[sibling_match_key] = member members.append(member) child.parents.append(member) diff --git a/tests/issue_134/sphinx-immaterial-pybind11-issue-134/sphinx_immaterial_pybind11_issue_134.cpp b/tests/issue_134/sphinx-immaterial-pybind11-issue-134/sphinx_immaterial_pybind11_issue_134.cpp index 591c9db32..3de655bc7 100644 --- a/tests/issue_134/sphinx-immaterial-pybind11-issue-134/sphinx_immaterial_pybind11_issue_134.cpp +++ b/tests/issue_134/sphinx-immaterial-pybind11-issue-134/sphinx_immaterial_pybind11_issue_134.cpp @@ -30,6 +30,24 @@ PYBIND11_MODULE(sphinx_immaterial_pybind11_issue_134, m) { [](const Example& self) -> int { return 42; }); } + cls.def( + "foo", [](Example& self, int x) { return 1; }, R"docstr( +Int overload. + +Overload: + int +)docstr"); + + cls.def( + "foo", [](Example& self, bool x) { return 1; }, R"docstr( +Bool overload. + +Overload: + bool +)docstr"); + + cls.attr("bar") = cls.attr("foo"); + cls.def_readonly("is_set_by_init", &Example::is_set_by_init, R"docstr( This read-only ``bool`` attribute is set by the constructor. )docstr"); diff --git a/tests/python_apigen_test.py b/tests/python_apigen_test.py index a5c8eb63d..f0013cfb3 100644 --- a/tests/python_apigen_test.py +++ b/tests/python_apigen_test.py @@ -1,8 +1,10 @@ +import json import pathlib import pytest import sphinx + from sphinx_immaterial.apidoc.python.apigen import _get_api_data if sphinx.version_info < (7, 2): @@ -184,5 +186,41 @@ def test_type_params(apigen_make_app): ) app.build() print(app._status.getvalue()) + assert not app._warning.getvalue() + + +def test_pybind11_overloaded_function(apigen_make_app, snapshot): + testmod = "sphinx_immaterial_pybind11_issue_134" + app = apigen_make_app( + confoverrides=dict( + python_apigen_modules={ + testmod: "api/", + }, + ), + ) + print(app._status.getvalue()) print(app._warning.getvalue()) assert not app._warning.getvalue() + + data = _get_api_data(app.env) + + def get_entity_info(key): + entity = data.entities[key] + return json.dumps( + { + k: getattr(entity, k) + for k in ["signatures", "primary_entity", "siblings"] + }, + indent=2, + ) + + for name in [ + "Example.foo(int)", + "Example.foo(bool)", + "Example.bar(int)", + "Example.bar(bool)", + ]: + snapshot.assert_match( + get_entity_info(f"{testmod}.{name}"), + name.replace("(", "_").replace(")", "") + ".json", + ) diff --git a/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_bool.json b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_bool.json new file mode 100644 index 000000000..75f1ab909 --- /dev/null +++ b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_bool.json @@ -0,0 +1,7 @@ +{ + "signatures": [ + "(self: sphinx_immaterial_pybind11_issue_134.Example, arg0: bool) -> int" + ], + "primary_entity": false, + "siblings": null +} \ No newline at end of file diff --git a/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_int.json b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_int.json new file mode 100644 index 000000000..e569ee038 --- /dev/null +++ b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.bar_int.json @@ -0,0 +1,7 @@ +{ + "signatures": [ + "(self: sphinx_immaterial_pybind11_issue_134.Example, arg0: int) -> int" + ], + "primary_entity": false, + "siblings": null +} \ No newline at end of file diff --git a/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_bool.json b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_bool.json new file mode 100644 index 000000000..be751c6eb --- /dev/null +++ b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_bool.json @@ -0,0 +1,9 @@ +{ + "signatures": [ + "(self: sphinx_immaterial_pybind11_issue_134.Example, arg0: bool) -> int" + ], + "primary_entity": true, + "siblings": { + "sphinx_immaterial_pybind11_issue_134.Example.bar(bool)": true + } +} \ No newline at end of file diff --git a/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_int.json b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_int.json new file mode 100644 index 000000000..2ba7704b3 --- /dev/null +++ b/tests/snapshots/python_apigen_test/test_pybind11_overloaded_function/Example.foo_int.json @@ -0,0 +1,9 @@ +{ + "signatures": [ + "(self: sphinx_immaterial_pybind11_issue_134.Example, arg0: int) -> int" + ], + "primary_entity": true, + "siblings": { + "sphinx_immaterial_pybind11_issue_134.Example.bar(int)": true + } +} \ No newline at end of file