redis / redis-py

Redis Python client
MIT License
12.63k stars 2.52k forks source link

xadd requires a very tricky type for the `fields` parameter. #2917

Open steve-mavens opened 1 year ago

steve-mavens commented 1 year ago

Version: What redis-py and what redis version is the issue happening on?

I'm using redis-py 4.5.4, but I think all recent versions. The Redis version is not relevant, it'll be the same issue for any.

Platform: What platform / version? (For example Python 3.5.1 on Windows 7 / Ubuntu 15.10 / Azure)

At least Windows and whatever *nix my github actions CI is using. Some Ubuntu or other :-)

Description: Description of your issue, stack traces from errors and code that reproduces the issue

I noticed this because of a change in types-redis 4.6.0.5 "Improve typing of xadd/xdel", https://github.com/typeshed-internal/stub_uploader/blob/main/data/changelogs/redis.md#4605-2023-08-22

That says it just copied the type from the redis-py docs, though, at https://redis-py.readthedocs.io/en/stable/commands.html#redis.commands.core.CoreCommands.xadd , so I don't think this is something types-redis can fix unless redis-py changes first. It's also not strictly true that the type is copied, since it's Dict in the docs but Mapping in types-redis. The same issue would apply either way.

The issue is that because the key type of either Dict or Mapping in Python is invariant, the type Dict[Union[bytes, memoryview, str, int, float], Union[bytes, memoryview, str, int, float]] is extremely restrictive. You are asking users to pass in a dictionary in which all of those types: bytes, memoryview, str, int, float are allowed as keys. In particular, therefore, something like a Dict[str, str], which is what mypy's type inference will come up with for the simplest cases, is not a subtype of the required type and hence is not an acceptable input to the function. It in fact works correctly in Python, of course, since at runtime you can always look up a key of any type in any dictionary. But that fact about Python is not represented in Python's built-in Dict or Mapping types, and so the type system thinks it might not work.

This is a known difficulty with Python's types: https://github.com/python/typing/issues/445, so there might not be an elegant solution. I raise this in case anyone has one and can apply it! I've resorted to typing the value I pass in as a Dict[Any, str], which is type-compatible. Any such workaround, though, requires every user to bounce off the issue and address it for themselves.

Perhaps one option would be for the function to accept Dict[Any, Union[bytes, memoryview, str, int, float]]. But I don't know if that would have bad consequences for people who've solved the issue in a different way. here might even be people who want an error if they accidentally pass in a Mapping object that doesn't permit memoryview as keys, and for whom therefore the current type annotation is perfect. But I raise this issue because I'm not one of those people ;-)

More seriously, Dict[Any, Union] would also fail to give a type-checking error if someone passes in, say a Dict[bool, str], with has a key of unsupported type. That's a missed opportunity to catch an error, but catching those errors has the (probably unforeseen) consequence that, adding a new type of supported keys is a breaking change to the type annotation, since it imposes an additional duty on the caller to supply a dictionary in which that new type of key can be looked up. Even though the xadd code doesn't need to do arbitrary lookups. So it's all a trade-off. Dict type restrictions in Python are basically fairly horrible.

Code to reproduce:

import redis

def main() -> None:
    redis_conn = redis.from_url("redis://localhost:6379")
    fields = {'foo': 'bar'}
    redis_conn.xadd('foo', fields)

The specific error from mypy is:

Argument "fields"
to "xadd" of "StreamCommands" has incompatible type "Dict[str, str]"; expected
"Mapping[Union[bytes, memoryview, str, float], Union[bytes, memoryview, str, float]]"

For what it's worth, the error does not happen for the code redis_conn.xadd('foo', {'foo': 'bar'}). I think this is because mypy does different type deduction if it knows what type of dictionary is wanted. If I'm right, it concludes that the specified literal is perfectly fine if seen as a Dict[Union[bytes, memoryview, str, int, float], Union[bytes, memoryview, str, int, float]] that just so happens not to have any keys that aren't strings, but is permitted to have other keys. It only goes wrong when fields has previously had mypy's type deduction applied to it without that context.

steve-mavens commented 1 year ago

Another possible approach, which I haven't thought through but might be worth considering. Permit passing an iterable of key-value pairs for fields, as well as permitting a dictionary. Iterable's type parameter is covariant, not invariant like Dict/Mapping, so it's fine to pass an Iterable[Tuple[str, str]] when asked for an Iterable[Tuple[Union[str, etc], Union[str, etc]]]. You'd need to be careful defining what happens if the iterable contains the same key more than once, though. That is of course not possible when passing a dictionary, so hasn't previously needed to be considered in the docs.

sileht commented 1 year ago

Maybe we can add the CovariantMapping proposed in https://github.com/python/typing/issues/445 until python/typing have something builtin?

KT_co = TypeVar("KT_co", covariant=True)
VT_co = TypeVar("VT_co", covariant=True)

class CovariantMapping(Protocol[KT_co, VT_co]):
    def __eq__(self, other: object) -> bool: ...
    def __ne__(self, other: object) -> bool: ...
    def keys(self) -> KeysView[KT_co]: ...
    def items(self) -> ItemsView[KT_co, VT_co]: ...
    def values(self) -> ValuesView[VT_co]: ...