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

mypy plugin to check @cached return types #14911

Merged
merged 44 commits into from
Oct 2, 2023
Merged
Changes from 1 commit
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
eb24cdb
WIP mypy plugin to check `@cached` return types
Jan 25, 2023
d2cbac3
Whoops, Sequence is immutable
Jan 31, 2023
90631ac
WIP
Jan 31, 2023
4eb7de9
Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…
clokep Sep 12, 2023
ad1c28a
Update comments.
clokep Sep 12, 2023
88323dd
Simplify code due to other knowledge.
clokep Sep 12, 2023
676c858
Treat containers more similarly.
clokep Sep 12, 2023
008ef3f
cachedList wraps Mapping
clokep Sep 12, 2023
8aa4e87
Fix-up errors in tests.
clokep Sep 13, 2023
0f3c036
Ignore a few calls which purposefully (?) return mutable objects.
clokep Sep 13, 2023
9c94574
Data exfilitration is read-only and update admin APIs.
clokep Sep 13, 2023
f5fec7f
Update account_data & tags methods to be immutable.
clokep Sep 13, 2023
9a62053
FIx-up push related caching.
clokep Sep 13, 2023
cc61862
Update filtering to return immutable objects.
clokep Sep 13, 2023
a27a67f
Update relations with immutable.
clokep Sep 13, 2023
8f7f4d7
Update receipts code.
clokep Sep 13, 2023
9fdc5a1
Update e2e keys & devices.
clokep Sep 13, 2023
d8cce5b
Update appservice stuff.
clokep Sep 13, 2023
ef61f3d
Ensure current hosts is immutable.
clokep Sep 13, 2023
96faa34
Properly check attrs for frozen-ness.
clokep Sep 13, 2023
2f75929
Kick CI
clokep Sep 14, 2023
7849b26
Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…
clokep Sep 14, 2023
8b1b15b
Fix-up return value of get_latest_event_ids_in_room.
clokep Sep 14, 2023
451c9b1
Revert "Kick CI"
clokep Sep 14, 2023
d52e30c
Newsfragment
clokep Sep 14, 2023
47b7ba7
FIx-up sync changes.
clokep Sep 14, 2023
ee77d82
Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…
clokep Sep 18, 2023
0f02ad1
Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…
clokep Sep 19, 2023
51a1a5f
Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…
clokep Sep 20, 2023
07c4531
Merge branch 'develop' into dmr/mypy-check-at-cached
clokep Sep 25, 2023
745ad61
Correct context
erikjohnston Sep 29, 2023
03b0e40
Remove ignores for call-sites.
clokep Sep 29, 2023
2c07b1f
Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…
clokep Sep 29, 2023
460ed3c
Add ignores at definition sites.
clokep Sep 29, 2023
fbecb56
Actually check cachedList.
clokep Sep 29, 2023
dba8e72
Fix incorrect generic.
clokep Sep 29, 2023
4f06d85
Lint
clokep Sep 29, 2023
3875662
Abstract shared code.
clokep Sep 29, 2023
fb4ff5d
ServerAclEvaluator is immutable.
clokep Sep 29, 2023
06ddf65
Update comments.
clokep Sep 29, 2023
9b7ee03
Lint
clokep Sep 29, 2023
e2f599d
Update comments and remove unnecessary argument.
clokep Oct 2, 2023
da377cf
Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…
clokep Oct 2, 2023
a2956a6
Fix duplicate word.
clokep Oct 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions scripts-dev/mypy_synapse_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ def get_method_signature_hook(


def _get_true_return_type(signature: CallableType) -> mypy.types.Type:
"""Get the "final" return type of a maybe async/Deferred."""
"""
Get the "final" return type of a callable which might return returned an Awaitable/Deferred.
clokep marked this conversation as resolved.
Show resolved Hide resolved
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aside: If we have function returning e.g. Deferred[Deferred[T]], it looks like this function doesn't loop and extract the inner T. But we shouldn't be doing that in the first place.

(Would such a type even be legitimate?)

Copy link
Member

Choose a reason for hiding this comment

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

(Would such a type even be legitimate?)

I think it wouldn't make sense? Please don't do that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that's even really doable since a Deferred returning a Deferred will wait on the inner Deferred? https://docs.twistedmatrix.com/en/stable/core/howto/defer.html#chaining-deferreds

Anyway, that interaction always confuses me and I'm willing to not support it.

if isinstance(signature.ret_type, Instance):
# If a coroutine, unwrap the coroutine's return type.
if signature.ret_type.type.fullname == "typing.Coroutine":
Expand All @@ -84,6 +86,7 @@ def cached_function_method_signature(ctx: MethodSigContext) -> CallableType:
1. the `self` argument needs to be marked as "bound";
2. any `cache_context` argument should be removed;
3. an optional keyword argument `on_invalidated` should be added.
4. Wrap the return type to always be a Deferred.
"""

# 1. Mark this as a bound function signature.
Expand All @@ -93,7 +96,7 @@ def cached_function_method_signature(ctx: MethodSigContext) -> CallableType:
#
# 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.
# it's not the end of the world.
Comment on lines -59 to +99
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<3

context_arg_index = None
for idx, name in enumerate(signature.arg_names):
if name == "cache_context":
Expand Down Expand Up @@ -153,6 +156,11 @@ def cached_function_method_signature(ctx: MethodSigContext) -> CallableType:


def check_is_cacheable_wrapper(ctx: MethodSigContext) -> CallableType:
clokep marked this conversation as resolved.
Show resolved Hide resolved
"""Asserts that the signature of a method returns a value which can be cached.

Makes no changes to the provided method signature.
Comment on lines +159 to +161
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we add a sentence explaining why there is a wrapper? It looks like the point is mainly to check that we have a proper CallableType(?)

OTOH if it's just "to have smaller functions" then let's leave it as is.

Copy link
Member

Choose a reason for hiding this comment

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

OTOH if it's just "to have smaller functions" then let's leave it as is.

I almost inlined it, but it seemed nice to keep them separate.

"""
# The true signature, this isn't being modified so this is what will be returned.
signature: CallableType = ctx.default_signature

if not isinstance(ctx.args[0][0], TempNode):
Expand All @@ -164,18 +172,25 @@ def check_is_cacheable_wrapper(ctx: MethodSigContext) -> CallableType:
ctx.api.fail("Cached 'function' is not a callable", ctx.context)
return signature

# Unwrap the true return type from the cached function.
ret_arg = _get_true_return_type(orig_sig)
check_is_cacheable(orig_sig, ctx, ret_arg)
check_is_cacheable(orig_sig, ctx)

return signature


def check_is_cacheable(
signature: CallableType,
ctx: Union[MethodSigContext, FunctionSigContext],
return_type: mypy.types.Type,
) -> None:
"""
Check if a callable returns a type which can be cached.

Args:
signature: The callable to check.
ctx: The signature context, used for error reporting.
"""
# Unwrap the true return type from the cached function.
return_type = _get_true_return_type(signature)

verbose = ctx.api.options.verbosity >= 1
# TODO Technically a cachedList only needs immutable values, but forcing them
# to return Mapping instead of Dict is fine.
Expand All @@ -190,7 +205,6 @@ def check_is_cacheable(
message += f" ({note})"
message = message.replace("builtins.", "").replace("typing.", "")

# TODO The context is the context of the caller, not the method itself.
if ok and note:
ctx.api.note(message, ctx.context) # type: ignore[attr-defined]
elif not ok:
Expand Down Expand Up @@ -240,6 +254,11 @@ def is_cacheable(
rt: mypy.types.Type, signature: CallableType, verbose: bool
) -> Tuple[bool, Optional[str]]:
"""
Check if a particular type is cachable.

A type is cachable if it is immutable; for complex types this recurses to
check each type parameter.

Returns: a 2-tuple (cacheable, message).
- cachable: False means the type is definitely not cacheable;
true means anything else.
Expand Down