From 04bd0df100809de350be89b64bb85c3524867132 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Mon, 25 Mar 2024 14:39:51 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=91=8C=20Improve=20`external`=20role=20wa?= =?UTF-8?q?rnings=20(and=20revert=20object=20fallback)=20(#12193)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The key issue this commit seeks to address, is that existing tools / documentation often lead users to mistakenly use object types and not role names, a classic example being `function` not `func` Previously, the warning message for using e.g. `` external:function`target` `` (with `py` as the default domain), would be: ``` WARNING: role for external cross-reference not found: function ``` This gives no information to the user on how to fix the issue, even though there is actually quite an easy fix This commit adds logic to create more specific / helpful warning messages, e.g. ``` WARNING: role for external cross-reference not found in domains 'py', 'std': 'function' (perhaps you meant one of: 'py:func', 'py:obj') ``` This goes through the same original logic but, if the role is not found, it will look if the role name is actually an available object type on the domain(s), and then suggest its related roles. This commit also reverts #12133, which provided a (silent) fallback to auto convert an object type to a role name. --- CHANGES.rst | 5 +- sphinx/ext/intersphinx.py | 161 ++++++++++++++---- .../roots/test-ext-intersphinx-role/index.rst | 6 +- tests/test_extensions/test_ext_intersphinx.py | 8 +- 4 files changed, 139 insertions(+), 41 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 7a495e0c84e..f5dc2d3c412 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -30,8 +30,9 @@ Features added to annotate the return type of their ``setup`` function. Patch by Chris Sewell. -* #12133: Allow ``external`` roles to reference object types - (rather than role names). Patch by Chris Sewell. +* #12193: Improve ``external`` warnings for unknown roles. + In particular, suggest related role names if an object type is mistakenly used. + Patch by Chris Sewell. * #12131: Added :confval:`show_warning_types` configuration option. Patch by Chris Sewell. diff --git a/sphinx/ext/intersphinx.py b/sphinx/ext/intersphinx.py index abc596f998b..a8a2cf13161 100644 --- a/sphinx/ext/intersphinx.py +++ b/sphinx/ext/intersphinx.py @@ -34,6 +34,7 @@ import sphinx from sphinx.addnodes import pending_xref from sphinx.builders.html import INVENTORY_FILENAME +from sphinx.deprecation import _deprecation_warning from sphinx.errors import ExtensionError from sphinx.locale import _, __ from sphinx.transforms.post_transforms import ReferencesResolver @@ -533,17 +534,90 @@ def run(self) -> tuple[list[Node], list[system_message]]: assert self.name == self.orig_name.lower() inventory, name_suffix = self.get_inventory_and_name_suffix(self.orig_name) if inventory and not inventory_exists(self.env, inventory): - logger.warning(__('inventory for external cross-reference not found: %s'), - inventory, location=(self.env.docname, self.lineno)) + self._emit_warning( + __('inventory for external cross-reference not found: %r'), inventory + ) return [], [] - role_name = self.get_role_name(name_suffix) + domain_name, role_name = self._get_domain_role(name_suffix) + if role_name is None: - logger.warning(__('role for external cross-reference not found: %s'), name_suffix, - location=(self.env.docname, self.lineno)) + self._emit_warning( + __('invalid external cross-reference suffix: %r'), name_suffix + ) return [], [] - result, messages = self.invoke_role(role_name) + # attempt to find a matching role function + role_func: RoleFunction | None + + if domain_name is not None: + # the user specified a domain, so we only check that + if (domain := self.env.domains.get(domain_name)) is None: + self._emit_warning( + __('domain for external cross-reference not found: %r'), domain_name + ) + return [], [] + if (role_func := domain.roles.get(role_name)) is None: + msg = 'role for external cross-reference not found in domain %r: %r' + if ( + object_types := domain.object_types.get(role_name) + ) is not None and object_types.roles: + self._emit_warning( + __(f'{msg} (perhaps you meant one of: %s)'), + domain_name, + role_name, + self._concat_strings(object_types.roles), + ) + else: + self._emit_warning(__(msg), domain_name, role_name) + return [], [] + + else: + # the user did not specify a domain, + # so we check first the default (if available) then standard domains + domains: list[Domain] = [] + if default_domain := self.env.temp_data.get('default_domain'): + domains.append(default_domain) + if ( + std_domain := self.env.domains.get('std') + ) is not None and std_domain not in domains: + domains.append(std_domain) + + role_func = None + for domain in domains: + if (role_func := domain.roles.get(role_name)) is not None: + domain_name = domain.name + break + + if role_func is None or domain_name is None: + domains_str = self._concat_strings(d.name for d in domains) + msg = 'role for external cross-reference not found in domains %s: %r' + possible_roles: set[str] = set() + for d in domains: + if o := d.object_types.get(role_name): + possible_roles.update(f'{d.name}:{r}' for r in o.roles) + if possible_roles: + msg = f'{msg} (perhaps you meant one of: %s)' + self._emit_warning( + __(msg), + domains_str, + role_name, + self._concat_strings(possible_roles), + ) + else: + self._emit_warning(__(msg), domains_str, role_name) + return [], [] + + result, messages = role_func( + f'{domain_name}:{role_name}', + self.rawtext, + self.text, + self.lineno, + self.inliner, + self.options, + self.content, + ) + for node in result: if isinstance(node, pending_xref): node['intersphinx'] = True @@ -573,57 +647,74 @@ def get_inventory_and_name_suffix(self, name: str) -> tuple[str | None, str]: msg = f'Malformed :external: role name: {name}' raise ValueError(msg) - def get_role_name(self, name: str) -> tuple[str, str] | None: - """Find (if any) the corresponding ``(domain, role name)`` for *name*. - - The *name* can be either a role name (e.g., ``py:function`` or ``function``) - given as ``domain:role`` or ``role``, or its corresponding object name - (in this case, ``py:func`` or ``func``) given as ``domain:objname`` or ``objname``. + def _get_domain_role(self, name: str) -> tuple[str | None, str | None]: + """Convert the *name* string into a domain and a role name. - If no domain is given, or the object/role name is not found for the requested domain, - the 'std' domain is used. + - If *name* contains no ``:``, return ``(None, name)``. + - If *name* contains a single ``:``, the domain/role is split on this. + - If *name* contains multiple ``:``, return ``(None, None)``. """ names = name.split(':') if len(names) == 1: + return None, names[0] + elif len(names) == 2: + return names[0], names[1] + else: + return None, None + + def _emit_warning(self, msg: str, /, *args: Any) -> None: + logger.warning( + msg, + *args, + type='intersphinx', + subtype='external', + location=(self.env.docname, self.lineno), + ) + + def _concat_strings(self, strings: Iterable[str]) -> str: + return ', '.join(f'{s!r}' for s in sorted(strings)) + + # deprecated methods + + def get_role_name(self, name: str) -> tuple[str, str] | None: + _deprecation_warning( + __name__, f'{self.__class__.__name__}.get_role_name', '', remove=(9, 0) + ) + names = name.split(':') + if len(names) == 1: + # role default_domain = self.env.temp_data.get('default_domain') domain = default_domain.name if default_domain else None - name = names[0] + role = names[0] elif len(names) == 2: + # domain:role: domain = names[0] - name = names[1] + role = names[1] else: return None - if domain and (role := self.get_role_name_from_domain(domain, name)): + if domain and self.is_existent_role(domain, role): return (domain, role) - elif (role := self.get_role_name_from_domain('std', name)): + elif self.is_existent_role('std', role): return ('std', role) else: return None - def is_existent_role(self, domain_name: str, role_or_obj_name: str) -> bool: - """Check if the given role or object exists in the given domain.""" - return self.get_role_name_from_domain(domain_name, role_or_obj_name) is not None - - def get_role_name_from_domain(self, domain_name: str, role_or_obj_name: str) -> str | None: - """Check if the given role or object exists in the given domain, - and return the related role name if it exists, otherwise return None. - """ + def is_existent_role(self, domain_name: str, role_name: str) -> bool: + _deprecation_warning( + __name__, f'{self.__class__.__name__}.is_existent_role', '', remove=(9, 0) + ) try: domain = self.env.get_domain(domain_name) + return role_name in domain.roles except ExtensionError: - return None - if role_or_obj_name in domain.roles: - return role_or_obj_name - if ( - (role_name := domain.role_for_objtype(role_or_obj_name)) - and role_name in domain.roles - ): - return role_name - return None + return False def invoke_role(self, role: tuple[str, str]) -> tuple[list[Node], list[system_message]]: """Invoke the role described by a ``(domain, role name)`` pair.""" + _deprecation_warning( + __name__, f'{self.__class__.__name__}.invoke_role', '', remove=(9, 0) + ) domain = self.env.get_domain(role[0]) if domain: role_func = domain.role(role[1]) diff --git a/tests/roots/test-ext-intersphinx-role/index.rst b/tests/roots/test-ext-intersphinx-role/index.rst index 65865523496..63bccf081a6 100644 --- a/tests/roots/test-ext-intersphinx-role/index.rst +++ b/tests/roots/test-ext-intersphinx-role/index.rst @@ -35,10 +35,14 @@ - a function with explicit inventory: - :external+inv:c:func:`CFunc` or :external+inv:c:function:`CFunc` + :external+inv:c:func:`CFunc` - a class with explicit non-existing inventory, which also has upper-case in name: :external+invNope:cpp:class:`foo::Bar` +- An object type being mistakenly used instead of a role name: + + - :external+inv:c:function:`CFunc` + - :external+inv:function:`CFunc` - explicit title: :external:cpp:type:`FoonsTitle ` diff --git a/tests/test_extensions/test_ext_intersphinx.py b/tests/test_extensions/test_ext_intersphinx.py index 7c4eb1ba82f..29fb579313c 100644 --- a/tests/test_extensions/test_ext_intersphinx.py +++ b/tests/test_extensions/test_ext_intersphinx.py @@ -555,9 +555,11 @@ def test_intersphinx_role(app, warning): warnings = strip_colors(warning.getvalue()).splitlines() index_path = app.srcdir / 'index.rst' assert warnings == [ - f'{index_path}:21: WARNING: role for external cross-reference not found: py:nope', - f'{index_path}:28: WARNING: role for external cross-reference not found: nope', - f'{index_path}:39: WARNING: inventory for external cross-reference not found: invNope', + f"{index_path}:21: WARNING: role for external cross-reference not found in domain 'py': 'nope'", + f"{index_path}:28: WARNING: role for external cross-reference not found in domains 'cpp', 'std': 'nope'", + f"{index_path}:39: WARNING: inventory for external cross-reference not found: 'invNope'", + f"{index_path}:44: WARNING: role for external cross-reference not found in domain 'c': 'function' (perhaps you meant one of: 'func', 'identifier', 'type')", + f"{index_path}:45: WARNING: role for external cross-reference not found in domains 'cpp', 'std': 'function' (perhaps you meant one of: 'cpp:func', 'cpp:identifier', 'cpp:type')", f'{index_path}:9: WARNING: external py:mod reference target not found: module3', f'{index_path}:14: WARNING: external py:mod reference target not found: module10', f'{index_path}:19: WARNING: external py:meth reference target not found: inv:Foo.bar',