element-hq / element-hq/synapse

Refactor database helper functions so they can be checked by mypy

Open
#14,002 0 comments 0 reactions 0 assignees View on GitHub
A-Database T-Task Z-Dev-Wishlist
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

This issue has been migrated from [#14002](https://github.com/matrix-org/synapse/issues/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.](https://github.com/matrix-org/synapse/pull/13892/commits/be843f7042448ef0e7f6acfbf4b1d4865b7296d1)) 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

```python
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](https://peps.python.org/pep-0612/#id2):

> 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](https://peps.python.org/pep-3102/). But what happens if `func` accepts a keyword parameter called `s` too? Ambiguity and pain.

For an simplified example, see [here](https://mypy-play.net/?mypy=latest&python=3.10&flags=strict&gist=2088c9b15f220bcc6f63d03be01f1e89).

# 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](https://github.com/matrix-org/synapse/blob/29269d9d3f3419a3d92cdd80dae4a37e2d99a395/synapse/storage/databases/main/events_worker.py#L1052-L1060)).

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

```python
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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.