Add typing to membership Replication class methods

[anoadragon453]

This PR grew out of a discussion in #6739, and tries to do two things:

1. Add typing to method arguments
2. Remove unnecessary arguments on those methods

Removing unneeded arguments is fine as _serialize_payload is called as:

synapse/synapse/replication/http/_base.py

Line 169 in 1781bbe

data = await cls._serialize_payload(**kwargs)

and _handle_request called as:

synapse/synapse/replication/http/_base.py

Line 263 in 1781bbe

return self.response_cache.wrap(txn_id, self._handle_request, request, **kwargs)

aka with the arguments request, **kwargs. This doesn't work :)

You'll notice that there are a lot of # type: ignores in here. This is due to the base methods not matching the overloads here. This is necessary to stop mypy complaining, but if there's a better way than just blanket ignoring things I'd love to know!

I imagine there's likely further work needed in other replication classes, however seeing as the discussion above only concerned membership replication classes, that is all this particular PR addresses.

[erikjohnston]

Removing unneeded arguments is fine as _serialize_payload is called as:

I think that's backwards, we're passing in all the arguments but _serialize_payload doesn't have a **kwargs so will fail with "called with too many arguments".

---

(The link to the discussion doesn't work, I think because that PR is huge).

I'm not sure that I'd actually want to remove the "unnecessary" args from _serialize_payload as a) I think it makes sense for the serializiation function to have access to all vars passed in to the invocation, and b) at least I use it to actually see what the args for the client are (without having to cross reference to other places).

---

You'll notice that there are a lot of # type: ignores in here. This is due to the base methods not matching the overloads here. This is necessary to stop mypy complaining, but if there's a better way than just blanket ignoring things I'd love to know!

I think the "correct" way of doing this is to write a mypy plugin, like we have for cache functions. This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

---

(Removing review request as this breaks the http replication APIs)

[anoadragon453]

@erikjohnston Thanks for the detailed explanation, sorry that this PR is a bit of a mess :)

Yep, these changes don't work, and thanks for the justification for keeping the arguments in. I'll rework this PR to just keep the added types, but bring back the removed args.

(The link to the discussion doesn't work, I think because that PR is huge).

Hm, it seems linking to a discussion via the /files endpoint doesn't work - but without that it resolves. Have updated the link.

I think the "correct" way of doing this is to write a mypy plugin, like we have for cache functions. This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

Oh, right! I'll file a followup issue for this then.

That file is really well documented btw.

[erikjohnston]

That file is really well documented btw.

Mainly because none of the functions are documented anywhere but in the source code and aaaaaaaaaargh

[anoadragon453]

@erikjohnston Have brought the args back! I'm currently writing up an issue for the mypy plugin, however:

This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

Isn't the problem that the _serialize_payload and _handle_request functions don't match that of the superclass, ReplicationEndpoint? Or am I missing something here?

[erikjohnston]

This would allow us to correctly set the signature of send_request to be the same as _serialize_payload so we actually get proper type checking.

Isn't the problem that the _serialize_payload and _handle_request functions don't match that of the superclass, ReplicationEndpoint? Or am I missing something here?

That's a problem, yes. The other is that we don't have type hints on the function returned by make_client, this means that even if you add type hints to _serialize_payload mypy won't check that we're passing the correct args when we call it.

[anoadragon453]

@erikjohnston Thanks for the explanation! I've created an issue here which hopefully covers it: #8828