Update CachingIterator::getCache stub.

[drupol]

This PR:

• Fix CachingIterator::getCache stub

Examples: https://3v4l.org/i5faS#v8.1.8
False positive in PSalm: https://psalm.dev/r/15a83f948f

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/15a83f948f

<?php

declare(strict_types=1);

namespace App;

use Generator;
use CachingIterator;


$generator = 
    /**
     * @return Generator<bool, bool>
     */
    static function (): Generator {
      yield true => true;
      yield false => false;
    };

$iterator = new CachingIterator($generator(), CachingIterator::FULL_CACHE);

/** @psalm-trace $cache */

$cache = $iterator->getCache();

Psalm output (using commit 8c716f8):

INFO: Trace - 24:1 - $cache: array<array-key, mixed>

[AndrolGenhald]

It'd be nice to show some error if FULL_CACHE isn't used: https://3v4l.org/G80RU.

I also expected CALL_TOSTRING to cause problems, but according to this FULL_CACHE overrides it (edit: nevermind, that guy just doesn't know how bit flags work). What a lovely class full of awful edge cases...

Given that FULL_CACHE probably shouldn't be used with CALL_TOSTRING anyway, it might be ok to add a template for $flags and use @psalm-if-this-is CachingIterator<256>. We'll also probably want a couple of tests to make sure that works correctly.

[drupol]

It'd be nice to show some error if FULL_CACHE isn't used: https://3v4l.org/G80RU.

Tell me how and I'll proceed asap.

What a lovely class full of awful edge cases...

Isn't it !!!

[AndrolGenhald]

I think the conservative route is to add a TFlags template for $flags and add @psalm-if-this-is CachingIterator<mixed, mixed, mixed, 256> sort of like this. That will only allow calling getCache() if FULL_CACHE is used.

The downside is that it will result a false positive for $flags = FULL_CACHE | CALL_TOSTRING, but that's probably fine since according to the documentation that should also change the value type to string (it actually doesn't which confuses the heck out of me, as far as I can tell it calls __toString, but then discards the result and uses the element itself as the value...).

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/4ea22ebdea

<?php

/**
 * @template T1
 * @template T2 of array-key
 * @template TFlags of int-mask<1,2,4,8,16,256>
 */
class Foo
{
    /**
     * @param T1 $a
     * @param T2 $b
     * @param TFlags $flags
     */
    public function __construct($a, $b, $flags) {}
    
    /** @psalm-if-this-is self<mixed, mixed, 256> */
    public function test(): void {}
}

$foo1 = new Foo(1, 2, 3);
$foo1->test();
$foo2 = new Foo(3, 4, 256);
$foo2->test();

Psalm output (using commit 8c716f8):

ERROR: IfThisIsMismatch - 22:8 - Class type must be Foo<mixed, mixed, 256> current type Foo<1, 2, 3>

[drupol]

Thanks, very clear. I will update the PR in a couple of hours.

[orklah]

Unfortunately, templates are not very suited for handling flags. While it could work for simple cases, it creates a whole world of weirdness, as @weirdan explained here: #5035 (comment)

I don't know how other languages handle that kind of thing. But again, other languages may not have such a funky internal API...

[AndrolGenhald]

I thought it might be ok in this case since it seems like it's probably fine to only care about the exact value and not the mask, would you be opposed to it? It would be nice to have a more general way to deal with bitmask templates eventually.

[drupol]

I just updated the PR, feel free to give another review.

TIL: @psalm-if-this-is is a very nice feature !!!

[orklah]

Hey @ondrejmirtes,

Do you have an opinion on adding templates to iterators (or possibly any class) to track flags in __construct in order to change return types in other methods?

If we started to do it, would PHPStan do the same in order to keep parity?

Right now, things like inheritance and covariance checks on templates makes me think it would not be such a great idea but I'd like to get more point of view

[ondrejmirtes]

When adding a new @template it's always a struggle of "Is this useful enough to bother every user with this parameter when they're typehinthing this class?"

Apart from @psalm-if-this-is self<mixed, mixed, 256> (Not sure what it does), I can update CachingIterator stub if you decide to merge this.

[AndrolGenhald]

Apart from @psalm-if-this-is self<mixed, mixed, 256> (Not sure what it does)

It shows an error for this issue because it requires $iterator to be constructed with $flags = 256, otherwise $iterator->getCache() won't be allowed (showing an error like this). But @orklah might be right that it's not worth the trouble, especially since this class seems terribly designed and full of other problems that we won't be able to warn about anyway.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/4ea22ebdea

<?php

/**
 * @template T1
 * @template T2 of array-key
 * @template TFlags of int-mask<1,2,4,8,16,256>
 */
class Foo
{
    /**
     * @param T1 $a
     * @param T2 $b
     * @param TFlags $flags
     */
    public function __construct($a, $b, $flags) {}
    
    /** @psalm-if-this-is self<mixed, mixed, 256> */
    public function test(): void {}
}

$foo1 = new Foo(1, 2, 3);
$foo1->test();
$foo2 = new Foo(3, 4, 256);
$foo2->test();

Psalm output (using commit 33b553e):

ERROR: IfThisIsMismatch - 22:8 - Class type must be Foo<mixed, mixed, 256> current type Foo<1, 2, 3>

[orklah]

Thanks for your input Ondrej!

Yeah, I don't think we should try to use templates for flags in iterator classes. They're really weird and will always have edge cases. I feel like every time we make a big change in those, it doesn't solve a lot of cases but it always bring back new unsolvable issues a few months later (users were used to the old wrong behaviour and the new one may not be better for their use case)

The rest of the PR seems good though

[drupol]

I updated the PR, removed the new @template TFlags and updated RecursiveCachingIterator by removing some extended methods because they were the same as in CachingIterator.

[orklah]

Why change this? The previous type should work the same way but is more flexible to future changes, no?

[drupol]

But then we need to provide a template when creating the CachingIterator, and this is what we want to avoid based on what I read on previous comments, no ?

[orklah]

unless I'm mistaken, int-mask-of<self::*> should not need any template

[drupol]

I don't know, could you confirm?

[AndrolGenhald]

Well, that's what it had before this PR, so if it works on psalm.dev right now it should be fine: https://psalm.dev/r/ecacca8230

[drupol]

Fixed.

[orklah]

Thanks!

[drupol]

cool, merci aussi !