jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
196 stars 29 forks source link

Issue with annotations display using `from __future__ import annotations` #161

Closed mhostetter closed 1 year ago

mhostetter commented 2 years ago

Hey Jeremy and Brendan. Hope you're well.

I noticed something after converting my library (https://github.com/mhostetter/galois/pull/418) to take advantage of from __future__ import annotations, allowing me to replace Union[a, b] annotations with the more concise a | b, even in Python versions before 3.10. I noticed that concise type hints (I believe they come from autodoc_typehints_format = "short") were not working or linking for the a | b case. I'm thinking this may be a Sphinx issue, but wanted to put it out here and check.

foo-161.zip

from typing import Union
import numpy as np

def func_with_union() -> Union[np.integer, np.ndarray]:
    """
    A function annotated with Union[np.integer, np.ndarray]
    """
    return np.array([], dtype=int)
from __future__ import annotations
import numpy as np

def func_with_pipe() -> np.integer | np.ndarray:
    """
    A function annotated with np.integer | np.ndarray
    """
    return np.array([], dtype=int)

Notice, the np.ndarray annotation is not "concise" and it isn't linked. The "ndarray" annotation is concise and linked.

image

mhostetter commented 2 years ago

If I add this to the bottom of conf.py

def autodoc_process_signature(app, what, name, obj, options, signature, return_annotation):
    signature = modify_type_hints(signature)
    return_annotation = modify_type_hints(return_annotation)
    return signature, return_annotation

def modify_type_hints(signature):
    if signature:
        signature = signature.replace("np", "~numpy")
    return signature

def setup(app):
    app.connect("autodoc-process-signature", autodoc_process_signature)

I get the following output. Perhaps that's evidence it is a Sphinx issue?

image

jbms commented 2 years ago

The issue is that Sphinx uses the builtin typing.get_type_hints function to evaluate the annotations (that is the recommended way to do it). But in Python < 3.9, that function fails to parse np.integer | np.ndarray so Sphinx just leaves it as a plain string when generating the signature. That isn't likely to be very easy to fix in Sphinx either. Later the Python domain directive sees this string and does correctly parse it as an AST (since it is still valid Python syntax).

Separately, even in Python 3.10 it still displays as numpy.integer | numpy.ndarray because of a Sphinx bug. On these lines:

https://github.com/sphinx-doc/sphinx/blob/ded734d5f99756033218d29c32758049adbe52d2/sphinx/util/typing.py#L448 https://github.com/sphinx-doc/sphinx/blob/ded734d5f99756033218d29c32758049adbe52d2/sphinx/util/typing.py#L451

sphinx should be passing the mode as well.

2bndy5 commented 1 year ago

It is my understanding that the __future__.annotations feature was still experimental. They recently decided to postpone making it the default in v3.10 (see this email). There are quite a few changes to it in the changelogs as well.

mhostetter commented 1 year ago

Thank you both for the response. I learned something new about the intricacies of type annotations and checkers.

The issue is that Sphinx uses the builtin typing.get_type_hints function to evaluate the annotations (that is the recommended way to do it).

It seems this failing in < Python 3.9 (TypeError: unsupported operand type(s) for |: 'type' and 'type') is a known limitation, as opposed to a bug. I'm guessing because type.__or__() wasn't defined before Python 3.9 or 3.10? I am surprised, though, that both mypy and pylance in Python 3.7 and 3.8 correctly parse and use the type annotation. Perhaps they have extra logic added for this case?

I'm ok with hacking a solution for the documentation, which I believe I have with autodoc-process-signature. I'm inclined to not open a Sphinx issue, given it likely can't be fixed... and they often go unnoticed / unaddressed.

It is my understanding that the future.annotations feature was still experimental. They recently decided to postpone making it the default in v3.10 (see this email). There are quite a few changes to it in the changelogs as well.

I wasn't aware of PEP 649. Perhaps, I'll upgrade to that and from __future__ import co_annotations if it gets accepted.

mhostetter commented 1 year ago

Separately, even in Python 3.10 it still displays as numpy.integer | numpy.ndarray because of a Sphinx bug. On these lines

To be clear. I meant it's not worth opening a Sphinx issue regarding processing the np.integer | np.ndarray annotation, not the bug @jbms found. I probably should bring that to their attention (if not already) and/or open an easy PR to fix it. Thanks for pointing that issue out, Jeremy.