Typings aren't generic enough when using `response_class`

[connebs]

Describe the bug

The ClientSession class allows you to specify a response_class which is then used for responses in place of ClientResponse when handling responses. However the typings are not generic, they are hard coded to use ClientResponse so custom aspects of the response_class result in type errors.

To Reproduce

1. Add a custom response_class
2. Use a request method on a ClientSession, e.g. client.post()
3. The response_class in 1 will be used, but it will still be typed as the default ClientResponse

Expected behavior

Use the response_class type in typings.

Versions

python=3.10.8
aiohttp=3.8.3

Code of Conduct

• I agree to follow the aio-libs Code of Conduct

[Dreamsorcerer]

If you could provide an example with the mypy errors, that would be great (or feel free to make a PR if you think you know how to fix it).

[DanielNoord]

I have tried to fix this, but got stuck on the fact that ClientSession does not actually require an argument to it's Generic.

from __future__ import annotations

from typing import Generic, TypeVar

ClientResponseT = TypeVar('ClientResponseT', bound='ClientResponse')


class ClientSession(Generic[ClientResponseT]):
    def __init__(self, response: type[ClientResponseT] | None = None) -> None:
        if response is None:
            self.response: type[ClientResponseT] = ClientResponse
        else:
            self.response = response

    def request(self) -> ClientResponseT:
        return self.response()


class ClientResponse:
    ...

We can't really show that just ClientSession will default to ClientSession[ClientResponse]. Perhaps a typing expert could help here but this seems hard to solve without actual changes to the class.

[Dreamsorcerer]

It has a default, you can therefore type it with overloads. See for example this PR, which types a Generic based on the class_ parameter:
https://github.com/sqlalchemy/sqlalchemy/pull/8842/files

[DanielNoord]

I don't really understand your comment and in the code there is an actual type: ignore which seems to ignore the same issue.

from __future__ import annotations

from typing import Generic, TypeVar

ClientResponseT = TypeVar("ClientResponseT", bound="ClientResponse")


class ClientSession(Generic[ClientResponseT]):
    def __init__(self, response: type[ClientResponseT] | None = None) -> None:
        if response is None:
            self.response: type[ClientResponseT] = ClientResponse
        else:
            self.response = response

    def request(self) -> ClientResponseT:
        return self.response()


class ClientResponse:
    ...


class MyResponse(ClientResponse):
    ...


session = ClientSession(response=MyResponse)
reveal_type(session.request())

base_session = ClientSession()
reveal_type(base_session.request())

The issue is with base_session. I don't want to have to do ClientSession[ClientResponse] whenever we don't pass a response type because if I would type that I might as well just pass a response type. We want the type checker to infer that if no response is being passed it should default to ClientSession[ClientResponse].

[Dreamsorcerer]

Yes, see the changes in that PR. You just need to add overloads. One would declare the type of self: ClientSession[ClientResponse] with no response parameter.

The linked PR has 1 overload where the type is defined by class_ and another where it is defined on self by there being no class_ present.

[Dreamsorcerer]

The type ignore there is on the implementation, which won't be used by the type checker to infer the type, it will only use the overloads.

(Your example is pretty much identical to the issue that PR is resolving).

[Dreamsorcerer]

So, it becomes something like:

class ClientSession(Generic[ClientResponseT]):
    @overload
    def __init__(self, response: type[ClientResponseT]) -> None:
        ...
    @overload
    def __init__(self: ClientSession[ClientResponse], response: None = ...) -> None:
        ...
    def __init__(self, response: type[ClientResponseT] | None = None) -> None:
        if response is None:
            self.response: type[ClientResponseT] = ClientResponse
        else:
            self.response = response

[DanielNoord]

from __future__ import annotations

from typing import Generic, TypeVar, overload

ClientResponseT = TypeVar("ClientResponseT", bound="ClientResponse")


class ClientSession(Generic[ClientResponseT]):
    @overload
    def __init__(self, response: type[ClientResponseT]) -> None:
        ...

    @overload
    def __init__(self: ClientSession[ClientResponse], response: None = None) -> None:
        ...

    def __init__(self, response: type[ClientResponseT] | None = None) -> None:
        if response is None:
            self.response = ClientResponse
        else:
            self.response = response

    def request(self) -> ClientResponseT:
        return self.response()


class ClientResponse:
    ...


class MyResponse(ClientResponse):
    ...


session = ClientSession(response=MyResponse)
reveal_type(session.request())

base_session = ClientSession()
reveal_type(base_session.request())

❯ mypy test.py
test.py:24: error: Incompatible return value type (got "ClientResponse", expected "ClientResponseT")  [return-value]
test.py:36: note: Revealed type is "test.MyResponse"
test.py:39: note: Revealed type is "test.ClientResponse"
Found 1 error in 1 file (checked 1 source file)

With:

        if response is None:
            self.response: type[ClientResponseT] = ClientResponse
        else:
            self.response = response

❯ mypy test.py
test.py:19: error: Incompatible types in assignment (expression has type "Type[ClientResponse]", variable has type "Type[ClientResponseT]")  [assignment]
test.py:36: note: Revealed type is "test.MyResponse"
test.py:39: note: Revealed type is "test.ClientResponse"
Found 1 error in 1 file (checked 1 source file)

[Dreamsorcerer]

Problem is that mypy type checks the implementation with the inner annotations, but really it should type check it once for each overload (python/mypy#8867).

But, the second one seems correct, you just have to type ignore the assignment. The important thing is that the typing the end user sees will be correct and error-free.

[DanielNoord]

diff --git a/aiohttp/client.py b/aiohttp/client.py
index c4074577..16774ba8 100644
--- a/aiohttp/client.py
+++ b/aiohttp/client.py
@@ -28,6 +28,7 @@ from typing import (
     Type,
     TypeVar,
     Union,
+    overload,
 )
 
 from multidict import CIMultiDict, MultiDict, MultiDictProxy, istr
@@ -162,11 +163,13 @@ class ClientTimeout:
 # 5 Minute default read timeout
 DEFAULT_TIMEOUT: Final[ClientTimeout] = ClientTimeout(total=5 * 60)
 
+
+_ClientResponseT = TypeVar("_ClientResponseT", bound="ClientResponse")
 _RetType = TypeVar("_RetType")
 
 
 @final
-class ClientSession:
+class ClientSession(Generic[_ClientResponseT]):
     """First-class interface for making HTTP requests."""
 
     __slots__ = (
@@ -193,6 +196,64 @@ class ClientSession:
         "_read_bufsize",
     )
 
+    @overload
+    def __init__(
+        self,
+        base_url: Optional[StrOrURL] = None,
+        *,
+        connector: Optional[BaseConnector] = None,
+        cookies: Optional[LooseCookies] = None,
+        headers: Optional[LooseHeaders] = None,
+        skip_auto_headers: Optional[Iterable[str]] = None,
+        auth: Optional[BasicAuth] = None,
+        json_serialize: JSONEncoder = json.dumps,
+        request_class: Type[ClientRequest] = ClientRequest,
+        response_class: Type[_ClientResponseT],
+        ws_response_class: Type[ClientWebSocketResponse] = ClientWebSocketResponse,
+        version: HttpVersion = http.HttpVersion11,
+        cookie_jar: Optional[AbstractCookieJar] = None,
+        connector_owner: bool = True,
+        raise_for_status: Union[
+            bool, Callable[[_ClientResponseT], Awaitable[None]]
+        ] = False,
+        timeout: Union[_SENTINEL, ClientTimeout, None] = sentinel,
+        auto_decompress: bool = True,
+        trust_env: bool = False,
+        requote_redirect_url: bool = True,
+        trace_configs: Optional[List[TraceConfig]] = None,
+        read_bufsize: int = 2**16,
+    ) -> None:
+        ...
+
+    @overload
+    def __init__(
+        self: "ClientSession[ClientResponse]",
+        base_url: Optional[StrOrURL] = None,
+        *,
+        connector: Optional[BaseConnector] = None,
+        cookies: Optional[LooseCookies] = None,
+        headers: Optional[LooseHeaders] = None,
+        skip_auto_headers: Optional[Iterable[str]] = None,
+        auth: Optional[BasicAuth] = None,
+        json_serialize: JSONEncoder = json.dumps,
+        request_class: Type[ClientRequest] = ClientRequest,
+        response_class: Type[_ClientResponseT] = ClientResponse,
+        ws_response_class: Type[ClientWebSocketResponse] = ClientWebSocketResponse,
+        version: HttpVersion = http.HttpVersion11,
+        cookie_jar: Optional[AbstractCookieJar] = None,
+        connector_owner: bool = True,
+        raise_for_status: Union[
+            bool, Callable[[ClientResponse], Awaitable[None]]
+        ] = False,
+        timeout: Union[_SENTINEL, ClientTimeout, None] = sentinel,
+        auto_decompress: bool = True,
+        trust_env: bool = False,
+        requote_redirect_url: bool = True,
+        trace_configs: Optional[List[TraceConfig]] = None,
+        read_bufsize: int = 2**16,
+    ) -> None:
+        ...
+
     def __init__(
         self,
         base_url: Optional[StrOrURL] = None,

This causes these errors:

aiohttp/client.py:240: error: Incompatible default for argument "response_class" (default has type "Type[ClientResponse]", argument has type "Type[_ClientResponseT]")  [assignment]
aiohttp/client.py:257: error: Overloaded function implementation does not accept all possible arguments of signature 1  [misc]

Besides, adding an overload for every combination of parameters doesn't seem maintainable in the long run..

[Dreamsorcerer]

aiohttp/client.py:240: error: Incompatible default for argument "response_class" (default has type "Type[ClientResponse]", argument has type "Type[_ClientResponseT]") [assignment]

Don't put concrete values for defaults in. Overloads should be stubs, all values should be ....

aiohttp/client.py:257: error: Overloaded function implementation does not accept all possible arguments of signature 1 [misc]

This may be a mistake somewhere, or the same cause with the defaults. If you want to start making a PR, we can look at any remaining errors in there.

Besides, adding an overload for every combination of parameters doesn't seem maintainable in the long run..

It's 2 overloads to handle the ClientResponse type. If you want it to be generic, that's the only option I'm aware of.

If we want to make it generic over more types, then it could become a bit of a problem, potentially requiring an exponential number of overloads. e.g. If we want to be generic over both request_class and response_class, then it probably needs 4 overloads. If a third thing is needed, it might become 8 overloads. To avoid that, we'd probably need to rearchitecture the class in some way, to separate out these generics.

[DanielNoord]

Yeah I wonder if the restructure isn't better anyway.. I haven't looked at ws_response_class but that also seems to be generic, based on its name.

However, I don't really have a good idea for a restructuring. It feels like we're more limited by the expressiveness of the typing system rather than actual poorly designed classes.

[Dreamsorcerer]

One option could potentially be to have a CustomClientSession or similar where these classes can be passed, and a ClientSession which basically just provides the defaults.

We did something like this for aiomcache's Client: aio-libs/aiomcache@0423669#diff-c51e0b8016ff603ad4be59f5cf8d113fcefb34bb608dafcb55ceaa90cb11faaaR494

Not sure if it's the best approach, just an idea I thought of.

[Dreamsorcerer]

If you're happy to play with it a little more, I think it may be possible to do this without overloads by using default= in the TypeVar, after importing TypeVar from typing_extensions.

Looks like it'll probably make it into Python 3.12: https://peps.python.org/pep-0696/#using-another-typevarlike-as-default
I don't see any examples that specifically show it inferring the type from the __init__(), but looks like it should work.