From 0bd78b135999f7cc0e4516030b597599936a87eb Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Sep 2023 15:59:55 +0100 Subject: [PATCH 1/6] _LinearizerEntry.deferred: key and value types --- synapse/util/async_helpers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/util/async_helpers.py b/synapse/util/async_helpers.py index 943ad5445604..2989106fbb89 100644 --- a/synapse/util/async_helpers.py +++ b/synapse/util/async_helpers.py @@ -43,6 +43,7 @@ ) import attr +import typing from typing_extensions import Concatenate, Literal, ParamSpec from twisted.internet import defer @@ -398,7 +399,7 @@ class _LinearizerEntry: # The number of things executing. count: int # Deferreds for the things blocked from executing. - deferreds: collections.OrderedDict + deferreds: typing.OrderedDict["defer.Deferred[None]", Literal[1]] class Linearizer: From a35fa4f474ece9e1e0b6cb1d805906bcb3637f0c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Sep 2023 17:05:39 +0100 Subject: [PATCH 2/6] Generic attrs classes I could spot --- synapse/storage/controllers/persist_events.py | 8 +++----- synapse/util/async_helpers.py | 2 +- synapse/util/caches/dictionary_cache.py | 10 ++++------ synapse/util/caches/expiringcache.py | 6 +++--- synapse/util/caches/ttlcache.py | 10 +++++----- 5 files changed, 16 insertions(+), 20 deletions(-) diff --git a/synapse/storage/controllers/persist_events.py b/synapse/storage/controllers/persist_events.py index abd1d149dbc5..6864f9309020 100644 --- a/synapse/storage/controllers/persist_events.py +++ b/synapse/storage/controllers/persist_events.py @@ -154,12 +154,13 @@ def try_merge(self, task: "_EventPersistQueueTask") -> bool: _EventPersistQueueTask = Union[_PersistEventsTask, _UpdateCurrentStateTask] +_PersistResult = TypeVar("_PersistResult") @attr.s(auto_attribs=True, slots=True) -class _EventPersistQueueItem: +class _EventPersistQueueItem(Generic[_PersistResult]): task: _EventPersistQueueTask - deferred: ObservableDeferred + deferred: ObservableDeferred[_PersistResult] parent_opentracing_span_contexts: List = attr.ib(factory=list) """A list of opentracing spans waiting for this batch""" @@ -168,9 +169,6 @@ class _EventPersistQueueItem: """The opentracing span under which the persistence actually happened""" -_PersistResult = TypeVar("_PersistResult") - - class _EventPeristenceQueue(Generic[_PersistResult]): """Queues up tasks so that they can be processed with only one concurrent transaction per room. diff --git a/synapse/util/async_helpers.py b/synapse/util/async_helpers.py index 2989106fbb89..f5ab204e16c5 100644 --- a/synapse/util/async_helpers.py +++ b/synapse/util/async_helpers.py @@ -19,6 +19,7 @@ import inspect import itertools import logging +import typing from contextlib import asynccontextmanager from typing import ( Any, @@ -43,7 +44,6 @@ ) import attr -import typing from typing_extensions import Concatenate, Literal, ParamSpec from twisted.internet import defer diff --git a/synapse/util/caches/dictionary_cache.py b/synapse/util/caches/dictionary_cache.py index 5eaf70c7abba..2fbc7b1e6c32 100644 --- a/synapse/util/caches/dictionary_cache.py +++ b/synapse/util/caches/dictionary_cache.py @@ -14,7 +14,7 @@ import enum import logging import threading -from typing import Any, Dict, Generic, Iterable, Optional, Set, Tuple, TypeVar, Union +from typing import Dict, Generic, Iterable, Optional, Set, Tuple, TypeVar, Union import attr from typing_extensions import Literal @@ -33,10 +33,8 @@ DV = TypeVar("DV") -# This class can't be generic because it uses slots with attrs. -# See: https://github.com/python-attrs/attrs/issues/313 @attr.s(slots=True, frozen=True, auto_attribs=True) -class DictionaryEntry: # should be: Generic[DKT, DV]. +class DictionaryEntry(Generic[DKT, DV]): """Returned when getting an entry from the cache If `full` is true then `known_absent` will be the empty set. @@ -50,8 +48,8 @@ class DictionaryEntry: # should be: Generic[DKT, DV]. """ full: bool - known_absent: Set[Any] # should be: Set[DKT] - value: Dict[Any, Any] # should be: Dict[DKT, DV] + known_absent: Set[DKT] + value: Dict[DKT, DV] def __len__(self) -> int: return len(self.value) diff --git a/synapse/util/caches/expiringcache.py b/synapse/util/caches/expiringcache.py index 01ad02af6703..841dd77710b2 100644 --- a/synapse/util/caches/expiringcache.py +++ b/synapse/util/caches/expiringcache.py @@ -73,7 +73,7 @@ def __init__( self._expiry_ms = expiry_ms self._reset_expiry_on_get = reset_expiry_on_get - self._cache: OrderedDict[KT, _CacheEntry] = OrderedDict() + self._cache: OrderedDict[KT, _CacheEntry[VT]] = OrderedDict() self.iterable = iterable @@ -218,6 +218,6 @@ def set_cache_factor(self, factor: float) -> bool: @attr.s(slots=True, auto_attribs=True) -class _CacheEntry: +class _CacheEntry(Generic[VT]): time: int - value: Any + value: VT diff --git a/synapse/util/caches/ttlcache.py b/synapse/util/caches/ttlcache.py index f6b3ee31e4b8..48a6e4a9067f 100644 --- a/synapse/util/caches/ttlcache.py +++ b/synapse/util/caches/ttlcache.py @@ -35,10 +35,10 @@ class TTLCache(Generic[KT, VT]): def __init__(self, cache_name: str, timer: Callable[[], float] = time.time): # map from key to _CacheEntry - self._data: Dict[KT, _CacheEntry] = {} + self._data: Dict[KT, _CacheEntry[KT, VT]] = {} # the _CacheEntries, sorted by expiry time - self._expiry_list: SortedList[_CacheEntry] = SortedList() + self._expiry_list: SortedList[_CacheEntry[KT, VT]] = SortedList() self._timer = timer @@ -160,11 +160,11 @@ def expire(self) -> None: @attr.s(frozen=True, slots=True, auto_attribs=True) -class _CacheEntry: # Should be Generic[KT, VT]. See python-attrs/attrs#313 +class _CacheEntry(Generic[KT, VT]): """TTLCache entry""" # expiry_time is the first attribute, so that entries are sorted by expiry. expiry_time: float ttl: float - key: Any # should be KT - value: Any # should be VT + key: KT + value: VT From 21571d1719b5059a39f908ffb0ccf1f5265db6e5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Sep 2023 17:05:50 +0100 Subject: [PATCH 3/6] Pattern->Pattern[str] --- synapse/config/oembed.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/oembed.py b/synapse/config/oembed.py index d7959639ee62..59bc0b55f4c8 100644 --- a/synapse/config/oembed.py +++ b/synapse/config/oembed.py @@ -30,7 +30,7 @@ class OEmbedEndpointConfig: # The API endpoint to fetch. api_endpoint: str # The patterns to match. - url_patterns: List[Pattern] + url_patterns: List[Pattern[str]] # The supported formats. formats: Optional[List[str]] From 0558c1eedd9ad0635f6079ee0c7c7870e492867d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Sep 2023 16:06:06 +0100 Subject: [PATCH 4/6] Annotate DoneAwaitable Simplify(?) the DoneAwaitable impl --- synapse/util/async_helpers.py | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/synapse/util/async_helpers.py b/synapse/util/async_helpers.py index f5ab204e16c5..0cbeb0c365d6 100644 --- a/synapse/util/async_helpers.py +++ b/synapse/util/async_helpers.py @@ -30,6 +30,7 @@ Collection, Coroutine, Dict, + Generator, Generic, Hashable, Iterable, @@ -718,30 +719,25 @@ def failure_cb(val: Failure) -> None: return new_d -# This class can't be generic because it uses slots with attrs. -# See: https://github.com/python-attrs/attrs/issues/313 @attr.s(slots=True, frozen=True, auto_attribs=True) -class DoneAwaitable: # should be: Generic[R] +class DoneAwaitable(Awaitable[R]): """Simple awaitable that returns the provided value.""" - value: Any # should be: R + value: R - def __await__(self) -> Any: - return self - - def __iter__(self) -> "DoneAwaitable": - return self - - def __next__(self) -> None: - raise StopIteration(self.value) + def __await__(self) -> Generator[Any, None, R]: + yield None + return self.value def maybe_awaitable(value: Union[Awaitable[R], R]) -> Awaitable[R]: """Convert a value to an awaitable if not already an awaitable.""" if inspect.isawaitable(value): - assert isinstance(value, Awaitable) return value + # For some reason mypy doesn't deduce that value is not Awaitable here, even though + # inspect.isawaitable returns a TypeGuard. + assert not isinstance(value, Awaitable) return DoneAwaitable(value) From b12a786954cd00552542e2542c5369025b39b28f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Sep 2023 17:25:51 +0100 Subject: [PATCH 5/6] Changelog --- changelog.d/16276.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/16276.misc diff --git a/changelog.d/16276.misc b/changelog.d/16276.misc new file mode 100644 index 000000000000..93ceaeafc9b9 --- /dev/null +++ b/changelog.d/16276.misc @@ -0,0 +1 @@ +Improve type hints. From a611a80056b82d127e00ff9141dce84042f3707d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 7 Sep 2023 17:43:06 +0100 Subject: [PATCH 6/6] Fix ExpiringCache Sized complaints --- synapse/util/caches/expiringcache.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/synapse/util/caches/expiringcache.py b/synapse/util/caches/expiringcache.py index 841dd77710b2..8e4c34039dac 100644 --- a/synapse/util/caches/expiringcache.py +++ b/synapse/util/caches/expiringcache.py @@ -14,7 +14,7 @@ import logging from collections import OrderedDict -from typing import Any, Generic, Optional, TypeVar, Union, overload +from typing import Any, Generic, Iterable, Optional, TypeVar, Union, overload import attr from typing_extensions import Literal @@ -100,7 +100,10 @@ def evict(self) -> None: while self._max_size and len(self) > self._max_size: _key, value = self._cache.popitem(last=False) if self.iterable: - self.metrics.inc_evictions(EvictionReason.size, len(value.value)) + # type-ignore, here and below: if self.iterable is true, then the value + # type VT should be Sized (i.e. have a __len__ method). We don't enforce + # this via the type system at present. + self.metrics.inc_evictions(EvictionReason.size, len(value.value)) # type: ignore[arg-type] else: self.metrics.inc_evictions(EvictionReason.size) @@ -134,7 +137,7 @@ def pop(self, key: KT, default: T = SENTINEL) -> Union[VT, T]: return default if self.iterable: - self.metrics.inc_evictions(EvictionReason.invalidation, len(value.value)) + self.metrics.inc_evictions(EvictionReason.invalidation, len(value.value)) # type: ignore[arg-type] else: self.metrics.inc_evictions(EvictionReason.invalidation) @@ -182,7 +185,7 @@ async def _prune_cache(self) -> None: for k in keys_to_delete: value = self._cache.pop(k) if self.iterable: - self.metrics.inc_evictions(EvictionReason.time, len(value.value)) + self.metrics.inc_evictions(EvictionReason.time, len(value.value)) # type: ignore[arg-type] else: self.metrics.inc_evictions(EvictionReason.time) @@ -195,7 +198,8 @@ async def _prune_cache(self) -> None: def __len__(self) -> int: if self.iterable: - return sum(len(entry.value) for entry in self._cache.values()) + g: Iterable[int] = (len(entry.value) for entry in self._cache.values()) # type: ignore[arg-type] + return sum(g) else: return len(self._cache)