mypy plugin to check `@cached` return types by DMRobertson · Pull Request #14911 · matrix-org/synapse

DMRobertson · 2023-01-25T00:26:21Z

The things that are cached should be immutable to avoid callers modifying cached values (which has lead to bugs in the past). This updates our mypy plug-in to enforce this via the type system.

Complements #13755.

Errors which have been fixed

The main obstruction seems to be that we have lots of Dict[str, Any] or Dict[str, str] in caches.

Sample:

+ mypy
synapse/handlers/read_marker.py:44: error: function get_account_data_for_room_and_type of AccountDataWorkerStore is @cached, but has mutable return value Union[dict[str, Any], None]  [synapse-@cached-mutable]
synapse/appservice/__init__.py:187: error: function get_local_users_in_room of RoomMemberWorkerStore is @cached, but has mutable return value list[str]  [synapse-@cached-mutable]
synapse/appservice/__init__.py:248: error: function get_aliases_for_room of DirectoryWorkerStore is @cached, but has mutable return value list[str]  [synapse-@cached-mutable]
synapse/app/phone_stats_home.py:203: error: function get_monthly_active_count_by_service of MonthlyActiveUsersWorkerStore is @cached, but has mutable return value dict[str, int]  [synapse-@cached-mutable]
synapse/api/filtering.py:170: error: function get_user_filter of FilteringWorkerStore is @cached, but has mutable return value dict[str, Any]  [synapse-@cached-mutable]
synapse/storage/databases/main/receipts.py:204: error: function _get_receipts_for_user_with_orderings of ReceiptsWorkerStore is @cached, but has mutable return value dict[str, Any]  [synapse-@cached-mutable]
synapse/storage/databases/main/receipts.py:283: error: function _get_linearized_receipts_for_rooms of ReceiptsWorkerStore is @cached, but has mutable return value dict[str, list[dict[str, Any]]]  [synapse-@cached-mutable]
synapse/storage/databases/main/receipts.py:309: error: function _get_linearized_receipts_for_room of ReceiptsWorkerStore is @cached, but has mutable return value list[dict[str, Any]]  [synapse-@cached-mutable]
synapse/storage/databases/main/events_worker.py:1618: error: function _have_seen_events_dict of EventsWorkerStore is @cached, but has mutable return value dict[str, bool]  [synapse-@cached-mutable]
synapse/storage/databases/main/events_worker.py:1668: error: function _have_seen_events_dict of EventsWorkerStore is @cached, but has mutable return value dict[str, bool]  [synapse-@cached-mutable]
synapse/storage/databases/main/signatures.py:77: error: function get_event_reference_hashes of SignatureWorkerStore is @cached, but has mutable return value dict[str, dict[str, bytes]]  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:385: error: function get_invited_rooms_for_local_user of RoomMemberWorkerStore is @cached, but has mutable return value list[synapse.storage.roommember.RoomsForUser]  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:723: error: function _get_rooms_for_users of RoomMemberWorkerStore is @cached, but has mutable return value dict[str, frozenset[str]]  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:795: error: function get_users_in_room of RoomMemberWorkerStore is @cached, but has mutable return value list[str]  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:861: error: function _get_user_ids_from_membership_event_ids of RoomMemberWorkerStore is @cached, but has mutable return value dict[str, Union[str, None]]  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:969: error: function get_users_in_room of RoomMemberWorkerStore is @cached, but has mutable return value list[str]  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:1020: error: function get_current_hosts_in_room of RoomMemberWorkerStore is @cached, but has mutable return value set[str]  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:1098: error: function _get_joined_hosts_cache of RoomMemberWorkerStore is @cached, but has mutable return value synapse.storage.databases.main.roommember._JoinedHostsCache (non-frozen attrs class)  [synapse-@cached-mutable]
synapse/storage/databases/main/roommember.py:1098: note: Error code "synapse-@cached-mutable" not covered by "type: ignore" comment

Some outstanding TODOs:

check that frozen attrs classes have immutable fields
work out how to handle types written in Rust

clokep · 2023-01-25T00:47:27Z

Can we return Mapping in that case? Or is your point that we need to audit a bunch of stuff?

clokep

You grabbed my interest. Seems reasonable but I have a couple thoughts!

scripts-dev/mypy_synapse_plugin.py

DMRobertson · 2023-02-08T14:09:52Z

(Thanks for your thoughts, Patrick. I'll need to give this and them more thought, when I get some spare time to stare at this again)

DMRobertson · 2023-09-12T19:12:43Z

Can we return Mapping in that case? Or is your point that we need to audit a bunch of stuff?

Yep, I think we should return a Mapping to make it clear that the caller can't mutate dictionaries that are kept as cache values.

…cached

DMRobertson · 2023-09-14T13:24:01Z

synapse/federation/federation_server.py

        servers_in_room: Optional[Collection[str]]
        if caller_supports_partial_state:
-            summary = await self.store.get_room_summary(room_id)
+            summary = await self.store.get_room_summary(room_id)  # type: ignore[synapse-@cached-mutable]


Exciting to see!

Terrifying IMO of how we use this.

DMRobertson · 2023-09-20T12:27:09Z

Does this slow down mypy a lot?

Testing with:

git rev-parse --short HEAD
rm -r .mypy_cache/
poetry install --all-extras --sync
mypy --version
time mypy

On this branch:

$ git rev-parse --short HEAD
51a1a5fc4
$ mypy --version
mypy 1.5.1 (compiled: yes)
$ time mypy
Success: no issues found in 851 source files

real	0m36.189s
user	0m34.196s
sys	0m1.093s

On develop:

$ git rev-parse --short HEAD
7ec0a141b
$ mypy --version
mypy 1.5.1 (compiled: yes)
$ time mypy
Success: no issues found in 851 source files

real	0m33.438s
user	0m31.706s
sys	0m0.934s

So ~3s slower, say 10% overhead. I think I'd consider that good enough as-is.

clokep · 2023-09-20T12:37:39Z

Yeah that sounds reasonable to me.

clokep · 2023-09-21T11:45:05Z

I think this probably needs a review from not @DMRobertson and not myself since we collaborated on this.

erikjohnston

Otherwise looks good!

synapse/federation/federation_server.py

synapse/storage/databases/main/event_push_actions.py

synapse/storage/databases/main/relations.py

…cached

erikjohnston

This is great! 🎉

scripts-dev/mypy_synapse_plugin.py

…cached

DMRobertson · 2023-10-02T12:19:39Z

scripts-dev/mypy_synapse_plugin.py

+    """
+    Get the "final" return type of a callable which might return returned an Awaitable/Deferred.
+    """


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?)

(Would such a type even be legitimate?)

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

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.

DMRobertson · 2023-10-02T12:20:02Z

scripts-dev/mypy_synapse_plugin.py

-    # its not the end of the world.
+    # it's not the end of the world.


DMRobertson · 2023-10-02T12:22:29Z

scripts-dev/mypy_synapse_plugin.py

+    """Asserts that the signature of a method returns a value which can be cached.
+
+    Makes no changes to the provided method signature.


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.

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.

scripts-dev/mypy_synapse_plugin.py

Co-authored-by: David Robertson <davidr@element.io>

clokep · 2023-10-02T14:35:39Z

Thanks so much for the help @erikjohnston and @DMRobertson! Happy to see this merged! 🎉

@gotmax23

No significant changes since 1.94.0rc1. - Render plain, CSS, CSV, JSON and common image formats in the browser (inline) when requested through the /download endpoint. ([\matrix-org#15988](matrix-org#15988)) - Add experimental support for [MSC4028](matrix-org/matrix-spec-proposals#4028) to push all encrypted events to clients. ([\matrix-org#16361](matrix-org#16361)) - Minor performance improvement when sending presence to federated servers. ([\matrix-org#16385](matrix-org#16385)) - Minor performance improvement by caching server ACL checking. ([\matrix-org#16360](matrix-org#16360)) - Add developer documentation concerning gradual schema migrations with column alterations. ([\matrix-org#15691](matrix-org#15691)) - Improve documentation of the user directory search algorithm. ([\matrix-org#16320](matrix-org#16320)) - Fix rendering of user admin API documentation around deactivation. This was broken in Synapse 1.91.0. ([\matrix-org#16355](matrix-org#16355)) - Update documentation around message retention policies. ([\matrix-org#16382](matrix-org#16382)) - Add note to `federation_domain_whitelist` config option to clarify its usage. ([\matrix-org#16416](matrix-org#16416)) - Improve legacy release notes. ([\matrix-org#16418](matrix-org#16418)) - Remove Python version from `/_synapse/admin/v1/server_version`. ([\matrix-org#16380](matrix-org#16380)) - Avoid running CI steps when the files they check have not been changed. ([\matrix-org#14745](matrix-org#14745), [\matrix-org#16387](matrix-org#16387)) - Improve type hints. ([\matrix-org#14911](matrix-org#14911), [\matrix-org#16350](matrix-org#16350), [\matrix-org#16356](matrix-org#16356), [\matrix-org#16395](matrix-org#16395)) - Added support for pydantic v2 in addition to pydantic v1. Contributed by Maxwell G (@gotmax23). ([\matrix-org#16332](matrix-org#16332)) - Get CI to check PRs have been signed-off. ([\matrix-org#16348](matrix-org#16348)) - Add missing licence header. ([\matrix-org#16359](matrix-org#16359)) - Improve type hints, and bump types-psycopg2 from 2.9.21.11 to 2.9.21.14. ([\matrix-org#16381](matrix-org#16381)) - Improve comments in `StateGroupBackgroundUpdateStore`. ([\matrix-org#16383](matrix-org#16383)) - Update maturin configuration. ([\matrix-org#16394](matrix-org#16394)) - Downgrade replication stream time out error log lines to warning. ([\matrix-org#16401](matrix-org#16401)) * Bump actions/checkout from 3 to 4. ([\matrix-org#16250](matrix-org#16250)) * Bump cryptography from 41.0.3 to 41.0.4. ([\matrix-org#16362](matrix-org#16362)) * Bump dawidd6/action-download-artifact from 2.27.0 to 2.28.0. ([\matrix-org#16374](matrix-org#16374)) * Bump docker/setup-buildx-action from 2 to 3. ([\matrix-org#16375](matrix-org#16375)) * Bump gitpython from 3.1.35 to 3.1.37. ([\matrix-org#16376](matrix-org#16376)) * Bump msgpack from 1.0.5 to 1.0.6. ([\matrix-org#16377](matrix-org#16377)) * Bump msgpack from 1.0.6 to 1.0.7. ([\matrix-org#16412](matrix-org#16412)) * Bump phonenumbers from 8.13.19 to 8.13.22. ([\matrix-org#16413](matrix-org#16413)) * Bump psycopg2 from 2.9.7 to 2.9.8. ([\matrix-org#16409](matrix-org#16409)) * Bump pydantic from 2.3.0 to 2.4.2. ([\matrix-org#16410](matrix-org#16410)) * Bump regex from 1.9.5 to 1.9.6. ([\matrix-org#16408](matrix-org#16408)) * Bump sentry-sdk from 1.30.0 to 1.31.0. ([\matrix-org#16378](matrix-org#16378)) * Bump types-netaddr from 0.8.0.9 to 0.9.0.1. ([\matrix-org#16411](matrix-org#16411)) * Bump types-psycopg2 from 2.9.21.11 to 2.9.21.14. ([\matrix-org#16381](matrix-org#16381)) * Bump urllib3 from 1.26.15 to 1.26.17. ([\matrix-org#16422](matrix-org#16422)) # -----BEGIN PGP SIGNATURE----- # # iQIzBAABCAAdFiEE8SRSDO7gYkSP4chELS76LzL74EcFAmUlINwACgkQLS76LzL7 # 4EdvExAAgjk6+/Fu45MRG7u5kFmFzoZWLOPD10XROANaSeqW1l/pBhFh+XvwR4TZ # l/FdkSfS9YpHnw3aof13TclLu6IVWDM+vqYFuY2HSY/yzbcGvJFHqr26kOccpTTd # 2r9m/AkguyHEBECDW8qJLXb8M7dqNa2SydTBu1+IrKfj6nq+fRxVyQhyAJXrI1Ta # Dnz0XJ4TcwTrMPVk4MYrAcYjID6IV89dtp7ttH4DwXKDeSjMtxM/46EIg4u+VXDz # fzK25JHVFYJA5+/rOn/RslmxjJHQfEIEB6NYxQwLeMeZuGSZooTebKn1odwogvhI # Srtfsytum+twgSHD1s+7KldM+EjTiu7ouKi8VcfOlFuLnuBiROEc5WUljcL5K63F # kVx2bXGU/eNkPp6ntNhYfgswx+yk2rXFqkTjz+xZQIZcOBqehHBDy8VhtwlRkTUw # bzocdKkLMA4nfSlq5fFOAErMqJKsPS8aN9yYPShqEUiSUOKle8eHfA1cTXJuK0MS # K2/YcDDZmJBrwVADyNDk5GKaDx39rR752OSuJb57Sp/edwUg6+H1I6lIN6YTeoJw # FzJwGMzuMCktOQRW2enxQiA6RZjXFCwvD1LoWMjyO4YTXQwXxNCXsb0kLKUqfwsy # qMGphWEl3rdzVSuFapNAgOLF0RfFNYZdhQnk+3fNEwxumxoqgho= # =hx8G # -----END PGP SIGNATURE----- # gpg: Signature made Tue Oct 10 11:01:00 2023 BST # gpg: using RSA key F124520CEEE062448FE1C8442D2EFA2F32FBE047 # gpg: Can't check signature: No public key # Conflicts: # .github/workflows/docker.yml # .github/workflows/docs-pr-netlify.yaml # .github/workflows/docs-pr.yaml # .github/workflows/docs.yaml # .github/workflows/latest_deps.yml # .github/workflows/poetry_lockfile.yaml # .github/workflows/push_complement_image.yml # .github/workflows/release-artifacts.yml # .github/workflows/tests.yml # .github/workflows/twisted_trunk.yml # poetry.lock # rust/src/push/base_rules.rs

WIP mypy plugin to check @cached return types

eb24cdb

DMRobertson mentioned this pull request Jan 25, 2023

Return read-only collections from @cached methods #13755

Merged

clokep reviewed Jan 25, 2023

View reviewed changes

scripts-dev/mypy_synapse_plugin.py Outdated Show resolved Hide resolved

scripts-dev/mypy_synapse_plugin.py Outdated Show resolved Hide resolved

David Robertson added 2 commits January 31, 2023 17:40

Whoops, Sequence is immutable

d2cbac3

WIP

90631ac

clokep reviewed Jan 31, 2023

View reviewed changes

scripts-dev/mypy_synapse_plugin.py Outdated Show resolved Hide resolved

DMRobertson mentioned this pull request Feb 8, 2023

Fix bug in replication where response is cached #15024

Merged

DMRobertson self-assigned this Feb 16, 2023

DMRobertson mentioned this pull request Sep 12, 2023

Use StrCollection in more places #16301

Merged

clokep added 17 commits September 12, 2023 15:22

Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…

4eb7de9

…cached

Update comments.

ad1c28a

Simplify code due to other knowledge.

88323dd

Treat containers more similarly.

676c858

cachedList wraps Mapping

008ef3f

Fix-up errors in tests.

8aa4e87

Ignore a few calls which purposefully (?) return mutable objects.

0f3c036

Data exfilitration is read-only and update admin APIs.

9c94574

Update account_data & tags methods to be immutable.

f5fec7f

FIx-up push related caching.

9a62053

Update filtering to return immutable objects.

cc61862

Update relations with immutable.

a27a67f

Update receipts code.

8f7f4d7

Update e2e keys & devices.

9fdc5a1

Update appservice stuff.

d8cce5b

Ensure current hosts is immutable.

ef61f3d

Properly check attrs for frozen-ness.

96faa34

DMRobertson commented Sep 14, 2023

View reviewed changes

clokep marked this pull request as ready for review September 21, 2023 11:44

clokep requested a review from a team as a code owner September 21, 2023 11:44

Merge branch 'develop' into dmr/mypy-check-at-cached

07c4531

clokep mentioned this pull request Sep 25, 2023

Make event objects unhashable #15305

Closed

erikjohnston reviewed Sep 29, 2023

View reviewed changes

synapse/federation/federation_server.py Outdated Show resolved Hide resolved

synapse/storage/databases/main/event_push_actions.py Show resolved Hide resolved

synapse/storage/databases/main/relations.py Show resolved Hide resolved

erikjohnston and others added 9 commits September 29, 2023 16:10

Correct context

745ad61

Remove ignores for call-sites.

03b0e40

Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…

2c07b1f

…cached

Add ignores at definition sites.

460ed3c

Actually check cachedList.

fbecb56

Fix incorrect generic.

dba8e72

Lint

4f06d85

Abstract shared code.

3875662

ServerAclEvaluator is immutable.

fb4ff5d

clokep requested a review from erikjohnston September 29, 2023 17:26

clokep added 2 commits September 29, 2023 13:33

Update comments.

06ddf65

Lint

9b7ee03

erikjohnston approved these changes Oct 2, 2023

View reviewed changes

scripts-dev/mypy_synapse_plugin.py Show resolved Hide resolved

clokep added 2 commits October 2, 2023 08:14

Update comments and remove unnecessary argument.

e2f599d

Merge remote-tracking branch 'origin/develop' into dmr/mypy-check-at-…

da377cf

…cached

DMRobertson commented Oct 2, 2023

View reviewed changes

Fix duplicate word.

a2956a6

Co-authored-by: David Robertson <davidr@element.io>

clokep enabled auto-merge (squash) October 2, 2023 12:36

clokep merged commit 1026776 into develop Oct 2, 2023

clokep deleted the dmr/mypy-check-at-cached branch October 2, 2023 14:22

		# its not the end of the world.
		# it's not the end of the world.

		"""Asserts that the signature of a method returns a value which can be cached.

		Makes no changes to the provided method signature.

Uh oh!

Conversation

DMRobertson commented Jan 25, 2023 • edited by clokep Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clokep commented Jan 25, 2023

Uh oh!

clokep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DMRobertson commented Feb 8, 2023

Uh oh!

DMRobertson commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DMRobertson commented Sep 20, 2023

Uh oh!

clokep commented Sep 20, 2023

Uh oh!

clokep commented Sep 21, 2023

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikjohnston left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

clokep commented Oct 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DMRobertson commented Jan 25, 2023 •

edited by clokep

Loading

DMRobertson commented Sep 12, 2023 •

edited

Loading