Docstrings are not inherited

[davfsa]

Describe the bug
Docstrings are not inherited. Considering this is not any special case and it defined by the Python standard, I thought it would be appropriate to open the issue here, but the end goal is for mkdocstrings.

To Reproduce
Steps to reproduce the behavior:

class A:
    def foo(self):
        """This is a docstring"""

class B(A):
    def foo(self):
        # Inherited docstring defined in A.foo

>>> from griffe.loader import GriffeLoader
>>> loader = GriffeLoader()
>>> module = loader.load_module("module")
>>> print(module.classes["A"].functions["foo"].docstring)
This is a docstring
>>> print(module.classes["B"].functions["foo"].docstring)
None

Expected behavior
Docstrings are inherited.

System:

• griffe version: 0.22.0
• Python version: 3.10.4
• OS: Linux

Additional context
N/A

[machow]

It seems like griffe's behavior matches python's behavior

class A:
    def foo(self):
        """This is a docstring"""

class B(A):
    def foo(self):
        # Inherited docstring defined in A.foo

        ...

# returns NoneType
type(B.foo.__doc__)

[davfsa]

It seems like griffe's behavior matches python's behavior

class A:
    def foo(self):
        """This is a docstring"""

class B(A):
    def foo(self):
        # Inherited docstring defined in A.foo

        ...

# returns NoneType
type(B.foo.__doc__)

That is correct, but griffe seems to be more like inspect, which does take into account inherited docstrings when using inspect.getdoc()

As I initially stated, the end goal is mkdocstrings, so I can open an issue there instead if needed.

cc @pawamoy

[pawamoy]

Hello @davfsa, sorry for the late reply and thanks for the report.
Thanks @machow as well for helping out.

Currently Griffe does not support any kind of inheritance. Since different users wish for different things, there is some work to do to specify exactly what we want to provide, and how it should be configurable.

Just wanted to let you know that I don't forget about issues. My time is limited but I'll eventually get to it!

[gab832]

Hi @pawamoy, I'm Gerard from Cloudblue. We are also interested on the inherited methods. How is it going? Is there any plan to do it? Thanks for all the work you are doing!

[pawamoy]

Hi Gerard, I've started working on it, it shouldn't take long :)

[gab832]

That's a really good news. Thank you again @pawamoy !!

[pawamoy]

So, we now have inheritance support, so this issue can be tackled.

I'm imagining a Griffe extension that would somehow post-process the whole tree, looking for class members (methods, attributes) that do not have a docstring, and checking in the parent classes (using MRO) for equally named members with docstrings. We would assign the first found docstring to the base members, without duplicating it (we need to maintain the docstring's parent, needed to parse annotations correctly, in the right scope).

What do you think?

[davfsa]

Sorry for taking so long to get back to you!

I would personally implement it in a way that when griffe gets the docstring of a specific object, if it were to be not set, it would traverse the inheritance members upwards in the search for that docstring, returning it if found. I feel like having an extension later re-parse the whole tree might be too expensive to do after the fact, instead of just in the first pass.

[pawamoy]

Thanks for your feedback.

It's not possible to do it in the first pass anyway since we need to be able to compute the inherited members, and for this the whole tree must have finished loading. Maybe you meant to do this dynamically, when trying to access the docstring? It would mean that we have to store the configuration option somewhere in the object hierarchy to know if we should try and get docstrings from parent classes.

[davfsa]

Maybe you meant to do this dynamically, when trying to access the docstring?

Yeah, that is what I meant 😅

---

Also, this may be slighly different to what you would like, but I found this comment while looking at the source code:

griffe/src/griffe/agents/inspector.py

Lines 149 to 151 in c1476d0

	# we avoid `inspect.getdoc` to avoid getting
	# the `__doc__` attribute from a parent class,
	# but we still want to clean the doc

would using inspect.getdoc be an easier "solution" to this?

It is possible I am also entirely misunderstanding the reason for the comment

[pawamoy]

That's a good remark. This comment is in the "inspector", which loads data from objects in memory (dynamic analysis). Using getdoc there wouldn't solve the case for static analysis (the "visitor"). On the contrary, solving the case for static analysis will also fix it for dynamic analysis 😄 That's because the solution would operate at an abstraction layer above: in Griffe data structures.

The main thing we have to consider when thinking about a solution for this, is how to integrate it well with Object.has_docstring and Object.has_docstrings, which are used by mkdocstrings depending on the show_if_no_docstring configuation option.

To be honest I'm fine with fetching inherited docstrings dynamically. The one thing that is a bit annoying is that, as said above, we have to store the user choice somewhere in the object hierarchy to be able to do that:

@property
def docstring(self) -> Docstring | None:
    if USER_CONFIGURED_INHERIT_DOCSTRINGS:  # where to store that?
        self._docstring = self.try_and_get_docstring_from_parent_classes()
    return self._docstring

Or maybe we could add an inherited_docstring property, but that will complicate things further in has_docstring and has_docstrings, as well as in the Python handler templates.

[pawamoy]

We also have to think about making this configurable per-object in mkdocstrings 🤔 And if that's even something users want (like, "inherit docstrings but not for this object").

[davfsa]

The main thing we have to consider when thinking about a solution for this, is how to integrate it well with Object.has_docstring and Object.has_docstrings, which are used by mkdocstrings depending on the show_if_no_docstring configuation option.

I would personally tackle this by only going through the inheritance and looking for the parent docstring once and then later storing it. Something along the lines of:

# UNKNOWN is used here as a singleton. This could easily be NotImplemented or ...

@property
def is_inherited_docstring(self) -> bool:
    if self._docstring is UNKNOWN:
        self._search_for_docstring()

    return self._is_inherited_docstring

@property
def docstring(self) -> bool:
    if self._docstring is UNKNOWN:
        self._search_for_docstring()

    return self._docstring

def _search_for_docstring(self):
    # This function will search for the docstring in the parents and
    # set `_is_inherited_docstring` and `_docstring` accordingly
    ...

this way, if the docstring is discovered immediately and passed, we can correctly set the attributes and never even search for it in the inheritance tree. Otherwise, we can do the search once, and have all the correct data.

---

The main thing we have to consider when thinking about a solution for this, is how to integrate it well with Object.has_docstring and Object.has_docstrings, which are used by mkdocstrings depending on the show_if_no_docstring configuation option.

I havent had a look at how this is implemented in mkdocstrings, this should be solved by the codeblock I gave before, as you will be able to properly decide whether the docstring is inherited or not.

---

We also have to think about making this configurable per-object in mkdocstrings 🤔 And if that's even something users want (like, "inherit docstrings but not for this object").

Althought maybe not performant, the way to do this per object would be to check if the docstring exists, and if it does, check is_inherited_docstring to avoid it if it were to be inherited.

---

My use of properties here is to make it reusable and have different independent parts of the code "calling" .docstring have the correct information, but the same can be achieved by, lets say, get_docstring which returns a tuple of the docstring and whether it comes from an inherited member or not, which even provides the advantage of taking in arguments to avoid going through the inheritance tree at all if we dont want it to (which is your other point, but should still be covered by the code I gave above):

The one thing that is a bit annoying is that, as said above, we have to store the user choice somewhere in the object hierarchy to be able to do that:

[machow]

I wonder if this is getting more into downstream tool behavior (like mkdocstrings-python, quartodoc) than griffe? It seems okay to have this behavior in griffe, but also like the downstream tools might have everything they need on the object (class bases) to find an inherited docstring.

In any event, it seems useful for people to have the option when documenting methods!

edit: otherwise, it seems nice to keep .docstring and .has_docstring behavior as is, but add a new property, since then the basic behavior still matches python, but there's an option to get some extra bit of inference (the inherited docstring) if needed..!

[pawamoy]

I tried implementing it downstream indeed, in mkdocstrings-python. It worked, but wasn't elegant or efficient, because to allow a local inherit_docstrings option we had to iterate on the whole object tree to set/unset docstrings on overwritten members, and this for every autodoc instruction (:::).

It also complicates the renderer (Python handler)'s code, which is not great. We already have a lot of options, and therefore many more combination of options that all have to work together.

For these reasons, I'm once again more inclined to provide this feature as a Griffe extension, which means the following limitation will apply: it can only be enabled/disabled once per package, not per object.

I'm kinda expecting users to go full-in (always inherit docstrings) or not at all. I myself don't see a use-case where I'd want one method to inherit a docstring and not the others (or the opposite).

Implementing it as an extension also lets us avoid additional complexity in Griffe itself. By complexity I mean storing downstream tools' desired state in Griffe data itself. Griffe should remain standalone. It does already store state though, for docstring parsers and options, and it's not super elegant (passing state along the call chain, modifying it in mkdocstrings-python), so I'd like to avoid making this worse.

I'm not 100% satisfied with the situation but I do think it's the less worse solution 😅

Other than that, yes, we can also add an inherited_docstring property on Griffe objects, which will help the Griffe extension and other downstream projects to provide this functionality!

[machow]

That's fair! I can see the value in telling people that the inheriting docstrings changes the behavior everywhere, so they don't have to worry about interactions (e.g. when switched on, all docstrings get inherited, so the .has_docstring() behavior changes also, but is unambiguous).

[pawamoy]

I published a Griffe extension that allows docstrings inheritance: https://mkdocstrings.github.io/griffe-inherited-docstrings/. It's in the $500/month funding goal. I'll update the home page to show how to use it. Basically, install it and list it in the extensions option:

plugins:
- mkdocstrings:
    handlers:
      python:
        options:
          extensions:
          - griffe_inherited_docstrings