Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add missing type hints to synapse.logging.context #11556

Merged
merged 17 commits into from
Dec 14, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 41 additions & 2 deletions synapse/logging/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,17 @@
import threading
import typing
import warnings
from typing import TYPE_CHECKING, Optional, Tuple, TypeVar, Union
from typing import (
TYPE_CHECKING,
Any,
Awaitable,
Callable,
Optional,
Tuple,
TypeVar,
Union,
overload,
)

import attr
from typing_extensions import Literal
Expand Down Expand Up @@ -711,6 +721,9 @@ def nested_logging_context(suffix: str) -> LoggingContext:
)


R = TypeVar("R")


def preserve_fn(f):
"""Function decorator which wraps the function with run_in_background"""

Expand All @@ -720,7 +733,30 @@ def g(*args, **kwargs):
return g


def run_in_background(f, *args, **kwargs) -> defer.Deferred:
@overload
def run_in_background( # type: ignore[misc]
f: Callable[..., Awaitable[R]], *args: Any, **kwargs: Any
) -> "defer.Deferred[R]":
# The `type: ignore[misc]` above suppresses
# "Overloaded function signatures 1 and 2 overlap with incompatible return types"
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
...


@overload
def run_in_background(
f: Callable[..., R], *args: Any, **kwargs: Any
) -> "defer.Deferred[R]":
...


def run_in_background(
f: Union[
Callable[..., R],
Callable[..., Awaitable[R]],
],
Comment on lines +771 to +774
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+764 seems to suggest that f could return a defer.Deferred[R] too.

(Is Deferred[T] considered compatible with Awaitable[T]?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is: https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/defer.py#L347

The T in Deferred[T] is contravariant for some reason. If it weren't contravariant, we wouldn't need the type: ignores for the overloads, for reasons I haven't figured out yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Well that makes life much easier!

The T in Deferred[T] is contravariant for some reason.

Maybe because the T gets passed to a callback, and Callable[T, ...] is contravariant in T?

Copy link
Contributor Author

@squahtx squahtx Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense!

*args: Any,
**kwargs: Any,
) -> "defer.Deferred[R]":
"""Calls a function, ensuring that the current context is restored after
return from the function, and that the sentinel context is set once the
deferred returned by the function completes.
Expand Down Expand Up @@ -751,6 +787,9 @@ def run_in_background(f, *args, **kwargs) -> defer.Deferred:
# At this point we should have a Deferred, if not then f was a synchronous
# function, wrap it in a Deferred for consistency.
if not isinstance(res, defer.Deferred):
# All `Awaitable`s in Synapse are either coroutines or `Deferred`s
assert not isinstance(res, Awaitable)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this, but also not sure if I can put my finger on why this gives me a bad feeling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered accepting a union, but decided it wasn't worth the hassle of replacing Awaitables throughout synapse.

I'm not sure how we'd handle other awaitables here. Create a dummy async function which awaits them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh. I don't think I'd fully appreciated that Awaitable and Coroutine were different concepts.

Would it help to have a Synapse-only type alias for Union[Coroutine-that-returns-T, Deferred[T]]?

Copy link
Contributor Author

@squahtx squahtx Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion in #synapse-dev, I'm going to leave the assert as is.

Creating a type alias looks tempting for correctness, but will end up propagating everywhere without much benefit, considering the lack of non-Coroutine, non-Deferred awaitables.

return defer.succeed(res)

if res.called and not res.paused:
Expand Down