PHPStan thinks loop element can be false when looping custom Iterator implementation

[InvisibleSmiley]

Bug report

I have a problem with a custom class implementing Iterator, specifically Iterator::current using PHP built-in function current() on an internal array. The current() function may return an array item or false:

If the internal pointer points beyond the end of the elements list or the array is empty, current() returns false.

PHPStan thinks foreach loop items may be false when in reality that cannot happen.
Not sure whether this stems from the Iterator::current method implementation though, because I also have "false" in annotations of methods next() (using function next()) and rewind() (using function reset()).

Code snippet that reproduces the problem

https://phpstan.org/r/2fefbc8d-c1f2-4ee8-bc04-59a3ef0be3cd

Note that two more errors are displayed in strict mode but I think these are wrong, too.

[stof]

Well, as you document current() as returning DateTimeInterface|false, that makes your class implement Iterator<int, DateTimeInterface|false>

[InvisibleSmiley]

Following your logic, this simple ArrayIterator on int[] should actually be Iterator<int, int|null>
https://phpstan.org/r/dbacf610-4448-4934-a7c3-ededd41afb23
Doesn't make much sense, does it?

Maybe someone can enlighten me regarding:

1. what the "correct" behavior/implementation would be in case there is no "current" element (return null instead of false? Throw an exception?)
2. where the "officially correct" behavior is documented (if anywhere)

I'm not trying to blame anyone here; it just seems very unclear to me.

[stof]

@InvisibleSmiley Adding @implements \Iterator<int, DateTimeInterface> on your class instead of letting phpstan infer it might help.

[InvisibleSmiley]

Umm, I have exactly that in line 20 of my initial example ;) Or am I getting you wrong?

[tomasfejfar]

It seems to me, that we either need to have a way to hint the "empty" type of current(), or the other way around - hint the "happy path" type for current(). One other option might be "premade" hint, something like @phpstan-iterator-current-falsey-on-empty, that would signify that it return a value that would evaluate to false when casted to bool if the iterator is empty. The return values then could be iterated and compared based on their boolean value.

Anyway, I think there is no way to implement a default behavior for this, because of the inability to determine what the "empty" return type of current() is.

[tomasfejfar]

Related #3763

[tomasfejfar]

Is there something I could help with here? Would a failing test help (I don't think so as there is already a replicable case in phpstan.org)?

In my case I use both current() as well as iterating, so at least one of them will be incorrect with the current implementation. Either the current will be missing the potential null or the iteration will need to have additional not null assertion somewhere in the code.

I understand the logic @hrach is articulating in #3763 (comment), but unfortunately it's not a matter of principle, but rather a matter of being able to describe a code that actually exists and cannot be easily changed.

[ondrejmirtes]

The problem with "nullable"/"falsable" return type of key() and current() is that we don't know if that type represents the an empty/already iterated iterator, or if it's actually one of the returned values.

But I realized it's unambiguous when @implements Iterator<int, DateTimeInterface> and similar is involved, so now it gets higher priority and this issue is fixed: phpstan/phpstan-src@a2acf64

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.