Make stringify_annotation recursive on builtins with args

[patacca]

Subject: Make stringify_annotation recursive on builtins with args

Bugfix

Purpose

The current implementation of stringify_annotation in case of a builtin type with arguments is to just return the internal representation of the type (repr(annotation)).
With the following patch it is recursively calling itself on the arguments. This way it correctly displaying all the types.

Consider the following example:

from other.external.package import SomeType

class Example:
    def foo(self) -> list[SomeType]:
        # do stuff
        return ...

The autodoc sphinx extension, even when autodoc_typehints_format = "short", it is showing the fully qualified name

foo() : list [other.external.package.SomeType]

While instead with the following patch it is showing

foo() : list [SomeType]

[patacca]

It seems there is a failing test:

app = <SphinxTestApp buildername='html'>

    @pytest.mark.sphinx('html', testroot='ext-autodoc')
    def test_autodoc_TYPE_CHECKING(app):
        options = {"members": None,
                   "undoc-members": None}
        actual = do_autodoc(app, 'module', 'target.TYPE_CHECKING', options)
>       assert list(actual) == [
            '',
            '.. py:module:: target.TYPE_CHECKING',
            '',
            '',
            '.. py:class:: Foo()',
            '   :module: target.TYPE_CHECKING',
            '',
            '',
            '   .. py:attribute:: Foo.attr1',
            '      :module: target.TYPE_CHECKING',
            '      :type: ~_io.StringIO',
            '',
            '',
            '.. py:function:: spam(ham: ~collections.abc.Iterable[str]) -> tuple[gettext.NullTranslations, bool]',
            '   :module: target.TYPE_CHECKING',
            '',
        ]
E       AssertionError: assert ['', '.. py:m...HECKING', ...] == ['', '.. py:m...HECKING', ...]
E         At index 13 diff: '.. py:function:: spam(ham: ~collections.abc.Iterable[str]) -> tuple[~gettext.NullTranslations, bool]' != '.. py:function:: spam(ham: ~collections.abc.Iterable[str]) -> tuple[gettext.NullTranslations, bool]'
E         Use -v to get more diff

I don't know if this is the intended behavior though. After all the result of stringify_annotation("gettext.NullTranslations") is actually gettext.NullTranslations, not ~gettext.NullTranslations.
Is there anybody that can tell me whether the test case is legit or not?

[picnixz]

The annotation is an annotation object, it's not necessarily a string (it shouldn't be one). If it's a "string-like" value, it should likely be a forward reference.

In addition, I would like to see some tests with different values of autodoc_typehints_format and including from __future__ import annotations statements since there is some recursivity involved. You should definitely add tests for the stringify_annotation function and do it the same for the restify function I think.

[picnixz]

• Apply a similar logic to restify.
• Add tests when you have more complex structures list[tuple[A, ...]] for instance (if it's not already the case) for stringify_annotation and restify functions or when you have forward references.

[patacca]

Thanks for the quick review. I am not a sphinx expert, I just noticed this problem and I am trying to fix it.

The annotation is an annotation object, it's not necessarily a string (it shouldn't be one). If it's a "string-like" value, it should likely be a forward reference.

If we are calling stringify_annotation I think the intended behavior is to actually stringify everything inside, isn't it?

After all (if I am not mistaken) if we are calling stringify_annotation(typing.List[~gettext.NullTranslations]) we are getting ~typing.List[gettext.NullTranslations], removing the initial ~ on the gettext.NullTranslations.

In addition, I would like to see some tests with different values of autodoc_typehints_format and including from __future__ import annotations statements since there is some recursivity involved. You should definitely add tests for the stringify_annotation function and do it the same for the restify function I think.

I am gonna try to add the tests.

[picnixz]

You are misunderstanding what stringify_annotation actually receives. Its input is usually a type object (e.g., list) or a generic alias (e.g., list[int]). Actually, I was a bit wrong before:

• When it's a string, the annotation is returned plainly.
• When it's a type, it's processed. When you are stringifying the type annotation of the return function, you are actually stringifying a real type (or in our case, a real generic alias). You can see it by printing the type of the annotation being stringified for instance before anything else. Check test_stringify_annotation in test_util_typing.py.

[patacca]

I've added more tests for both stringify_annotation and restify. I noticed that apart for a very tiny detail, the restify implementation was already supporting the PEP 585 typing with builtins generics.

I've also corrected the test test_autodoc_TYPE_CHECKING so that now it produces

.. py:function:: spam(ham: ~collections.abc.Iterable[str]) -> tuple[~gettext.NullTranslations, bool]',

instead of

.. py:function:: spam(ham: ~collections.abc.Iterable[str]) -> tuple[gettext.NullTranslations, bool]',

[picnixz]

Good. Can you do some visual checks for the HTML builder as well? just to be on the safe side (you can do them locally, no need to implement them) but just to be sure that the RST content produced can be properly converted into a "nice" visual.

[patacca]

This is a screenshot from my project

The class is defined like this

from qbindiff.features.extractor import FeatureExtractor

class Visitor(metaclass=ABCMeta):
    @property
    @abstractmethod
    def feature_extractors(self) -> list[FeatureExtractor]:
        # stuff

[picnixz]

Can you also check that it works properly when guarded by if TYPE_CHECKING blocks and when using from __future__ import annotations ?

[patacca]

There's already a test for that in test_ext_autodoc.py, function test_autodoc_TYPE_CHECKING() that uses the file in tests/roots/test-ext-autodoc/target/TYPE_CHECKING.py that indeed uses both from __future__ import annotations and the guard if TYPE_CHECKING.

What is giving me a headache instead are the two linter problems:

sphinx/util/typing.py: note: In function "stringify_annotation":
sphinx/util/typing.py:327:23: error: Incompatible types in assignment (expression has type "Optional[Any]", variable has type "str")  [assignment]
Found 1 error in 1 file (checked 170 source files)

and

./tests/test_util_typing.py:190:42: F821 undefined name 'myint'
./tests/test_util_typing.py:214:48: E128 continuation line under-indented for visual indent
./tests/test_util_typing.py:215:48: E128 continuation line under-indented for visual indent
./tests/test_util_typing.py:216:48: E128 continuation line under-indented for visual indent
./tests/test_util_typing.py:218:49: E128 continuation line under-indented for visual indent
./tests/test_util_typing.py:219:49: E128 continuation line under-indented for visual indent
./tests/test_util_typing.py:220:49: E128 continuation line under-indented for visual indent
./tests/test_util_typing.py:512:1: E302 expected 2 blank lines, found 1
./tests/test_util_typing.py:521:55: F821 undefined name 'myint'
./tests/test_util_typing.py:522:55: F821 undefined name 'myint'
./tests/test_util_typing.py:523:55: F821 undefined name 'myint'

[picnixz]

What I meant by testing with TYPE_CHECKING is to see the result in HTML (I don't think there is any test checking the actual docutil nodes)

For the first error, it's likely that you are reusing a variable that has already a decided type.

For the second error, it's because of ForwardRef("myint"). You should declare the class somewhere else (I think?). Also, functions are separated by 2 lines (see the rest of the file).

You need:

     assert (stringify_annotation(Union[MyClass1, MyClass2], 'fully-qualified-except-typing') ==
            "tests.test_util_typing.MyClass1 | tests.test_util_typing.<MyClass2>")

to be

     assert (stringify_annotation(Union[MyClass1, MyClass2], 'fully-qualified-except-typing') ==
                "tests.test_util_typing.MyClass1 | tests.test_util_typing.<MyClass2>")

I think. Or maybe

     assert (stringify_annotation(Union[MyClass1, MyClass2], 'fully-qualified-except-typing') ==
             "tests.test_util_typing.MyClass1 | tests.test_util_typing.<MyClass2>")

[patacca]

Finally the CI for the linter passed! Whew, this took longer than it should have. Thanks @picnixz for the help.

There is still a failing job, just for windows but it really seems unrelated to my changes. Moreover it seems on my fork of sphinx it's not failing https://github.com/patacca/sphinx/actions/runs/5822600118/job/15787579726

Regarding the html TYPE_CHECKING test I am not sure how to do it. But the test test_autodoc_TYPE_CHECKING is producing a :py:class output, I guess that should work

[picnixz]

For the HTML TYPE_CHECKING test, you could do it similarly as in test_builder_html or, for now, I would suggest to do it from a visual point of view. The reason why I am asking is that there are a lot of issues with autodoc when the imported member is actually guarded by a TYPE_CHECKING block (this is due to the ModuleAnalyzer implementation) or when the type is declared in such block or aliased.

Anyway, could you also add a CHANGES entry?

[patacca]

I added my line in CHANGES

I tested the TYPE_CHECKING visual html like this (it's an extract from my project):

from __future__ import annotations
from typing import TYPE_CHECKING
from abc import ABCMeta, abstractmethod

if TYPE_CHECKING:
    from qbindiff.features.extractor import FeatureExtractor

class Visitor(metaclass=ABCMeta):
    """
    Abstract class representing interface that a visitor
    must implements to work with a Differ object.
    """
    @property
    @abstractmethod
    def feature_extractors(self) -> list[FeatureExtractor]:
        """
        Returns the list of registered features extractor
        """
        # stuff

It seems to be working correctly. Please tell me if there are other corrections to be made.

[picnixz]

It appears to me that it's fine. At least it appears that everything is clickable so it should be ok I think.

[patacca]

Yes, I can confirm it is indeed clickable and pointing to the right URL

[picnixz]

Can you resolve the conflicts?

[patacca]

I rebased on master

[AA-Turner]

@patacca -- for future reference, please create a PR from a named brach rather than master!

A

[AA-Turner]

Thank you!

I wonder if you might be interested into looking at another change -- now that generic alias types exist, it doesn't make much sense for Sphinx to output ~typing.List when we could output :py:class:`list`. Would you be interested in attempting this?

A

[patacca]

I wonder if you might be interested into looking at another change -- now that generic alias types exist, it doesn't make much sense for Sphinx to output ~typing.List when we could output :py:class:`list`. Would you be interested in attempting this?

I can try to do this.