From 56ac1aaa0ffc6f4502ede9091f15feeb3612ca34 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 9 Feb 2023 00:29:55 +0000
Subject: [PATCH 1/7] -> None for test methods

---
 tests/util/caches/test_descriptors.py | 58 +++++++++++++--------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/tests/util/caches/test_descriptors.py b/tests/util/caches/test_descriptors.py
index 13f1edd5332c..4197d1d9a3de 100644
--- a/tests/util/caches/test_descriptors.py
+++ b/tests/util/caches/test_descriptors.py
@@ -45,7 +45,7 @@ def run_on_reactor():
 
 class DescriptorTestCase(unittest.TestCase):
     @defer.inlineCallbacks
-    def test_cache(self):
+    def test_cache(self) -> None:
         class Cls:
             def __init__(self):
                 self.mock = mock.Mock()
@@ -77,7 +77,7 @@ def fn(self, arg1, arg2):
         obj.mock.assert_not_called()
 
     @defer.inlineCallbacks
-    def test_cache_num_args(self):
+    def test_cache_num_args(self) -> None:
         """Only the first num_args arguments should matter to the cache"""
 
         class Cls:
@@ -111,7 +111,7 @@ def fn(self, arg1, arg2):
         obj.mock.assert_not_called()
 
     @defer.inlineCallbacks
-    def test_cache_uncached_args(self):
+    def test_cache_uncached_args(self) -> None:
         """
         Only the arguments not named in uncached_args should matter to the cache
 
@@ -152,7 +152,7 @@ def __init__(self):
         obj.mock.assert_not_called()
 
     @defer.inlineCallbacks
-    def test_cache_kwargs(self):
+    def test_cache_kwargs(self) -> None:
         """Test that keyword arguments are treated properly"""
 
         class Cls:
@@ -188,7 +188,7 @@ def fn(self, arg1, kwarg1=2):
         self.assertEqual(r, "fish")
         obj.mock.assert_not_called()
 
-    def test_cache_with_sync_exception(self):
+    def test_cache_with_sync_exception(self) -> None:
         """If the wrapped function throws synchronously, things should continue to work"""
 
         class Cls:
@@ -209,7 +209,7 @@ def fn(self, arg1):
         d = obj.fn(1)
         self.failureResultOf(d, SynapseError)
 
-    def test_cache_with_async_exception(self):
+    def test_cache_with_async_exception(self) -> None:
         """The wrapped function returns a failure"""
 
         class Cls:
@@ -260,7 +260,7 @@ def fn(self, arg1):
         self.assertEqual(self.successResultOf(d3), 100)
         self.assertEqual(obj.call_count, 2)
 
-    def test_cache_logcontexts(self):
+    def test_cache_logcontexts(self) -> None:
         """Check that logcontexts are set and restored correctly when
         using the cache."""
 
@@ -304,7 +304,7 @@ def check_result(r):
 
         return defer.gatherResults([d1, d2])
 
-    def test_cache_logcontexts_with_exception(self):
+    def test_cache_logcontexts_with_exception(self) -> None:
         """Check that the cache sets and restores logcontexts correctly when
         the lookup function throws an exception"""
 
@@ -347,7 +347,7 @@ def do_lookup():
         return d1
 
     @defer.inlineCallbacks
-    def test_cache_default_args(self):
+    def test_cache_default_args(self) -> None:
         class Cls:
             def __init__(self):
                 self.mock = mock.Mock()
@@ -384,7 +384,7 @@ def fn(self, arg1, arg2=2, arg3=3):
         self.assertEqual(r, "chips")
         obj.mock.assert_not_called()
 
-    def test_cache_iterable(self):
+    def test_cache_iterable(self) -> None:
         class Cls:
             def __init__(self):
                 self.mock = mock.Mock()
@@ -417,7 +417,7 @@ def fn(self, arg1, arg2):
         self.assertEqual(r.result, ["chips"])
         obj.mock.assert_not_called()
 
-    def test_cache_iterable_with_sync_exception(self):
+    def test_cache_iterable_with_sync_exception(self) -> None:
         """If the wrapped function throws synchronously, things should continue to work"""
 
         class Cls:
@@ -438,7 +438,7 @@ def fn(self, arg1):
         d = obj.fn(1)
         self.failureResultOf(d, SynapseError)
 
-    def test_invalidate_cascade(self):
+    def test_invalidate_cascade(self) -> None:
         """Invalidations should cascade up through cache contexts"""
 
         class Cls:
@@ -463,7 +463,7 @@ async def func3(self, key, cache_context):
         obj.invalidate()
         top_invalidate.assert_called_once()
 
-    def test_cancel(self):
+    def test_cancel(self) -> None:
         """Test that cancelling a lookup does not cancel other lookups"""
         complete_lookup: "Deferred[None]" = Deferred()
 
@@ -488,7 +488,7 @@ async def fn(self, arg1):
         self.failureResultOf(d1, CancelledError)
         self.assertEqual(d2.result, "123")
 
-    def test_cancel_logcontexts(self):
+    def test_cancel_logcontexts(self) -> None:
         """Test that cancellation does not break logcontexts.
 
         * The `CancelledError` must be raised with the correct logcontext.
@@ -542,7 +542,7 @@ class CacheDecoratorTestCase(unittest.HomeserverTestCase):
     """
 
     @defer.inlineCallbacks
-    def test_passthrough(self):
+    def test_passthrough(self) -> None:
         class A:
             @cached()
             def func(self, key):
@@ -554,7 +554,7 @@ def func(self, key):
         self.assertEqual((yield a.func("bar")), "bar")
 
     @defer.inlineCallbacks
-    def test_hit(self):
+    def test_hit(self) -> None:
         callcount = [0]
 
         class A:
@@ -572,7 +572,7 @@ def func(self, key):
         self.assertEqual(callcount[0], 1)
 
     @defer.inlineCallbacks
-    def test_invalidate(self):
+    def test_invalidate(self) -> None:
         callcount = [0]
 
         class A:
@@ -592,7 +592,7 @@ def func(self, key):
 
         self.assertEqual(callcount[0], 2)
 
-    def test_invalidate_missing(self):
+    def test_invalidate_missing(self) -> None:
         class A:
             @cached()
             def func(self, key):
@@ -601,7 +601,7 @@ def func(self, key):
         A().func.invalidate(("what",))
 
     @defer.inlineCallbacks
-    def test_max_entries(self):
+    def test_max_entries(self) -> None:
         callcount = [0]
 
         class A:
@@ -626,7 +626,7 @@ def func(self, key):
             callcount[0] >= 14, msg="Expected callcount >= 14, got %d" % (callcount[0])
         )
 
-    def test_prefill(self):
+    def test_prefill(self) -> None:
         callcount = [0]
 
         d = defer.succeed(123)
@@ -645,7 +645,7 @@ def func(self, key):
         self.assertEqual(callcount[0], 0)
 
     @defer.inlineCallbacks
-    def test_invalidate_context(self):
+    def test_invalidate_context(self) -> None:
         callcount = [0]
         callcount2 = [0]
 
@@ -678,7 +678,7 @@ def func2(self, key, cache_context):
         self.assertEqual(callcount2[0], 2)
 
     @defer.inlineCallbacks
-    def test_eviction_context(self):
+    def test_eviction_context(self) -> None:
         callcount = [0]
         callcount2 = [0]
 
@@ -715,7 +715,7 @@ def func2(self, key, cache_context):
         self.assertEqual(callcount2[0], 3)
 
     @defer.inlineCallbacks
-    def test_double_get(self):
+    def test_double_get(self) -> None:
         callcount = [0]
         callcount2 = [0]
 
@@ -763,7 +763,7 @@ def func2(self, key, cache_context):
 
 class CachedListDescriptorTestCase(unittest.TestCase):
     @defer.inlineCallbacks
-    def test_cache(self):
+    def test_cache(self) -> None:
         class Cls:
             def __init__(self):
                 self.mock = mock.Mock()
@@ -824,7 +824,7 @@ async def list_fn(self, args1, arg2):
             obj.mock.assert_called_once_with({40}, 2)
             self.assertEqual(r, {10: "fish", 40: "gravy"})
 
-    def test_concurrent_lookups(self):
+    def test_concurrent_lookups(self) -> None:
         """All concurrent lookups should get the same result"""
 
         class Cls:
@@ -867,7 +867,7 @@ def list_fn(self, args1) -> "Deferred[dict]":
         self.assertEqual(self.successResultOf(d3), {10: "peas"})
 
     @defer.inlineCallbacks
-    def test_invalidate(self):
+    def test_invalidate(self) -> None:
         """Make sure that invalidation callbacks are called."""
 
         class Cls:
@@ -908,7 +908,7 @@ async def list_fn(self, args1, arg2):
         invalidate0.assert_called_once()
         invalidate1.assert_called_once()
 
-    def test_cancel(self):
+    def test_cancel(self) -> None:
         """Test that cancelling a lookup does not cancel other lookups"""
         complete_lookup: "Deferred[None]" = Deferred()
 
@@ -936,7 +936,7 @@ async def list_fn(self, args):
         self.failureResultOf(d1, CancelledError)
         self.assertEqual(d2.result, {123: "123", 456: "456", 789: "789"})
 
-    def test_cancel_logcontexts(self):
+    def test_cancel_logcontexts(self) -> None:
         """Test that cancellation does not break logcontexts.
 
         * The `CancelledError` must be raised with the correct logcontext.
@@ -983,7 +983,7 @@ async def do_lookup():
         )
         self.assertEqual(current_context(), SENTINEL_CONTEXT)
 
-    def test_num_args_mismatch(self):
+    def test_num_args_mismatch(self) -> None:
         """
         Make sure someone does not accidentally use @cachedList on a method with
         a mismatch in the number args to the underlying single cache method.

From 35258e0a1dd104ca891ef25efded31844c7ab2c4 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 9 Feb 2023 00:39:47 +0000
Subject: [PATCH 2/7] Oh, some of these are inline callbacks jobbies

---
 tests/util/caches/test_descriptors.py | 32 +++++++++++++++------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/tests/util/caches/test_descriptors.py b/tests/util/caches/test_descriptors.py
index 4197d1d9a3de..3fa62ca7fdcf 100644
--- a/tests/util/caches/test_descriptors.py
+++ b/tests/util/caches/test_descriptors.py
@@ -13,7 +13,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import Iterable, Set, Tuple, cast
+from typing import Any, Generator, Iterable, Set, Tuple, cast
 from unittest import mock
 
 from twisted.internet import defer, reactor
@@ -37,7 +37,7 @@
 logger = logging.getLogger(__name__)
 
 
-def run_on_reactor():
+def run_on_reactor() -> "Deferred[int]":
     d: "Deferred[int]" = defer.Deferred()
     cast(IReactorTime, reactor).callLater(0, d.callback, 0)
     return make_deferred_yieldable(d)
@@ -45,18 +45,19 @@ def run_on_reactor():
 
 class DescriptorTestCase(unittest.TestCase):
     @defer.inlineCallbacks
-    def test_cache(self) -> None:
+    def test_cache(self) -> Generator["Deferred[Any]", Any, None]:
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached()
-            def fn(self, arg1, arg2):
+            def fn(self, arg1: int, arg2: int) -> mock.Mock:
                 return self.mock(arg1, arg2)
 
         obj = Cls()
 
         obj.mock.return_value = "fish"
+        r: mock.Mock
         r = yield obj.fn(1, 2)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, 2)
@@ -77,19 +78,20 @@ def fn(self, arg1, arg2):
         obj.mock.assert_not_called()
 
     @defer.inlineCallbacks
-    def test_cache_num_args(self) -> None:
+    def test_cache_num_args(self) -> Generator["Deferred[Any]", Any, None]:
         """Only the first num_args arguments should matter to the cache"""
 
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached(num_args=1)
-            def fn(self, arg1, arg2):
+            def fn(self, arg1: int, arg2: int) -> mock.Mock:
                 return self.mock(arg1, arg2)
 
         obj = Cls()
         obj.mock.return_value = "fish"
+        r: mock.Mock
         r = yield obj.fn(1, 2)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, 2)
@@ -111,7 +113,7 @@ def fn(self, arg1, arg2):
         obj.mock.assert_not_called()
 
     @defer.inlineCallbacks
-    def test_cache_uncached_args(self) -> None:
+    def test_cache_uncached_args(self) -> Generator["Deferred[Any]", Any, None]:
         """
         Only the arguments not named in uncached_args should matter to the cache
 
@@ -123,14 +125,15 @@ class Cls:
             # Note that it is important that this is not the last argument to
             # test behaviour of skipping arguments properly.
             @descriptors.cached(uncached_args=("arg2",))
-            def fn(self, arg1, arg2, arg3):
+            def fn(self, arg1: int, arg2: int, arg3: int) -> mock.Mock:
                 return self.mock(arg1, arg2, arg3)
 
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
         obj = Cls()
         obj.mock.return_value = "fish"
+        r: mock.Mock
         r = yield obj.fn(1, 2, 3)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, 2, 3)
@@ -152,19 +155,20 @@ def __init__(self):
         obj.mock.assert_not_called()
 
     @defer.inlineCallbacks
-    def test_cache_kwargs(self) -> None:
+    def test_cache_kwargs(self) -> Generator["Deferred[Any]", Any, None]:
         """Test that keyword arguments are treated properly"""
 
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached()
-            def fn(self, arg1, kwarg1=2):
+            def fn(self, arg1: int, kwarg1: int=2) -> mock.Mock:
                 return self.mock(arg1, kwarg1=kwarg1)
 
         obj = Cls()
         obj.mock.return_value = "fish"
+        r: mock.Mock
         r = yield obj.fn(1, kwarg1=2)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, kwarg1=2)

From dd92bedbcf0dc7c7e84c45b77768467ba3bd567c Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 9 Feb 2023 02:11:46 +0000
Subject: [PATCH 3/7] Everything else, sorry

---
 tests/util/caches/test_descriptors.py | 243 ++++++++++++++------------
 1 file changed, 136 insertions(+), 107 deletions(-)

diff --git a/tests/util/caches/test_descriptors.py b/tests/util/caches/test_descriptors.py
index 3fa62ca7fdcf..a2275d165a68 100644
--- a/tests/util/caches/test_descriptors.py
+++ b/tests/util/caches/test_descriptors.py
@@ -13,7 +13,18 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 import logging
-from typing import Any, Generator, Iterable, Set, Tuple, cast
+from typing import (
+    Any,
+    Dict,
+    Generator,
+    Iterable,
+    List,
+    Optional,
+    Sequence,
+    Set,
+    Tuple,
+    cast,
+)
 from unittest import mock
 
 from twisted.internet import defer, reactor
@@ -29,7 +40,7 @@
     make_deferred_yieldable,
 )
 from synapse.util.caches import descriptors
-from synapse.util.caches.descriptors import cached, cachedList
+from synapse.util.caches.descriptors import _CacheContext, cached, cachedList
 
 from tests import unittest
 from tests.test_utils import get_awaitable_result
@@ -57,7 +68,6 @@ def fn(self, arg1: int, arg2: int) -> mock.Mock:
         obj = Cls()
 
         obj.mock.return_value = "fish"
-        r: mock.Mock
         r = yield obj.fn(1, 2)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, 2)
@@ -91,7 +101,6 @@ def fn(self, arg1: int, arg2: int) -> mock.Mock:
 
         obj = Cls()
         obj.mock.return_value = "fish"
-        r: mock.Mock
         r = yield obj.fn(1, 2)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, 2)
@@ -133,7 +142,6 @@ def __init__(self) -> None:
 
         obj = Cls()
         obj.mock.return_value = "fish"
-        r: mock.Mock
         r = yield obj.fn(1, 2, 3)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, 2, 3)
@@ -163,12 +171,11 @@ def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached()
-            def fn(self, arg1: int, kwarg1: int=2) -> mock.Mock:
+            def fn(self, arg1: int, kwarg1: int = 2) -> mock.Mock:
                 return self.mock(arg1, kwarg1=kwarg1)
 
         obj = Cls()
         obj.mock.return_value = "fish"
-        r: mock.Mock
         r = yield obj.fn(1, kwarg1=2)
         self.assertEqual(r, "fish")
         obj.mock.assert_called_once_with(1, kwarg1=2)
@@ -197,39 +204,42 @@ def test_cache_with_sync_exception(self) -> None:
 
         class Cls:
             @cached()
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> None:
                 raise SynapseError(100, "mai spoon iz too big!!1")
 
         obj = Cls()
 
         # this should fail immediately
-        d = obj.fn(1)
+        # type-ignore: mypy thinks the RHS is None; we need to fix the annotation for
+        # @cached here.
+        d: "Deferred[None]" = obj.fn(1)  # type: ignore[assignment]
         self.failureResultOf(d, SynapseError)
 
         # ... leaving the cache empty
         self.assertEqual(len(obj.fn.cache.cache), 0)
 
         # and a second call should result in a second exception
-        d = obj.fn(1)
+        d = obj.fn(1)  # type: ignore[assignment]
         self.failureResultOf(d, SynapseError)
 
     def test_cache_with_async_exception(self) -> None:
         """The wrapped function returns a failure"""
 
         class Cls:
-            result = None
+            result: "Optional[Deferred[int]]" = None
             call_count = 0
 
             @cached()
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> "Deferred[int]":
                 self.call_count += 1
+                assert self.result is not None
                 return self.result
 
         obj = Cls()
         callbacks: Set[str] = set()
 
         # set off an asynchronous request
-        origin_d: Deferred = defer.Deferred()
+        origin_d: Deferred[int] = defer.Deferred()
         obj.result = origin_d
 
         d1 = obj.fn(1, on_invalidate=lambda: callbacks.add("d1"))
@@ -264,17 +274,17 @@ def fn(self, arg1):
         self.assertEqual(self.successResultOf(d3), 100)
         self.assertEqual(obj.call_count, 2)
 
-    def test_cache_logcontexts(self) -> None:
+    def test_cache_logcontexts(self) -> "Deferred[List[int]]":
         """Check that logcontexts are set and restored correctly when
         using the cache."""
 
-        complete_lookup: Deferred = defer.Deferred()
+        complete_lookup: "Deferred[None]" = defer.Deferred()
 
         class Cls:
             @descriptors.cached()
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> Deferred[int]:
                 @defer.inlineCallbacks
-                def inner_fn():
+                def inner_fn() -> Generator["Deferred[Any]", Any, int]:
                     with PreserveLoggingContext():
                         yield complete_lookup
                     return 1
@@ -282,13 +292,14 @@ def inner_fn():
                 return inner_fn()
 
         @defer.inlineCallbacks
-        def do_lookup():
+        def do_lookup() -> Generator["Deferred[Any]", Any, int]:
             with LoggingContext("c1") as c1:
+                r: int
                 r = yield obj.fn(1)
                 self.assertEqual(current_context(), c1)
             return r
 
-        def check_result(r):
+        def check_result(r: Any) -> None:
             self.assertEqual(r, 1)
 
         obj = Cls()
@@ -308,15 +319,15 @@ def check_result(r):
 
         return defer.gatherResults([d1, d2])
 
-    def test_cache_logcontexts_with_exception(self) -> None:
+    def test_cache_logcontexts_with_exception(self) -> "Deferred[None]":
         """Check that the cache sets and restores logcontexts correctly when
         the lookup function throws an exception"""
 
         class Cls:
             @descriptors.cached()
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> "Deferred[None]":
                 @defer.inlineCallbacks
-                def inner_fn():
+                def inner_fn() -> Generator["Deferred[Any]", Any, None]:
                     # we want this to behave like an asynchronous function
                     yield run_on_reactor()
                     raise SynapseError(400, "blah")
@@ -324,7 +335,7 @@ def inner_fn():
                 return inner_fn()
 
         @defer.inlineCallbacks
-        def do_lookup():
+        def do_lookup() -> Generator["Deferred[Any]", Any, None]:
             with LoggingContext("c1") as c1:
                 try:
                     d = obj.fn(1)
@@ -351,13 +362,13 @@ def do_lookup():
         return d1
 
     @defer.inlineCallbacks
-    def test_cache_default_args(self) -> None:
+    def test_cache_default_args(self) -> Generator["Deferred[Any]", Any, None]:
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached()
-            def fn(self, arg1, arg2=2, arg3=3):
+            def fn(self, arg1: int, arg2: int = 2, arg3: int = 3) -> mock.Mock:
                 return self.mock(arg1, arg2, arg3)
 
         obj = Cls()
@@ -390,11 +401,11 @@ def fn(self, arg1, arg2=2, arg3=3):
 
     def test_cache_iterable(self) -> None:
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached(iterable=True)
-            def fn(self, arg1, arg2):
+            def fn(self, arg1: int, arg2: int) -> mock.Mock:
                 return self.mock(arg1, arg2)
 
         obj = Cls()
@@ -426,7 +437,7 @@ def test_cache_iterable_with_sync_exception(self) -> None:
 
         class Cls:
             @descriptors.cached(iterable=True)
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> None:
                 raise SynapseError(100, "mai spoon iz too big!!1")
 
         obj = Cls()
@@ -447,15 +458,15 @@ def test_invalidate_cascade(self) -> None:
 
         class Cls:
             @cached(cache_context=True)
-            async def func1(self, key, cache_context):
+            async def func1(self, key: str, cache_context: _CacheContext) -> int:
                 return await self.func2(key, on_invalidate=cache_context.invalidate)
 
             @cached(cache_context=True)
-            async def func2(self, key, cache_context):
+            async def func2(self, key: str, cache_context: _CacheContext) -> int:
                 return await self.func3(key, on_invalidate=cache_context.invalidate)
 
             @cached(cache_context=True)
-            async def func3(self, key, cache_context):
+            async def func3(self, key: str, cache_context: _CacheContext) -> int:
                 self.invalidate = cache_context.invalidate
                 return 42
 
@@ -473,14 +484,16 @@ def test_cancel(self) -> None:
 
         class Cls:
             @cached()
-            async def fn(self, arg1):
+            async def fn(self, arg1: int) -> str:
                 await complete_lookup
                 return str(arg1)
 
         obj = Cls()
 
-        d1 = obj.fn(123)
-        d2 = obj.fn(123)
+        # type-ignore: mypy sees the RHS as a coroutine. We need to improve the
+        # annotation of @cached to fix this.
+        d1: "Deferred[str]" = obj.fn(123)  # type: ignore[assignment]
+        d2: "Deferred[str]" = obj.fn(123)  # type: ignore[assignment]
         self.assertFalse(d1.called)
         self.assertFalse(d2.called)
 
@@ -505,14 +518,14 @@ class Cls:
             inner_context_was_finished = False
 
             @cached()
-            async def fn(self, arg1):
+            async def fn(self, arg1: int) -> str:
                 await make_deferred_yieldable(complete_lookup)
                 self.inner_context_was_finished = current_context().finished
                 return str(arg1)
 
         obj = Cls()
 
-        async def do_lookup():
+        async def do_lookup() -> None:
             with LoggingContext("c1") as c1:
                 try:
                     await obj.fn(123)
@@ -525,7 +538,7 @@ async def do_lookup():
                     )
                     # suppress the error and succeed
 
-        d = defer.ensureDeferred(do_lookup())
+        d: "Deferred[None]" = defer.ensureDeferred(do_lookup())
         d.cancel()
 
         complete_lookup.callback(None)
@@ -544,87 +557,90 @@ class CacheDecoratorTestCase(unittest.HomeserverTestCase):
     There are probably duplicates of the tests in DescriptorTestCase. Ideally the
     duplicates would be removed and the two sets of classes combined.
     """
+    # Through this class, mypy complains that the `xyz` expression in `(yield xyz)` has
+    # a concrete type when it should have type `Deferred[Any]`. I think this would be
+    # fixed by a better annotation for @cached.
 
     @defer.inlineCallbacks
-    def test_passthrough(self) -> None:
+    def test_passthrough(self) -> Generator["Deferred[Any]", Any, None]:
         class A:
             @cached()
-            def func(self, key):
+            def func(self, key: str) -> str:
                 return key
 
         a = A()
 
-        self.assertEqual((yield a.func("foo")), "foo")
-        self.assertEqual((yield a.func("bar")), "bar")
+        self.assertEqual((yield a.func("foo")), "foo")  # type: ignore[misc]
+        self.assertEqual((yield a.func("bar")), "bar")  # type: ignore[misc]
 
     @defer.inlineCallbacks
-    def test_hit(self) -> None:
+    def test_hit(self) -> Generator["Deferred[Any]", Any, None]:
         callcount = [0]
 
         class A:
             @cached()
-            def func(self, key):
+            def func(self, key: str) -> str:
                 callcount[0] += 1
                 return key
 
         a = A()
-        yield a.func("foo")
+        yield a.func("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 1)
 
-        self.assertEqual((yield a.func("foo")), "foo")
+        self.assertEqual((yield a.func("foo")), "foo")  # type: ignore[misc]
         self.assertEqual(callcount[0], 1)
 
     @defer.inlineCallbacks
-    def test_invalidate(self) -> None:
+    def test_invalidate(self) -> Generator["Deferred[Any]", Any, None]:
         callcount = [0]
 
         class A:
             @cached()
-            def func(self, key):
+            def func(self, key: str) -> str:
                 callcount[0] += 1
                 return key
 
         a = A()
-        yield a.func("foo")
+        yield a.func("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 1)
 
         a.func.invalidate(("foo",))
 
-        yield a.func("foo")
+        yield a.func("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 2)
 
     def test_invalidate_missing(self) -> None:
         class A:
             @cached()
-            def func(self, key):
+            def func(self, key: str) -> str:
                 return key
 
         A().func.invalidate(("what",))
 
     @defer.inlineCallbacks
-    def test_max_entries(self) -> None:
+    def test_max_entries(self) -> Generator["Deferred[Any]", Any, None]:
         callcount = [0]
 
         class A:
             @cached(max_entries=10)
-            def func(self, key):
+            def func(self, key: int) -> int:
                 callcount[0] += 1
                 return key
 
         a = A()
 
         for k in range(0, 12):
-            yield a.func(k)
+            yield a.func(k)  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 12)
 
         # There must have been at least 2 evictions, meaning if we calculate
         # all 12 values again, we must get called at least 2 more times
         for k in range(0, 12):
-            yield a.func(k)
+            yield a.func(k)  # type: ignore[misc]
 
         self.assertTrue(
             callcount[0] >= 14, msg="Expected callcount >= 14, got %d" % (callcount[0])
@@ -637,7 +653,7 @@ def test_prefill(self) -> None:
 
         class A:
             @cached()
-            def func(self, key):
+            def func(self, key: str) -> "Deferred[int]":
                 callcount[0] += 1
                 return d
 
@@ -649,95 +665,95 @@ def func(self, key):
         self.assertEqual(callcount[0], 0)
 
     @defer.inlineCallbacks
-    def test_invalidate_context(self) -> None:
+    def test_invalidate_context(self) -> Generator["Deferred[Any]", Any, None]:
         callcount = [0]
         callcount2 = [0]
 
         class A:
             @cached()
-            def func(self, key):
+            def func(self, key: str) -> str:
                 callcount[0] += 1
                 return key
 
             @cached(cache_context=True)
-            def func2(self, key, cache_context):
+            def func2(self, key: str, cache_context: _CacheContext) -> str:
                 callcount2[0] += 1
                 return self.func(key, on_invalidate=cache_context.invalidate)
 
         a = A()
-        yield a.func2("foo")
+        yield a.func2("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 1)
         self.assertEqual(callcount2[0], 1)
 
         a.func.invalidate(("foo",))
-        yield a.func("foo")
+        yield a.func("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 2)
         self.assertEqual(callcount2[0], 1)
 
-        yield a.func2("foo")
+        yield a.func2("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 2)
         self.assertEqual(callcount2[0], 2)
 
     @defer.inlineCallbacks
-    def test_eviction_context(self) -> None:
+    def test_eviction_context(self) -> Generator["Deferred[Any]", Any, None]:
         callcount = [0]
         callcount2 = [0]
 
         class A:
             @cached(max_entries=2)
-            def func(self, key):
+            def func(self, key: str) -> str:
                 callcount[0] += 1
                 return key
 
             @cached(cache_context=True)
-            def func2(self, key, cache_context):
+            def func2(self, key: str, cache_context: _CacheContext) -> str:
                 callcount2[0] += 1
                 return self.func(key, on_invalidate=cache_context.invalidate)
 
         a = A()
-        yield a.func2("foo")
-        yield a.func2("foo2")
+        yield a.func2("foo")  # type: ignore[misc]
+        yield a.func2("foo2")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 2)
         self.assertEqual(callcount2[0], 2)
 
-        yield a.func2("foo")
+        yield a.func2("foo")  # type: ignore[misc]
         self.assertEqual(callcount[0], 2)
         self.assertEqual(callcount2[0], 2)
 
-        yield a.func("foo3")
+        yield a.func("foo3")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 3)
         self.assertEqual(callcount2[0], 2)
 
-        yield a.func2("foo")
+        yield a.func2("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 4)
         self.assertEqual(callcount2[0], 3)
 
     @defer.inlineCallbacks
-    def test_double_get(self) -> None:
+    def test_double_get(self) -> Generator["Deferred[Any]", Any, None]:
         callcount = [0]
         callcount2 = [0]
 
         class A:
             @cached()
-            def func(self, key):
+            def func(self, key: str) -> str:
                 callcount[0] += 1
                 return key
 
             @cached(cache_context=True)
-            def func2(self, key, cache_context):
+            def func2(self, key: str, cache_context: _CacheContext) -> str:
                 callcount2[0] += 1
                 return self.func(key, on_invalidate=cache_context.invalidate)
 
         a = A()
         a.func2.cache.cache = mock.Mock(wraps=a.func2.cache.cache)
 
-        yield a.func2("foo")
+        yield a.func2("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 1)
         self.assertEqual(callcount2[0], 1)
@@ -745,7 +761,7 @@ def func2(self, key, cache_context):
         a.func2.invalidate(("foo",))
         self.assertEqual(a.func2.cache.cache.del_multi.call_count, 1)
 
-        yield a.func2("foo")
+        yield a.func2("foo")  # type: ignore[misc]
         a.func2.invalidate(("foo",))
         self.assertEqual(a.func2.cache.cache.del_multi.call_count, 2)
 
@@ -754,12 +770,12 @@ def func2(self, key, cache_context):
 
         a.func.invalidate(("foo",))
         self.assertEqual(a.func2.cache.cache.del_multi.call_count, 3)
-        yield a.func("foo")
+        yield a.func("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 2)
         self.assertEqual(callcount2[0], 2)
 
-        yield a.func2("foo")
+        yield a.func2("foo")  # type: ignore[misc]
 
         self.assertEqual(callcount[0], 2)
         self.assertEqual(callcount2[0], 3)
@@ -767,17 +783,17 @@ def func2(self, key, cache_context):
 
 class CachedListDescriptorTestCase(unittest.TestCase):
     @defer.inlineCallbacks
-    def test_cache(self) -> None:
+    def test_cache(self) -> Generator["Deferred[Any]", Any, None]:
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached()
-            def fn(self, arg1, arg2):
+            def fn(self, arg1: int, arg2: int) -> None:
                 pass
 
             @descriptors.cachedList(cached_method_name="fn", list_name="args1")
-            async def list_fn(self, args1, arg2):
+            async def list_fn(self, args1: Iterable[int], arg2: int) -> mock.Mock:
                 context = current_context()
                 assert isinstance(context, LoggingContext)
                 assert context.name == "c1"
@@ -793,7 +809,9 @@ async def list_fn(self, args1, arg2):
             obj.mock.return_value = {10: "fish", 20: "chips"}
 
             # start the lookup off
-            d1 = obj.list_fn([10, 20], 2)
+            # type-ignore: mypy thinks the RHS is a coroutine; cachedList annotation
+            # needs improving.
+            d1: "Deferred[mock.Mock]" = obj.list_fn([10, 20], 2)  # type: ignore[assignment, misc]
             self.assertEqual(current_context(), SENTINEL_CONTEXT)
             r = yield d1
             self.assertEqual(current_context(), c1)
@@ -803,19 +821,19 @@ async def list_fn(self, args1, arg2):
 
             # a call with different params should call the mock again
             obj.mock.return_value = {30: "peas"}
-            r = yield obj.list_fn([20, 30], 2)
+            r = yield obj.list_fn([20, 30], 2)  # type: ignore[misc]
             obj.mock.assert_called_once_with({30}, 2)
             self.assertEqual(r, {20: "chips", 30: "peas"})
             obj.mock.reset_mock()
 
             # all the values should now be cached
-            r = yield obj.fn(10, 2)
+            r = yield obj.fn(10, 2)  # type: ignore[misc]
             self.assertEqual(r, "fish")
-            r = yield obj.fn(20, 2)
+            r = yield obj.fn(20, 2)  # type: ignore[misc]
             self.assertEqual(r, "chips")
-            r = yield obj.fn(30, 2)
+            r = yield obj.fn(30, 2)  # type: ignore[misc]
             self.assertEqual(r, "peas")
-            r = yield obj.list_fn([10, 20, 30], 2)
+            r = yield obj.list_fn([10, 20, 30], 2)  # type: ignore[misc]
             obj.mock.assert_not_called()
             self.assertEqual(r, {10: "fish", 20: "chips", 30: "peas"})
 
@@ -824,7 +842,7 @@ async def list_fn(self, args1, arg2):
             obj.mock.reset_mock()
             obj.mock.return_value = {40: "gravy"}
             iterable = (x for x in [10, 40, 40])
-            r = yield obj.list_fn(iterable, 2)
+            r = yield obj.list_fn(iterable, 2)  # type: ignore[misc]
             obj.mock.assert_called_once_with({40}, 2)
             self.assertEqual(r, {10: "fish", 40: "gravy"})
 
@@ -832,19 +850,19 @@ def test_concurrent_lookups(self) -> None:
         """All concurrent lookups should get the same result"""
 
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached()
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> None:
                 pass
 
             @descriptors.cachedList(cached_method_name="fn", list_name="args1")
-            def list_fn(self, args1) -> "Deferred[dict]":
+            def list_fn(self, args1: Sequence[int]) -> mock.Mock:
                 return self.mock(args1)
 
         obj = Cls()
-        deferred_result: "Deferred[dict]" = Deferred()
+        deferred_result: "Deferred[Dict[int, str]]" = Deferred()
         obj.mock.return_value = deferred_result
 
         # start off several concurrent lookups of the same key
@@ -871,19 +889,19 @@ def list_fn(self, args1) -> "Deferred[dict]":
         self.assertEqual(self.successResultOf(d3), {10: "peas"})
 
     @defer.inlineCallbacks
-    def test_invalidate(self) -> None:
+    def test_invalidate(self) -> Generator["Deferred[Any]", Any, None]:
         """Make sure that invalidation callbacks are called."""
 
         class Cls:
-            def __init__(self):
+            def __init__(self) -> None:
                 self.mock = mock.Mock()
 
             @descriptors.cached()
-            def fn(self, arg1, arg2):
+            def fn(self, arg1: int, arg2: int) -> None:
                 pass
 
             @descriptors.cachedList(cached_method_name="fn", list_name="args1")
-            async def list_fn(self, args1, arg2):
+            async def list_fn(self, args1: Sequence[int], arg2: int) -> mock.Mock:
                 # we want this to behave like an asynchronous function
                 await run_on_reactor()
                 return self.mock(args1, arg2)
@@ -894,13 +912,19 @@ async def list_fn(self, args1, arg2):
 
         # cache miss
         obj.mock.return_value = {10: "fish", 20: "chips"}
-        r1 = yield obj.list_fn([10, 20], 2, on_invalidate=invalidate0)
+        r1: Dict[int, str]
+        # type-ignore: mypy thinks the expression being yielded is a coroutine (and
+        # similarly for r2 below). But the cachedList implementation should ensure the
+        # expression is a deferred. Stick a type-ignore here, in lieu of a better
+        # cachedList annotation.
+        r1 = yield obj.list_fn([10, 20], 2, on_invalidate=invalidate0)  # type: ignore[misc]
         obj.mock.assert_called_once_with({10, 20}, 2)
         self.assertEqual(r1, {10: "fish", 20: "chips"})
         obj.mock.reset_mock()
 
         # cache hit
-        r2 = yield obj.list_fn([10, 20], 2, on_invalidate=invalidate1)
+        r2: Dict[int, str]
+        r2 = yield obj.list_fn([10, 20], 2, on_invalidate=invalidate1)  # type: ignore[misc]
         obj.mock.assert_not_called()
         self.assertEqual(r2, {10: "fish", 20: "chips"})
 
@@ -918,18 +942,23 @@ def test_cancel(self) -> None:
 
         class Cls:
             @cached()
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> None:
                 pass
 
             @cachedList(cached_method_name="fn", list_name="args")
-            async def list_fn(self, args):
+            async def list_fn(self, args: Sequence[int]) -> Dict[int, str]:
                 await complete_lookup
                 return {arg: str(arg) for arg in args}
 
         obj = Cls()
 
-        d1 = obj.list_fn([123, 456])
-        d2 = obj.list_fn([123, 456, 789])
+        # type-ignore: the inner `list_fn` is a coroutine, so mypy thinks that the RHS
+        # expressions below are coroutines. (Presumably, at runtime the cachedList
+        # decorator uses ensureDeferred to convert them to Deferreds.) We could try to
+        # improve the annotation on `cachedList` in the future to remove these
+        # type-ignores.
+        d1: "Deferred[Dict[int, str]]" = obj.list_fn([123, 456])  # type: ignore[assignment]
+        d2: "Deferred[Dict[int, str]]" = obj.list_fn([123, 456, 789])  # type: ignore[assignment]
         self.assertFalse(d1.called)
         self.assertFalse(d2.called)
 
@@ -953,18 +982,18 @@ class Cls:
             inner_context_was_finished = False
 
             @cached()
-            def fn(self, arg1):
+            def fn(self, arg1: int) -> None:
                 pass
 
             @cachedList(cached_method_name="fn", list_name="args")
-            async def list_fn(self, args):
+            async def list_fn(self, args: Sequence[int]) -> Dict[int, str]:
                 await make_deferred_yieldable(complete_lookup)
                 self.inner_context_was_finished = current_context().finished
                 return {arg: str(arg) for arg in args}
 
         obj = Cls()
 
-        async def do_lookup():
+        async def do_lookup() -> None:
             with LoggingContext("c1") as c1:
                 try:
                     await obj.list_fn([123])
@@ -995,14 +1024,14 @@ def test_num_args_mismatch(self) -> None:
 
         class Cls:
             @descriptors.cached(tree=True)
-            def fn(self, room_id, event_id):
+            def fn(self, room_id: str, event_id: str) -> None:
                 pass
 
             # This is wrong ❌. `@cachedList` expects to be given the same number
             # of arguments as the underlying cached function, just with one of
             # the arguments being an iterable
             @descriptors.cachedList(cached_method_name="fn", list_name="keys")
-            def list_fn(self, keys: Iterable[Tuple[str, str]]):
+            def list_fn(self, keys: Iterable[Tuple[str, str]]) -> None:
                 pass
 
             # Corrected syntax ✅

From 03389c71ec3246b848a43a3349bcec37f29a150c Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 9 Feb 2023 02:11:56 +0000
Subject: [PATCH 4/7] Disallow-untyped defs tho

---
 mypy.ini | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mypy.ini b/mypy.ini
index 3f144e61fbba..5a77a563198e 100644
--- a/mypy.ini
+++ b/mypy.ini
@@ -62,9 +62,6 @@ disallow_untyped_defs = False
 [mypy-tests.unittest]
 disallow_untyped_defs = False
 
-[mypy-tests.util.caches.test_descriptors]
-disallow_untyped_defs = False
-
 ;; Dependencies without annotations
 ;; Before ignoring a module, check to see if type stubs are available.
 ;; The `typeshed` project maintains stubs here:

From 4cb9ca8f9533bd3ef8eb1c9a3ce8488ad621caa9 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 9 Feb 2023 02:14:58 +0000
Subject: [PATCH 5/7] Changelog

---
 changelog.d/15032.misc | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 changelog.d/15032.misc

diff --git a/changelog.d/15032.misc b/changelog.d/15032.misc
new file mode 100644
index 000000000000..93ceaeafc9b9
--- /dev/null
+++ b/changelog.d/15032.misc
@@ -0,0 +1 @@
+Improve type hints.

From fb6fd6c5ca232837d647852463bedcbc79cdad4e Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 9 Feb 2023 10:48:33 +0000
Subject: [PATCH 6/7] Lint

---
 tests/util/caches/test_descriptors.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/util/caches/test_descriptors.py b/tests/util/caches/test_descriptors.py
index a2275d165a68..b40afaf2c44b 100644
--- a/tests/util/caches/test_descriptors.py
+++ b/tests/util/caches/test_descriptors.py
@@ -557,6 +557,7 @@ class CacheDecoratorTestCase(unittest.HomeserverTestCase):
     There are probably duplicates of the tests in DescriptorTestCase. Ideally the
     duplicates would be removed and the two sets of classes combined.
     """
+
     # Through this class, mypy complains that the `xyz` expression in `(yield xyz)` has
     # a concrete type when it should have type `Deferred[Any]`. I think this would be
     # fixed by a better annotation for @cached.

From f3f195217472284246f9150bd7686a821ba68b69 Mon Sep 17 00:00:00 2001
From: David Robertson <davidr@element.io>
Date: Thu, 9 Feb 2023 11:06:30 +0000
Subject: [PATCH 7/7] Olddeps syntax

---
 tests/util/caches/test_descriptors.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/util/caches/test_descriptors.py b/tests/util/caches/test_descriptors.py
index b40afaf2c44b..8bbd6d841174 100644
--- a/tests/util/caches/test_descriptors.py
+++ b/tests/util/caches/test_descriptors.py
@@ -282,7 +282,7 @@ def test_cache_logcontexts(self) -> "Deferred[List[int]]":
 
         class Cls:
             @descriptors.cached()
-            def fn(self, arg1: int) -> Deferred[int]:
+            def fn(self, arg1: int) -> "Deferred[int]":
                 @defer.inlineCallbacks
                 def inner_fn() -> Generator["Deferred[Any]", Any, int]:
                     with PreserveLoggingContext():