Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sliding Sync: Make PerConnectionState immutable #17600

Merged
merged 9 commits into from
Aug 29, 2024
Merged

Sliding Sync: Make PerConnectionState immutable #17600

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e2ade85
Move sliding sync types
erikjohnston Aug 21, 2024
5b77f4a
Update mypy plugin to handle enums and typevars
erikjohnston Aug 22, 2024
7087c7c
Make RoomSyncConfig immutable
erikjohnston Aug 22, 2024
e34d634
Make PerConnectionState immutable
erikjohnston Aug 22, 2024
87d5336
Newsfile
erikjohnston Aug 22, 2024
5fe6466
Update synapse/types/handlers/sliding_sync.py
erikjohnston Aug 27, 2024
174e1ad
Merge remote-tracking branch 'origin/develop' into erikj/ss_immutable
erikjohnston Aug 29, 2024
088c6b2
Remove deep_copy
erikjohnston Aug 29, 2024
24b1e28
Merge remote-tracking branch 'origin/develop' into erikj/ss_immutable
erikjohnston Aug 29, 2024
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
19 changes: 18 additions & 1 deletion scripts-dev/mypy_synapse_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
NoneType,
TupleType,
TypeAliasType,
TypeVarType,
UninhabitedType,
UnionType,
)
Expand Down Expand Up @@ -233,6 +234,7 @@ def check_is_cacheable(
"synapse.synapse_rust.push.FilteredPushRules",
# This is technically not immutable, but close enough.
"signedjson.types.VerifyKey",
"synapse.types.StrCollection",
}

# Immutable containers only if the values are also immutable.
Expand Down Expand Up @@ -298,7 +300,7 @@ def is_cacheable(

elif rt.type.fullname in MUTABLE_CONTAINER_TYPES:
# Mutable containers are mutable regardless of their underlying type.
return False, None
return False, f"container {rt.type.fullname} is mutable"

elif "attrs" in rt.type.metadata:
# attrs classes are only cachable iff it is frozen (immutable itself)
Expand All @@ -318,6 +320,9 @@ def is_cacheable(
else:
return False, "non-frozen attrs class"

elif rt.type.is_enum:
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
# We assume Enum values are immutable
Copy link
Contributor

Choose a reason for hiding this comment

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

We could check if the enum member values are all immutable to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I could figure out how I would, but I couldn't :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work?

        elif rt.type.is_enum:
            for enum_member_name in rt.get_enum_values():
                enum_member_type = rt.type.names[enum_member_name].type
                # Enum members should have types
                assert enum_member_type is not None
                ok, note = is_cacheable(enum_member_type, signature, verbose)
                if not ok:
                    return (
                        False,
                        f"enum member {enum_member_name} value of type {enum_member_type} not cacheable: {note}",
                    )

            # All Enum member values are immutable
            return True, None

It hasn't been tested as this code doesn't seem to matter anymore in this PR? I commented this whole enum logic branch out and mypy still passes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had no idea that get enum values was a thing. I'll have a try and see if it works! Very good catch either way

Yeah, turns out this matters for the next PR where we add a cache on this value. Not sure why I added this to this pr.

return True, None
else:
# Ensure we fail for unknown types, these generally means that the
# above code is not complete.
Expand All @@ -326,6 +331,18 @@ def is_cacheable(
f"Don't know how to handle {rt.type.fullname} return type instance",
)

elif isinstance(rt, TypeVarType):
# We consider TypeVars immutable if they are bound to a set of immutable
# types.
if rt.values:
for value in rt.values:
ok, note = is_cacheable(value, signature, verbose)
if not ok:
return False, f"TypeVar bound not cacheable {value}"
return True, None

return False, "TypeVar is unbound"

elif isinstance(rt, NoneType):
# None is cachable.
return True, None
Expand Down