Skip to content

Commit

Permalink
Allow covariants of __enter__, __aenter__, and @classmethod (#1336)
Browse files Browse the repository at this point in the history
* Allow covariants of __enter__, __aenter__, and @classmethod

The problem we currently have is the return type of classes such as
Client does not allow covariants when subclassing or context manager.
In other words:

```python
class Base:
    def __enter__(self) -> Base:  # XXX
        return self

class Derived(Base):
    ...

with Derived() as derived:
   # The type of derived is Base but not Derived. It is WRONG
    ...
```

There are three approaches to improve type annotations.
1. Just do not type-annotate and let the type checker infer
   `return self`.
2. Use a generic type with a covariant bound
   `_AsyncClient = TypeVar('_AsyncClient', bound=AsyncClient)`
3. Use a generic type `T = TypeVar('T')` or `Self = TypeVar('Self')`

They have pros and cons.
1. It just works and is not friendly to developers as there is no type
   annotation at the first sight. A developer has to reveal its type via
   a type checker. Aslo, documentation tools that rely on type
   annotations lack the type. I haven't found any python docuementation
   tools that rely on type inference to infer `return self`. There are
   some tools simply check annotations.

2. This approach is correct and has a nice covariant bound that adds
   type safety. It is also nice to documentation tools and _somewhat_
   friendly to developers. Type checkers, pyright that I use, always
   shows the the bounded type '_AsyncClient' rather than the subtype.
   Aslo, it requires more key strokes. Not good, not good.

   It is used by `BaseException.with_traceback`
   See https://github.com/python/typeshed/pull/4298/files

3. This approach always type checks, and I believe it _will_ be the
   official solution in the future. Fun fact, Rust has a Self type
   keyword. It is slightly unfriendly to documentation, but is simple to
   implement and easy to understand for developers. Most importantly,
   type checkers love it.

   See python/mypy#1212

But, we can have 2 and 3 combined:

```python
_Base = typing.TypeVar('_Base', bound=Base)

class Base:
   def __enter__(self: _Base) -> _Base:
      return self

class Derive(Base): ...

with Derived() as derived:
   ...  # type of derived is Derived and it's a subtype of Base
```

* revert back type of of SteamContextManager to Response

* Remove unused type definitions

* Add comment and link to PEP484 for clarification

* Switch to `T = TypeVar("T", covariant=True)`

* fixup! Switch to `T = TypeVar("T", covariant=True)`

* Add back bound=xxx in TypeVar

Co-authored-by: Florimond Manca <[email protected]>
  • Loading branch information
Congee and florimondmanca authored Oct 2, 2020
1 parent 65b69fa commit 4b87740
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions httpx/_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@
warn_deprecated,
)

# The type annotation for @classmethod and context managers here follows PEP 484
# https://www.python.org/dev/peps/pep-0484/#annotating-instance-and-class-methods
T = typing.TypeVar("T", bound="Client")
U = typing.TypeVar("U", bound="AsyncClient")


logger = get_logger(__name__)

KEEPALIVE_EXPIRY = 5.0
Expand Down Expand Up @@ -1106,7 +1112,7 @@ def close(self) -> None:
if proxy is not None:
proxy.close()

def __enter__(self) -> "Client":
def __enter__(self: T) -> T:
self._transport.__enter__()
for proxy in self._proxies.values():
if proxy is not None:
Expand Down Expand Up @@ -1752,7 +1758,7 @@ async def aclose(self) -> None:
if proxy is not None:
await proxy.aclose()

async def __aenter__(self) -> "AsyncClient":
async def __aenter__(self: U) -> U:
await self._transport.__aenter__()
for proxy in self._proxies.values():
if proxy is not None:
Expand Down

0 comments on commit 4b87740

Please sign in to comment.