`_do_execute` (database function) could be more precisely typed with better support for `ParamSpec`

[reivilibre]

Sprouted from #12311.

Once Mypy supports Concatenate, it could be written as

     def _do_execute(
         self,
         func: Callable[Concatenate[str, P], R],
         sql: str,
         *args: P.args,
         **kwargs: P.kwargs,
     ) -> R:

so long as we also pass kwargs.

[clokep]

Once Mypy supports Concatenate

Which seems to be python/mypy#11847, according to @squahtx

[squahtx]

related: #11711

[squahtx]

Blocked on #12585

[DMRobertson]

I've just given this a quick try.

I was a bit surprised that we need to include **kwargs: P.kwargs in the outer function definition, but that does seem to be consistent with PEP 612:

A function of type Callable[P, R] can be called with (*args, **kwargs) if and only if args has the type P.args and kwargs has the type P.kwargs, and that those types both originated from the same function declaration.

I was then surprised by the next error from looking at args[0]:

error: Value of type "P.args" is not indexable [index]

synapse/synapse/storage/database.py

Lines 341 to 352 in a50fb41

	def _do_execute(self, func: Callable[..., R], sql: str, *args: Any) -> R:
	sql = self._make_sql_one_line(sql)
	# TODO(paul): Maybe use 'info' and 'debug' for values?
	sql_logger.debug("[SQL] {%s} %s", self.name, sql)
	sql = self.database_engine.convert_param_style(sql)
	if args:
	try:
	sql_logger.debug("[SQL values] {%s} %r", self.name, args[0])
	except Exception:

This is again consistent with PEP 612:

Inside the function, args has the type P.args, not Tuple[P.args, ...] as would be with a normal annotation (and likewise with the **kwargs)

Given that this is already caught at runtime, I'd be inclined to type-ignore this.

[DMRobertson]

Additionally: This doesn't actually give us a huge amount of type safety. Using a ParamSpec allows us to ensure that the args (and now kwargs) we pass to _do_ execute are suitable for func. In order for this to actually enforce anything, we need meaningful annotations where func is defined and of the arguments where they're passed.

There are four call sites at present, and none of them really have those meaningful annotations AFAICS.

execute and executemany

synapse/synapse/storage/database.py

Lines 332 to 336 in a50fb41

	def execute(self, sql: str, *args: Any) -> None:
	self._do_execute(self.txn.execute, sql, *args)
	def executemany(self, sql: str, *args: Any) -> None:
	self._do_execute(self.txn.executemany, sql, *args)

*args: Any means that we don't get checking here, even though self.txn.execute(many) have annotations.

execute_batch

synapse/synapse/storage/database.py

Lines 299 to 310 in a50fb41

	def execute_batch(self, sql: str, args: Iterable[Iterable[Any]]) -> None:
	"""Similar to `executemany`, except `txn.rowcount` will not be correct
	afterwards.
	More efficient than `executemany` on PostgreSQL
	"""
	if isinstance(self.database_engine, PostgresEngine):
	from psycopg2.extras import execute_batch
	self._do_execute(
	lambda the_sql: execute_batch(self.txn, the_sql, args), sql

Here the lambda expression has no annotations. Proof by example:

$ cat temp.py
def accepts_string(s: str): print(s)
f = lambda x: accepts_string(x)
reveal_type(f)

2022-05-07 21:38:24 ✔  $ mypy temp.py
temp.py:3: note: Revealed type is "def (x: Any) -> Any"
Success: no issues found in 1 source file

It's also worth noting that the stubs for execute_batch aren't very strict either:

# from reveal_type(execute_batch)
Revealed type is "def (cur: Any, sql: Any, argslist: Any, page_size: builtins.int =)"

This is using the locked version of types-psycopg2, but it's no stricter in the latest release (2.9.13).

execute_values

Very similar to execute_batch.

synapse/synapse/storage/database.py

Lines 315 to 330 in a50fb41

	def execute_values(
	self, sql: str, values: Iterable[Iterable[Any]], fetch: bool = True
	) -> List[Tuple]:
	"""Corresponds to psycopg2.extras.execute_values. Only available when
	using postgres.
	The `fetch` parameter must be set to False if the query does not return
	rows (e.g. INSERTs).
	"""
	assert isinstance(self.database_engine, PostgresEngine)
	from psycopg2.extras import execute_values
	return self._do_execute(
	lambda the_sql: execute_values(self.txn, the_sql, values, fetch=fetch),
	sql,
	)

The underlying postgres function has a few more annotations, but not any that would give us more checking.

"def (cur: Any, sql: Any, argslist: Any, template: Union[Any, None] =, page_size: builtins.int =, fetch: builtins.bool =) -> Any

[squahtx]

I was then surprised by the next error from looking at args[0]:

error: Value of type "P.args" is not indexable [index]

The next version of mypy will include a fix for this: python/mypy#12668

[DMRobertson]

The next version of mypy will include a fix for this: python/mypy#12668

Good to know. I found python/mypy#11826 by searching, but assumed that the behaviour there was intentional.