Correct check_username_for_spam annotations and docs

[DMRobertson]

Fixes #12245.

I'm not completely sure how I feel about using a TypedDict here, but

• using a namedtuple or attrs struct would mean the diff had to update a bunch of tests
• we don't do very much with the user profile data; deserialising to a struct only to reseralise to a dict seemed like overkill
• more compellingly, the spam checker callback already expects a dictionary, so let's keep giving them a dictionary

Not convinced the documentation change is super easy to read. All thoughts are welcome.

[squahtx]

Looks good!
Left some questions

[squahtx]

It's worth noting that Mappings aren't necessarily JSON-serialisable, unless they're really dicts or frozendicts. Essentially when we use JsonMapping, we're pinky promising that it's really a (frozen)dict.

The same kind of thing applies to JsonSerializable below I guess.

[DMRobertson]

I wasn't 100% sold on this either. I wanted to see if there was a way to make more use of TypedDicts without having to cast those back to JsonDicts everywhere. Maybe I could just make the return type of this function SearchResult? That feels a bit off though---it's exposing a type from storage.databases.main in rest.client.

synapse/synapse/rest/client/user_directory.py

Line 41 in 7fe7b42

async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonMapping]:

[squahtx]

I do think there is a case for having a JsonMapping type, to signal that functions aren't allowed to modify the contents of a dict. Especially around caches.

I'm not opposed to making the return type some sort of TypedDict. I think we really ought to have a file somewhere full of TypedDict definitions that come from the spec, since that's the source of truth for the return type here (and not the database layer).

In any case I'm happy with the JsonMapping as is.

[DMRobertson]

I do think there is a case for having a JsonMapping type, to signal that functions aren't allowed to modify the contents of a dict. Especially around caches.

I broadly sympathise, but I don't think that'd be fool proof: e.g. given d: JsonMapping = {"a": [1,2,3]} I think d["a"].append(4) would still type check?

I think we really ought to have a file somewhere full of TypedDict definitions that come from the spec, since that's the source of truth for the return type here (and not the database layer).

I like the sound of this; e.g. synapse.types.spec.UserDirectorySearchResult. I guess the dream would be to automatically generate such a file from the openapi definition in the spec.

[squahtx]

I do think there is a case for having a JsonMapping type, to signal that functions aren't allowed to modify the contents of a dict. Especially around caches.

I broadly sympathise, but I don't think that'd be fool proof: e.g. given d: JsonMapping = {"a": [1,2,3]} I think d["a"].append(4) would still type check?

True! Unless we manage to define JsonMapping = Mapping[str, ImmutableJsonSerializable] recursively, which mypy won't let us do. (grumble grumble this would be possible in typescript)

[DMRobertson]

Looks like I need to import TypedDict from typing_extensions on older Pythons.