Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

autodoc_typehints cause bogus Return type sections to be generated in autoclass #12472

Closed
ktbarrett opened this issue Jun 24, 2024 · 10 comments
Closed

Comments

@ktbarrett
Copy link

ktbarrett commented Jun 24, 2024

Describe the bug

When autodoc_typehints = "description", a Return type: field is generated for an autoclass, which is nonsense, and the generated return type is not even correct. Changing to autodoc_typehints = "none" resolves the issue.

image

The autoclass is described simply as

.. autoclass: cocotb.triggers.Join
    :members:

The docstring is located on the class itself:

class Join(Trigger, Generic[T], metaclass=_ParameterizedSingletonGPITriggerMetaclass):
    r"""Fires when a task completes.

    Args:
        task: The task upon which to wait for completion.

    .. code-block:: python3

        async def coro_inner():
            await Timer(1, units="ns")
            return "Hello world"


        task = cocotb.start_soon(coro_inner())
        await Join(task)
        assert task.result() == "Hello world"

    .. versionchanged:: 2.0

        :keyword:`await`\ ing this trigger no longer returns the result of the task, but returns the trigger.
        To get the result, use :meth:`Task.result() <cocotb.task.Task.result>` of the completed task,
        or simply ``await task`` to get the old behavior.

    .. note::
        Typically there is no reason to directly instantiate this trigger,
        instead call ``task.join()``.
    """

How to Reproduce

git clone https://github.com/ktbarrett/cocotb/tree/change-join
cd cocotb
pip install nox
nox -e docs

Environment Information

Platform:              linux; (Linux-6.8.0-76060800daily20240311-generic-x86_64-with-glibc2.35)
Python version:        3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0])
Python implementation: CPython
Sphinx version:        7.3.7
Docutils version:      0.20.1
Jinja2 version:        3.1.3
Pygments version:      2.17.2

Sphinx extensions

[
    "sphinx.ext.autodoc",
    "sphinx.ext.napoleon",
]

Additional context

No response

@electric-coder
Copy link

electric-coder commented Jun 25, 2024

Did you set the default value of autodoc_typehints_description_target to "documented" because the default value being "all" implies the return type would be documented whether it's in the docstring or not. Although under "documented_params" it says:

(except if it is None)

but to me it isn't entirely clear if this should only apply for the "documented_params" or also the other possible values.


Besides that, this is a somewhat complicated signature:

class Join(Trigger, Generic[T], metaclass=_ParameterizedSingletonGPITriggerMetaclass):

With a generic and a metaclass, what should the run-time return type be exactly? (Here it should -maybe- be said, that linking to a repo doesn't make for an MRE and it makes life a lot simpler if you can distill an MRE that can be copy pasted and executed with only the non-default configuration settings included, otherwise reports require Sphinx contributors/triagers to extract an MRE themselves and the report becomes mostly useless to other potential future readers...)

When autodoc_typehints = "description", a Return type: field is generated for an autoclass

This is a bit of a special case because it's a class constructor thus the generated docs also depend on other options like autoclass_content. Since you only mention autodoc_typehints from the report it's still unclear what your desired result is. Looking just at the only mentioned setting (as I said above) it would seem Sphinx might be working correctly.


Anyway, sometimes autodoc does get the signatures wrong in which case the simplest workaround might be pasting the desired signature into the autoclass directive explicitly. This does have its own drawbacks but often it's the surest way of not having to change project wide settings to satisfy a single bugged signature.

In case you missed it the Sphinx repository has issue #11991 pinned at the top. It groups these kinds of bugs but some more diagnoses/research would be needed to ascertain if this is a duplicate of a previously reported issue or if it's something new.

@ktbarrett
Copy link
Author

With a generic and a metaclass, what should the run-time return type be exactly?

An instance of the Join class. The metaclass is just instrumenting a cache to singletons. If it was intended to be function-like, returning some other type, I would assume I would not or could not use autoclass.

Since you only mention autodoc_typehints from the report it's still unclear what your desired result is.

I do not understand your question. I presume when documenting a class no one would want to document a return value as the return type of instancing a class is an instance of the class.

What's really strange to me is that return values are not generated for the other very-similar class definitions, like First and Combine which are also both Generic and have metaclasses, but inherit them from an undocumented class. It sounds like if this is "working as intended" then this is hitting some corner case where it conspires to make a ton of surprising assumptions.

Did you set the default value of autodoc_typehints_description_target to "documented" because the default value being "all" implies the return type would be documented whether it's in the docstring or not.

I did not. I didn't figure I'd have to. But that works, thanks.

@electric-coder
Copy link

electric-coder commented Jun 25, 2024

I presume when documenting a class no one would want to document a return value as the return type of instancing a class is an instance of the class.

From a documentation POV it's a somewhat common option to omit an explicit __init__ and document the constructor params directly on the class (although PEP 257 says otherwise). From a pythonic POV I'm not sure if __new__ would allow a return type that isn't self, but Sphinx isn't limited to documenting Python so conceivably you could want to document a return type on the class in some other language for whatever reason.

What's really strange to me is that return values are not generated for the other very-similar class definitions, like First and Combine which are also both Generic and have metaclasses, but inherit them from an undocumented class.

The autodoc/napoleon configuration values are mostly orthogonal but they're not always independent. If a superclass has a docstring in one inheritance but not in another autodoc_inherit_docstrings comes into play and the issue you're describing now might be controlled by that config value taken together with autodoc_typehints_description_target and autodoc_typehints . That seems like the most plausible cause for the apparent discrepancy you're seeing.

I did not. I didn't figure I'd have to. But that works, thanks.

Always glad to help a fellow Sphinx user :D

@picnixz
Copy link
Member

picnixz commented Jun 25, 2024

I'd say it's a bug but I don't really have time (nor the mood) to fix this one. Autodoc is great in general but it suffers from a lot of issues that when you try to fix one, the code becomes messy. I still haven't written a formal RFC for the next autodoc version so I'll add it to the list of known issues.

@ktbarrett
Copy link
Author

Is there any way to turn off deduced return type on a per-class basis?

@picnixz
Copy link
Member

picnixz commented Jun 25, 2024

I don't remember but I don't think so. I'm not sure this autodoc option is actually picked up at the level of the class, but maybe writing your own documentation for __init__ or __new__ could be done. Maybe if you exclude __init__ and __new__ from the members being documented, maybe it could work (sorry for that though)

@ktbarrett
Copy link
Author

autodoc_content is currently "both" because we are transitioning to put stuff in __init__ per PEP257, but at least for the class I'm having issues with the __init__ is not documented, so it should have no effect.

@electric-coder
Copy link

electric-coder commented Jun 25, 2024

Is there any way to turn off deduced return type on a per-class basis?

As I said before the most effective way of doing this is pasting the signature directly into the autodoc directive (likely having to manually adjust the module.class syntax for the types if you want to keep all the links working). This solution can require that your .rst isn't just a top-level automodule but you actually keep separate directive declarations if and when necessary.

Could you please include a link to the example class in the docs (if you've published this problematic build) and its .rst file?

@ktbarrett
Copy link
Author

the most effective way of doing this is pasting the signature directly into the autodoc directive

I'm documenting a class. I do not want a signature to show up at all. I want to turn it off, not correct it.

The RST: https://github.com/cocotb/cocotb/blob/master/docs/source/library_reference.rst#python-triggers
The docs: https://docs.cocotb.org/en/latest/library_reference.html#cocotb.triggers.Join

@electric-coder
Copy link

electric-coder commented Jun 25, 2024

There's a way to select the docstring applied to a given autoclass directive by using class-doc-from. It's incompletly documented in autodoc_default_options but there's a full example on SO see this answer https://stackoverflow.com/a/68818512.

I looked at your source code and I'd say Sphinx is working as expected. Your conf.py only sets 2 autodoc config values with everything else being default. I'd start by removing the task from the Args: section in the Join class - if it's calling a superclass __init__ and inheriting the docstring no need to define it twice.


Additionally if you want to remove task from being rendered in cocotb.triggers.join(task) you can declare an empty signature in the autoclass, so writing it like this should work: .. autcoclass:: cocotb.triggers.join() originally taken from here.

@jayaddison jayaddison converted this issue into discussion #12497 Jun 30, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

4 participants