python-hyper / h11

A pure-Python, bring-your-own-I/O implementation of HTTP/1.1
https://h11.readthedocs.io/
MIT License
490 stars 62 forks source link

Update `next_event` annotation to reflect possible return types #144

Closed zanieb closed 2 years ago

zanieb commented 2 years ago

Hi! I'm updating httpcore's type annotations (https://github.com/encode/httpcore/pull/526) to be compatible with the latest h11 version and encountering a difficulty with the type annotations on next_event. Since the return type includes Sentinel which is a private type in h11, the return type in httpcore cannot be defined correctly.

To resolve this, the Sentinel type could be exposed publicly. However, arbitrary sentinels cannot be handled by downstream libraries. Instead, they should have a well defined set of sentinels that they know to handle. NEEDS_DATA and PAUSED are already publicly exposed and are the two sentinel types that can be returned by needs_data. Narrowing the type definition to these specific types will confer to consumers that they should handle these explicit sentinel types. If you add more sentinel types in the future, type checkers will warn users that they are not handling all of the possible return types for this function.

There may be other Sentinel return type annotations that could/should be updated as well.

zanieb commented 2 years ago

mypy is righteously failing with

 h11/_connection.py:481: error: Incompatible return value type (got "Union[Event, Type[Sentinel]]",
  expected "Union[Event, NEED_DATA, PAUSED]")  [return-value]

The type of the return value is from event = self._extract_next_receive_event() which can return other sentinel types. These are handled with

if event not in [NEED_DATA, PAUSED]:
   self._process_event(self.their_role, cast(Event, event))

which I presume mutates the event, but it does not return a value so the type is not narrowed.

sethmlarson commented 2 years ago

I don't like the idea of exposing Sentinel, does Union[..., Type[NEED_DATA], Type[PAUSED]] not work for this case?

zanieb commented 2 years ago

Unfortunately not. Since those types are narrower than Sentinel which is the indicated return type here, users would have to cast your return type to avoid type errors. Narrowing the return type here to include the two sentinel types expected is the one-line change I've done at https://github.com/python-hyper/h11/pull/144/files#diff-ffc5037b2fa9563b8ff06d6dd65417389ec0ce6fb5fb38dc63dc9038ae12bb4fR424. However, this causes your type check suite to fail as described in my comment above.

sethmlarson commented 2 years ago

This change seems good, it's unfortunate we got some formatting changes from black. Could you separate the formatting changes and change to the types into two separate commits so I can rebase merge them?

zanieb commented 2 years ago

They are two separate commits already, here's a separate PR for the format changes https://github.com/python-hyper/h11/pull/145 though. I can drop the format commit from this PR if you want to merge that one separately.

Note this PR will still fail mypy without an additional cast or change to _process_event.

zanieb commented 2 years ago

I've rebased onto master, dropping the formatting commit. We'll still need a cast or an update to how _process_event works to pass type checks though.

pgjones commented 2 years ago

Thanks, I'll merge this and #151 and consolidate if required.