sphinx-doc / sphinx

The Sphinx documentation generator
https://www.sphinx-doc.org/
Other
6.42k stars 2.09k forks source link

Feature request: Automatically link inherited methods #11434

Open erzoe opened 1 year ago

erzoe commented 1 year ago

Describe the bug

Referencing a method defined in the parent class from a child class with :meth:`x` does not work. It looks as it should but there is no link.

As a workaround it is possible to reference the method in the parent class directly with :meth:`~Parent.x` but that is no solution because if someone decides to override that method in the child class all links will point to a wrong method.

Issue #4600 looks related but there it was blamed on epytext which I am not using.

How to Reproduce

$ tree
.
├── docs
│   ├── conf.py
│   └── index.rst
├── main.py
└── venv
$ cat main.py
#!/usr/bin/env python3

class Parent:

    def x(self, x: int) -> int:
        '''
        Whatever
        '''
        return x * 2

class Child(Parent):

    def y(self, x: int) -> 'tuple[int, int]':
        '''
        Referencing :meth:`~Parent.x` works, referencing :meth:`x` does not work
        '''
        x = self.x(x)
        return (x, x)
$ cat docs/index.rst
.. toctree::
   :maxdepth: 2

   modules
$ cat docs/conf.py
import os, sys
sys.path.insert(0, os.path.abspath('..'))

extensions = ['sphinx.ext.autodoc']

$ sphinx-apidoc --separate -o docs/ .
Creating file docs/main.rst.
Creating file docs/modules.rst.
$ sphinx-build -M html docs/ docs/
[...]
build succeeded.

The HTML pages are in docs/html.

Environment Information

Platform:              linux; (Linux-6.2.13-arch1-1-x86_64-with-glibc2.37)
Python version:        3.10.10 (main, Mar  5 2023, 22:26:53) [GCC 12.2.1 20230201])
Python implementation: CPython
Sphinx version:        7.0.1
Docutils version:      0.20.1
Jinja2 version:        3.1.2
Pygments version:      2.15.1

Sphinx extensions

['sphinx.ext.autodoc']

Additional context

No response

picnixz commented 1 year ago

This is an expected behaviour.

You need to include the inherited members as an autodoc option since otherwise :meth:`x` is actually referencing Child.x. Since the inherited members are not shown by default, a reference to Child.x will obviously fail to render.

erzoe commented 1 year ago

Thank you very much for your tip that I could work around this by adding autodoc_default_options = {'inherited-members': True} to conf.py [documentation]. That would work. But actually I think it's a reasonable default that sphinx does not duplicate equal documentation and I would like to keep that.

I can use self.x in methods of Child and therefore I would have expected that I can use :meth:`x` in the doc strings of these methods. And if inherited members is False and the documentation of Child.x is in Parent.x then I would have expected that sphinx creates a link to Parent.x.

picnixz commented 1 year ago

I recall that you could actually document using :meth:`.x` (It should work most of the time)

erzoe commented 1 year ago

yes, that does what I want. Thank you very much.

erzoe commented 1 year ago

Is there another trick to make :meth:`Child.x` work? Of course I could write :meth:`Child.x() <Parent.x>` but that would be unreadable and difficult to maintain.

To elaborate on your "most of the time": :meth:`.x` results in a wrong link if I add another class with a method called x:

class Other:

    def x() -> None:
        pass

but at least it gives a warning in that case.

I have found the documentation for the dot syntax here.

picnixz commented 1 year ago

:meth:.x results in a wrong link

Now I remember why I removed this and re-implemented PyXRefRole. On my side I actually use :role:`::name` with :: indicating that "name" should be found in the current "context". What I essentially do is change the way PyXRefRole.process_link works by replacing (at this point) :: with the class actually implementing it (it's not perfect but it does more or less the work in general). However, the implementation is not the most efficient and I'm not confident that it'd work for arbitrary projects.

Since this is actually an expected behaviour, I would suggest you changing the title of your issue because it is more a feature that you request (namely "be able to implicitly reference something that is not documented").

electric-coder commented 1 year ago

Of course I could write :meth:`Child.x() <Parent.x>` but that would be unreadable and difficult to maintain.

I'd argue this is the right way of doing things, if you use an IDE it should find all mentions of the parent method when refactoring.

child class with :meth:`x` does not work.

If instead you use the abbreviated form :meth:`x` any refactoring tool is likely to confuse the name with any other occurrence of that name (it's common to repeat method names across classes).

Per Google's Python style guide 3.8.3 unless there are side effects that need to be explicitly mentioned when a method is inherited there's no need to repeat its documentation in the child classes.

:meth:`~Parent.x` but that is no solution because if someone decides to override that method in the child class all links will point to a wrong method.

If someone decides to override a method in the child class they're responsible for updating the documentation, or to know the consequences of overriding a method defined in the parent class. If you write the cross-references explicitly the user is also constrained to refactor cross-references correctly.

In conclusion when you say:

:meth:`Child.x() <Parent.x>` but that would be unreadable and difficult to maintain.

It is in fact more readable and easier to maintain overall.

tacaswell commented 2 weeks ago

Since this is actually an expected behaviour ...

It is expected from a sphinx-developer point of view, but very surprising from a sphinx-user point of view!

I can see if you are just writing straight standalone rst why this would be hard to get right. However, if you are using any of the autodoc extensions (and I have in over 10 years never not used autodoc) than you know sphinx has imported your classes and has access to the correct mro so given all of the other magic sphinx is doing, it is frustrating sphinx can not also sort this out.

On my side I actually use :role:::name with :: indicating that "name" should be found in the current "context". What I essentially do is change the way PyXRefRole.process_link works by replacing (at this point) :: with the class actually implementing it (it's not perfect but it does more or less the work in general). However, the implementation is not the most efficient and I'm not confident that it'd work for arbitrary projects.

@picnixz is this implementation someplace public?

da1910 commented 1 week ago

I'm slightly surprised that, as the previous commenter mentions, with autodoc this is not supported as-is. A child class has a method inherited from the parent accessible as Child.method() precisely because the person calling should not have to know if that is silently delegated to Parent.method() or if Child.method() is actually different.

Relying on users to document the difference between these is needless duplication and very surprising. Perhaps this is an issue local to autodoc rather than to sphinx in general, but this seems like an absolutely basic use-case which is just not supported?

picnixz commented 1 week ago

I'll try to find some bandwidth to improve autodoc. It's been a year that I wanted to do it but the entire extension needs to be rethought at some point.

I feel we are adding things here and there and it's becoming more and more fragile. OTOH, creating the :: syntax could be a standalone feature.

I still consider it to be a feature rather than a bug, but I acknowledge that it's frustrating.

electric-coder commented 1 week ago

@tacaswell this doesn't lend any support to the argument:

(and I have in over 10 years never not used autodoc)

Then you should know the proposal is questionable for a number of reasons that should be addressed.

""":meth:`~Parent.x` works, referencing :meth:`x`"""

(Not to mention the obvious: it's an Nth duplicate FR that has had many variations over the years. Too many to count in fact, as requests for shortened cross-reference syntax are about as old as Sphinx itself. We could make an aggregate thread just to group all such requests and its many variations.)

This specific FR doesn't solve anything relevant other than adding syntatic sugar of questionable usefulness. When you have to refactor something by searching for :meth:`x` what you'll get is a bunch of polluted search results. E.g. If you want to say: "using the superclasses' __str__" or any other collision prone name it'll become unmaintainable.

Using fully qualified names is considered a good practice, so using :meth:`~Parent.x` is also more natural because you also have to call super().x() in the code.

Besides that, there are over a hundred autodoc bugs and many don't have workarounds -assuming you use autodoc a lot you must have also lost plenty of dev time solving them- so the type of nicety in this FR is low priority because it is possible to get the functionality at negligible to no cost.

it is frustrating sphinx can not also sort this out.

Let me give you a counter-example of frustrating: an experienced dev posts a bug here in the Sphinx repo of a gargantuan code base with the usual: "here repo, you fix" request. He can't even extract an MRE because his codebase uses duplicate method names, and duplicate class names that then get imported selectively at run time. Hence, when his docs broke there wasn't even a meaningful error message because he used shortened cross-reference syntax for duplicated instance names (ohh, and did I mention this was also with a recursive autosummary?). As usual, I helped out the chap (and I won't link the issue here) but anyone choosing "syntatic sugar cross-references" might sooner or later find themselves with broken builds they just can't figure out how to solve.

da1910 commented 1 week ago

Respectfully, just because there are bugs in a package does not mean that another report is invalid, and I personally find the tone of your response a little confrontational.

In python if I have two classes:

class Parent:
    def x(self, x: int) -> int:
        '''
        Whatever
        '''
        return x * 2

class Child(Parent):
    def y(self, x: int) -> 'tuple[int, int]':
        '''
        Referencing :meth:`~Parent.x` works, referencing :meth:`x` does not work
        '''
        x = self.x(x)
        return (x, x)

Then I do not have to call super().x() to invoke the method x(). Python resolves the method for us, as it should do. This is exactly the expected behaviour of a class, we have encapsulated the behaviour. If we later decide we want to override the method on the class Child then that change should be transparent to users of the class.

The issue here is that despite python (and to my knowledge most other languages) being happy to resolve Child.x() without error, sphinx is not.

The explanation given that the references do not exist unless inherited-members is reasonable but at least a little surprising, a naive user will assume that because their interpreter or compiler is able to resolve a method then the documentation generator would also be able to.

This doesn't really have anything to do with automated refactoring tools or maintainability, it's an issue where a language feature leads developers to implement something in python which then fails to be documented in sphinx via autodoc.

picnixz commented 1 week ago

The issue here is that despite python (and to my knowledge most other languages) being happy to resolve Child.x() without error, sphinx is not. I fail to see how this is a feature request, it is a defect. If this is not a supported use of autodoc or sphinx then it should be documented (forgive me if I missed that documentation page somewhere).

It is kind of documented but burried in https://www.sphinx-doc.org/en/master/usage/domains/python.html#role-py-meth

Normally, names in these roles are searched first without any further qualification, then with the current module name prepended, then with the current module and class name (if any) prepended. If you prefix the name with a dot, this order is reversed. For example, in the documentation of Python’s codecs module, :py:func:open always refers to the built-in function, while :py:func:.open refers to codecs.open().

A similar heuristic is used to determine whether the name is an attribute of the currently documented class.

Also, if the name is prefixed with a dot, and no exact match is found, the target is taken as a suffix and all object names with that suffix are searched. For example, :py:meth:.TarFile.close references the tarfile.TarFile.close() function, even if the current module is not tarfile. Since this can get ambiguous, if there is more than one possible match, you will get a warning from Sphinx.


Now, concerning why Python works and not autodoc it's because Python has contextual information that autodoc does not always have. This is actually because of the way autodoc is built (it tries to be smart but sometimes being smart fails). Why does Python manages to call the correct method at runtime? because there is something behind the scene that looks up that method and searches for it correctly. But autodoc does not have this kind of algorithm (but I think it should) because otherwise it could be VERY slow (we'll need to crawl up classes, resolve the MRO and inspect them, and those steps are non-trivial to perform).

When you call super().meth(...) you implicitly rely on MRO resolution to find the correct method to dispatch to. But you are doing it at runtime, so you have all information you want. On the other hand, Sphinx only has a textual information and needs to find methods with the matching name. But inspecting the __dict__ of a class is really a non-trivial task.

da1910 commented 1 week ago

Thank you for the additional context, yes it makes sense that the interpreter builds that context, I suspect my surprise is partly because autodoc is clearly able to determine which methods are inherited, since we can use the :inherited-members: directive.

As far as I can tell we have it set for our classes, but I imagine it's also nontrivial to schedule when that resolution ought to be done.

electric-coder commented 1 week ago

@da1910

I personally find the tone of your response a little confrontational.

That's not my intention. Please notice carefully when I wrote:

there are over a hundred autodoc bugs and many don't have workarounds (...) so the type of nicety in this FR is low priority

That's coming from a guy (me) who's lost months of his life (if I add up the hours) working around Sphinx and mypy bugs. So when I say it's "low priority" it's because it is low priority to me. I can live without what this FR proposes and not loose any additional time for lack thereof.


Without being confrontational, you didn't adequately address any of the technical reservations I raised:

'''
Referencing :meth:`~Parent.x` works, referencing :meth:`x` does not work
'''

This is sound application behavior!

do not exist unless inherited-members is reasonable but at least a little surprising, a naive user will assume that because their interpreter or compiler is able to resolve a method then the documentation generator would also be able to.

You have to write or generate domain declarations to be able to reference something (same for autodoc, napoleon, etc...), that's always been the case with Sphinx. And it's also a very common complaint from beginning users, the good old XY problem: "I don't want to know about domain declarations so someone should change how cross-references work instead."

So, yes it would ease the Sphinx learning curve, but users would run again into the exact same difficulties with domain declarations not far down the road.

tacaswell commented 16 hours ago

This may be a naive sphinx question: is there a way to express an alias for a domain definition?

electric-coder commented 13 hours ago

This may be a naive sphinx question: is there a way to express an alias for a domain definition?

Off-topic to this thread see Get support and consider asking at Stack Overflow.

Check the Extending Sphinx section, it isn't trivial and to make things worst up until recently neither the Sphinx nor docutils APIs were fully documented. Questions about custom hackery with both tools generally went unanswered at Stack Overflow even if they had a bounty because of the learning curve it requires.

There's one strong reason not to try using an alias: it'd be a POLA violation for anyone trying to read your rst.

This last consideration makes me want to quote a Pylons maintainer: "that's the right way to keep it a one-man project", and a Python core dev: "avoid mucking around in the internals".