From c65b8775ae3183627df40bef97ff67f346dde39b Mon Sep 17 00:00:00 2001 From: James Addison Date: Fri, 26 Apr 2024 00:56:13 +0100 Subject: [PATCH 01/17] Intersphinx lookups: log warning for ambiguous target resolution. --- sphinx/ext/intersphinx/_resolve.py | 5 +++++ tests/test_extensions/test_ext_intersphinx.py | 19 +++++++++++++++++++ tests/test_util/intersphinx_data.py | 1 + 3 files changed, 25 insertions(+) diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index 08961e071b4..6aa1a96de03 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -15,6 +15,7 @@ from sphinx.ext.intersphinx._shared import LOGGER, InventoryAdapter from sphinx.locale import _, __ from sphinx.transforms.post_transforms import ReferencesResolver +from sphinx.util import logging from sphinx.util.docutils import CustomReSTDispatcher, SphinxRole if TYPE_CHECKING: @@ -30,6 +31,8 @@ from sphinx.environment import BuildEnvironment from sphinx.util.typing import Inventory, InventoryItem, RoleFunction +logger = logging.getLogger(__name__) + def _create_element_from_result(domain: Domain, inv_name: str | None, data: InventoryItem, @@ -80,6 +83,8 @@ def _resolve_reference_in_domain_by_target( target_lower = target.lower() insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) + if len(insensitive_matches) > 1: + logger.warning(f"{domain.name}: multiple matches found for {objtype}:{target}") if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] else: diff --git a/tests/test_extensions/test_ext_intersphinx.py b/tests/test_extensions/test_ext_intersphinx.py index c9d53787b57..d6a5ffb5813 100644 --- a/tests/test_extensions/test_ext_intersphinx.py +++ b/tests/test_extensions/test_ext_intersphinx.py @@ -2,6 +2,7 @@ import http.server from unittest import mock +from warnings import catch_warnings import pytest from docutils import nodes @@ -247,6 +248,24 @@ def test_missing_reference_stddomain(tmp_path, app, status, warning): assert rn.astext() == 'The Julia Domain' +def test_ambiguous_reference_warning(tmp_path, app, warning): + inv_file = tmp_path / 'inventory' + inv_file.write_bytes(INVENTORY_V2) + set_config(app, { + 'cmd': ('https://docs.python.org/', str(inv_file)), + }) + + # load the inventory + normalize_intersphinx_mapping(app, app.config) + load_mappings(app) + + # term reference (case insensitive) + node, contnode = fake_node('std', 'term', 'A TERM', 'A TERM') + missing_reference(app, app.env, node, contnode) + + assert 'multiple matches found for std:term:A TERM' in warning.getvalue() + + @pytest.mark.sphinx('html', testroot='ext-intersphinx-cppdomain') def test_missing_reference_cppdomain(tmp_path, app, status, warning): inv_file = tmp_path / 'inventory' diff --git a/tests/test_util/intersphinx_data.py b/tests/test_util/intersphinx_data.py index 042ee76d779..5154e7a54a2 100644 --- a/tests/test_util/intersphinx_data.py +++ b/tests/test_util/intersphinx_data.py @@ -32,6 +32,7 @@ foons cpp:type 1 index.html#foons - foons::bartype cpp:type 1 index.html#foons_bartype - a term std:term -1 glossary.html#term-a-term - +A term std:term -1 glossary.html#term-a-term - ls.-l std:cmdoption 1 index.html#cmdoption-ls-l - docname std:doc -1 docname.html - foo js:module 1 index.html#foo - From fe3a647ec84b933bb383c1f09d6ef457111009a1 Mon Sep 17 00:00:00 2001 From: James Addison Date: Fri, 26 Apr 2024 13:21:08 +0100 Subject: [PATCH 02/17] Refactor: use existing shared Intersphinx logger. --- sphinx/ext/intersphinx/_resolve.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index 6aa1a96de03..8dec43be6b5 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -15,7 +15,6 @@ from sphinx.ext.intersphinx._shared import LOGGER, InventoryAdapter from sphinx.locale import _, __ from sphinx.transforms.post_transforms import ReferencesResolver -from sphinx.util import logging from sphinx.util.docutils import CustomReSTDispatcher, SphinxRole if TYPE_CHECKING: @@ -31,8 +30,6 @@ from sphinx.environment import BuildEnvironment from sphinx.util.typing import Inventory, InventoryItem, RoleFunction -logger = logging.getLogger(__name__) - def _create_element_from_result(domain: Domain, inv_name: str | None, data: InventoryItem, @@ -84,7 +81,7 @@ def _resolve_reference_in_domain_by_target( insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) if len(insensitive_matches) > 1: - logger.warning(f"{domain.name}: multiple matches found for {objtype}:{target}") + LOGGER.warning(f"{domain.name}: multiple matches found for {objtype}:{target}") if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] else: From 57099f1bd1d2351874ba032edf66cd9c0feacd83 Mon Sep 17 00:00:00 2001 From: James Addison Date: Fri, 26 Apr 2024 13:36:43 +0100 Subject: [PATCH 03/17] Code review feedback: include type, subtype and location in warning. --- sphinx/ext/intersphinx/_resolve.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index 8dec43be6b5..e4abf4a7002 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -81,7 +81,12 @@ def _resolve_reference_in_domain_by_target( insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) if len(insensitive_matches) > 1: - LOGGER.warning(f"{domain.name}: multiple matches found for {objtype}:{target}") + LOGGER.warning( + f"{domain.name}: multiple matches found for {objtype}:{target}", + type='intersphinx', + subtype='external', + location=node, + ) if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] else: From c99232928c804930282776ac04bb89ad69fde6a8 Mon Sep 17 00:00:00 2001 From: James Addison Date: Fri, 26 Apr 2024 13:37:10 +0100 Subject: [PATCH 04/17] Cleanup: remove unused import from test module. --- tests/test_extensions/test_ext_intersphinx.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_extensions/test_ext_intersphinx.py b/tests/test_extensions/test_ext_intersphinx.py index d6a5ffb5813..98ff809c236 100644 --- a/tests/test_extensions/test_ext_intersphinx.py +++ b/tests/test_extensions/test_ext_intersphinx.py @@ -2,7 +2,6 @@ import http.server from unittest import mock -from warnings import catch_warnings import pytest from docutils import nodes From d9417e0b2abf5941460bf73738ac8c110a5eb6cf Mon Sep 17 00:00:00 2001 From: James Addison Date: Fri, 26 Apr 2024 13:50:51 +0100 Subject: [PATCH 05/17] Tests: refactor: extract INVENTORY_V2_AMBIGUOUS_TERMS fixture data. --- tests/test_extensions/test_ext_intersphinx.py | 8 ++++++-- tests/test_util/intersphinx_data.py | 11 ++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/tests/test_extensions/test_ext_intersphinx.py b/tests/test_extensions/test_ext_intersphinx.py index 98ff809c236..d475c60f9e7 100644 --- a/tests/test_extensions/test_ext_intersphinx.py +++ b/tests/test_extensions/test_ext_intersphinx.py @@ -19,7 +19,11 @@ from sphinx.ext.intersphinx._load import _get_safe_url, _strip_basic_auth from sphinx.util.console import strip_colors -from tests.test_util.intersphinx_data import INVENTORY_V2, INVENTORY_V2_NO_VERSION +from tests.test_util.intersphinx_data import ( + INVENTORY_V2, + INVENTORY_V2_AMBIGUOUS_TERMS, + INVENTORY_V2_NO_VERSION, +) from tests.utils import http_server @@ -249,7 +253,7 @@ def test_missing_reference_stddomain(tmp_path, app, status, warning): def test_ambiguous_reference_warning(tmp_path, app, warning): inv_file = tmp_path / 'inventory' - inv_file.write_bytes(INVENTORY_V2) + inv_file.write_bytes(INVENTORY_V2_AMBIGUOUS_TERMS) set_config(app, { 'cmd': ('https://docs.python.org/', str(inv_file)), }) diff --git a/tests/test_util/intersphinx_data.py b/tests/test_util/intersphinx_data.py index 5154e7a54a2..889645903dd 100644 --- a/tests/test_util/intersphinx_data.py +++ b/tests/test_util/intersphinx_data.py @@ -32,7 +32,6 @@ foons cpp:type 1 index.html#foons - foons::bartype cpp:type 1 index.html#foons_bartype - a term std:term -1 glossary.html#term-a-term - -A term std:term -1 glossary.html#term-a-term - ls.-l std:cmdoption 1 index.html#cmdoption-ls-l - docname std:doc -1 docname.html - foo js:module 1 index.html#foo - @@ -51,3 +50,13 @@ ''' + zlib.compress(b'''\ module1 py:module 0 foo.html#module-module1 Long Module desc ''') + +INVENTORY_V2_AMBIGUOUS_TERMS: Final[bytes] = b'''\ +# Sphinx inventory version 2 +# Project: foo +# Version: 2.0 +# The remainder of this file is compressed with zlib. +''' + zlib.compress(b'''\ +a term std:term -1 glossary.html#term-a-term - +A term std:term -1 glossary.html#term-a-term - +''') From 436297c57f0ba9600a38b1245c5ac4058493dae6 Mon Sep 17 00:00:00 2001 From: Adam Turner <9087854+aa-turner@users.noreply.github.com> Date: Mon, 29 Apr 2024 02:53:01 +0100 Subject: [PATCH 06/17] Allow translation --- sphinx/ext/intersphinx/_resolve.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index e4abf4a7002..435df6f6177 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -81,12 +81,9 @@ def _resolve_reference_in_domain_by_target( insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) if len(insensitive_matches) > 1: - LOGGER.warning( - f"{domain.name}: multiple matches found for {objtype}:{target}", - type='intersphinx', - subtype='external', - location=node, - ) + LOGGER.warning(__("%s: multiple matches found for %s:%s"), + domain.name, objtype, target, + type='intersphinx', subtype='external', location=node) if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] else: From ea7349bf24e82392c2e33097aae56f99332bbd3c Mon Sep 17 00:00:00 2001 From: James Addison <55152140+jayaddison@users.noreply.github.com> Date: Mon, 29 Apr 2024 11:17:20 +0100 Subject: [PATCH 07/17] Cleanup: remove redundant domain.name from log message line. Co-authored-by: Chris Sewell --- sphinx/ext/intersphinx/_resolve.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index 435df6f6177..e23ebb961cd 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -81,8 +81,8 @@ def _resolve_reference_in_domain_by_target( insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) if len(insensitive_matches) > 1: - LOGGER.warning(__("%s: multiple matches found for %s:%s"), - domain.name, objtype, target, + LOGGER.warning(__("multiple matches found for %s:%s"), + objtype, target, type='intersphinx', subtype='external', location=node) if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] From 72b266d4ef102028b735ba89b2773f2fdb668483 Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 12:05:35 +0100 Subject: [PATCH 08/17] Intersphinx lookups: add origin-hyperlink to log warnings about ambiguous target resolution. --- sphinx/ext/intersphinx/_resolve.py | 43 +++++++++++++++++++----------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index e23ebb961cd..6f38c3a7b59 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -64,7 +64,9 @@ def _resolve_reference_in_domain_by_target( inv_name: str | None, inventory: Inventory, domain: Domain, objtypes: Iterable[str], target: str, - node: pending_xref, contnode: TextElement) -> nodes.reference | None: + node: pending_xref, contnode: TextElement, + app: Sphinx | None = None + ) -> nodes.reference | None: for objtype in objtypes: if objtype not in inventory: # Continue if there's nothing of this kind in the inventory @@ -81,8 +83,13 @@ def _resolve_reference_in_domain_by_target( insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) if len(insensitive_matches) > 1: - LOGGER.warning(__("multiple matches found for %s:%s"), - objtype, target, + inventory_location = ( + app.config.intersphinx_mapping[inv_name][1][0] + if (inv_name and app and inv_name in app.config.intersphinx_mapping) else + "./objects.inv" + ) + LOGGER.warning(__("multiple matches found for %s:%s in <%s>"), + objtype, target, inventory_location, type='intersphinx', subtype='external', location=node) if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] @@ -103,6 +110,7 @@ def _resolve_reference_in_domain(env: BuildEnvironment, honor_disabled_refs: bool, domain: Domain, objtypes: Iterable[str], node: pending_xref, contnode: TextElement, + app: Sphinx | None = None, ) -> nodes.reference | None: obj_types: dict[str, None] = {}.fromkeys(objtypes) @@ -129,7 +137,7 @@ def _resolve_reference_in_domain(env: BuildEnvironment, # without qualification res = _resolve_reference_in_domain_by_target(inv_name, inventory, domain, objtypes, - node['reftarget'], node, contnode) + node['reftarget'], node, contnode, app) if res is not None: return res @@ -138,12 +146,14 @@ def _resolve_reference_in_domain(env: BuildEnvironment, if full_qualified_name is None: return None return _resolve_reference_in_domain_by_target(inv_name, inventory, domain, objtypes, - full_qualified_name, node, contnode) + full_qualified_name, node, contnode, app) def _resolve_reference(env: BuildEnvironment, inv_name: str | None, inventory: Inventory, honor_disabled_refs: bool, - node: pending_xref, contnode: TextElement) -> nodes.reference | None: + node: pending_xref, contnode: TextElement, + app: Sphinx | None = None + ) -> nodes.reference | None: # disabling should only be done if no inventory is given honor_disabled_refs = honor_disabled_refs and inv_name is None @@ -160,7 +170,7 @@ def _resolve_reference(env: BuildEnvironment, inv_name: str | None, inventory: I res = _resolve_reference_in_domain(env, inv_name, inventory, honor_disabled_refs, domain, objtypes, - node, contnode) + node, contnode, app) if res is not None: return res return None @@ -179,7 +189,7 @@ def _resolve_reference(env: BuildEnvironment, inv_name: str | None, inventory: I return _resolve_reference_in_domain(env, inv_name, inventory, honor_disabled_refs, domain, objtypes, - node, contnode) + node, contnode, app) def inventory_exists(env: BuildEnvironment, inv_name: str) -> bool: @@ -189,6 +199,7 @@ def inventory_exists(env: BuildEnvironment, inv_name: str) -> bool: def resolve_reference_in_inventory(env: BuildEnvironment, inv_name: str, node: pending_xref, contnode: TextElement, + app: Sphinx | None = None, ) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references. @@ -198,12 +209,13 @@ def resolve_reference_in_inventory(env: BuildEnvironment, """ assert inventory_exists(env, inv_name) return _resolve_reference(env, inv_name, InventoryAdapter(env).named_inventory[inv_name], - False, node, contnode) + False, node, contnode, app) def resolve_reference_any_inventory(env: BuildEnvironment, honor_disabled_refs: bool, node: pending_xref, contnode: TextElement, + app: Sphinx | None = None, ) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references. @@ -211,11 +223,12 @@ def resolve_reference_any_inventory(env: BuildEnvironment, """ return _resolve_reference(env, None, InventoryAdapter(env).main_inventory, honor_disabled_refs, - node, contnode) + node, contnode, app) def resolve_reference_detect_inventory(env: BuildEnvironment, node: pending_xref, contnode: TextElement, + app: Sphinx | None = None ) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references. @@ -225,7 +238,7 @@ def resolve_reference_detect_inventory(env: BuildEnvironment, is tried in that inventory with the new target. """ # ordinary direct lookup, use data as is - res = resolve_reference_any_inventory(env, True, node, contnode) + res = resolve_reference_any_inventory(env, True, node, contnode, app) if res is not None: return res @@ -237,7 +250,7 @@ def resolve_reference_detect_inventory(env: BuildEnvironment, if not inventory_exists(env, inv_name): return None node['reftarget'] = newtarget - res_inv = resolve_reference_in_inventory(env, inv_name, node, contnode) + res_inv = resolve_reference_in_inventory(env, inv_name, node, contnode, app) node['reftarget'] = target return res_inv @@ -245,7 +258,7 @@ def resolve_reference_detect_inventory(env: BuildEnvironment, def missing_reference(app: Sphinx, env: BuildEnvironment, node: pending_xref, contnode: TextElement) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references.""" - return resolve_reference_detect_inventory(env, node, contnode) + return resolve_reference_detect_inventory(env, node, contnode, app) class IntersphinxDispatcher(CustomReSTDispatcher): @@ -484,9 +497,9 @@ def run(self, **kwargs: Any) -> None: inv_name = node['inventory'] if inv_name is not None: assert inventory_exists(self.env, inv_name) - newnode = resolve_reference_in_inventory(self.env, inv_name, node, contnode) + newnode = resolve_reference_in_inventory(self.env, inv_name, node, contnode, self.app) else: - newnode = resolve_reference_any_inventory(self.env, False, node, contnode) + newnode = resolve_reference_any_inventory(self.env, False, node, contnode, self.app) if newnode is None: typ = node['reftype'] msg = (__('external %s:%s reference target not found: %s') % From 4935023ea9c6a18d83e61633322c62894b302c5f Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 14:37:00 +0100 Subject: [PATCH 09/17] Revert "Intersphinx lookups: add origin-hyperlink to log warnings about ambiguous target resolution." This reverts commit 72b266d4ef102028b735ba89b2773f2fdb668483. --- sphinx/ext/intersphinx/_resolve.py | 43 +++++++++++------------------- 1 file changed, 15 insertions(+), 28 deletions(-) diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index 6f38c3a7b59..e23ebb961cd 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -64,9 +64,7 @@ def _resolve_reference_in_domain_by_target( inv_name: str | None, inventory: Inventory, domain: Domain, objtypes: Iterable[str], target: str, - node: pending_xref, contnode: TextElement, - app: Sphinx | None = None - ) -> nodes.reference | None: + node: pending_xref, contnode: TextElement) -> nodes.reference | None: for objtype in objtypes: if objtype not in inventory: # Continue if there's nothing of this kind in the inventory @@ -83,13 +81,8 @@ def _resolve_reference_in_domain_by_target( insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) if len(insensitive_matches) > 1: - inventory_location = ( - app.config.intersphinx_mapping[inv_name][1][0] - if (inv_name and app and inv_name in app.config.intersphinx_mapping) else - "./objects.inv" - ) - LOGGER.warning(__("multiple matches found for %s:%s in <%s>"), - objtype, target, inventory_location, + LOGGER.warning(__("multiple matches found for %s:%s"), + objtype, target, type='intersphinx', subtype='external', location=node) if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] @@ -110,7 +103,6 @@ def _resolve_reference_in_domain(env: BuildEnvironment, honor_disabled_refs: bool, domain: Domain, objtypes: Iterable[str], node: pending_xref, contnode: TextElement, - app: Sphinx | None = None, ) -> nodes.reference | None: obj_types: dict[str, None] = {}.fromkeys(objtypes) @@ -137,7 +129,7 @@ def _resolve_reference_in_domain(env: BuildEnvironment, # without qualification res = _resolve_reference_in_domain_by_target(inv_name, inventory, domain, objtypes, - node['reftarget'], node, contnode, app) + node['reftarget'], node, contnode) if res is not None: return res @@ -146,14 +138,12 @@ def _resolve_reference_in_domain(env: BuildEnvironment, if full_qualified_name is None: return None return _resolve_reference_in_domain_by_target(inv_name, inventory, domain, objtypes, - full_qualified_name, node, contnode, app) + full_qualified_name, node, contnode) def _resolve_reference(env: BuildEnvironment, inv_name: str | None, inventory: Inventory, honor_disabled_refs: bool, - node: pending_xref, contnode: TextElement, - app: Sphinx | None = None - ) -> nodes.reference | None: + node: pending_xref, contnode: TextElement) -> nodes.reference | None: # disabling should only be done if no inventory is given honor_disabled_refs = honor_disabled_refs and inv_name is None @@ -170,7 +160,7 @@ def _resolve_reference(env: BuildEnvironment, inv_name: str | None, inventory: I res = _resolve_reference_in_domain(env, inv_name, inventory, honor_disabled_refs, domain, objtypes, - node, contnode, app) + node, contnode) if res is not None: return res return None @@ -189,7 +179,7 @@ def _resolve_reference(env: BuildEnvironment, inv_name: str | None, inventory: I return _resolve_reference_in_domain(env, inv_name, inventory, honor_disabled_refs, domain, objtypes, - node, contnode, app) + node, contnode) def inventory_exists(env: BuildEnvironment, inv_name: str) -> bool: @@ -199,7 +189,6 @@ def inventory_exists(env: BuildEnvironment, inv_name: str) -> bool: def resolve_reference_in_inventory(env: BuildEnvironment, inv_name: str, node: pending_xref, contnode: TextElement, - app: Sphinx | None = None, ) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references. @@ -209,13 +198,12 @@ def resolve_reference_in_inventory(env: BuildEnvironment, """ assert inventory_exists(env, inv_name) return _resolve_reference(env, inv_name, InventoryAdapter(env).named_inventory[inv_name], - False, node, contnode, app) + False, node, contnode) def resolve_reference_any_inventory(env: BuildEnvironment, honor_disabled_refs: bool, node: pending_xref, contnode: TextElement, - app: Sphinx | None = None, ) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references. @@ -223,12 +211,11 @@ def resolve_reference_any_inventory(env: BuildEnvironment, """ return _resolve_reference(env, None, InventoryAdapter(env).main_inventory, honor_disabled_refs, - node, contnode, app) + node, contnode) def resolve_reference_detect_inventory(env: BuildEnvironment, node: pending_xref, contnode: TextElement, - app: Sphinx | None = None ) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references. @@ -238,7 +225,7 @@ def resolve_reference_detect_inventory(env: BuildEnvironment, is tried in that inventory with the new target. """ # ordinary direct lookup, use data as is - res = resolve_reference_any_inventory(env, True, node, contnode, app) + res = resolve_reference_any_inventory(env, True, node, contnode) if res is not None: return res @@ -250,7 +237,7 @@ def resolve_reference_detect_inventory(env: BuildEnvironment, if not inventory_exists(env, inv_name): return None node['reftarget'] = newtarget - res_inv = resolve_reference_in_inventory(env, inv_name, node, contnode, app) + res_inv = resolve_reference_in_inventory(env, inv_name, node, contnode) node['reftarget'] = target return res_inv @@ -258,7 +245,7 @@ def resolve_reference_detect_inventory(env: BuildEnvironment, def missing_reference(app: Sphinx, env: BuildEnvironment, node: pending_xref, contnode: TextElement) -> nodes.reference | None: """Attempt to resolve a missing reference via intersphinx references.""" - return resolve_reference_detect_inventory(env, node, contnode, app) + return resolve_reference_detect_inventory(env, node, contnode) class IntersphinxDispatcher(CustomReSTDispatcher): @@ -497,9 +484,9 @@ def run(self, **kwargs: Any) -> None: inv_name = node['inventory'] if inv_name is not None: assert inventory_exists(self.env, inv_name) - newnode = resolve_reference_in_inventory(self.env, inv_name, node, contnode, self.app) + newnode = resolve_reference_in_inventory(self.env, inv_name, node, contnode) else: - newnode = resolve_reference_any_inventory(self.env, False, node, contnode, self.app) + newnode = resolve_reference_any_inventory(self.env, False, node, contnode) if newnode is None: typ = node['reftype'] msg = (__('external %s:%s reference target not found: %s') % From ab781829ef6d7fc7b7e66ee37da55b2d0f1698be Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 14:53:37 +0100 Subject: [PATCH 10/17] Intersphinx lookups: log inventory name at both load-time and ambiguous-resolution-time. --- sphinx/ext/intersphinx/_load.py | 4 +++- sphinx/ext/intersphinx/_resolve.py | 5 +++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/sphinx/ext/intersphinx/_load.py b/sphinx/ext/intersphinx/_load.py index 8e951ecd4ae..b458d6a7e18 100644 --- a/sphinx/ext/intersphinx/_load.py +++ b/sphinx/ext/intersphinx/_load.py @@ -117,7 +117,9 @@ def fetch_inventory_group( # files; remote ones only if the cache time is expired if '://' not in inv or uri not in cache or cache[uri][1] < cache_time: safe_inv_url = _get_safe_url(inv) - LOGGER.info(__('loading intersphinx inventory from %s...'), safe_inv_url) + inv_descriptor = name or 'main_inventory' + LOGGER.info(__("loading intersphinx inventory '%s' from %s..."), + inv_descriptor, safe_inv_url) try: invdata = fetch_inventory(app, uri, inv) except Exception as err: diff --git a/sphinx/ext/intersphinx/_resolve.py b/sphinx/ext/intersphinx/_resolve.py index e23ebb961cd..0a3cc8949d2 100644 --- a/sphinx/ext/intersphinx/_resolve.py +++ b/sphinx/ext/intersphinx/_resolve.py @@ -81,8 +81,9 @@ def _resolve_reference_in_domain_by_target( insensitive_matches = list(filter(lambda k: k.lower() == target_lower, inventory[objtype].keys())) if len(insensitive_matches) > 1: - LOGGER.warning(__("multiple matches found for %s:%s"), - objtype, target, + inv_descriptor = inv_name or 'main_inventory' + LOGGER.warning(__("inventory '%s': multiple matches found for %s:%s"), + inv_descriptor, objtype, target, type='intersphinx', subtype='external', location=node) if insensitive_matches: data = inventory[objtype][insensitive_matches[0]] From 88a338ab604b0ada7e1d515803424e71dc7d3c62 Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 16:51:20 +0100 Subject: [PATCH 11/17] Intersphinx inventories: add loading-time warning when ambiguous name definitions exist. --- sphinx/util/inventory.py | 27 ++++++++++++++++++++------ tests/test_util/test_util_inventory.py | 8 ++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/sphinx/util/inventory.py b/sphinx/util/inventory.py index a43fd0379ea..d1c82dbbe23 100644 --- a/sphinx/util/inventory.py +++ b/sphinx/util/inventory.py @@ -6,6 +6,7 @@ import zlib from typing import IO, TYPE_CHECKING, Callable +from sphinx.locale import __ from sphinx.util import logging BUFSIZE = 16 * 1024 @@ -86,11 +87,15 @@ def load( reader = InventoryFileReader(stream) line = reader.readline().rstrip() if line == '# Sphinx inventory version 1': - return cls.load_v1(reader, uri, joinfunc) + invdata, ambiguities = cls.load_v1(reader, uri, joinfunc) elif line == '# Sphinx inventory version 2': - return cls.load_v2(reader, uri, joinfunc) + invdata, ambiguities = cls.load_v2(reader, uri, joinfunc) else: raise ValueError('invalid inventory header: %s' % line) + for ambiguity in ambiguities: + logger.warning(__("inventory <%s> contains multiple definitions for %s"), + uri, ambiguity, type='intersphinx', subtype='external') + return invdata @classmethod def load_v1( @@ -98,7 +103,7 @@ def load_v1( stream: InventoryFileReader, uri: str, join: Callable[[str, str], str], - ) -> Inventory: + ) -> tuple[Inventory, set]: invdata: Inventory = {} projname = stream.readline().rstrip()[11:] version = stream.readline().rstrip()[11:] @@ -113,7 +118,7 @@ def load_v1( type = 'py:' + type location += '#' + name invdata.setdefault(type, {})[name] = (projname, version, location, '-') - return invdata + return invdata, frozenset() @classmethod def load_v2( @@ -121,10 +126,12 @@ def load_v2( stream: InventoryFileReader, uri: str, join: Callable[[str, str], str], - ) -> Inventory: + ) -> tuple[Inventory, set]: invdata: Inventory = {} projname = stream.readline().rstrip()[11:] version = stream.readline().rstrip()[11:] + potential_ambiguities = set() + actual_ambiguities = set() line = stream.readline() if 'zlib' not in line: raise ValueError('invalid inventory header (not compressed): %s' % line) @@ -147,12 +154,20 @@ def load_v2( # for Python modules, and the first # one is correct continue + if type in {'std:label', 'std:term'}: + # Some types require case insensitive matches: + # * 'term': https://github.com/sphinx-doc/sphinx/issues/9291 + # * 'label': https://github.com/sphinx-doc/sphinx/issues/12008 + if name.lower() in potential_ambiguities: + actual_ambiguities.add(f"{type}:{name}") + else: + potential_ambiguities.add(name.lower()) if location.endswith('$'): location = location[:-1] + name location = join(uri, location) inv_item: InventoryItem = projname, version, location, dispname invdata.setdefault(type, {})[name] = inv_item - return invdata + return invdata, actual_ambiguities @classmethod def dump( diff --git a/tests/test_util/test_util_inventory.py b/tests/test_util/test_util_inventory.py index 81d31b0ef44..2c7b0f1bf69 100644 --- a/tests/test_util/test_util_inventory.py +++ b/tests/test_util/test_util_inventory.py @@ -10,6 +10,7 @@ from tests.test_util.intersphinx_data import ( INVENTORY_V1, INVENTORY_V2, + INVENTORY_V2_AMBIGUOUS_TERMS, INVENTORY_V2_NO_VERSION, ) @@ -48,6 +49,13 @@ def test_read_inventory_v2_not_having_version(): ('foo', '', '/util/foo.html#module-module1', 'Long Module desc') +def test_ambiguous_definition_warning(warning): + f = BytesIO(INVENTORY_V2_AMBIGUOUS_TERMS) + invdata = InventoryFile.load(f, '/util', posixpath.join) + + assert 'contains multiple definitions for std:term:a' in warning.getvalue().lower() + + def _write_appconfig(dir, language, prefix=None): prefix = prefix or language os.makedirs(dir / prefix, exist_ok=True) From 8b51aaf397384a18a8804c074a1af270622fa64b Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 17:01:07 +0100 Subject: [PATCH 12/17] linting: resolve type-inconsistency warnings by returning an empty set instead of frozenset, with type hints. --- sphinx/util/inventory.py | 6 +++--- tests/test_util/test_util_inventory.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sphinx/util/inventory.py b/sphinx/util/inventory.py index d1c82dbbe23..8cb332c8c45 100644 --- a/sphinx/util/inventory.py +++ b/sphinx/util/inventory.py @@ -103,7 +103,7 @@ def load_v1( stream: InventoryFileReader, uri: str, join: Callable[[str, str], str], - ) -> tuple[Inventory, set]: + ) -> tuple[Inventory, set[str]]: invdata: Inventory = {} projname = stream.readline().rstrip()[11:] version = stream.readline().rstrip()[11:] @@ -118,7 +118,7 @@ def load_v1( type = 'py:' + type location += '#' + name invdata.setdefault(type, {})[name] = (projname, version, location, '-') - return invdata, frozenset() + return invdata, set() @classmethod def load_v2( @@ -126,7 +126,7 @@ def load_v2( stream: InventoryFileReader, uri: str, join: Callable[[str, str], str], - ) -> tuple[Inventory, set]: + ) -> tuple[Inventory, set[str]]: invdata: Inventory = {} projname = stream.readline().rstrip()[11:] version = stream.readline().rstrip()[11:] diff --git a/tests/test_util/test_util_inventory.py b/tests/test_util/test_util_inventory.py index 2c7b0f1bf69..0bdef9f67d9 100644 --- a/tests/test_util/test_util_inventory.py +++ b/tests/test_util/test_util_inventory.py @@ -51,7 +51,7 @@ def test_read_inventory_v2_not_having_version(): def test_ambiguous_definition_warning(warning): f = BytesIO(INVENTORY_V2_AMBIGUOUS_TERMS) - invdata = InventoryFile.load(f, '/util', posixpath.join) + InventoryFile.load(f, '/util', posixpath.join) assert 'contains multiple definitions for std:term:a' in warning.getvalue().lower() From 897e268a4a89c0e214a3fcbc1d7c6f429acad1b9 Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 17:01:20 +0100 Subject: [PATCH 13/17] linting: cleanup: remove unused variable. --- tests/test_util/test_util_inventory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_util/test_util_inventory.py b/tests/test_util/test_util_inventory.py index 0bdef9f67d9..2c7b0f1bf69 100644 --- a/tests/test_util/test_util_inventory.py +++ b/tests/test_util/test_util_inventory.py @@ -51,7 +51,7 @@ def test_read_inventory_v2_not_having_version(): def test_ambiguous_definition_warning(warning): f = BytesIO(INVENTORY_V2_AMBIGUOUS_TERMS) - InventoryFile.load(f, '/util', posixpath.join) + invdata = InventoryFile.load(f, '/util', posixpath.join) assert 'contains multiple definitions for std:term:a' in warning.getvalue().lower() From 7f0b37513f66f99ae2bf877a4e5f1cfb0583897a Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 17:06:55 +0100 Subject: [PATCH 14/17] Fixup: include type namespace when checking for ambiguities. --- sphinx/util/inventory.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/sphinx/util/inventory.py b/sphinx/util/inventory.py index 8cb332c8c45..0cd3b6bde21 100644 --- a/sphinx/util/inventory.py +++ b/sphinx/util/inventory.py @@ -158,10 +158,11 @@ def load_v2( # Some types require case insensitive matches: # * 'term': https://github.com/sphinx-doc/sphinx/issues/9291 # * 'label': https://github.com/sphinx-doc/sphinx/issues/12008 - if name.lower() in potential_ambiguities: - actual_ambiguities.add(f"{type}:{name}") + definition = f"{type}:{name}" + if definition.lower() in potential_ambiguities: + actual_ambiguities.add(definition) else: - potential_ambiguities.add(name.lower()) + potential_ambiguities.add(definition.lower()) if location.endswith('$'): location = location[:-1] + name location = join(uri, location) From 4d61b142f079225e698339cf91ee1d9031ec79cd Mon Sep 17 00:00:00 2001 From: James Addison Date: Mon, 29 Apr 2024 17:09:50 +0100 Subject: [PATCH 15/17] Revert "linting: cleanup: remove unused variable." This reverts commit 897e268a4a89c0e214a3fcbc1d7c6f429acad1b9. --- tests/test_util/test_util_inventory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_util/test_util_inventory.py b/tests/test_util/test_util_inventory.py index 2c7b0f1bf69..0bdef9f67d9 100644 --- a/tests/test_util/test_util_inventory.py +++ b/tests/test_util/test_util_inventory.py @@ -51,7 +51,7 @@ def test_read_inventory_v2_not_having_version(): def test_ambiguous_definition_warning(warning): f = BytesIO(INVENTORY_V2_AMBIGUOUS_TERMS) - invdata = InventoryFile.load(f, '/util', posixpath.join) + InventoryFile.load(f, '/util', posixpath.join) assert 'contains multiple definitions for std:term:a' in warning.getvalue().lower() From 15d2416b951b07e1b97e224a9296d59579e75dd7 Mon Sep 17 00:00:00 2001 From: James Addison Date: Sat, 4 May 2024 19:45:43 +0100 Subject: [PATCH 16/17] Add CHANGES.rst entry. --- CHANGES.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.rst b/CHANGES.rst index 037869b4284..d2e84799e27 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -21,6 +21,9 @@ Features added * Flatten ``Union[Literal[T], Literal[U], ...]`` to ``Literal[T, U, ...]`` when turning annotations into strings. Patch by Adam Turner. +* Add detection of ambiguous ``std:label`` and ``std:term`` references during + loading and resolution of Intersphinx targets. + Patch by James Addison. Bugs fixed ---------- From a8ee7aa8a4d19f45f0006806bb90ccea1d974d62 Mon Sep 17 00:00:00 2001 From: James Addison Date: Tue, 11 Jun 2024 17:50:39 +0100 Subject: [PATCH 17/17] Intersphinx inventories: code review feedback: retain the existing public loader APIs. --- sphinx/util/inventory.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/sphinx/util/inventory.py b/sphinx/util/inventory.py index 0cd3b6bde21..55d7efd8396 100644 --- a/sphinx/util/inventory.py +++ b/sphinx/util/inventory.py @@ -87,15 +87,11 @@ def load( reader = InventoryFileReader(stream) line = reader.readline().rstrip() if line == '# Sphinx inventory version 1': - invdata, ambiguities = cls.load_v1(reader, uri, joinfunc) + return cls.load_v1(reader, uri, joinfunc) elif line == '# Sphinx inventory version 2': - invdata, ambiguities = cls.load_v2(reader, uri, joinfunc) + return cls.load_v2(reader, uri, joinfunc) else: raise ValueError('invalid inventory header: %s' % line) - for ambiguity in ambiguities: - logger.warning(__("inventory <%s> contains multiple definitions for %s"), - uri, ambiguity, type='intersphinx', subtype='external') - return invdata @classmethod def load_v1( @@ -103,7 +99,7 @@ def load_v1( stream: InventoryFileReader, uri: str, join: Callable[[str, str], str], - ) -> tuple[Inventory, set[str]]: + ) -> Inventory: invdata: Inventory = {} projname = stream.readline().rstrip()[11:] version = stream.readline().rstrip()[11:] @@ -118,7 +114,7 @@ def load_v1( type = 'py:' + type location += '#' + name invdata.setdefault(type, {})[name] = (projname, version, location, '-') - return invdata, set() + return invdata @classmethod def load_v2( @@ -126,7 +122,7 @@ def load_v2( stream: InventoryFileReader, uri: str, join: Callable[[str, str], str], - ) -> tuple[Inventory, set[str]]: + ) -> Inventory: invdata: Inventory = {} projname = stream.readline().rstrip()[11:] version = stream.readline().rstrip()[11:] @@ -168,7 +164,10 @@ def load_v2( location = join(uri, location) inv_item: InventoryItem = projname, version, location, dispname invdata.setdefault(type, {})[name] = inv_item - return invdata, actual_ambiguities + for ambiguity in actual_ambiguities: + logger.warning(__("inventory <%s> contains multiple definitions for %s"), + uri, ambiguity, type='intersphinx', subtype='external') + return invdata @classmethod def dump(