Intersphinx, refactoring and intersphinx_disable_domains

[jakobandersen]

Feature or Bugfix

• Feature
• Bugfix
• Refactoring

Purpose

In short, this PR implements #8981 (comment) and thus fixes #2068 and replaces #8981 (second commit) and part of #8929 (first commit).
At the same time the PR serves as a basis for a better implementation of #9062, and contains part of the refactoring in #8929.

Detail

The first commit is primarily refactoring, though in the case where a target contains a prefix something: and the resolution fails, the old code would strip the prefix in the final output. Now this prefix is shown in the output to avoid hiding the information from the source code. I consider this a minor "bug" fix.

The refactoring consists splitting the resolution into multiple functions:

• The top-level is the three functions
  • resolve_reference_in_inventory: lookup in a user-specified inventory
  • resolve_reference_any_inventory: lookup in any inventory
  • resolve_reference_detect_inventory: try first any and otherwise try to extract a prefix from the target. The missing_reference handler directly calls this one.
• The domain-specific resolution is in _resolve_reference_in_domain and _resolve_reference_in_domain_by_target. Abstractly, these are the ones that need to be split and moved to the domain classes (see Intersphinx delegation to domains #8929).

The second commit implements intersphinx_disable_domains which defaults to [] for backwards compatibility. For now I added

intersphinx_disabled_domains = ['std']

to the conf.py generated by sphinx-quickstart. The rationale is that I believe :ref and :doc: references most often are intended to be local, and a simple typo or missing file may silently make them resolve to external documents (i.e., the problem discussed in #2068 and #8981. All other built-in domains either do not export objects for intersphinx (changeset, citation, index, math) or they are programming language domains (c, cpp, js, py, rst) which generates cross-references in declarations where the user can not add an inventory specification.
Note, std also contains the object types term, token, envvar, and cmdoption, but my guess is that users rarely have external links for these, and in all cases one can provide an explicit inventory specification.

[jakobandersen]

@adamtheturtle, @nijel, and @phlax, does this solve your original issues?

[adamtheturtle]

@jakobandersen I'm no longer on the project with the issue so I can't really say.

[phlax]

@adamtheturtle, @nijel, and @phlax, does this solve your original issues?

i can test

[nijel]

It seems that it should address the issue I had. Unfortunately, I probably won't have time to test this in next two weeks...

[astrojuanlu]

I just tested intersphinx_disabled_domains = ['std'] and it works as expected. Previously, :doc:`relative <contents>` would look a contents in any of the intersphinx-mapped projects if it happened to not exist in that relative path, now it gives a WARNING: unknown document: contents. On the other hand, I can still do :doc:`Sphinx contents <sphinx:contents>` and it will correctly point to https://www.sphinx-doc.org/en/master/contents.html.

The name intersphinx_disabled_domains seems to suggest that it's going to disable the :doc: resolution altogether though. The documentation clarifies it (emphasis mine):

When a cross-reference without an explicit inventory specification is being resolve [sic] by intersphinx, skip resolution if [...] the domain of the cross-reference is in this list [...]

But I can't think of a better name that is not super long (intersphinx_disabled_automatic_fallback_domains).

Some more comments on your (super thorough!) pull request overview:

All other built-in domains either do not export objects for intersphinx (changeset, citation, index, math)

• To my understanding, changeset, citation, and index are not documented in https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html, right? Should they?
• Would it be interesting for math to export objects for intersphinx?

or they are programming language domains (c, cpp, js, py, rst) which generates cross-references in declarations where the user can not add an inventory specification

I'm not sure I follow. The py domain does allow the user to add an inventory specification, right? I just tried :py:class:`sphinx:sphinx.builders.changes.ChangesBuilder` and it works fine. However, you mentioned in #9424 (comment) that "the roles in the C and C++ domains do not support this prefix".

[jakobandersen]

is being resolve [sic] by intersphinx

(Typo fixed)

But I can't think of a better name that is not super long (intersphinx_disabled_automatic_fallback_domains).

Right, I got to the same conclusion. I was thinking of trying to get "implicit" in there, but did find a good name.

All other built-in domains either do not export objects for intersphinx (changeset, citation, index, math)

• To my understanding, changeset, citation, and index are not documented in https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html, right? Should they?

Hmm, like the standard domain, I guess they contain roles/directives that are not naturally described in terms of a domain. I don't know the details of them, and haven't used them my self, but some bits I could find:

• for changeset it has the directives at

  sphinx/sphinx/domains/changeset.py

  Lines 155 to 157 in 810a1e2

  	app.add_directive('deprecated', VersionChange)
  	app.add_directive('versionadded', VersionChange)
  	app.add_directive('versionchanged', VersionChange)

  , and it seems to interact with the changes builder.
  Docs: https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-versionadded
• citation`' seems to handle the citation`` node from docutils (https://docutils.sourceforge.io/docs/ref/doctree.html#citation)
• the directive/role in index seems to have docs at https://www.sphinx-doc.org/en/master/usage/restructuredtext/directives.html#directive-index

• Would it be interesting for math to export objects for intersphinx?

Yes, it looks like the math domain seems to contain equation references already for internal resolution, so I don't immediately see why they couldn't be exposed via intersphinx. I guess no user has requested it before?

or they are programming language domains (c, cpp, js, py, rst) which generates cross-references in declarations where the user can not add an inventory specification

I'm not sure I follow. The py domain does allow the user to add an inventory specification, right? I just tried :py:class:`sphinx:sphinx.builders.changes.ChangesBuilder` and it works fine. However, you mentioned in #9424 (comment) that "the roles in the C and C++ domains do not support this prefix".

For references generated from roles intended to reference a single entity, yes, there the Python domain (and perhaps js and rst?) support the prefix to some degree. However, this is because they either don't need to perform detailed parsing of the role argument, or they specifically need to take intersphinx into account. This is why the prefix in a C/C++ role gives a parsing warning/error (#8418).
The C and C++ domains additionally support a role where you can render an (almost) arbitrary type or expression (:expr: role), where it would be strange to have intersphinx prefixes scattered throughout.
In general, all the programming domains generally generate cross-references implicitly in declarations, e.g., in Python:

.. py:function:: f(i: int) -> dict

Here int and dict are cross-references that will be resolved as normal, including with intersphinx, and I don't think there is a way to annotate with which inventory to perform lookup (and I don't think there should be).

[astrojuanlu]

Thanks a lot for the clarifications about these extra roles and directives. I am still trying to make sense of the Sphinx API and how it is described in the documentation.

Yes, it looks like the math domain seems to contain equation references already for internal resolution, so I don't immediately see why they couldn't be exposed via intersphinx. I guess no user has requested it before?

Done :) #9483

However, this is because they either don't need to perform detailed parsing of the role argument, or they specifically need to take intersphinx into account.

Thanks! That's what I understood from your previous comments. I was distracted by your wording here, I now get that programming-language domains don't necessarily or always support explicit inventory specification (aka intersphinx prefixes).

FWIW, this looks like a good addition to me and I'm looking forward for this to pave the way towards #9062.

[tk0miya]

I think explicit references should be also disabled. It would be replaced by #9062 (comment).

[jakobandersen]

Note that I wrote this PR as if #9062 (comment) didn't exist. My plan is to have the functionality change incrementally:

1. This PR, for which this part is as intended: the user must intentionally write something intersphinx-specific to activate it for the disabled domains.
2. Then an updated Add intersphinx role #9062 which uses the functionality in this PR. By then the old style of explicit intersphinx references, as the one you quoted here, can be deprecated/removed.
3. The whole domain delegation part.

So in short, yes, once an updated #9062 has been merged, I don't mind this option disabling the explicit ones as well, but I don't think it is harmful if they are not.

[phlax]

@jakobandersen i tested this branch this morning (here envoyproxy/envoy#18090) and afaict everything is working as i would hope with non/intersphinx ref links - thanks for addressing this

lmk if you would like any further testing

[jakobandersen]

@phlax, thanks for the testing!

[jakobandersen]

@tk0miya, rebased and quickstart stuff removed. I think this is good to merge, assuming something like the following incremental roadmap:

1. (4.x) this PR.
2. (master) change the default value of intersphinx_disabled_domains to ['std'] as a breaking change. (see Change default value of intersphinx_disabled_reftypes #9804)
3. (4.x) merge Add intersphinx role #9062 updated with the following:
   • Use the refactored interface of this PR.
   • Change the documentation to not refer to the old explicit inventory syntax (e.g., :doc:`otherbook:installation` ) but instead use the new role. This includes modifying the docs added in this PR.
   • Optionally: let intersphinx_disabled_domains apply to the old explicit inventory syntax as well, not that the PR provides an alternative.
4. (master) deprecate the old explicit inventory syntax.
5. (master) The whole domain delegation part and other large changes.

[tk0miya]

Note: I found #9626 contains remote :envvar: references. The environment variables belong to the std domain, but this disables the reference by default. I need to reconsider the target of this feature.

[jakobandersen]

Note: I found #9626 contains remote :envvar: references. The environment variables belong to the std domain, but this disables the reference by default. I need to reconsider the target of this feature.

Doh, right. Disabling all of stdis a no-go. Theoretically we could split std into two or more domains, with one of them having doc, but realistically this might break a lot of things if we are not careful.
As an alternative, I have pushed the following changes:

• Instead of intersphinx_disabled_domains it is now intersphinx_disabled_refs. Not sure if this is the best name.
• You can write all and domain names as before, but now also specific references types, e.g., std:doc.

The breaking change to the master branch after this PR is then to default this option to ['std:doc'].

[astrojuanlu]

Anything we can do to move this forward? It would be great to have this in Sphinx 4.3, I could play with it a bit and document it as part of the tutorial.

[tk0miya]

Is there any case that honor_disabled_refs is True, but inv_name is None?

[jakobandersen]

Yes, that is the default for most pending_xref:

1. From the missing_reference callback it will honor the disabling: https://github.com/jakobandersen/sphinx/blob/778a3fe6a7a4caf9152caa5c9b10d5378de671c2/sphinx/ext/intersphinx.py#L467
2. going through resolve_reference_detect_inventory: https://github.com/jakobandersen/sphinx/blob/778a3fe6a7a4caf9152caa5c9b10d5378de671c2/sphinx/ext/intersphinx.py#L446
3. and then in resolve_reference_any_inventory the None is added: https://github.com/jakobandersen/sphinx/blob/778a3fe6a7a4caf9152caa5c9b10d5378de671c2/sphinx/ext/intersphinx.py#L428

[tk0miya]

LGTM with nits.

[jakobandersen]

LGTM with nits.

Great, thanks! All nits addressed basically as suggested, so I'll merge once CI is green.

[tk0miya]

🎉