Refactor database helper functions so they can be checked by mypy

[matrixbot]

This issue has been migrated from #14002.

---

This issue contains notes on a type annotation problem I spent some time thinking about. Comments are welcome.

Background

In #13892 I made a mistake where I tried invoke a function f using runInteraction but gave it the arguments for another function g with a different signature. (This is the diff that corrected it.) I was surprised that mypy didn't catch it, since we've got better coverage of the source code and we make more use of mypy.

Problem

Annotating some of these DB helper functions with ParamSpec doesn't work. E.g. given

https://github.com/matrix-org/synapse/blob/ad4c14e4b0c44d6a8ee42e760d7e1fe1755559a2/synapse/storage/database.py#L883-L890

and the place where we use func

https://github.com/matrix-org/synapse/blob/ad4c14e4b0c44d6a8ee42e760d7e1fe1755559a2/synapse/storage/database.py#L966-L969

the annotations to use would be

func: Callable[Concatenate[LoggingDatabaseConnection, P], R]
args: P.args
kwargs: P.kwargs

But these don't work:

synapse/storage/database.py:887: error: Name "P.args" is not defined  [name-defined]
synapse/storage/database.py:890: error: Name "P.kwargs" is not defined  [name-defined]

Mypy's error message is crap here. I think what's going on here is buried in PEP 612:

Note that this also why we have to reject signatures of the form (*args: P.args, s: str, **kwargs: P.kwargs)

There are some examples---I find it tough to follow---but in short, I think you can only have *P.args and **P.kwargs directly adjacent in a function signature. Anything after *args, like s above, has to be a keyword-only argument. But what happens if func accepts a keyword parameter called s too? Ambiguity and pain.

For an simplified example, see here.

Proposal

If we want to have mypy check these helper functions, I think their last three arguments need to be func, *args and **kwargs; everything else needs to be a mandatory(?) position parameter before func. The downside of doing this is that every call site needs to explicitly list out a bunch of mandatory arguments which weren't mandatory before.

new_transaction is an example of such a function taking its arguments in this way:

https://github.com/matrix-org/synapse/blob/ad4c14e4b0c44d6a8ee42e760d7e1fe1755559a2/synapse/storage/database.py#L611-L621

(though its call sites are quite verbose).

One way we could avoid the verbosity is to provide "simple" and "advanced" versions of a function. Take runInteraction for example.

https://github.com/matrix-org/synapse/blob/ad4c14e4b0c44d6a8ee42e760d7e1fe1755559a2/synapse/storage/database.py#L801-L809

The majority of its call sites don't specify db_autocommit or isolation_level values. Maybe we could split the function up into

    async def run_interaction_simple(
        self,
        desc: str,
        func: Callable[..., R],
        *args: Any,
        **kwargs: Any,
    ) -> R:
        ...

    async def run_interaction_advanced(
        self,
        desc: str,
        db_autocommit: bool,
        isolation_level: Optional[int],
        func: Callable[..., R],
        *args: Any,
        **kwargs: Any,
    ) -> R:

...

Both of these are in a form that I think mypy can meaningfully process, and the former avoids us repeatedly passing in False, None at the majority of call sites.