[redis] pipeline return types (again)

[brianmaissy]

In #4989 the return types of the pipeline methods were changed so that they would all return pipeline instances, but that's not always correct. If the pipeline is watching keys and isn't in the explicit transaction (after the MULTI) then it actually executes the command and returns the result.

@chdsbd @srittau

[Akuli]

Detecting this at type check time doesn't seem to be possible, as it depends on self.watching and self.explicit_transaction, both of which can change after instantiating: https://github.com/andymccurdy/redis-py/blob/9ed5cd7808789f791fdc7ee368bd268307ac9847/redis/client.py#L1587-L1591

This would be yet another good use case for AnyOf (python/typing#566 ), but that doesn't exist yet.

One workaround would be to define the return type as Pipeline | Any. Then you will get autocompletions and type checking for Pipeline methods, so this works as expected:

pipe = pipe.set('foo', 'bar').incr('baz').decrrr('bang')  # error: no method 'decrrr'

But you can also do this:

foo = pipe_that_does_not_return_pipes.some_method()
assert not isinstance(foo, Pipeline)
# now foo has type Any

[PeterJCLaw]

While manual setting of the watches isn't easy to track, in theory it is possible to handle the cases where the Redis.transaction method is used to generate the pipeline since it takes the watches as arguments upfront.

Its signatures are something like this:

    def transaction(self, func: Callable[[LazyPipeline], Any], **kwargs: Any): ...

    def transaction(self, func: Callable[[ImmediatePipeline], Any], *watches, **kwargs: Any): ...

However I'm not sure how overload detection will work given that we want it to only pick the latter when *watches is non-empty.

(transaction also has a dynamic return type based on the value_from_callable kwarg, I'm ignoring that here)

[sobolevn]

One more thing: in redis.asyncio.Pipeline all pipeline methods are sync. But current stubs think that they are async.

For example:

current_rate, _ = await pipeline.incr(cache_key).expire(  # type: ignore
    cache_key,
    self._rate_spec.seconds,
    nx=True,
).execute()

I have to use type: ignore here, because stubs define that pipeline.incr(cache_key) returns a Coroutine, which does not have .expire.

In real life it works just fine: https://github.com/wemake-services/asyncio-redis-rate-limit/blob/a47c75d01bdd9d8b95cdc65c95bf0677261d863e/asyncio_redis_rate_limit/__init__.py#L104-L109

[mjpieters]

One more thing: in redis.asyncio.Pipeline all pipeline methods are sync. But current stubs think that they are async.

This was fixed in #8325.

[srittau]

The redis stubs are scheduled for removal (see #10592) and only support redis 4. While we still accept contributions, I'm closing this issue now for housekeeping purposes. Improvements to the types of redis 5 should be directed to the redis-py repository.