Iterated value is assumed to be always nullable if implementation of Iterator::current() allows null return value

[sad-spirit]

https://psalm.dev/r/d81595727c

It is possible to add @psalm-assert-if-true / @psalm-assert-if-false to valid() implementation and this helps with explicit current() call.

However, when using the iterator in foreach loop, psalm ignores both the @implements annotation which states that iterated value is not nullable and the annotations on valid() method which obviously returns true inside foreach.

Is it by design? Are there means to specify that iterated value is not nullable on Iterator implementation?

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/d81595727c

<?php


class Foo
{
    public function bar(): void
    {
        echo "Doing bar";
    }
}

/**
 * @implements Iterator<int, Foo>
 */
class FooIterator implements \Iterator
{
    /** @var Foo[] */
    private $items;
    /** @var int */
    private $position = 0;

    public function __construct(Foo ...$items)
    {
        $this->items = \array_values($items);
    }

    public function current(): ?Foo
    {
        return $this->valid() ? $this->items[$this->position] : null;
    }

    public function next(): void
    {
        $this->position++;
    }

    public function key(): int
    {
        return $this->position;
    }

    /**
     * @psalm-assert-if-true Foo $this->current()
     * @psalm-assert-if-false null $this->current()
     */
    public function valid(): bool
    {
        return $this->position < \count($this->items);
    }

    public function rewind(): void
    {
        $this->position = 0;
    }
}

$iterator = new FooIterator(new Foo(), new Foo());

if ($iterator->valid()) {
    $iterator->current()->bar();
}

foreach ($iterator as $foo) {
    $foo->bar();
}

Psalm output (using commit a97b6b8):

ERROR: PossiblyNullReference - 64:11 - Cannot call method bar on possibly null value

[sad-spirit]

BTW, I looked at the stubs provided with psalm and current() methods of built-in iterators are marked with @psalm-ignore-nullable-return:

psalm/stubs/CoreGenericIterators.phpstub

Lines 23 to 29 in 1a43956

	interface Iterator extends Traversable {
	/**
	* @return TValue|null returns current value or null when iterator is drained
	* @psalm-ignore-nullable-return
	*/
	public function current();

So we essentially get a false positive on the most generic use of an iterator -- iterating with foreach -- so we silence that error and prevent finding legitimate problems when calling current() directly. 🤔

I also found a possibly related issue: #4141

[orklah]

The most recent talk we had about that was here: #4725 (comment)

@psalm-ignore-nullable-return ignores the null part only if the related setting is true in the config file so it's not really relevant.

But if we can get stubs to work correctly with assertions, it would be better than adding null (or not) systematically