Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update next_event annotation to reflect possible return types #144

Merged
merged 1 commit into from
Aug 25, 2022

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Mar 25, 2022

Hi! I'm updating httpcore's type annotations (encode/httpcore#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
Copy link
Contributor Author

zanieb commented Mar 25, 2022

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
Copy link
Member

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

@zanieb
Copy link
Contributor Author

zanieb commented Mar 26, 2022

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
Copy link
Member

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
Copy link
Contributor Author

zanieb commented Mar 26, 2022

They are two separate commits already, here's a separate PR for the format changes #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
Copy link
Contributor Author

zanieb commented Apr 8, 2022

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
Copy link
Member

pgjones commented Aug 25, 2022

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

@pgjones pgjones merged commit 04cc0f7 into python-hyper:master Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants