search

aio-libs / aiohttp

Asynchronous HTTP client/server framework for asyncio and Python
https://docs.aiohttp.org
Other
14.94k stars 1.99k forks source link

Typings aren't generic enough when using `response_class` #7109

Open acnebs opened 1 year ago

acnebs commented 1 year ago

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

Dreamsorcerer commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago 
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 commented 1 year ago

Problem is that mypy type checks the implementation with the inner annotations, but really it should type check it once for each overload (https://github.com/python/mypy/issues/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 commented 1 year ago 
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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/aio-libs/aiomcache/commit/0423669b047458d1aed98bfba7d1d701cbe26feb#diff-c51e0b8016ff603ad4be59f5cf8d113fcefb34bb608dafcb55ceaa90cb11faaaR494

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

Dreamsorcerer commented 1 year ago

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.

DanielNoord commented 1 year ago

That default change looks awesome. I'm going to see if I can make that work. Thanks for the pointer, I had completely missed that part of the PEP!

DanielNoord commented 1 year ago

We might need to wait for this to become generally available. Upgrading typing_extensions and using a TypeVar from it with Generic creates:

Free type variable expected in Generic[...]
diff --git a/aiohttp/client.py b/aiohttp/client.py
index c4074577..c55755ef 100644
--- a/aiohttp/client.py
+++ b/aiohttp/client.py
@@ -31,7 +31,7 @@ from typing import (
 )

 from multidict import CIMultiDict, MultiDict, MultiDictProxy, istr
-from typing_extensions import Final, final
+from typing_extensions import Final, TypeVar as TypeVarFuture, final
 from yarl import URL

 from . import hdrs, http, payload
@@ -163,10 +163,11 @@ class ClientTimeout:
 DEFAULT_TIMEOUT: Final[ClientTimeout] = ClientTimeout(total=5 * 60)

 _RetType = TypeVar("_RetType")
+_ClientResponseT = TypeVarFuture("_ClientResponseT")

 @final
-class ClientSession:
+class ClientSession(Generic[_ClientResponseT]):
     """First-class interface for making HTTP requests."""

     __slots__ = (
diff --git a/requirements/constraints.txt b/requirements/constraints.txt
index cbb29329..912f7e5a 100644
--- a/requirements/constraints.txt
+++ b/requirements/constraints.txt
@@ -107,7 +107,7 @@ multidict==5.2.0
     # via
     #   -r requirements/multidict.txt
     #   yarl
-mypy==0.982 ; implementation_name == "cpython"
+mypy==0.991 ; implementation_name == "cpython"
     # via
     #   -r requirements/lint.txt
     #   -r requirements/test.txt
@@ -237,7 +237,7 @@ trustme==0.9.0 ; platform_machine != "i686"
     # via -r requirements/test.txt
 typer==0.4.0
     # via python-on-whales
-typing-extensions==4.1.1
+typing-extensions==4.4.0
     # via
     #   -r requirements/typing-extensions.txt
     #   aioredis
diff --git a/requirements/lint.txt b/requirements/lint.txt
index 1bab0a04..1f6c446a 100644
--- a/requirements/lint.txt
+++ b/requirements/lint.txt
@@ -1,6 +1,6 @@
 -r typing-extensions.txt
 aioredis==2.0.1
-mypy==0.982; implementation_name=="cpython"
+mypy==0.991; implementation_name=="cpython"
 pre-commit==2.17.0
 pytest==6.2.5
 slotscheck==0.8.0
diff --git a/requirements/test.txt b/requirements/test.txt
index 1ab1c614..fdc43956 100644
--- a/requirements/test.txt
+++ b/requirements/test.txt
@@ -3,7 +3,7 @@ Brotli==1.0.9
 coverage==6.4.2
 cryptography==36.0.1; platform_machine!="i686" # no 32-bit wheels; no python 3.9 wheels yet
 freezegun==1.1.0
-mypy==0.982; implementation_name=="cpython"
+mypy==0.991; implementation_name=="cpython"
 mypy-extensions==0.4.3; implementation_name=="cpython"
 proxy.py ~= 2.4.4rc3
 pytest==6.2.5
diff --git a/requirements/typing-extensions.txt b/requirements/typing-extensions.txt
index 37e36b21..7849d5da 100644
--- a/requirements/typing-extensions.txt
+++ b/requirements/typing-extensions.txt
@@ -1 +1 @@
-typing_extensions==4.1.1
+typing_extensions==4.4.0
Dreamsorcerer commented 1 year ago

OK, mypy's not ready yet. That bug should be fixed in the next release: https://github.com/python/mypy/issues/14312 But, the default implementation isn't finished yet.

Dreamsorcerer commented 1 year ago

Feature has been merged: https://github.com/python/mypy/pull/14872 So, if someone wants to prepare a PR, it should hopefully be good to merge as soon as a new mypy release is out.