[tracking issue] Validation through Invariance

[ShadowJonathan]

Description

This is a tracking issue for PRs and issues related to invariance in the synapse codebase, to ensure extra stability and reduce undefined behaviour.

The plan is two-fold:

1. Reduce dynamic behaviour to statically-verifiable behaviour, where behaviour is explicitly notated instead of implicitly processed.
2. Introduce static typing annotations everywhere, to make it possible to check for undefined or variant behaviour.

Small PRs will be introduced cleaning these up iteratively.

PRs

To-Dos / Spotted Issues

• Convert synapse.server.HomeServer.__init__() from dynamic to static behaviour.
  (e.g. by adding __init__() signatures to subclasses, then disallowing receiving **kwargs)

[ShadowJonathan]

One thing I still want the core team's opinion on: What is the best way I could properly and idiomatically change some functions that build and return data? (so that passing around this data is still valid by type)

What I mean by this; How should I change e.g. synapse.handlers.stats.StatsHandler._handle_deltas() to stop building on a plain dict (room_stats_delta)? Maybe use @attr.s classes?

@attr.s(slots=True)
class RoomStatsDelta:
  current_state_events = attr.ib(default=0)
  joined_members = attr.ib(default=0)
  invited_members = attr.ib(default=0)
  left_members = attr.ib(default=0)
  banned_members = attr.ib(default=0)

This allows the program pass around objects which are assured to have those values. The mypy linter is happy to follow these conventions implicitly, and then point out any violations if/when they'd occur.

Later on, when sending this to the user, attr.asdict() can be used to transparently sterilize any (nested) attr.s class to a final dict. This upholds invariance of this data-building throughout the program until it is time to pass it off to the user.

(This could also (maybe) be a very good candidate for #8361)

[clokep]

Later on, when sending this to the user, attr.asdict() can be used to transparently sterilize any (nested) attr.s class to a final dict.

For this particular example I'm not sure if deltas are ever sent as JSON or if it is completely an internal data-structure. Note that in this case it seems like the room_stats_delta is actually a Counter instance, not just a raw dictionary.

[ShadowJonathan]

Note that in this case it seems like the room_stats_delta is actually a Counter instance, not just a raw dictionary.

I do note that, but I wanted to pick an easy example for the moment. It seems like I cant subclass Counter here with an attr.s class (and still retain attribute assignment + features from Counter), and i'd have to trace where abilities of Counter are actually used, and/or convert this class somewhere along the way if that is true, I'd have to look into it.

Still, eliminating "blind key-getting/setting" is the point here; to make sure the static linter can follow what's going on.

Edit: seems like these eventually end up under synapse.storage.databases.main.stats.StatsStore._updatestats_delta_txn(), as stats_type ("room" or "user", though that is assumed) and stats_id, and eventually in ._upsert_with_additive_relatives_txn()

Edit 2: and it looks like keys added to room_stats_delta eventually are used to reference database columns, so locking these over the course of handing it over to the database would be a good idea, as then no extra keys would be added that then throw a wrench in stats-inserting.

[ShadowJonathan]

I think #8351 covers most of this, the other part (making every class statically verifiable) should be hit at some point as well, but i'm going to close this as it's not exactly doing much good being open onto itself.