Intersphinx: log warnings for ambiguous target resolutions.

[jayaddison]

Feature or Bugfix

• Refactoring

Purpose

• Add a warning when intersphinx target resolution is ambiguous due to case-insensitivity.
• Expands on some discussion from pull request Normalize name for 'std:label' when loading inventory #12009.

Detail

• When performing case-insensitive matching for Intersphinx targets (currently the case for std:label and std:term object types, as of Sphinx v7.3.7), add a logged warning when target resolution is ambiguous.

Relates

• Relates to Intersphinx cannot link to section headers with uppercase names #12008.

cc @goerz from the initial discussion in #12009, and @picnixz as reviewer of the approved replacement pull request #12033.

[chrisjsewell]

Heya thanks, I would suggest though that the warning requires extra metadata, including a type/subtype and a source/origin location

It would also be good to fully check the warnings in the test.

Perhaps have a look at #12193, where I did something similar

[chrisjsewell]

oops that was meant to be request change

[goerz]

Not much to add: the warning seems useful.

Marginally related: why can I no longer comment on #12009? That PR was superseded by #12033 and is no longer relevant. It doesn't need an "awaiting:response" tag. There's nothing open to respond to.

[jayaddison]

@goerz can you confirm that the absence of the messaging logic that we discussed in the original pull-request from your follow-up changeset was not an attempt to check whether it's possible to bypass our review processes?

I feel that some institutions do spend time attempting such things (such as a case involving the University of Minnesota and the Linux kernel, for example) -- and to some extent I can understand that confirming and reporting those possibilities can provide research data. However, generally I feel like that time would be better spent assisting projects rather than confirming that human error can occur.

[goerz]

Are you asking whether I deliberately not implemented the warning messages as a form of pentesting? Definitely not. I can confirm that I wasn't trying to poke holes in your review process as some kind of research activity.

I forget what my exact reasoning was for why I didn't feel it necessary to include log messages in the second PR. Probably that it ended up being a trivial extension of code that had been there for years, and that it wasn't really my responsibility to add extra bikeshedding to that existing code (in particular because messaging with localization isn't completely trivial). And maybe even more importantly: None of the two systems that I'm aware of that can write objects.inv files (Sphinx iteself and Documenter.jl) have the possibility to generate inventories with the kind of ambiguous targets being addressed in the PR. At least not ambiguous in Sphinx, given Sphinxes reference syntax. What could happen is that Sphinx generates an inventory that's ambiguous for use within Documenter.jl, but that's an issue for Documenter.jl to address.

Frankly, Sphinx is much too big and contributions to it are too bureaucratized (as a consequence, I'm not pointing fingers at anyone), and thus I would like to limit my development interactions with it to the smallest amount possible. In this case, there was a bug that made interoperability with Julia's documentation generator impossible, and that was something that I felt really needed fixing (and is now fixed), thanks to the latest release. Implementing error handling for errors that cannot currently occur is just not something I have the capacity for.

But of course, I have no objection if someone else wants to add more error logging to the inventory resolution, so I fully support this PR. But it really only addresses objects.inv files from a hypothetical future documentation generator, or specific hand-crafted files.

[jayaddison]

Are you asking whether I deliberately not implemented the warning messages as a form of pentesting? Definitely not. I can confirm that I wasn't trying to poke holes in your review process as some kind of research activity.

Thank you! Yes; I didn't think it would be the case, but I did want some kind of logging for the ambiguous-resolution scenario. My sense was that we had agreed that, so it surprised me to later learn that the feature had been implemented without it.

I forget what my exact reasoning was for why I didn't feel it necessary to include log messages in the second PR. Probably that it ended up being a trivial extension of code that had been there for years, and that it wasn't really my responsibility to add extra bikeshedding to that existing code (in particular because messaging with localization isn't completely trivial). And maybe even more importantly: None of the two systems that I'm aware of that can write objects.inv files (Sphinx iteself and Documenter.jl) have the possibility to generate inventories with the kind of ambiguous targets being addressed in the PR. At least not ambiguous in Sphinx, given Sphinxes reference syntax. What could happen is that Sphinx generates an inventory that's ambiguous for use within Documenter.jl, but that's an issue for Documenter.jl to address.

Yep - the PR I'm suggesting here lacks localization so far, and I don't know whether it'll be acceptable without that. I'm not too familiar with the objects.inv-related tooling, but I do think that client software should be cautious about the input it accepts, and also that it's good to make it possible to report problems upstream when they're found.

Frankly, Sphinx is much too big and contributions to it are too bureaucratized (as a consequence, I'm not pointing fingers at anyone), and thus I would like to limit my development interactions with it to the smallest amount possible. In this case, there was a bug that made interoperability with Julia's documentation generator impossible, and that was something that I felt really needed fixing (and is now fixed), thanks to the latest release. Implementing error handling for errors that cannot currently occur is just not something I have the capacity for.

Indeed, I understand that it can feel like results are more likely to be accepted on a prompt timeline when changes are minimized; and also, yes, thank you for stepping in to fix a system compatibility problem - that is much appreciated.

But of course, I have no objection if someone else wants to add more error logging to the inventory resolution, so I fully support this PR. But it really only addresses objects.inv files from a hypothetical future documentation generator, or specific hand-crafted files.

👍

sub-type and location information has been added to the warning

[chrisjsewell]

Mainly all good cheers, just a nitpick

[chrisjsewell]

@jayaddison I'm -1 on 72b266d; you've taken a simple PR and suddenly increased its complexity 10-fold, for minimal gain 😅

[chrisjsewell]

If you want to include inv_name in the message, sure, but the path to the objects.inv is overkill, and really an implementation detail that users should not be particularly concerned

[jayaddison]

Yep, it does raise the complexity and that is unfortunate. However, if we discover ambiguous entries within an objects.inv file, then I think it's important to report the specific source of that at the time-of-detection; otherwise it could be more difficult than necessary for users to determine where it originated.

[chrisjsewell]

I think it's important to report the specific source

Yes and reporting the inventory key inv_name is sufficient for this,
I don't think intersphinx_mapping[inv_name][1][0] is even handling it correctly, because this could just be None https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html#confval-intersphinx_mapping

[jayaddison]

I think it's important to report the specific source

Yes and reporting the inventory key inv_name is sufficient for this, I don't think intersphinx_mapping[inv_name][1][0] is even handling it correctly, because this could just be None https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html#confval-intersphinx_mapping

Mostly agree, although I do worry about time spent trying to figure out what the inventory mapping config evaluated was at the time-of-build. I've added logging of the inv_name at load-time too, although that is at info-level (and so there's a possibility that only the warning would appear, under some circumstances).

[jayaddison]

A thought: perhaps the warnings could/should be detected during the inventory loading process instead of at resolution-time?

[jayaddison]

Ok; I've no further changes planned on this branch and will await further review.

The changeset has been expanded to emit warnings when ambiguous references are discovered during loading of an inventory file -- I believe that this is only relevant for V2 inventories -- in addition to the initial logic to detect ambiguous-resolution of targets.

[chrisjsewell]

This is a breaking change to public API which, without deprecation warnings, I think is not ideal.

either,

just log the warnings within load_v2 (this seems easiest)

or you should create two methods for each version, like: load_v2_with_ambiguties (new signature), load_v2 (calls load_v2_with_ambiguties, but then just drops the ambiguties and only returns the inventory)

[chrisjsewell]

all good cheers

[jayaddison]

Thank you @chrisjsewell!