-
Notifications
You must be signed in to change notification settings - Fork 392
Sliding Sync: Make PerConnectionState
immutable
#17600
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.
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
Changes from all commits
e2ade85
5b77f4a
7087c7c
e34d634
87d5336
5fe6466
174e1ad
088c6b2
24b1e28
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Make the sliding sync `PerConnectionState` class immutable. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ | |
NoneType, | ||
TupleType, | ||
TypeAliasType, | ||
TypeVarType, | ||
UninhabitedType, | ||
UnionType, | ||
) | ||
|
@@ -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. | ||
|
@@ -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) | ||
|
@@ -318,6 +320,9 @@ def is_cacheable( | |
else: | ||
return False, "non-frozen attrs class" | ||
|
||
elif rt.type.is_enum: | ||
# We assume Enum values are immutable | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I could figure out how I would, but I couldn't :( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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 | ||
|
Uh oh!
There was an error while loading. Please reload this page.