improvement: show return type annotations in fixtures

[kianelbo]

closes #13676

[The-Compiler]

This seems odd to me, for various reasons:

• inspect._empty is private so it shouldn't be used
• inspect.formatannotation, while public, is undocumented so probably shouldn't be used either
• Why remove ' from it, which is something inspect itself doesn't seem to do?

[kianelbo]

Yes, that inspect._empty condition is actually redundant. I investigated the logic further and there were lots of inconsistent behaviors in the test (that's why I previously did replace("'", "")) and corner cases. However, the new solution (though a bit hacky) is the most general one and hopefully covering everything.

[The-Compiler]

Suggested change

	Added return type annotations in ``fixtures`` and ``fixtures-per-test``.
	Added return type annotations in ``--fixtures`` and ``--fixtures-per-test``.

[The-Compiler]

Why would ValueError and TypeError happen here? The only thing that could happen I think is AttributeError, which already happens with e.g. -> None: as annotation.

That being said, None should definitively handled correctly by the code and tested properly as well.

[kianelbo]

TypeError happens when the argument is not a callable (e.g. a number). I doubt this case happens. ValueError happens when the passed argument is a callable but a signature can't be obtained (e.g. range). Also unlikely imo.
For -> None: on my local test didn't raise any error.

[The-Compiler]

Both of those cause neither of those exceptions, but cause an AttributeError for .__module__.

[The-Compiler]

Maybe turn this into a test class and split the individual asserts into separate test functions? That way, if one of them fails, the rest of the tests will still run.

[The-Compiler]

Ah, this test happens to work (despite the bug above) because this module uses from __future__ import annotations, so the annotation already is 'None' rather than None.

Not sure how to best get around this. Maybe those tests should be in a separate module which deliberately doesn't use that?

[kianelbo]

On my local test (3.13) with or without from __future__ import annotations it worked the same. I might be very wrong but I think it behaves differently in different Python versions. Still, I added a exclusive check for none. Do you think moving it to another module is worth it? Seems too much for a humble helper function!

PS: The reason I didn't use `isinstance(annotation, types.NoneType) was that pylance was complaining.

[kianelbo]

@The-Compiler Could you please review again?
And any tips to satisfy codecov? The indicated uncovered lines are mostly test codes themselves.

[The-Compiler]

Sorry for the radio silence, I never got around to taking a deeper look at this, seems like getting a type annotation as a string is surprisingly difficult with a lot of corner cases...

I still think having a file without the __future__ import is absolutely crucial for those tests - because as-is, they don't actually test most of your code, and that shows in coverage.

Trying with -> tuple[int, str] in a new file, all that's displayed is tuple, and I'm pretty sure after looking at the implementation of inspect.formatannotation that there are more funny corner cases that possibly cause a crash currently (e.g. -> 42 currently does - while not a valid annotation semantically, it is valid Python).

Given the amount of corner cases, I wonder if (despite what I said earlier) we should go back at letting inspectlib handle those. Perhaps we can lessen the future impact of using an undocumented function by passing annotation_format=annotationlib.Format.STRING to inspect.signature with Python >= 3.14, then we can take an easy shortcut there and simply return the string directly in that case.

#13550 added a helper signature method that does this already, so I'd recommend basing things on that.

[kianelbo]

Given the amount of corner cases, I wonder if (despite what I said earlier) we should go back at letting inspectlib handle those. Perhaps we can lessen the future impact of using an undocumented function by passing annotation_format=annotationlib.Format.STRING to inspect.signature with Python >= 3.14, then we can take an easy shortcut there and simply return the string directly in that case.

Are you suggesting to use formatannotation again?

[The-Compiler]

At least for Python < 3.14, for two reasons:

• A implementation of this considering all possible corner cases evidently is trickier than I first thought (and just copy-pasting that code into pytest isn't really better I think)
• If we can use a more proper way on Python 3.14 (simply getting the annotations as strings in the first place, and using them directly), the risk of Python removing the undocumented function in the future is minimal, as I doubt such a thing would happen in a patch release of Python.

[kianelbo]

In addition to the current code, I've tried inspect.get_annotation (with or without formatannotation) and typing.get_type_hints and even with lots of tinkering they all fail miserably on the test cases. It seems impossible! There's another approach, parsing with ast, which works decently, however, the tests fail because it can't access the source code. Any suggestions @The-Compiler ?

[The-Compiler]

I suspected this is going to be trickier than it looked on the surface 😅. I really hope we don't need to resort to AST shenanigans, after all, other tools do sometimes print type annotations as well I think?

Can you elaborate on "all fail miserably on the test cases"? I'd hope that inspect.signature (or inspect.get_annotations which is internally used by inspect.signature) and formatannotation (+ forcing a string on Python 3.14+) should do the right thing here, but it sounds like that's not the case?

[kianelbo]

Can you elaborate on "all fail miserably on the test cases"?

I mean the test cases fail. For example doing:

try:
        annotation = inspect.get_annotations(fixture_func)["return"]
        return inspect.formatannotation(annotation)
    except (KeyError, ValueError, TypeError):
        pass
    return ""

will give collections.abc.Callable[..., typing.Any] instead of Callable[..., Any] in the test case. The more surprising thing is that when I run the code in IPython for testing, it produces the desired output Callable[..., Any].

[The-Compiler]

I think that's fine, after all that's what inspect.signature does as well:

import inspect
from collections.abc import Callable

def func(x: Callable[[], None]) -> None:
    pass

print(inspect.signature(func))

And with the approach outlined above, starting on Python 3.14 (or before with from __future__ import annotations) it will print whatever is there in the source file anyways, so I wouldn't worry about it too much.

[kianelbo]

The best that I could come up with was treating signature as string (as you suggested) and use regex to remove clutterings (if present) and it passed the tests locally. But again the tests are failing in CI and codecov is complaining unfairly. Any tips?

[The-Compiler]

That's not what I meant with treating it as a string at all 😅

Parsing the string output definitely isn't necessary here, as you're just picking out what was rendered using formatannotation. Despite the caveat of that being undocumented, calling it is definitely a lot better than parsing things from the string.

What I meant was that with Python 3.14, you can pass annotation_format=annotationlib.Format.STRING to inspect.signature (or probably better to use annotationlib.get_annotations() at this point, as you need to import it anyways and we don't need anything from inspect). Then from my understanding, you always get a string back, and you don't need to format it any way.

So basically I see three scenarios the code needs to handle:

• Python < 3.14, no future import (annotations are objects): Use inspect.signature (or inspect.get_annotations(), doesn't matter too much) and the (undocumented but stable hopefully) formatannotation.
• Python < 3.14, string annotations (literally or via from __future__ import annotations): You might want to skip formatannotation and just use it directly.
• Python >= 3.14 (annotations are always strings): Use annotationlib.get_annotations() with annotationlib.Format.STRING and use the resulting string directly.

As for the coverage, looks like pre-commit tried to be helpful and introduced the __future__ import again: 1c18e59

This seems to happen via ruff:

pytest/pyproject.toml

Line 102 in a55c959

"FA100", # add future annotations

So if you add a # ruff: noqa: FA100 to the top of that file, that should hopefully stop happening (docs).

[kianelbo]

Understood, but formatannotation doesn't work well, for example it returns:
test_custom_class_return_type.<locals>.T instead of just T. That's why I used the regex.

[The-Compiler]

That's because it formats the __module__ and the __qualname__ into the output.

Maybe that could actually be useful information in real-world scenarios (try e.g. with class T being defined at module level instead of inside a function), but yeah, it could also be too verbose.

It looks like formatannotation also supports a base_module you can pass to avoid including that part if the class is defined in the same module the fixture is (by passing the fixture module as base).

If you disagree, I suppose we're back to copying the formatannotation code, and adjusting it so that it uses annotation.__name__ in case the annotation is a type (i.e. a class). Would be fine in my book as well, as it's only some 10 lines.

[kianelbo]

It looks like formatannotation also supports a base_module you can pass to avoid including that part if the class is defined in the same module the fixture is (by passing the fixture module as base).

I've tried it but it doesn't work as intended. Do you object using regex to remove the extra verbose parts?

So if you add a # ruff: noqa: FA100 to the top of that file, that should hopefully stop happening (docs).
Didn't work!