phpredis stubs wrong again?

[kkmuffme]

https://psalm.dev/r/c49242e965

I think since 4.2.0, phpredis stubs seem to be completely wrong and reporting lots of false positive issues
(had that same issue some time ago, but was working fine until recently)

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/c49242e965

<?php

$redis = new Redis();

$redis->zPopMin( 'hello', 10 );

$redis->zAdd( 'hello', array( 'NX' ), 100, 'world' );

$foo = $redis->mGet( 'foo' );

if ( !is_array( $foo ) ) {
	echo "bar";
}

Psalm output (using commit c5722dd):

ERROR: TooManyArguments - 5:9 - Too many arguments for method Redis::zpopmin - saw 2

ERROR: InvalidArgument - 7:24 - Argument 2 of Redis::zAdd expects float, array{'NX'} provided

ERROR: InvalidScalarArgument - 7:39 - Argument 3 of Redis::zAdd expects string, 100 provided

ERROR: InvalidScalarArgument - 7:44 - Argument 4 of Redis::zAdd expects float, 'world' provided

ERROR: InvalidArgument - 9:22 - Argument 1 of Redis::mget expects array<array-key, string>, 'foo' provided

ERROR: TypeDoesNotContainType - 11:7 - Type array<array-key, mixed> for $foo is always !array<array-key, mixed>

[orklah]

4.20 shouldn't have changed on this matter, however, psalm.dev runs on dev-master which contains this change: #7107

And because Redis is neither on composer.json nor in the config for psalm.dev, stubs are simply not loaded.

But again, it should not be an issue on an install of 4.20.0. Are you sure you have this issue?

[kkmuffme]

Yes, I'm sure:

vimeo/psalm 4.20.0

And I get the exact same errors as on psalm.dev for the above code.
Didn't get these errors before.

[AndrolGenhald]

Should have realized it when I read this earlier, it can't be #7107 because redis isn't in the supported extensions list, we don't have a stubfile for it.

[orklah]

When I look at redis official stubs, I see:
zAdd expects an int as second param, you sent an array
https://github.com/phpredis/phpredis/blob/552f41a44f0687c221e5d4adf9ee387479bb78a1/redis.stub.php#L488

zpopmin was apparently added a few months ago and I think we never had definitions for it:
https://github.com/phpredis/phpredis/blame/552f41a44f0687c221e5d4adf9ee387479bb78a1/redis.stub.php#L502

mget expects an array, you passed a string:
https://github.com/phpredis/phpredis/blob/552f41a44f0687c221e5d4adf9ee387479bb78a1/redis.stub.php#L267

mget indeed returns something else than an array, in their stub, contrary to what we have in Psalm, but we have this wrong (or old?) definition since 2016

I think you may have just baselined all these errors and they somehow get out of the baseline when upgrading to another version.

You're welcome to fix the definitions in Psalm :)

[AndrolGenhald]

Or, if you want to be a guinea pig, you could try following the instructions here to help Psalm support the extension with the new design, which will allow analyzing the extension even when it's not present when running Psalm. Fair warning though, it's not merged yet and the process may end up changing a bit.

[kkmuffme]

@orklah phpredis stubs you reference here https://github.com/phpredis/phpredis/blob/552f41a44f0687c221e5d4adf9ee387479bb78a1/redis.stub.php#L488 are completely wrong unfortunately.
Not only are they missing half of the possible params (see the docs, as they are mostly correct), but their return types are almost all wrong (since almost all phpredis functions return false if redis has an error, see phpredis/phpredis#1921)

Anyway:
I have phpredis installed on my server. I assume psalm uses that? (and the referenced stubs file)

What do I need to fix - phpredis stubs file https://github.com/phpredis/phpredis/blob/552f41a44f0687c221e5d4adf9ee387479bb78a1/redis.stub.php or something in psalm to get the correct stubs for phpredis?

[orklah]

I don't think Psalm uses Reflection if it has stubs or a callmap.

So what would need to be fixed - for now - would be the callmap files. We need to have the correct definitions on those files: https://github.com/vimeo/psalm/tree/4.x/dictionaries

Alternatively, if you think it would be easier to provide a complete stub instead, you could add it in here: https://github.com/vimeo/psalm/tree/4.x/stubs

Our goal for future versions is to standardize stubs for extensions by generating them through reflection and shipping a versionned stub for each PHP version, which we hope should resolve issues in the long term.

Ideally, stubs in the redis repo should be fixed to reflect the documentation of the last version.

[kkmuffme]

Ideally, stubs in the redis repo should be fixed to reflect the documentation of the last version.

I will fix the stubs and docs for phpredis directly

Do you want me to add a PR with phpredis stubs in https://github.com/vimeo/psalm/tree/4.x/stubs too? (would be more effort to maintain in the long term, but could be shipped faster, since it will take a long time until phpredis will merge the PR and ship it)

[orklah]

we'll need a copy of the stubs anyway, we don't retrieve stubs automatically, so yeah, if you have updated stubs, feel free to PR them here and we'll merge that