From be40e7f5ab7b7ffce89e4862fcb4f6b6a36ba8e5 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Sep 2020 11:47:53 +0100 Subject: [PATCH 1/7] Fixup typing in cache descriptors --- mypy.ini | 1 + synapse/util/caches/descriptors.py | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/mypy.ini b/mypy.ini index ae3290d5bbf1..7cbc61cf14eb 100644 --- a/mypy.ini +++ b/mypy.ini @@ -51,6 +51,7 @@ files = synapse/storage/util, synapse/streams, synapse/types.py, + synapse/util/caches/descriptors.py, synapse/util/caches/stream_change_cache.py, synapse/util/metrics.py, tests/replication, diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 49d9fddcf057..28bcec2fdb3b 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -18,7 +18,7 @@ import inspect import logging import threading -from typing import Any, Tuple, Union, cast +from typing import Any, Optional, Tuple, Union, cast from weakref import WeakValueDictionary from prometheus_client import Gauge @@ -47,7 +47,9 @@ class _CachedFunction(Protocol): cache = None # type: Any num_args = None # type: Any - def __name__(self): + __name__ = None # type: str + + def __call__(self, *args, **kwargs): ... @@ -123,7 +125,7 @@ def __init__( self.name = name self.keylen = keylen - self.thread = None + self.thread = None # type: Optional[threading.Thread] self.metrics = register_cache( "cache", name, From e15f6f6bb05a56a2bac057b39dc1f967f192a940 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Sep 2020 12:13:01 +0100 Subject: [PATCH 2/7] Pass return types through cached descriptors --- synapse/handlers/federation.py | 10 +++++----- synapse/util/caches/descriptors.py | 23 +++++++++++++++-------- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py index bd8efbb768db..310c7f7138c9 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -440,11 +440,11 @@ async def _get_missing_events_for_pdu(self, origin, pdu, prevs, min_depth): if not prevs - seen: return - latest = await self.store.get_latest_event_ids_in_room(room_id) + latest_list = await self.store.get_latest_event_ids_in_room(room_id) # We add the prev events that we have seen to the latest # list to ensure the remote server doesn't give them to us - latest = set(latest) + latest = set(latest_list) latest |= seen logger.info( @@ -781,7 +781,7 @@ async def _process_received_pdu( # keys across all devices. current_keys = [ key - for device in cached_devices + for device in cached_devices.values() for key in device.get("keys", {}).get("keys", {}).values() ] @@ -2119,8 +2119,8 @@ async def _check_for_soft_fail( if backfilled or event.internal_metadata.is_outlier(): return - extrem_ids = await self.store.get_latest_event_ids_in_room(event.room_id) - extrem_ids = set(extrem_ids) + extrem_ids_list = await self.store.get_latest_event_ids_in_room(event.room_id) + extrem_ids = set(extrem_ids_list) prev_event_ids = set(event.prev_event_ids()) if extrem_ids == prev_event_ids: diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 28bcec2fdb3b..37ced221fb6b 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -18,11 +18,10 @@ import inspect import logging import threading -from typing import Any, Optional, Tuple, Union, cast +from typing import Any, Callable, Generic, Optional, Tuple, TypeVar, Union, cast from weakref import WeakValueDictionary from prometheus_client import Gauge -from typing_extensions import Protocol from twisted.internet import defer @@ -38,8 +37,10 @@ CacheKey = Union[Tuple, Any] +R = TypeVar("R") -class _CachedFunction(Protocol): + +class _CachedFunction(Generic[R]): invalidate = None # type: Any invalidate_all = None # type: Any invalidate_many = None # type: Any @@ -49,7 +50,7 @@ class _CachedFunction(Protocol): __name__ = None # type: str - def __call__(self, *args, **kwargs): + def __call__(self, *args, **kwargs) -> R: ... @@ -665,8 +666,8 @@ def get_instance(cls, cache, cache_key): # type: (Cache, CacheKey) -> _CacheCon def cached( max_entries=1000, num_args=None, tree=False, cache_context=False, iterable=False -): - return lambda orig: CacheDescriptor( +) -> Callable[[Callable[..., R]], _CachedFunction[R]]: + func = lambda orig: CacheDescriptor( orig, max_entries=max_entries, num_args=num_args, @@ -675,8 +676,12 @@ def cached( iterable=iterable, ) + return cast(Callable[[Callable[..., R]], _CachedFunction[R]], func) + -def cachedList(cached_method_name, list_name, num_args=None): +def cachedList( + cached_method_name, list_name, num_args=None +) -> Callable[[Callable[..., R]], _CachedFunction[R]]: """Creates a descriptor that wraps a function in a `CacheListDescriptor`. Used to do batch lookups for an already created cache. A single argument @@ -704,9 +709,11 @@ def do_something(self, first_arg): def batch_do_something(self, first_arg, second_args): ... """ - return lambda orig: CacheListDescriptor( + func = lambda orig: CacheListDescriptor( orig, cached_method_name=cached_method_name, list_name=list_name, num_args=num_args, ) + + return cast(Callable[[Callable[..., R]], _CachedFunction[R]], func) From f999af4b96d505f7ef4db68d03ea17326781a966 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Sep 2020 12:54:35 +0100 Subject: [PATCH 3/7] Add mypy plugin to correctly configure types for @cached --- mypy.ini | 2 +- scripts-dev/mypy_synapse_plugin.py | 76 ++++++++++++++++++++++++++++++ synapse/util/caches/descriptors.py | 17 +++---- 3 files changed, 86 insertions(+), 9 deletions(-) create mode 100644 scripts-dev/mypy_synapse_plugin.py diff --git a/mypy.ini b/mypy.ini index 7cbc61cf14eb..8a351eabfebb 100644 --- a/mypy.ini +++ b/mypy.ini @@ -1,6 +1,6 @@ [mypy] namespace_packages = True -plugins = mypy_zope:plugin +plugins = mypy_zope:plugin, scripts-dev/mypy_synapse_plugin.py follow_imports = silent check_untyped_defs = True show_error_codes = True diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py new file mode 100644 index 000000000000..545673dec8d2 --- /dev/null +++ b/scripts-dev/mypy_synapse_plugin.py @@ -0,0 +1,76 @@ +# -*- coding: utf-8 -*- +# Copyright 2020 The Matrix.org Foundation C.I.C. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from typing import Callable, Optional + +from mypy.plugin import MethodSigContext, Plugin +from mypy.typeops import bind_self +from mypy.types import CallableType + + +class SynapsePlugin(Plugin): + def get_method_signature_hook( + self, fullname: str + ) -> Optional[Callable[[MethodSigContext], CallableType]]: + if fullname.startswith( + "synapse.util.caches.descriptors._CachedFunction.__call__" + ): + return cached_function_method_signature + return None + + +def cached_function_method_signature(ctx: MethodSigContext) -> CallableType: + """Fixes the `_CachedFunction.__call__` signature to be correct. + + It already has *almost* the correct signature, except: + + 1. the `self` argument needs to be marked as "bound"; and + 2. any `cache_context` argument should be removed. + """ + + # First we mark this as a bound function signature. + signature = bind_self(ctx.default_signature) + + # Secondly, we remove any "cache_context" args. + # + # Note: We should be only doing this if `cache_context=True` is set, but if + # it isn't then the code will raise an exception when its called anyway, so + # its not the end of the world. + context_arg_index = None + for idx, name in enumerate(signature.arg_names): + if name == "cache_context": + context_arg_index = idx + break + + if context_arg_index: + arg_types = list(signature.arg_types) + arg_types.pop(context_arg_index) + + arg_names = list(signature.arg_names) + arg_names.pop(context_arg_index) + + arg_kinds = list(signature.arg_kinds) + arg_kinds.pop(context_arg_index) + + signature = signature.copy_modified( + arg_types=arg_types, arg_names=arg_names, arg_kinds=arg_kinds, + ) + + return signature + + +def plugin(version: str): + # ignore version argument if the plugin works with all mypy versions. + return SynapsePlugin diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 37ced221fb6b..8d214a13fb67 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -37,10 +37,10 @@ CacheKey = Union[Tuple, Any] -R = TypeVar("R") +F = TypeVar("F", bound=Callable[..., Any]) -class _CachedFunction(Generic[R]): +class _CachedFunction(Generic[F]): invalidate = None # type: Any invalidate_all = None # type: Any invalidate_many = None # type: Any @@ -50,8 +50,9 @@ class _CachedFunction(Generic[R]): __name__ = None # type: str - def __call__(self, *args, **kwargs) -> R: - ... + # Note: This function signature is actually fiddled with by the synapse mypy + # plugin to a) make it a bound methd, and b) remove any `cache_context` arg. + __call__ = None # type: F cache_pending_metric = Gauge( @@ -666,7 +667,7 @@ def get_instance(cls, cache, cache_key): # type: (Cache, CacheKey) -> _CacheCon def cached( max_entries=1000, num_args=None, tree=False, cache_context=False, iterable=False -) -> Callable[[Callable[..., R]], _CachedFunction[R]]: +) -> Callable[[F], _CachedFunction[F]]: func = lambda orig: CacheDescriptor( orig, max_entries=max_entries, @@ -676,12 +677,12 @@ def cached( iterable=iterable, ) - return cast(Callable[[Callable[..., R]], _CachedFunction[R]], func) + return cast(Callable[[F], _CachedFunction[F]], func) def cachedList( cached_method_name, list_name, num_args=None -) -> Callable[[Callable[..., R]], _CachedFunction[R]]: +) -> Callable[[F], _CachedFunction[F]]: """Creates a descriptor that wraps a function in a `CacheListDescriptor`. Used to do batch lookups for an already created cache. A single argument @@ -716,4 +717,4 @@ def batch_do_something(self, first_arg, second_args): num_args=num_args, ) - return cast(Callable[[Callable[..., R]], _CachedFunction[R]], func) + return cast(Callable[[F], _CachedFunction[F]], func) From 00bca5f0d53df267929fbcf1750c1ed9aefa793c Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Sep 2020 12:57:58 +0100 Subject: [PATCH 4/7] Newsfile --- changelog.d/8240.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8240.misc diff --git a/changelog.d/8240.misc b/changelog.d/8240.misc new file mode 100644 index 000000000000..acfbd89e2402 --- /dev/null +++ b/changelog.d/8240.misc @@ -0,0 +1 @@ +Fix type hints for functions decorated with `@cached`. From 4a1bacced6052fed3184ea542375e60edd2aeab6 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Sep 2020 13:43:39 +0100 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: Patrick Cloke --- synapse/util/caches/descriptors.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 8d214a13fb67..9f8602901554 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -51,7 +51,7 @@ class _CachedFunction(Generic[F]): __name__ = None # type: str # Note: This function signature is actually fiddled with by the synapse mypy - # plugin to a) make it a bound methd, and b) remove any `cache_context` arg. + # plugin to a) make it a bound method, and b) remove any `cache_context` arg. __call__ = None # type: F From 4714ecb22c381d477c27ffa83377feb71ff4d312 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Sep 2020 13:47:10 +0100 Subject: [PATCH 6/7] Add type hints to decorator args --- synapse/util/caches/descriptors.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/synapse/util/caches/descriptors.py b/synapse/util/caches/descriptors.py index 9f8602901554..825810eb166c 100644 --- a/synapse/util/caches/descriptors.py +++ b/synapse/util/caches/descriptors.py @@ -666,7 +666,11 @@ def get_instance(cls, cache, cache_key): # type: (Cache, CacheKey) -> _CacheCon def cached( - max_entries=1000, num_args=None, tree=False, cache_context=False, iterable=False + max_entries: int = 1000, + num_args: Optional[int] = None, + tree: bool = False, + cache_context: bool = False, + iterable: bool = False, ) -> Callable[[F], _CachedFunction[F]]: func = lambda orig: CacheDescriptor( orig, @@ -681,7 +685,7 @@ def cached( def cachedList( - cached_method_name, list_name, num_args=None + cached_method_name: str, list_name: str, num_args: Optional[int] = None ) -> Callable[[F], _CachedFunction[F]]: """Creates a descriptor that wraps a function in a `CacheListDescriptor`. @@ -692,11 +696,11 @@ def cachedList( cache. Args: - cached_method_name (str): The name of the single-item lookup method. + cached_method_name: The name of the single-item lookup method. This is only used to find the cache to use. - list_name (str): The name of the argument that is the list to use to + list_name: The name of the argument that is the list to use to do batch lookups in the cache. - num_args (int): Number of arguments to use as the key in the cache + num_args: Number of arguments to use as the key in the cache (including list_name). Defaults to all named parameters. Example: From a7d3acbc8038522e7910ecbb16f7aac52447c07a Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 3 Sep 2020 13:49:00 +0100 Subject: [PATCH 7/7] Add some comments to plugin. --- scripts-dev/mypy_synapse_plugin.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/scripts-dev/mypy_synapse_plugin.py b/scripts-dev/mypy_synapse_plugin.py index 545673dec8d2..a5b88731f172 100644 --- a/scripts-dev/mypy_synapse_plugin.py +++ b/scripts-dev/mypy_synapse_plugin.py @@ -13,6 +13,10 @@ # See the License for the specific language governing permissions and # limitations under the License. +"""This is a mypy plugin for Synpase to deal with some of the funky typing that +can crop up, e.g the cache descriptors. +""" + from typing import Callable, Optional from mypy.plugin import MethodSigContext, Plugin @@ -72,5 +76,10 @@ def cached_function_method_signature(ctx: MethodSigContext) -> CallableType: def plugin(version: str): - # ignore version argument if the plugin works with all mypy versions. + # This is the entry point of the plugin, and let's us deal with the fact + # that the mypy plugin interface is *not* stable by looking at the version + # string. + # + # However, since we pin the version of mypy Synapse uses in CI, we don't + # really care. return SynapsePlugin