Set refdoc to be used by missing-reference, intersphinx

[MatthijsBurgh]

Feature or Bugfix

Bugfix

Purpose

Intersphinx checks for refdoc attribute in node during the missing-reference event. To determine to correct relative reference uri.

sphinx/sphinx/ext/intersphinx.py

Lines 330 to 332 in cf38356

	if '://' not in uri and node.get('refdoc'):
	# get correct path in case of subdirectories
	uri = path.join(relative_path(node['refdoc'], '.'), uri)

It is called from post_transforms, all other options that are tried/checked before, do get a separate refdoc as an argument. Though the missing-reference event only gets it if it was already in the node, as it doesn't have a separate refdoc argument.

sphinx/sphinx/transforms/post_transforms/__init__.py

Lines 98 to 100 in cf38356

	newnode = self.app.emit_firstresult('missing-reference', self.env,
	node, contnode,
	allowed_exceptions=(NoUri,))

I have not experienced a situation yet, where it was already set.

By not providing the missing-reference event with the back-up 'refdoc' from

sphinx/sphinx/transforms/post_transforms/__init__.py

Line 81 in cf38356

refdoc = node.get('refdoc', self.env.docname)

Relative reference uri's don't work in intersphinx.

So either we need to set the refdoc attribute to node or change the API of missing-reference, so that it accepts refdoc as a separate argument. I choose the first option, but I am open to implement the second one if you prefer that one.

P.S.: also the viewcode extension expects the refdoc attribute of the node to be set. So that would favor the first option, which I implemented.

sphinx/sphinx/ext/viewcode.py

Lines 204 to 205 in cf38356

	return make_refnode(app.builder, node['refdoc'], node['reftarget'],
	node['refid'], contnode)

Detail

By not having the refdoc in intersphinx, it doesn't know how deep you are in your package, therefore it can't determine relative paths to references. It will use assume your are in the root of your package, which will generate broken links as the depth mismatches.

Relates

Fixes #7634

[jakobandersen]

I can't immediately judge if this is the right solution, but I think it is very similar to what was suggested in #7634 and thereby also fix that.

[MatthijsBurgh]

Yes, I have read the issue. This looks like the same problem, but different solution.

I think we both agree that the refdoc attribute should be set. I think this is the correct tile, as the back-up is passed to the other functions that try to resolve the reference, but not the missing-reference event.

So we should only decide where in that function we added it as an attribute or pass it as an extra argument.

[jakobandersen]

So we should only decide where in that function we added it as an attribute or pass it as an extra argument.

While the ideal thing perhaps would be to add it as an extra argument for the event, that would be a breaking change. The attribute already exists so I think your suggested solution is the right one.

[tk0miya]

As I commented at #7634 (comment), this is a bug of the code that generates the pending_xref node. But, as you said, I understand this code automatically recovers the missing refdoc attribute bug. So I can agree with your proposal.

off topic: I don't understand why the refdoc is needed. If my understanding is correct, it always equals to the current docname (ex. env.docname). So it's no reason to store it to the node...

[MatthijsBurgh]

As I commented at #7634 (comment), this is a bug of the code that generates the pending_xref node. But, as you said, I understand this code automatically recovers the missing refdoc attribute bug. So I can agree with your proposal.

I semi agree with you. As it should be set on generation of the nodes, then we could fix it there. But what code does generate the nodes?
On the other side, as we assign a backup value here to be used by the other resolve function, then also provide it to the intersphinx event.

off topic: I don't understand why the refdoc is needed. If my understanding is correct, it always equals to the current docname (ex. env.docname). So it's no reason to store it to the node...

Why it is needed, is because intersphinx uses it to determine the relative path of the node to the root of the package, so that it can resolve relative paths to references correctly.

[tk0miya]

But what code does generate the nodes?

I'm not sure. We need to investigate if needed.

Why it is needed, is because intersphinx uses it to determine the relative path of the node to the root of the package, so that it can resolve relative paths to references correctly.

AFAIK, it is always the same as env.docname. If my understanding is correct, it's okay to use env.docname instead of node['refdoc'] in intersphinx.

[MatthijsBurgh]

The use of refdoc was chosen in 2013 by @birkenfeld. Maybe he knows why refdoc was chosen over using env.docname.

Otherwise I suggest we just go for refdoc solution for now. As refdoc is provided to the other resolve functions. And then we can investigate what code generates the nodes and we can add a good solution there.

[birkenfeld]

sadly, no idea anymore.

[MatthijsBurgh]

I have discovered two places where a pending_xref node is created.

1. roles.py

   sphinx/sphinx/roles.py

   Lines 128 to 133 in c922189

   	options = {'refdoc': self.env.docname,
   	'refdomain': self.refdomain,
   	'reftype': self.reftype,
   	'refexplicit': self.has_explicit_title,
   	'refwarn': self.warn_dangling}
   	refnode = self.nodeclass(self.rawtext, **options)

   
   self.nodeclass is set as classattribute

   sphinx/sphinx/roles.py

   Line 68 in c922189

   nodeclass: Type[Element] = addnodes.pending_xref

   
   So here the refdoc is correctly set. Sphinx ended up here when parsing autoapisummary (https://github.com/readthedocs/sphinx-autoapi)
2. util/docfields.py

   sphinx/sphinx/util/docfields.py

   Lines 84 to 85 in c922189

   	refnode = addnodes.pending_xref('', refdomain=domain, refexplicit=False,
   	reftype=rolename, reftarget=target)

   
   Here the refdoc attribute is not set. I have experienced that the nodes created here are the nodes causing trouble.
   This code is reached from the make_xref function of PyXrefMixin in domains/python.py

So the second option is the cause of the problem. I think we can set the refdoc in the make_xref function of the Field class in docfields.py. Other Field classes inherit from that class, so that provides a solution for all of them.

[jakobandersen]

Unfortunately it's not only in those two places:

[]/sphinx/sphinx$ grep -Rn pending_xref\(
util/docfields.py:68:        refnode = addnodes.pending_xref('', refdomain=domain, refexplicit=False,
domains/python.py:105:    return pending_xref('', *contnodes,
domains/std.py:466:        refnode = pending_xref(title, reftype='token', refdomain='std',
domains/std.py:804:            contnode = pending_xref('')
domains/citation.py:136:            ref = pending_xref(target, refdomain='citation', reftype='ref',
domains/c.py:140:            pnode = addnodes.pending_xref('', refdomain='c',
domains/cpp.py:613:            pnode = addnodes.pending_xref('', refdomain='cpp',
domains/cpp.py:635:            pnode = addnodes.pending_xref('', refdomain='cpp',
domains/cpp.py:1641:            pnode = addnodes.pending_xref('', refdomain='cpp',
# and other hits for the actual class and the writer methods

It seems that the refdoc attribute is basically not optional anymore. How many of the other attributes are actually optional?
Would it be better to make a breaking change for pending_xref.__init__ so we are forced to provide all the things that are mandatory? Of course that should only be done in master, but then the change in this PR could be used as a partial fix in 4.x.

[MatthijsBurgh]

I am just a user of Sphinx. As Sphinx is pretty big, I don't have the complete overview how everything is/should be working. Therefore I can't say which attributes are/should be required and which are/should be optional.

Also it is undocumented what the meaning of all attributes are(So it is unknown which attributes there are). So the best solution would be to gather all the attributes for all nodes. Document their value, purpose and which node requires which attribute. And combine this with making the attributes required in the code as well.

I would prefer merging this PR as a partial fix. I don't mind my solution or your solution (#9685 (comment))

[tk0miya]

I would prefer merging this PR as a partial fix. I don't mind my solution or your solution (#9685 (comment))

As I commented on it, I prefer to fill the refdoc on the top of the handler. I will merge this if fixed.

Additionally, as I commented as "off topic", the refdoc attribute should be deprecated in the near future. But it does not prevent merging this.

[MatthijsBurgh]

@tk0miya I have updated the PR to match your suggestion. I also changed the target branch to 4.x, as you added this PR to the 4.3.0 milestone.

[MatthijsBurgh]

@tk0miya anything that is blocking this PR to be approved/merged?

[tk0miya]

Sorry for not responding long. I'm back now. It seems good to me. I'll merge this after CI passed.

[tk0miya]

Merged. Thank you for your contribution!