RedisArray::keys (and many others ) return type is wrong

[InvisibleSmiley]

Bug report

I originally filed #9747 but that was closed automatically, and there's more to it than the fact that "false" is missing.
I'd like to focus this issue on the specific RedisArray::keys issue rather than the "false missing everywhere" issue.

Actually, RedisArray::keys returns a different array structure than Redis::keys.
Quoting from https://github.com/phpredis/phpredis/blob/develop/arrays.md#limitations:

Running KEYS() on a RedisArray object will execute the command on each node and return an associative array of keys, indexed by host name.

Right now, PHPStan seems to reuse the Redis::keys definition for RedisArray::keys, which currently reads:
array<int,string>
However RedisArray::keys should really be:
array<string, array<int, string>|false
which I verified locally (you'll receive false if no redis server is running, and the multidimensional array structure if everything is set up correctly).

The main question for me is how this should be solved in PHPStan. Options:

• fix only the Redis::keys definition to be broad enough (e.g. PhpStorm and Psalm stubs simply use array|false|Redis)
• have separate, detailed definitions for Redis::keys and RedisArray::keys

Code snippet that reproduces the problem

No response

Expected output

• no issue if checking the result of RedisArray::keys for false
• no issue if accessing the result of RedisArray::keys correctly (string key, array value)

Did PHPStan help you today? Did it make you happy in any way?

No response

[mergeable]

This bug report is missing a link to reproduction at phpstan.org/try.

It will most likely be closed after manual review.

[InvisibleSmiley]

https://phpstan.org/r/f9c6932c-a7a0-4ba3-8942-c8995a46c02d

[InvisibleSmiley]

FTR I don't know how PHPStan handles the relationship between RedisArray and Redis internally.
If PHPStan assumed that RedisArray extends Redis then the different array iterable types would violate covariance.
In reality though, RedisArray and Redis are separate, which is why e.g. the PhpStorm stubs use @mixin Redis for RedisArray.

<?php
$x = new RedisArray(['localhost']);
$y = $x instanceof RedisArray;
$z = $x instanceof Redis;
var_dump($x, $y, $z);

will give:

class RedisArray#1 (0) {
}
bool(true)
bool(false)

[phpstan-bot]

@InvisibleSmiley After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
- 5: Strict comparison using === between array<int, string> and false will always evaluate to false.
  9: Call to function assert() with false will always evaluate to false.
  9: Call to function is_string() with int will always evaluate to false.
 10: Argument of an invalid type string supplied for foreach, only iterables are supported.

Full report

Line	Error
9	Call to function assert() with false will always evaluate to false.
9	Call to function is_string() with int will always evaluate to false.
10	Argument of an invalid type string supplied for foreach, only iterables are supported.

[ondrejmirtes]

/cc @Vaalyn this could be improved too :)

[phpstan-bot]

@InvisibleSmiley After the latest push in 2.1.x, PHPStan now reports different result with your code snippet:

@@ @@
- 5: Strict comparison using === between array<int, string> and false will always evaluate to false.
- 9: Call to function assert() with false will always evaluate to false.
- 9: Call to function is_string() with int will always evaluate to false.
-10: Argument of an invalid type string supplied for foreach, only iterables are supported.
+ 8: Argument of an invalid type array|true supplied for foreach, only iterables are supported.

Full report

Line	Error
8	`Argument of an invalid type array

[VincentLanglet]

This has been solved on PHPStorm side with JetBrains/phpstorm-stubs#1772

Since there is not a lot of docs and I don't use redis a lot, I only followed https://github.com/phpredis/phpredis/blob/develop/redis_array.stub.php

You have now no error with
https://phpstan.org/r/ec51f3a7-f190-4e3a-960b-3d20907bdc0f
@InvisibleSmiley

Should it be closed then ?

[InvisibleSmiley]

The result of RedisArray::keys is now inferred as array|bool. So at least it's not completely wrong anymore, and in line with what PhpStorm has (or will have).

There's still a lot of room for improvement, e.g. excluding true and defining the iterable key/value type of the array.

So as is I'd say the bug is fixed and what's left is more like an enhancement.

Overview of the current situation:

• https://github.com/phpredis/phpredis/blob/develop/redis.stub.php is detailed (includes iterable value types)
• https://github.com/phpredis/phpredis/blob/develop/redis_array.stub.php (and hence PhpStorm and PHPStan) lacks detail
• https://github.com/vimeo/psalm/blob/master/stubs/extensions/redis.phpstub is in between (no iterable value types)
• Psalm infers the result of RedisArray::keys as array<int, string> (even on master), which is completely wrong. Didn't check where that definition is coming from.

Assuming the upstream stubs/definitions are correct, Redis can only be a result of Redis::keys, not of RedisArray::keys. Didn't check that either.

[VincentLanglet]

• phpredis/phpredis@develop/redis_array.stub.php (and hence PhpStorm and PHPStan) lacks detail

If you know some details @InvisibleSmiley I would say you can either list them and I'll try a PR or (even better) doing yourself the PR on redis_array_stub.php.

Then, a PR will be needed to https://github.com/JetBrains/phpstorm-stubs (linking the redis_array_stub)

And PHPStan will be automatically updated (because, without override, it relies on PHPStorm).

[InvisibleSmiley]

I don't know anything for certain, I'm not a phpredis contributor, just a user.

I filed phpredis/phpredis#2692 to account for the fact that any improvement should start at the source.

[phpstan-bot]

Closed via merging phpstan/phpstan-src#5743