type hint handling

[buhman]

I tried to autofunction this function:

QueueItem = Union[asyncio.Future, str]

# the expansion of `Optional` makes me realize that `wait: bool=False` is probably more correct

async def pump_queue(queue: Deque[QueueItem], wait: Optional[bool]=False) -> AsyncGenerator[str, None]:
    """
    foo

    Args:
        queue (Deque[QueueItem]):
        wait (Optional[bool]):

    Returns:
        AsyncGenerator[str, None]:
    """

Result:

---

There are a few problems with this:

• PEP 484 type hints using typing module are awful to read sphinx-doc/sphinx#3570
• the docstring parser doesn't interpret types at all (QueueItem isn't expanded like it is in the function definition)
• the type hints already give us enough information to build args, returns, etc… so it might be smarter to ignore/combine the types from the docstring when type hints are present.
• Returns: AsyncGenerator[…] is probably more accurate than saying this yields, but I'm not sure.
• it's uglier than the source code

Do you have any ideas on how/if return types should be reconciled with the implied return types from the await and async for … prefixes that sphinxcontrib-trio adds?

Is PEP484 and/or modifying docstring parsing behavior in-scope for this extension at all?

[buhman]

After thinking about this, I'm not even sure how AsyncGenerator is a useful annotation, because the interpreter should already know this for a fact--unless it's something like this:

async def gen():
    yield

def f() -> AsyncGenerator[None, None]:
    return gen() 

[njsmith]

I don't have much in the way of conclusions here myself; I haven't played with type hints yet. sphinxcontrib-trio obviously needs a better story for handling them, since they're trying to provide overlapping information in overlapping space. (And also, it looks like the stdlib might start using them to communicate with sphinxcontrib-trio in some cases.) But I'm sort of hoping that someone who actually uses them both will tell me how this should work :-)

I would really like to stay out of the docstring parsing business if possible...

[njsmith]

It looks like there are some standalone extensions for making type hint information more readable:

• https://github.com/agronholm/sphinx-autodoc-typehints
• https://github.com/nicolashainaux/sphinx-autodoc-annotation

I don't know what the difference between them is (if any). It'd be interesting to know whether they play well with sphinxcontrib-trio, since we all need to hook into some similar bits of sphinx and could potentially collide. Actually, from a quick look @agronholm's version ("sphinx-autodoc-typehints") looks more likely to work with sphinxcontrib-trio... the other one ("sphinx-autodoc-annotation") is trying to replace the same classes that sphinxcontrib-trio does, so I doubt they can work together at all.

On further thought, the overlap here is really only in the return type: the information that sphinxcontrib-trio's trying to communicate is all about return type, and the only annotations its text is likely to collide with are return type annotations.

So maybe the first thing to try is to start looking at __annotations__ as another source of information about what the return type is, and when it's one of the types we recognize (iterator or context manager) then hide the -> Generator[...]-style text? One limitation though is that we would lose the parameter information.

[buhman]

Yeah, that's more approaching what I hoped the default behavior would be:

looking at annotations as another source of information about what the return type is

I think ideally sphinxcontrib-trio would warn when there is conflicting information, so that sphinx-build -W would error, because the annotation is clearly wrong.

Otherwise, I think having the return type in both places is fine, as long as they match.

[njsmith]

What is that screenshot? Is that using sphinx-autodoc-hints and sphinxcontrib-trio together?

I think ideally sphinxcontrib-trio would warn when there is conflicting information, so that sphinx-build -W would error, because the annotation is clearly wrong.

I think you might be overestimating the reliability of sphinxcontrib-trio's heuristics 😄

(Generally they're pretty good, but when you're trying to figure out the return type of a function that has a stack of decorators on top of it, you inevitably have to make some guesses. So from this point of view, an explicit annotation is great – we can use it instead of guessing!)

[buhman]

we can use it instead of guessing!

Speaking of guessing, I already found a bug in sphinx-autodoc-hints:

test.py

async def test() -> str:
    return 'foo'

test2.py

from typing import Coroutine
async def test() -> Coroutine[None, None, str]:
    return 'foo'

$ mypy --strict test.py
$ mypy --strict test2.py
test2.py:3: error: Incompatible return value type (got "str", expected Coroutine[None, None, str])

---

This was confusing until I read typing#119

We could standardize the type annotation in the context of a async def to have a special meaning: it could be translated by a static checker to Coroutine[...] automatically.

And in that discussion, that's what they agreed on, though the documentation doesn't explain this very well.

The problem, is that sphinx-autodoc-hints blindly copies the annotations, instead of properly translating contextual sugar:

The expected output, if no additional information is formatted, is:

Return type: Coroutine[None, None, str]

This could probably be made prettier if desired; something like:

Coroutine returns: str
Coroutine sends: …
Coroutine yields: …

[buhman]

I think you might be overestimating the reliability of sphinxcontrib-trio's heuristics

After thinking about this, it's probably better that that be left to a real static type checker anyway, so I guess the conclusion is that sphinx-autodoc-hints (or whichever sphinx extension decides they want to support typing properly) should fully resolve sugar'ed annotations.

[njsmith]

Hmm, I think await foo() -> str looks exactly right. Recall that part of trio house style is to treat async functions as being just like regular functions but with a special call syntax, and the existence of coroutine objects is treated as an internal implementation detail. So if anything in sphinxcontrib-trio I'd be inclined to replace Coroutine[str] with str.

[buhman]

regular functions but with a special call syntax

For this to be consistent, I think this transformation would need to be applied to all "extended functions":

• async: Coroutine[…] → …
• async-for: AsyncGenerator[…] → …
• for: Generator[…] → …
• etc…

I'd be inclined to replace Coroutine[str] with str.

I think only if the transformation were explicitly:

• async: Coroutine[None, None, T] → Awaitable[T] → T

Otherwise:

• async: Coroutine[Y, S, R] →

Returns: R
Sends: S
Yields: Y

I don't think you can do this if the only information you have is the sugar'ed annotation, maybe?

I guess because we know coroutine functions are the only "extended function" that has sugar (I think), there would just need to be transformations for all of the other extended functions.

[maxfischer2781]

We've generally stopped giving :returns:-like docstrings annotations in favour of Type Hints. This keeps information tighter together and doesn't require people to match docstring elements to the signature. I can wholeheartedly recommend this approach and don't think sphinxcontrib-trio should generate :returns: entries.

---

The one thing missing is that with a pretty signature, the type hints for complex expressions end up at the wrong position. For example, I currently have this (shortened) signature

async def map(function: Callable[[T], R], iterable: AsyncIterable[T]) -> AsyncIterator[R]:
    ...

which gets rendered with sphinxcontrib-trio as

async for r in map(function: Callable[[T], R], iterable: AsyncIterable[T]) → AsyncIterator[R]

where the "return type" both duplicates and collides with the async for ... in header. We obviously async iterate, but over Rs or AsyncIterator[R]s? One could manually override the signature in such a case, say to

async for r in map(function: Callable[[T], R], iterable: AsyncIterable[T]) → R

but that just gets us back to the same confusion. Is R the return type of map or of async for ... in map?

It would be nicer if the return types where included at the correct positions, say

async for r: R in map(function: Callable[[T], R], iterable: AsyncIterable[T])