From 8cffbb50530e4a21c86f65a607aab0782aea4c07 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 13:16:28 +0100 Subject: [PATCH 01/21] Make synapse.util.versionstring pass `no-untyped-def` --- mypy.ini | 3 +++ synapse/util/versionstring.py | 18 +++++++++++------- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/mypy.ini b/mypy.ini index 3cb6cecd7e8c..c5329774c4e6 100644 --- a/mypy.ini +++ b/mypy.ini @@ -160,6 +160,9 @@ disallow_untyped_defs = True [mypy-synapse.util.wheel_timer] disallow_untyped_defs = True +[mypy-synapse.util.versionstring] +disallow_untyped_defs = True + [mypy-pymacaroons.*] ignore_missing_imports = True diff --git a/synapse/util/versionstring.py b/synapse/util/versionstring.py index 1c20b24bbe68..a67a38f68218 100644 --- a/synapse/util/versionstring.py +++ b/synapse/util/versionstring.py @@ -15,14 +15,18 @@ import logging import os import subprocess +from types import ModuleType +from typing import Dict logger = logging.getLogger(__name__) +version_cache: Dict[ModuleType, str] = {} -def get_version_string(module) -> str: + +def get_version_string(module: ModuleType) -> str: """Given a module calculate a git-aware version string for it. - If called on a module not in a git checkout will return `__verison__`. + If called on a module not in a git checkout will return `__version__`. Args: module (module) @@ -31,11 +35,11 @@ def get_version_string(module) -> str: str """ - cached_version = getattr(module, "_synapse_version_string_cache", None) - if cached_version: + cached_version = version_cache.get(module) + if cached_version is not None: return cached_version - version_string = module.__version__ + version_string = getattr(module, "__version__", "") try: null = open(os.devnull, "w") @@ -97,10 +101,10 @@ def get_version_string(module) -> str: s for s in (git_branch, git_tag, git_commit, git_dirty) if s ) - version_string = "%s (%s)" % (module.__version__, git_version) + version_string = "%s (%s)" % (version_string, git_version) except Exception as e: logger.info("Failed to check for git repository: %s", e) - module._synapse_version_string_cache = version_string + version_cache[module] = version_string return version_string From f3ad10150b954bdbbb96fcaa3e1a7f189a1e54dc Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 13:28:39 +0100 Subject: [PATCH 02/21] Make synapse.utils.daemonize pass `no-untyped-def` --- mypy.ini | 3 +++ synapse/util/daemonize.py | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/mypy.ini b/mypy.ini index c5329774c4e6..e7f0500c08ce 100644 --- a/mypy.ini +++ b/mypy.ini @@ -103,6 +103,9 @@ disallow_untyped_defs = True [mypy-synapse.util.caches.dictionary_cache] disallow_untyped_defs = True +[mypy-synapse.util.daemonize] +disallow_untyped_defs = True + [mypy-synapse.util.file_consumer] disallow_untyped_defs = True diff --git a/synapse/util/daemonize.py b/synapse/util/daemonize.py index f1a351cfd4a6..de04f34e4e5c 100644 --- a/synapse/util/daemonize.py +++ b/synapse/util/daemonize.py @@ -19,6 +19,8 @@ import os import signal import sys +from types import FrameType, TracebackType +from typing import NoReturn, Type def daemonize_process(pid_file: str, logger: logging.Logger, chdir: str = "/") -> None: @@ -97,7 +99,9 @@ def daemonize_process(pid_file: str, logger: logging.Logger, chdir: str = "/") - # (we don't normally expect reactor.run to raise any exceptions, but this will # also catch any other uncaught exceptions before we get that far.) - def excepthook(type_, value, traceback): + def excepthook( + type_: Type[BaseException], value: BaseException, traceback: TracebackType + ) -> None: logger.critical("Unhanded exception", exc_info=(type_, value, traceback)) sys.excepthook = excepthook @@ -119,7 +123,7 @@ def excepthook(type_, value, traceback): sys.exit(1) # write a log line on SIGTERM. - def sigterm(signum, frame): + def sigterm(signum: signal.Signals, frame: FrameType) -> NoReturn: logger.warning("Caught signal %s. Stopping daemon." % signum) sys.exit(0) From 0a70837a2dc55e28e60fff148a55f4ad973b0399 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 14:55:44 +0100 Subject: [PATCH 03/21] Additional typing in synapse.util.metrics Didn't get this to pass `no-untyped-def`, think I'll need to watch #10847 --- synapse/util/metrics.py | 35 ++++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/synapse/util/metrics.py b/synapse/util/metrics.py index 1b82dca81b08..bdd9cbc7ec13 100644 --- a/synapse/util/metrics.py +++ b/synapse/util/metrics.py @@ -14,7 +14,8 @@ import logging from functools import wraps -from typing import Any, Callable, Optional, TypeVar, cast +from types import TracebackType +from typing import Any, Callable, Optional, Protocol, Type, TypeVar, cast from prometheus_client import Counter @@ -24,6 +25,7 @@ current_context, ) from synapse.metrics import InFlightGauge +from synapse.util import Clock logger = logging.getLogger(__name__) @@ -61,10 +63,15 @@ sub_metrics=["real_time_max", "real_time_sum"], ) -T = TypeVar("T", bound=Callable[..., Any]) +R = TypeVar("R") +F = Callable[..., R] -def measure_func(name: Optional[str] = None) -> Callable[[T], T]: +class HasClock(Protocol): + clock: Clock + + +def measure_func(name: Optional[str] = None) -> Callable[[F], F]: """ Used to decorate an async function with a `Measure` context manager. @@ -82,16 +89,16 @@ async def foo(...): """ - def wrapper(func: T) -> T: + def wrapper(func: F) -> F: block_name = func.__name__ if name is None else name @wraps(func) - async def measured_func(self, *args, **kwargs): + async def measured_func(self: HasClock, *args: Any, **kwargs: Any) -> R: with Measure(self.clock, block_name): r = await func(self, *args, **kwargs) return r - return cast(T, measured_func) + return cast(F, measured_func) return wrapper @@ -104,10 +111,10 @@ class Measure: "start", ] - def __init__(self, clock, name: str): + def __init__(self, clock: Clock, name: str) -> None: """ Args: - clock: A n object with a "time()" method, which returns the current + clock: An object with a "time()" method, which returns the current time in seconds. name: The name of the metric to report. """ @@ -124,7 +131,7 @@ def __init__(self, clock, name: str): assert isinstance(curr_context, LoggingContext) parent_context = curr_context self._logging_context = LoggingContext(str(curr_context), parent_context) - self.start: Optional[int] = None + self.start: Optional[float] = None def __enter__(self) -> "Measure": if self.start is not None: @@ -138,7 +145,12 @@ def __enter__(self) -> "Measure": return self - def __exit__(self, exc_type, exc_val, exc_tb): + def __exit__( + self, + exc_type: Optional[Type[BaseException]], + exc_val: Optional[BaseException], + exc_tb: Optional[TracebackType], + ) -> None: if self.start is None: raise RuntimeError("Measure() block exited without being entered") @@ -168,8 +180,9 @@ def get_resource_usage(self) -> ContextResourceUsage: """ return self._logging_context.get_resource_usage() - def _update_in_flight(self, metrics): + def _update_in_flight(self, metrics) -> None: """Gets called when processing in flight metrics""" + assert self.start is not None duration = self.clock.time() - self.start metrics.real_time_max = max(metrics.real_time_max, duration) From 9de84e6ec9b6e4e63b9209b59ecbabceea647365 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 15:50:03 +0100 Subject: [PATCH 04/21] Make synapse.util.caches.ttlcache pass `no-untyped-defs` --- mypy.ini | 3 +++ synapse/util/caches/ttlcache.py | 12 ++++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/mypy.ini b/mypy.ini index e7f0500c08ce..0bb2c9398d93 100644 --- a/mypy.ini +++ b/mypy.ini @@ -103,6 +103,9 @@ disallow_untyped_defs = True [mypy-synapse.util.caches.dictionary_cache] disallow_untyped_defs = True +[mypy-synapse.util.caches.ttl_cache] +disallow_untyped_defs = True + [mypy-synapse.util.daemonize] disallow_untyped_defs = True diff --git a/synapse/util/caches/ttlcache.py b/synapse/util/caches/ttlcache.py index 46afe3f934ab..f7f57b7f7320 100644 --- a/synapse/util/caches/ttlcache.py +++ b/synapse/util/caches/ttlcache.py @@ -159,12 +159,12 @@ def expire(self) -> None: del self._expiry_list[0] -@attr.s(frozen=True, slots=True) -class _CacheEntry: +@attr.s(frozen=True, slots=True, auto_attribs=True) +class _CacheEntry(Generic[KT, VT]): """TTLCache entry""" # expiry_time is the first attribute, so that entries are sorted by expiry. - expiry_time = attr.ib(type=float) - ttl = attr.ib(type=float) - key = attr.ib() - value = attr.ib() + expiry_time: float + ttl: float + key: KT + value: VT From 19bdaeceba9eddafd0bd3c57f2fa99f5f728fbff Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 16:43:55 +0100 Subject: [PATCH 05/21] Make patch_inline_callbacks pass `no-untyped-defs` --- mypy.ini | 3 +++ synapse/util/patch_inline_callbacks.py | 16 +++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/mypy.ini b/mypy.ini index 0bb2c9398d93..d1682433c275 100644 --- a/mypy.ini +++ b/mypy.ini @@ -145,6 +145,9 @@ disallow_untyped_defs = True [mypy-synapse.util.msisdn] disallow_untyped_defs = True +[mypy-synapse.util.patch_inline_callbacks] +disallow_untyped_defs = True + [mypy-synapse.util.ratelimitutils] disallow_untyped_defs = True diff --git a/synapse/util/patch_inline_callbacks.py b/synapse/util/patch_inline_callbacks.py index 9dd010af3b0e..fb0214d1fec8 100644 --- a/synapse/util/patch_inline_callbacks.py +++ b/synapse/util/patch_inline_callbacks.py @@ -14,7 +14,7 @@ import functools import sys -from typing import Any, Callable, List +from typing import Any, Callable, Generator, List from twisted.internet import defer from twisted.internet.defer import Deferred @@ -37,12 +37,14 @@ def do_patch() -> None: if _already_patched: return - def new_inline_callbacks(f): + def new_inline_callbacks(f: Callable[..., Generator]) -> Callable[..., Any]: @functools.wraps(f) - def wrapped(*args, **kwargs): + def wrapped(*args: Any, **kwargs: Any) -> Any: start_context = current_context() changes: List[str] = [] - orig = orig_inline_callbacks(_check_yield_points(f, changes)) + orig: Callable[..., Deferred] = orig_inline_callbacks( + _check_yield_points(f, changes) + ) try: res = orig(*args, **kwargs) @@ -84,7 +86,7 @@ def wrapped(*args, **kwargs): print(err, file=sys.stderr) raise Exception(err) - def check_ctx(r): + def check_ctx(r: Any) -> Any: if current_context() != start_context: for err in changes: print(err, file=sys.stderr) @@ -107,7 +109,7 @@ def check_ctx(r): _already_patched = True -def _check_yield_points(f: Callable, changes: List[str]) -> Callable: +def _check_yield_points(f: Callable[..., Generator], changes: List[str]) -> Callable: """Wraps a generator that is about to be passed to defer.inlineCallbacks checking that after every yield the log contexts are correct. @@ -127,7 +129,7 @@ def _check_yield_points(f: Callable, changes: List[str]) -> Callable: from synapse.logging.context import current_context @functools.wraps(f) - def check_yield_points_inner(*args, **kwargs): + def check_yield_points_inner(*args: Any, **kwargs: Any) -> Any: gen = f(*args, **kwargs) last_yield_line_no = gen.gi_frame.f_lineno From 53dae76188f0b5e406dffd3d843ae3a296fcdf7f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 16:49:03 +0100 Subject: [PATCH 06/21] Make `stream_change_cache` pass `no-untyped-defs` --- mypy.ini | 3 +++ synapse/util/caches/stream_change_cache.py | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/mypy.ini b/mypy.ini index d1682433c275..dbe46a8e2f20 100644 --- a/mypy.ini +++ b/mypy.ini @@ -103,6 +103,9 @@ disallow_untyped_defs = True [mypy-synapse.util.caches.dictionary_cache] disallow_untyped_defs = True +[mypy-synapse.util.caches.stream_change_cache] +disallow_untyped_defs = True + [mypy-synapse.util.caches.ttl_cache] disallow_untyped_defs = True diff --git a/synapse/util/caches/stream_change_cache.py b/synapse/util/caches/stream_change_cache.py index 27b1da235ef3..330709b8b778 100644 --- a/synapse/util/caches/stream_change_cache.py +++ b/synapse/util/caches/stream_change_cache.py @@ -40,10 +40,10 @@ def __init__( self, name: str, current_stream_pos: int, - max_size=10000, + max_size: int = 10000, prefilled_cache: Optional[Mapping[EntityType, int]] = None, - ): - self._original_max_size = max_size + ) -> None: + self._original_max_size: int = max_size self._max_size = math.floor(max_size) self._entity_to_key: Dict[EntityType, int] = {} From 8d1da44ac7362fb58300e0de9646dd98c085557d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 17:08:54 +0100 Subject: [PATCH 07/21] Make `cached_call` pass `no-untyped-def` --- mypy.ini | 3 +++ synapse/util/caches/cached_call.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mypy.ini b/mypy.ini index dbe46a8e2f20..bf97d3244226 100644 --- a/mypy.ini +++ b/mypy.ini @@ -100,6 +100,9 @@ disallow_untyped_defs = True [mypy-synapse.util.batching_queue] disallow_untyped_defs = True +[mypy-synapse.util.caches.cached_call] +disallow_untyped_defs = True + [mypy-synapse.util.caches.dictionary_cache] disallow_untyped_defs = True diff --git a/synapse/util/caches/cached_call.py b/synapse/util/caches/cached_call.py index e58dd91eda7b..470f4f91a59b 100644 --- a/synapse/util/caches/cached_call.py +++ b/synapse/util/caches/cached_call.py @@ -85,7 +85,7 @@ async def get(self) -> TV: # result in the deferred, since `awaiting` a deferred destroys its result. # (Also, if it's a Failure, GCing the deferred would log a critical error # about unhandled Failures) - def got_result(r): + def got_result(r: Union[TV, Failure]) -> None: self._result = r self._deferred.addBoth(got_result) From 9f9073fd52e2938d939bf8ad3870cf60a67926d4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 17:09:15 +0100 Subject: [PATCH 08/21] Make `lrucache` pass `no-untyped-def` --- mypy.ini | 3 ++ synapse/util/caches/lrucache.py | 52 ++++++++++++++++++--------------- 2 files changed, 32 insertions(+), 23 deletions(-) diff --git a/mypy.ini b/mypy.ini index bf97d3244226..688d73c0a201 100644 --- a/mypy.ini +++ b/mypy.ini @@ -106,6 +106,9 @@ disallow_untyped_defs = True [mypy-synapse.util.caches.dictionary_cache] disallow_untyped_defs = True +[mypy-synapse.util.caches.lrucache] +disallow_untyped_defs = True + [mypy-synapse.util.caches.stream_change_cache] disallow_untyped_defs = True diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py index ea6e8dc8d19e..5635b674f054 100644 --- a/synapse/util/caches/lrucache.py +++ b/synapse/util/caches/lrucache.py @@ -21,6 +21,7 @@ Any, Callable, Collection, + Dict, Generic, Iterable, List, @@ -52,7 +53,7 @@ try: from pympler.asizeof import Asizer - def _get_size_of(val: Any, *, recurse=True) -> int: + def _get_size_of(val: Any, *, recurse: bool = True) -> int: """Get an estimate of the size in bytes of the object. Args: @@ -71,7 +72,7 @@ def _get_size_of(val: Any, *, recurse=True) -> int: except ImportError: - def _get_size_of(val: Any, *, recurse=True) -> int: + def _get_size_of(val: Any, *, recurse: bool = True) -> int: return 0 @@ -86,7 +87,8 @@ def _get_size_of(val: Any, *, recurse=True) -> int: T = TypeVar("T") -def enumerate_leaves(node, depth): +# TODO I think this is this unused? +def enumerate_leaves(node: Dict, depth: int) -> Any: if depth == 0: yield node else: @@ -102,7 +104,7 @@ class _TimedListNode(ListNode[P]): __slots__ = ["last_access_ts_secs"] - def update_last_access(self, clock: Clock): + def update_last_access(self, clock: Clock) -> None: self.last_access_ts_secs = int(clock.time()) @@ -115,7 +117,7 @@ def update_last_access(self, clock: Clock): @wrap_as_background_process("LruCache._expire_old_entries") -async def _expire_old_entries(clock: Clock, expiry_seconds: int): +async def _expire_old_entries(clock: Clock, expiry_seconds: int) -> None: """Walks the global cache list to find cache entries that haven't been accessed in the given number of seconds. """ @@ -163,7 +165,7 @@ async def _expire_old_entries(clock: Clock, expiry_seconds: int): logger.info("Dropped %d items from caches", i) -def setup_expire_lru_cache_entries(hs: "HomeServer"): +def setup_expire_lru_cache_entries(hs: "HomeServer") -> None: """Start a background job that expires all cache entries if they have not been accessed for the given number of seconds. """ @@ -183,7 +185,7 @@ def setup_expire_lru_cache_entries(hs: "HomeServer"): ) -class _Node: +class _Node(Generic[KT, VT]): __slots__ = [ "_list_node", "_global_list_node", @@ -197,8 +199,8 @@ class _Node: def __init__( self, root: "ListNode[_Node]", - key, - value, + key: KT, + value: VT, cache: "weakref.ReferenceType[LruCache]", clock: Clock, callbacks: Collection[Callable[[], None]] = (), @@ -407,7 +409,7 @@ def evict() -> None: def synchronized(f: FT) -> FT: @wraps(f) - def inner(*args, **kwargs): + def inner(*args: Any, **kwargs: Any) -> Any: with lock: return f(*args, **kwargs) @@ -416,17 +418,19 @@ def inner(*args, **kwargs): cached_cache_len = [0] if size_callback is not None: - def cache_len(): + def cache_len() -> int: return cached_cache_len[0] else: - def cache_len(): + def cache_len() -> int: return len(cache) self.len = synchronized(cache_len) - def add_node(key, value, callbacks: Collection[Callable[[], None]] = ()): + def add_node( + key: KT, value: VT, callbacks: Collection[Callable[[], None]] = () + ) -> None: node = _Node(list_root, key, value, weak_ref_to_self, real_clock, callbacks) cache[key] = node @@ -436,7 +440,7 @@ def add_node(key, value, callbacks: Collection[Callable[[], None]] = ()): if caches.TRACK_MEMORY_USAGE and metrics: metrics.inc_memory_usage(node.memory) - def move_node_to_front(node: _Node): + def move_node_to_front(node: _Node) -> None: node.move_to_front(real_clock, list_root) def delete_node(node: _Node) -> int: @@ -478,7 +482,7 @@ def cache_get( default: Optional[T] = None, callbacks: Collection[Callable[[], None]] = (), update_metrics: bool = True, - ): + ) -> Union[None, T, VT]: node = cache.get(key, None) if node is not None: move_node_to_front(node) @@ -492,7 +496,9 @@ def cache_get( return default @synchronized - def cache_set(key: KT, value: VT, callbacks: Iterable[Callable[[], None]] = ()): + def cache_set( + key: KT, value: VT, callbacks: Iterable[Callable[[], None]] = () + ) -> None: node = cache.get(key, None) if node is not None: # We sometimes store large objects, e.g. dicts, which cause @@ -537,7 +543,7 @@ def cache_pop(key: KT, default: T) -> Union[T, VT]: ... @synchronized - def cache_pop(key: KT, default: Optional[T] = None): + def cache_pop(key: KT, default: Optional[T] = None) -> Union[None, T, VT]: node = cache.get(key, None) if node: delete_node(node) @@ -602,25 +608,25 @@ def cache_contains(key: KT) -> bool: self.contains = cache_contains self.clear = cache_clear - def __getitem__(self, key): + def __getitem__(self, key: KT) -> VT: result = self.get(key, self.sentinel) if result is self.sentinel: raise KeyError() else: - return result + return cast(VT, result) - def __setitem__(self, key, value): + def __setitem__(self, key: KT, value: VT) -> None: self.set(key, value) - def __delitem__(self, key, value): + def __delitem__(self, key: KT, value: VT) -> None: result = self.pop(key, self.sentinel) if result is self.sentinel: raise KeyError() - def __len__(self): + def __len__(self) -> int: return self.len() - def __contains__(self, key): + def __contains__(self, key: KT) -> bool: return self.contains(key) def set_cache_factor(self, factor: float) -> bool: From 894be5fb9a9b6fa4d5ef3d4daf1926975d26b303 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 17:10:46 +0100 Subject: [PATCH 09/21] Make response_cache pass `no-untyped-def` --- mypy.ini | 3 +++ synapse/util/caches/response_cache.py | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/mypy.ini b/mypy.ini index 688d73c0a201..c1cbd7b30b5c 100644 --- a/mypy.ini +++ b/mypy.ini @@ -109,6 +109,9 @@ disallow_untyped_defs = True [mypy-synapse.util.caches.lrucache] disallow_untyped_defs = True +[mypy-synapse.util.caches.response_cache] +disallow_untyped_defs = True + [mypy-synapse.util.caches.stream_change_cache] disallow_untyped_defs = True diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py index ed7204336f7f..dbb45fc3d482 100644 --- a/synapse/util/caches/response_cache.py +++ b/synapse/util/caches/response_cache.py @@ -126,7 +126,7 @@ def _set( key = context.cache_key self.pending_result_cache[key] = result - def on_complete(r): + def on_complete(r: Any) -> Any: # if this cache has a non-zero timeout, and the callback has not cleared # the should_cache bit, we leave it in the cache for now and schedule # its removal later. From 0f21b3d6888d7056cbd19ad6ee32d26cca14755e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 17:11:16 +0100 Subject: [PATCH 10/21] Make deferred_cache pass `no-untyped-def` --- mypy.ini | 3 +++ synapse/util/caches/deferred_cache.py | 11 ++++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/mypy.ini b/mypy.ini index c1cbd7b30b5c..7d1c98e60a69 100644 --- a/mypy.ini +++ b/mypy.ini @@ -103,6 +103,9 @@ disallow_untyped_defs = True [mypy-synapse.util.caches.cached_call] disallow_untyped_defs = True +[mypy-synapse.util.caches.deferred_cache] +disallow_untyped_defs = True + [mypy-synapse.util.caches.dictionary_cache] disallow_untyped_defs = True diff --git a/synapse/util/caches/deferred_cache.py b/synapse/util/caches/deferred_cache.py index f05590da0d54..ffad0707190e 100644 --- a/synapse/util/caches/deferred_cache.py +++ b/synapse/util/caches/deferred_cache.py @@ -31,6 +31,7 @@ from twisted.internet import defer from twisted.python import failure +from twisted.python.failure import Failure from synapse.util.async_helpers import ObservableDeferred from synapse.util.caches.lrucache import LruCache @@ -110,7 +111,7 @@ def metrics_cb() -> None: self.thread: Optional[threading.Thread] = None @property - def max_entries(self): + def max_entries(self) -> int: return self.cache.max_size def check_thread(self) -> None: @@ -256,7 +257,7 @@ def compare_and_pop() -> bool: return False - def cb(result) -> None: + def cb(result: VT) -> None: if compare_and_pop(): self.cache.set(key, result, entry.callbacks) else: @@ -268,7 +269,7 @@ def cb(result) -> None: # not have been. Either way, let's double-check now. entry.invalidate() - def eb(_fail) -> None: + def eb(_fail: Failure) -> None: compare_and_pop() entry.invalidate() @@ -282,11 +283,11 @@ def eb(_fail) -> None: def prefill( self, key: KT, value: VT, callback: Optional[Callable[[], None]] = None - ): + ) -> None: callbacks = [callback] if callback else [] self.cache.set(key, value, callbacks=callbacks) - def invalidate(self, key): + def invalidate(self, key: KT) -> None: """Delete a key, or tree of entries If the cache is backed by a regular dict, then "key" must be of From 79c2ec706953e3b237e03bf39535177b03f3f5f1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 22 Sep 2021 17:28:23 +0100 Subject: [PATCH 11/21] changelog --- changelog.d/10888.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10888.misc diff --git a/changelog.d/10888.misc b/changelog.d/10888.misc new file mode 100644 index 000000000000..d9c991788125 --- /dev/null +++ b/changelog.d/10888.misc @@ -0,0 +1 @@ +Improve type hinting in `synapse.util`. \ No newline at end of file From c3b6effd4f99405703afe468b1d1e9af5d880a54 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 23 Sep 2021 12:30:17 +0100 Subject: [PATCH 12/21] Corrections to type annotations (thanks Sean) --- synapse/util/caches/response_cache.py | 6 +++--- synapse/util/metrics.py | 6 +++--- synapse/util/patch_inline_callbacks.py | 24 ++++++++++++++++-------- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py index dbb45fc3d482..312491dd52c8 100644 --- a/synapse/util/caches/response_cache.py +++ b/synapse/util/caches/response_cache.py @@ -104,8 +104,8 @@ def get(self, key: KV) -> Optional[defer.Deferred]: return None def _set( - self, context: ResponseCacheContext[KV], deferred: defer.Deferred - ) -> defer.Deferred: + self, context: ResponseCacheContext[KV], deferred: defer.Deferred[RV] + ) -> defer.Deferred[RV]: """Set the entry for the given key to the given deferred. *deferred* should run its callbacks in the sentinel logcontext (ie, @@ -126,7 +126,7 @@ def _set( key = context.cache_key self.pending_result_cache[key] = result - def on_complete(r: Any) -> Any: + def on_complete(r: RV) -> RV: # if this cache has a non-zero timeout, and the callback has not cleared # the should_cache bit, we leave it in the cache for now and schedule # its removal later. diff --git a/synapse/util/metrics.py b/synapse/util/metrics.py index bdd9cbc7ec13..4edb741878a4 100644 --- a/synapse/util/metrics.py +++ b/synapse/util/metrics.py @@ -15,7 +15,7 @@ import logging from functools import wraps from types import TracebackType -from typing import Any, Callable, Optional, Protocol, Type, TypeVar, cast +from typing import Any, Callable, Optional, Protocol, Type, TypeVar, cast, Awaitable from prometheus_client import Counter @@ -64,7 +64,7 @@ ) R = TypeVar("R") -F = Callable[..., R] +F = Callable[..., Awaitable[R]] class HasClock(Protocol): @@ -98,7 +98,7 @@ async def measured_func(self: HasClock, *args: Any, **kwargs: Any) -> R: r = await func(self, *args, **kwargs) return r - return cast(F, measured_func) + return measured_func return wrapper diff --git a/synapse/util/patch_inline_callbacks.py b/synapse/util/patch_inline_callbacks.py index fb0214d1fec8..78697ba339e3 100644 --- a/synapse/util/patch_inline_callbacks.py +++ b/synapse/util/patch_inline_callbacks.py @@ -14,7 +14,7 @@ import functools import sys -from typing import Any, Callable, Generator, List +from typing import Any, Callable, Generator, List, TypeVar from twisted.internet import defer from twisted.internet.defer import Deferred @@ -24,6 +24,9 @@ _already_patched = False +T = TypeVar("T") + + def do_patch() -> None: """ Patch defer.inlineCallbacks so that it checks the state of the logcontext on exit @@ -37,17 +40,19 @@ def do_patch() -> None: if _already_patched: return - def new_inline_callbacks(f: Callable[..., Generator]) -> Callable[..., Any]: + def new_inline_callbacks( + f: Callable[..., Generator[Deferred[object], object, T]] + ) -> Callable[..., Deferred[T]]: @functools.wraps(f) - def wrapped(*args: Any, **kwargs: Any) -> Any: + def wrapped(*args: Any, **kwargs: Any) -> Deferred[T]: start_context = current_context() changes: List[str] = [] - orig: Callable[..., Deferred] = orig_inline_callbacks( + orig: Callable[..., Deferred[T]] = orig_inline_callbacks( _check_yield_points(f, changes) ) try: - res = orig(*args, **kwargs) + res: Deferred[T] = orig(*args, **kwargs) except Exception: if current_context() != start_context: for err in changes: @@ -86,7 +91,7 @@ def wrapped(*args: Any, **kwargs: Any) -> Any: print(err, file=sys.stderr) raise Exception(err) - def check_ctx(r: Any) -> Any: + def check_ctx(r: T) -> T: if current_context() != start_context: for err in changes: print(err, file=sys.stderr) @@ -109,7 +114,10 @@ def check_ctx(r: Any) -> Any: _already_patched = True -def _check_yield_points(f: Callable[..., Generator], changes: List[str]) -> Callable: +def _check_yield_points( + f: Callable[..., Generator[Deferred[object], object, T]], + changes: List[str], +)-> Callable: """Wraps a generator that is about to be passed to defer.inlineCallbacks checking that after every yield the log contexts are correct. @@ -129,7 +137,7 @@ def _check_yield_points(f: Callable[..., Generator], changes: List[str]) -> Call from synapse.logging.context import current_context @functools.wraps(f) - def check_yield_points_inner(*args: Any, **kwargs: Any) -> Any: + def check_yield_points_inner(*args: Any, **kwargs: Any) -> Generator[Deferred[object], object, T]: gen = f(*args, **kwargs) last_yield_line_no = gen.gi_frame.f_lineno From 5c740ef1934e18455d8e14c7dadfbbb245cde1d4 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 27 Sep 2021 14:54:25 +0100 Subject: [PATCH 13/21] Remove unused `enumerate_leaves` --- synapse/util/caches/lrucache.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/synapse/util/caches/lrucache.py b/synapse/util/caches/lrucache.py index 5635b674f054..4da0a74eb69c 100644 --- a/synapse/util/caches/lrucache.py +++ b/synapse/util/caches/lrucache.py @@ -21,7 +21,6 @@ Any, Callable, Collection, - Dict, Generic, Iterable, List, @@ -86,16 +85,6 @@ def _get_size_of(val: Any, *, recurse: bool = True) -> int: # a general type var, distinct from either KT or VT T = TypeVar("T") - -# TODO I think this is this unused? -def enumerate_leaves(node: Dict, depth: int) -> Any: - if depth == 0: - yield node - else: - for n in node.values(): - yield from enumerate_leaves(n, depth - 1) - - P = TypeVar("P") From fe9d0451086f212d34e7cc167650f981670cce0e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 27 Sep 2021 15:26:43 +0100 Subject: [PATCH 14/21] Revert `measure_func` changes --- synapse/util/metrics.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/synapse/util/metrics.py b/synapse/util/metrics.py index 4edb741878a4..7c1458c9aca9 100644 --- a/synapse/util/metrics.py +++ b/synapse/util/metrics.py @@ -15,7 +15,7 @@ import logging from functools import wraps from types import TracebackType -from typing import Any, Callable, Optional, Protocol, Type, TypeVar, cast, Awaitable +from typing import Any, Callable, Optional, Protocol, Type, TypeVar, cast from prometheus_client import Counter @@ -63,15 +63,14 @@ sub_metrics=["real_time_max", "real_time_sum"], ) -R = TypeVar("R") -F = Callable[..., Awaitable[R]] +T = TypeVar("T", bound=Callable[..., Any]) class HasClock(Protocol): clock: Clock -def measure_func(name: Optional[str] = None) -> Callable[[F], F]: +def measure_func(name: Optional[str] = None) -> Callable[[T], T]: """ Used to decorate an async function with a `Measure` context manager. @@ -89,16 +88,16 @@ async def foo(...): """ - def wrapper(func: F) -> F: + def wrapper(func: T) -> T: block_name = func.__name__ if name is None else name @wraps(func) - async def measured_func(self: HasClock, *args: Any, **kwargs: Any) -> R: + async def measured_func(self: HasClock, *args: Any, **kwargs: Any) -> Any: with Measure(self.clock, block_name): r = await func(self, *args, **kwargs) return r - return measured_func + return cast(T, measured_func) return wrapper From f0802b3eb3d7c2bf5184f23d37cc63695b33984e Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 27 Sep 2021 15:27:14 +0100 Subject: [PATCH 15/21] I think I missed some linting --- synapse/util/patch_inline_callbacks.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/util/patch_inline_callbacks.py b/synapse/util/patch_inline_callbacks.py index 78697ba339e3..9d2534a7e6d2 100644 --- a/synapse/util/patch_inline_callbacks.py +++ b/synapse/util/patch_inline_callbacks.py @@ -117,7 +117,7 @@ def check_ctx(r: T) -> T: def _check_yield_points( f: Callable[..., Generator[Deferred[object], object, T]], changes: List[str], -)-> Callable: +) -> Callable: """Wraps a generator that is about to be passed to defer.inlineCallbacks checking that after every yield the log contexts are correct. @@ -137,7 +137,9 @@ def _check_yield_points( from synapse.logging.context import current_context @functools.wraps(f) - def check_yield_points_inner(*args: Any, **kwargs: Any) -> Generator[Deferred[object], object, T]: + def check_yield_points_inner( + *args: Any, **kwargs: Any + ) -> Generator[Deferred[object], object, T]: gen = f(*args, **kwargs) last_yield_line_no = gen.gi_frame.f_lineno From ee5e6765e004e46711d580dd983e8f86c991ac0a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 27 Sep 2021 15:32:05 +0100 Subject: [PATCH 16/21] Let's not annotate DeferredCache.invalidate --- mypy.ini | 3 --- synapse/util/caches/deferred_cache.py | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/mypy.ini b/mypy.ini index 7d1c98e60a69..c1cbd7b30b5c 100644 --- a/mypy.ini +++ b/mypy.ini @@ -103,9 +103,6 @@ disallow_untyped_defs = True [mypy-synapse.util.caches.cached_call] disallow_untyped_defs = True -[mypy-synapse.util.caches.deferred_cache] -disallow_untyped_defs = True - [mypy-synapse.util.caches.dictionary_cache] disallow_untyped_defs = True diff --git a/synapse/util/caches/deferred_cache.py b/synapse/util/caches/deferred_cache.py index ffad0707190e..7f5b5128bb46 100644 --- a/synapse/util/caches/deferred_cache.py +++ b/synapse/util/caches/deferred_cache.py @@ -287,7 +287,7 @@ def prefill( callbacks = [callback] if callback else [] self.cache.set(key, value, callbacks=callbacks) - def invalidate(self, key: KT) -> None: + def invalidate(self, key) -> None: """Delete a key, or tree of entries If the cache is backed by a regular dict, then "key" must be of From 9ec04a485f44a191d8c6580f2703bdd424f2c6bf Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 27 Sep 2021 15:42:03 +0100 Subject: [PATCH 17/21] Restore exception raising in versionstring.py --- synapse/util/versionstring.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/synapse/util/versionstring.py b/synapse/util/versionstring.py index a67a38f68218..899ee0adc803 100644 --- a/synapse/util/versionstring.py +++ b/synapse/util/versionstring.py @@ -39,7 +39,9 @@ def get_version_string(module: ModuleType) -> str: if cached_version is not None: return cached_version - version_string = getattr(module, "__version__", "") + # We want this to fail loudly with an AttributeError. Type-ignore this so + # mypy only considers the happy path. + version_string = module.__version__ # type: ignore[attr-defined] try: null = open(os.devnull, "w") @@ -101,7 +103,12 @@ def get_version_string(module: ModuleType) -> str: s for s in (git_branch, git_tag, git_commit, git_dirty) if s ) - version_string = "%s (%s)" % (version_string, git_version) + version_string = "%s (%s)" % ( + # If the __version__ attribute doesn't exist, we'll have failed + # loudly above. + module.__version__, # type: ignore[attr-defined] + git_version, + ) except Exception as e: logger.info("Failed to check for git repository: %s", e) From 96b84816e8722a5ea300bc5c48c915e141abf167 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 27 Sep 2021 16:49:57 +0100 Subject: [PATCH 18/21] Protocol is in typing_extensions until 3.8 --- synapse/util/metrics.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/synapse/util/metrics.py b/synapse/util/metrics.py index 7c1458c9aca9..1e784b3f1f8d 100644 --- a/synapse/util/metrics.py +++ b/synapse/util/metrics.py @@ -15,9 +15,10 @@ import logging from functools import wraps from types import TracebackType -from typing import Any, Callable, Optional, Protocol, Type, TypeVar, cast +from typing import Any, Callable, Optional, Type, TypeVar, cast from prometheus_client import Counter +from typing_extensions import Protocol from synapse.logging.context import ( ContextResourceUsage, From 723c4316c6847f7e6ca42a36dc1b377655fc0d47 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 29 Sep 2021 12:54:43 +0100 Subject: [PATCH 19/21] Workaround attrs + slots + generic problem --- synapse/util/caches/ttlcache.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/util/caches/ttlcache.py b/synapse/util/caches/ttlcache.py index f7f57b7f7320..de3d73688cd5 100644 --- a/synapse/util/caches/ttlcache.py +++ b/synapse/util/caches/ttlcache.py @@ -159,9 +159,11 @@ def expire(self) -> None: del self._expiry_list[0] -@attr.s(frozen=True, slots=True, auto_attribs=True) +# Manual __slots__ due to https://github.com/python-attrs/attrs/issues/313 +@attr.s(frozen=True, auto_attribs=True) class _CacheEntry(Generic[KT, VT]): """TTLCache entry""" + __slots__ = ["expiry_time", "ttl", "key", "value"] # expiry_time is the first attribute, so that entries are sorted by expiry. expiry_time: float From cf923f6112fc479c18e49604d5258de581faf102 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 29 Sep 2021 12:56:04 +0100 Subject: [PATCH 20/21] Use string literals `"Deferred[T]"` in annotations Seems to be required for the "olddeps" step (which runs on Ubuntu bionic) --- synapse/util/caches/response_cache.py | 4 ++-- synapse/util/patch_inline_callbacks.py | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/synapse/util/caches/response_cache.py b/synapse/util/caches/response_cache.py index 312491dd52c8..88ccf443377c 100644 --- a/synapse/util/caches/response_cache.py +++ b/synapse/util/caches/response_cache.py @@ -104,8 +104,8 @@ def get(self, key: KV) -> Optional[defer.Deferred]: return None def _set( - self, context: ResponseCacheContext[KV], deferred: defer.Deferred[RV] - ) -> defer.Deferred[RV]: + self, context: ResponseCacheContext[KV], deferred: "defer.Deferred[RV]" + ) -> "defer.Deferred[RV]": """Set the entry for the given key to the given deferred. *deferred* should run its callbacks in the sentinel logcontext (ie, diff --git a/synapse/util/patch_inline_callbacks.py b/synapse/util/patch_inline_callbacks.py index 9d2534a7e6d2..1f18654d47e7 100644 --- a/synapse/util/patch_inline_callbacks.py +++ b/synapse/util/patch_inline_callbacks.py @@ -41,18 +41,18 @@ def do_patch() -> None: return def new_inline_callbacks( - f: Callable[..., Generator[Deferred[object], object, T]] - ) -> Callable[..., Deferred[T]]: + f: Callable[..., Generator["Deferred[object]", object, T]] + ) -> Callable[..., "Deferred[T]"]: @functools.wraps(f) - def wrapped(*args: Any, **kwargs: Any) -> Deferred[T]: + def wrapped(*args: Any, **kwargs: Any) -> "Deferred[T]": start_context = current_context() changes: List[str] = [] - orig: Callable[..., Deferred[T]] = orig_inline_callbacks( + orig: Callable[..., "Deferred[T]"] = orig_inline_callbacks( _check_yield_points(f, changes) ) try: - res: Deferred[T] = orig(*args, **kwargs) + res: "Deferred[T]" = orig(*args, **kwargs) except Exception: if current_context() != start_context: for err in changes: @@ -115,7 +115,7 @@ def check_ctx(r: T) -> T: def _check_yield_points( - f: Callable[..., Generator[Deferred[object], object, T]], + f: Callable[..., Generator["Deferred[object]", object, T]], changes: List[str], ) -> Callable: """Wraps a generator that is about to be passed to defer.inlineCallbacks @@ -139,7 +139,7 @@ def _check_yield_points( @functools.wraps(f) def check_yield_points_inner( *args: Any, **kwargs: Any - ) -> Generator[Deferred[object], object, T]: + ) -> Generator["Deferred[object]", object, T]: gen = f(*args, **kwargs) last_yield_line_no = gen.gi_frame.f_lineno From 59b524bc3028eb6d80368c6b095c06f9b1e3b2de Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 29 Sep 2021 13:17:13 +0100 Subject: [PATCH 21/21] Workaround didn't work, sod it --- synapse/util/caches/ttlcache.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/synapse/util/caches/ttlcache.py b/synapse/util/caches/ttlcache.py index de3d73688cd5..0b9ac26b6949 100644 --- a/synapse/util/caches/ttlcache.py +++ b/synapse/util/caches/ttlcache.py @@ -159,14 +159,12 @@ def expire(self) -> None: del self._expiry_list[0] -# Manual __slots__ due to https://github.com/python-attrs/attrs/issues/313 -@attr.s(frozen=True, auto_attribs=True) -class _CacheEntry(Generic[KT, VT]): +@attr.s(frozen=True, slots=True, auto_attribs=True) +class _CacheEntry: # Should be Generic[KT, VT]. See python-attrs/attrs#313 """TTLCache entry""" - __slots__ = ["expiry_time", "ttl", "key", "value"] # expiry_time is the first attribute, so that entries are sorted by expiry. expiry_time: float ttl: float - key: KT - value: VT + key: Any # should be KT + value: Any # should be VT