Redis setOption() emit InvalidScalarArgument

[mrVrAlex]

Hi
I found (look subject):
https://psalm.dev/r/a002786368
Because of stub: https://github.com/vimeo/psalm/blob/4.x/stubs/phpredis.phpstub#L390

Why first param it is string?
If look to source code of phpredis https://github.com/phpredis/phpredis/blob/develop/redis_arginfo.h#L652 this is should be integer

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/a002786368

<?php

$redis = new Redis();
$redis->setOption(Redis::OPT_SERIALIZER, Redis::SERIALIZER_PHP);

Psalm output (using commit 997bded):

ERROR: InvalidScalarArgument - 4:19 - Argument 1 of Redis::setOption expects string, 1 provided

[mrVrAlex]

Also found https://psalm.dev/r/0244c2dfb5
(misses constants in stubs?)

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/0244c2dfb5

<?php

    function getSerializerValue() : int
    {
        if (defined('Redis::SERIALIZER_IGBINARY') && extension_loaded('igbinary')) {
            return Redis::SERIALIZER_IGBINARY;
        }

        return Redis::SERIALIZER_PHP;
    }

Psalm output (using commit 997bded):

INFO: MixedReturnStatement - 6:20 - Could not infer a return type

INFO: MixedInferredReturnType - 3:37 - Could not verify return type 'int' for getSerializerValue

[orklah]

@kkmuffme could there be an error in the stub introduced in #7614?

[simitter]

Based on the stubs many methods can now return the class Redis itself which seems wrong?

Example: exists

https://psalm.dev/r/ab6bfb4ce0

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/ab6bfb4ce0

<?php

$redis = new Redis();

$exists = $redis->exists('test');

/** @psalm-trace $exists */

Psalm output (using commit 997bded):

INFO: Trace - 7:28 - $exists: Redis|bool|int

INFO: UnusedVariable - 5:1 - $exists is never referenced or the value is not used

[orklah]

I don't personally use Redis, but this looks to be messy...

Just for reference:
An issue was created here: #7581 because psalm's definitions seemed wrong on some methods. Apparently, they were also wrong on the stub provided by Redis repo itself (#7581 (comment)). A new stub was created based on an unmerged PR created by a maintainer of the repo here: phpredis/phpredis#2015

But apparently it would be wrong too?

I think this would really benefit from having someone comparing redis documentation with our current stubs and making fixes when necessary. If possible, a check in source code or some reproducable snippet could be used to update both their documentation and Psalm stub if there are still errors

Note: we hope to provide generated stubs from reflection on Psalm 5, however, it would be great to have a correct version of it in Psalm 4 so we could compare the two and make sure the generated one is not faulty

[kkmuffme]

@orklah the stubs are by the maintainer of phpredis, so they're definitely correct - however the stubs are still a WIP phpredis/phpredis#2015 as you say
Those stubs pass phpredis' own tests (see the PR title), so I assume they're correct.

The problem with phpredis is that
a) the documentation does not match the code
b) the stubs (not psalm's, I mean the one currently provided by phpredis main branch) do not match the documentation AND the actual code
c) their release cycle is forever long and random, so god knows when those WIP stubs will actually be correct and merged.

Currently only b) is addressed in that PR (but apparently not correct in all cases, even though the tests pass), but a) will/should also be addressed there.
So now we're stuck with c)

If we actually want to get correct stubs, we'd need to feed back any feedback to that PR to get it corrected there (and also directly fix it in the psalm stubs), so within (hopefully) a couple months we will have correct stubs.

===

@simitter yes I know it's really weird. e.g.
public function expire(string $key, int $timeout): Redis|bool;

No idea, how this returns Redis. Can you fix those and please also feed back your changes to phpredis/phpredis#2015 so we dont get forked stubs and reduce amount of maintenance necessary in the long run

===

@shandyDev yes, constants are missing in the stubs too. Please put them together and comment on phpredis/phpredis#2015 so we get the included there (and thereby also in psalm)

[AndrolGenhald]

For reference, I added the generated stubs here. Unfortunately Reflection doesn't support return types for extensions, so I'm not sure how useful it'll be.

[orklah]

@simitter see phpredis/phpredis#2015 (comment). The fact that it returns a Redis instance is expected in some cases, depending on the context of the usage. Stub is correct on that.

However, this is not a standard behaviour, so it might need a special handling if you want to have the exact returned type.

I'm not personally interested in improving the inference on that more than what's described on the stub. If you want, I could guide you in the creation of a plugin for that if you're up for it.

@shandyDev Thanks for your feedback. It will be fixed upstream. Feel free to send a PR to fix this in Psalm!

@AndrolGenhald That's unfortunate. It creates a good skeleton for building on it, but it means starting from scratch on each new php version if we want updated stubs...

[AndrolGenhald]

That's unfortunate. It creates a good skeleton for building on it, but it means starting from scratch on each new php version if we want updated stubs...

That's one of several reasons I decided to improve @since and only use generated stubs for diffing to see what's changed when I did #7641.

[simitter]

The fact that it returns a Redis instance is expected in some cases, depending on the context of the usage. Stub is correct on that.

However, this is not a standard behaviour, so it might need a special handling if you want to have the exact returned type.

I'm not personally interested in improving the inference on that more than what's described on the stub. If you want, I could guide you in the creation of a plugin for that if you're up for it.

Thank you for clearing that up. The stubs make sense now.
I'll suppress it in CI for now and may look to create a plugin at a later date.

[ADmad]

I'm not personally interested in improving the inference on that more than what's described on the stub. If you want, I could guide you in the creation of a plugin for that if you're up for it.

@orklah I totally understand you not wanting to spend time on this to deal with Redis' wacky API. But in that case adding this stub doesn't bring much value when we have to resort to suppressing the errors caused by the Redis| return type. It would have been better of just wait for someone to develop a plugin for it.

[ADmad]

IMHO this stub should be just reverted.

[orklah]

I don't agree. If someone is using Redis new mode in order to retrieve a Redis instance, stubs would be completely wrong for them leading to a lot of confusion.

I prefer have imprecise but correct stubs than stubs that are precise but only for a some cases

That aside, the stub fixes a lot of other errors so it won't just be reverted

[ADmad]

https://www.youtube.com/watch?v=hou0lU8WMgo

The stub is indeed technically correct, the best kind of correct 🙂

[kkmuffme]

Updated the stubs to latest version to include the fix for setOption #7752

In general the phpredis stubs are amazing now - I could remove 100s of unused psalm-suppress thanks to the new stubs.

[orklah]

I'll close this, the original issue is now fixed and we use official Redis stubs. If someone is interested in building a Redis plugin (or a type provider in Psalm's core), feel free to create a new issue for discussion

Also, I think constants were not included. Feel free to PR them here or in Redis repo if they accept those