Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extra template on certain iterators #8053

Open
AndrolGenhald opened this issue Jun 3, 2022 · 7 comments
Open

Extra template on certain iterators #8053

AndrolGenhald opened this issue Jun 3, 2022 · 7 comments

Comments

@AndrolGenhald
Copy link
Collaborator

Hey hi,

I'm testing Psalm 5b1 and I have this new issue:

ERROR: MissingTemplateParam - src/SimpleCachingIteratorAggregate.php:30:13 - CachingIterator has missing template params, expecting 3 (see https://psalm.dev/182)

This comes from there: https://github.com/loophp/iterators/blob/main/src/SimpleCachingIteratorAggregate.php#L28

I'm trying to understand why does \CachingIterator needs 3 template parameters ?
Therefore, how to fix this then ? https://psalm.dev/r/17b32189e1

Originally posted by @drupol in #6970 (comment)

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/17b32189e1
<?php

/**
 * For the full copyright and license information, please view
 * the LICENSE file that was distributed with this source code.
 */

declare(strict_types=1);

namespace loophp\iterators;

use CachingIterator;
use Generator;
use Iterator;
use IteratorAggregate;

// phpcs:disable Generic.Files.LineLength.TooLong

/**
 * @template TKey of array-key
 * @template T
 *
 * @implements IteratorAggregate<array-key, T>
 */
final class SimpleCachingIteratorAggregate implements IteratorAggregate
{
    /**
     * @var CachingIterator<array-key, T>
     */
    private CachingIterator $iterator;

    /**
     * @param Iterator<array-key, T> $iterator
     */
    public function __construct(Iterator $iterator)
    {
        $this->iterator = new CachingIterator(
            $iterator,
            CachingIterator::FULL_CACHE
        );
    }

    /**
     * @return Generator<array-key, T>
     */
    public function getIterator(): Generator
    {
        yield from $this->iterator->getCache();

        while ($this->iterator->hasNext()) {
            $this->iterator->next();

            yield $this->iterator->key() => $this->iterator->current();
        }
    }
}
Psalm output (using commit 3a24488):

ERROR: MissingTemplateParam - 30:13 - CachingIterator has missing template params, expecting 3

@AndrolGenhald
Copy link
Collaborator Author

AndrolGenhald commented Jun 3, 2022

@drupol It's the type of the iterator it's wrapping, but afaict there seems to be a bug with template comparisons: https://psalm.dev/r/ffa90f6c5b (@orklah am I missing something?)

You also have a TKey template that's unused btw, this is probably more what you want.

You should also probably be using @template-covariant here since it's just a wrapper that doesn't need to modify the collection.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ffa90f6c5b
<?php

/**
 * @template TKey of array-key
 * @template T
 *
 * @implements IteratorAggregate<array-key, T>
 */
final class SimpleCachingIteratorAggregate implements IteratorAggregate
{
    /**
     * @var CachingIterator<array-key, T, Iterator<array-key, T>>
     */
    private CachingIterator $iterator;

    /**
     * @param Iterator<array-key, T> $iterator
     */
    public function __construct(Iterator $iterator)
    {
        $this->iterator = new CachingIterator(
            $iterator,
            CachingIterator::FULL_CACHE
        );
    }

    /**
     * @return Generator<array-key, T>
     */
    public function getIterator(): Generator
    {
        yield from $this->iterator->getCache();

        while ($this->iterator->hasNext()) {
            $this->iterator->next();

            yield $this->iterator->key() => $this->iterator->current();
        }
    }
}
Psalm output (using commit 3a24488):

ERROR: InvalidTemplateParam - 14:13 - Extended template param TIterator of CachingIterator<array-key, T:SimpleCachingIteratorAggregate as mixed, Iterator<array-key, T:SimpleCachingIteratorAggregate as mixed>> expects type Iterator<TKey:CachingIterator as mixed, TValue:CachingIterator as mixed>, type Iterator<array-key, T:SimpleCachingIteratorAggregate as mixed> given
https://psalm.dev/r/2196c8e242
<?php

/**
 * @template-covariant TKey of array-key
 * @template-covariant TValue
 *
 * @implements IteratorAggregate<TKey, TValue>
 */
final class SimpleCachingIteratorAggregate implements IteratorAggregate
{
    /**
     * @var CachingIterator<TKey, TValue, Iterator<TKey, TValue>>
     */
    private CachingIterator $iterator;

    /**
     * @param Iterator<TKey, TValue> $iterator
     */
    public function __construct(Iterator $iterator)
    {
        $this->iterator = new CachingIterator(
            $iterator,
            CachingIterator::FULL_CACHE
        );
    }

    /**
     * @return Generator<TKey, TValue>
     */
    public function getIterator(): Generator
    {
        yield from $this->iterator->getCache();

        while ($this->iterator->hasNext()) {
            $this->iterator->next();

            yield $this->iterator->key() => $this->iterator->current();
        }
    }

    public function hasNext(): bool
    {
        return $this->iterator->hasNext();
    }
}
Psalm output (using commit 3a24488):

ERROR: InvalidTemplateParam - 14:13 - Extended template param TIterator of CachingIterator<TKey:SimpleCachingIteratorAggregate as array-key, TValue:SimpleCachingIteratorAggregate as mixed, Iterator<TKey:SimpleCachingIteratorAggregate as array-key, TValue:SimpleCachingIteratorAggregate as mixed>> expects type Iterator<TKey:CachingIterator as mixed, TValue:CachingIterator as mixed>, type Iterator<TKey:SimpleCachingIteratorAggregate as array-key, TValue:SimpleCachingIteratorAggregate as mixed> given

@drupol
Copy link
Contributor

drupol commented Jun 4, 2022

@drupol It's the type of the iterator it's wrapping, but afaict there seems to be a bug with template comparisons: https://psalm.dev/r/ffa90f6c5b (@orklah am I missing something?)

You also have a TKey template that's unused btw, this is probably more what you want.

You should also probably be using @template-covariant here since it's just a wrapper that doesn't need to modify the collection.

Hi,

Thanks for the suggestion. I'm not using @template-covariant because I haven't yet understood when to use it.

If you could help me on this, I'll be glad to implement the necessary changes.

Thanks!

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ffa90f6c5b
<?php

/**
 * @template TKey of array-key
 * @template T
 *
 * @implements IteratorAggregate<array-key, T>
 */
final class SimpleCachingIteratorAggregate implements IteratorAggregate
{
    /**
     * @var CachingIterator<array-key, T, Iterator<array-key, T>>
     */
    private CachingIterator $iterator;

    /**
     * @param Iterator<array-key, T> $iterator
     */
    public function __construct(Iterator $iterator)
    {
        $this->iterator = new CachingIterator(
            $iterator,
            CachingIterator::FULL_CACHE
        );
    }

    /**
     * @return Generator<array-key, T>
     */
    public function getIterator(): Generator
    {
        yield from $this->iterator->getCache();

        while ($this->iterator->hasNext()) {
            $this->iterator->next();

            yield $this->iterator->key() => $this->iterator->current();
        }
    }
}
Psalm output (using commit 3a24488):

ERROR: InvalidTemplateParam - 14:13 - Extended template param TIterator of CachingIterator<array-key, T:SimpleCachingIteratorAggregate as mixed, Iterator<array-key, T:SimpleCachingIteratorAggregate as mixed>> expects type Iterator<TKey:CachingIterator as mixed, TValue:CachingIterator as mixed>, type Iterator<array-key, T:SimpleCachingIteratorAggregate as mixed> given
https://psalm.dev/r/2196c8e242
<?php

/**
 * @template-covariant TKey of array-key
 * @template-covariant TValue
 *
 * @implements IteratorAggregate<TKey, TValue>
 */
final class SimpleCachingIteratorAggregate implements IteratorAggregate
{
    /**
     * @var CachingIterator<TKey, TValue, Iterator<TKey, TValue>>
     */
    private CachingIterator $iterator;

    /**
     * @param Iterator<TKey, TValue> $iterator
     */
    public function __construct(Iterator $iterator)
    {
        $this->iterator = new CachingIterator(
            $iterator,
            CachingIterator::FULL_CACHE
        );
    }

    /**
     * @return Generator<TKey, TValue>
     */
    public function getIterator(): Generator
    {
        yield from $this->iterator->getCache();

        while ($this->iterator->hasNext()) {
            $this->iterator->next();

            yield $this->iterator->key() => $this->iterator->current();
        }
    }

    public function hasNext(): bool
    {
        return $this->iterator->hasNext();
    }
}
Psalm output (using commit 3a24488):

ERROR: InvalidTemplateParam - 14:13 - Extended template param TIterator of CachingIterator<TKey:SimpleCachingIteratorAggregate as array-key, TValue:SimpleCachingIteratorAggregate as mixed, Iterator<TKey:SimpleCachingIteratorAggregate as array-key, TValue:SimpleCachingIteratorAggregate as mixed>> expects type Iterator<TKey:CachingIterator as mixed, TValue:CachingIterator as mixed>, type Iterator<TKey:SimpleCachingIteratorAggregate as array-key, TValue:SimpleCachingIteratorAggregate as mixed> given

@AndrolGenhald
Copy link
Collaborator Author

I'm not using @template-covariant because I haven't yet understood when to use it.

Basically, by default Collection<Cat> is not a subtype of Collection<Animal> because this will be broken. If you mark the template as covariant, then Collection<Cat> will be considered a subtype of Collection<Animal>, but using the template as a method argument is forbidden.

Generally, if you have a templated class that doesn't need to take the template as an argument you should prefer to use covariant templates since they allow code using the class more freedom by imposing extra restrictions on the class itself.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/5565ec3b9f
<?php

/**
 * @template T
 */
class Collection
{
    /** @param list<T> $items */
	public function __construct(private array $items) {}
    
    /** @param T $item */
    public function add($item): void
    {
        $this->items[] = $item;
    }
    
    /** @return Iterator<int, T> */
    public function iterate(): Iterator
    {
        return new ArrayIterator($this->items);
    }
}

class Animal {}

class Cat extends Animal
{
    public function meow(): void {}
}

class Dog extends Animal
{
    public function bark(): void {}
}

$cats = new Collection([new Cat()]);

/** @param Collection<Animal> $animals */
function addDog($animals): void
{
    $animals->add(new Dog());
}

addDog($cats); // Collection<Cat> is not a Collection<Animal>

foreach ($cats->iterate() as $cat) {
    $cat->meow(); // Runtime error, Dog cannot meow
}
Psalm output (using commit 3a24488):

ERROR: InvalidArgument - 44:8 - Argument 1 of addDog expects Collection<Animal>, Collection<Cat> provided
https://psalm.dev/r/18aca553c9
<?php

/**
 * @template-covariant T
 */
class Collection
{
    /** @param list<T> $items */
	public function __construct(private array $items) {}
    
    /** @param T $item This is disallowed so that the collection type cannot be modified to include sibling types */
    public function add($item): void
    {
        $this->items[] = $item;
    }
    
    /** @return Iterator<int, T> */
    public function iterate(): Iterator
    {
        return new ArrayIterator($this->items);
    }
}

class Animal {}

class Cat extends Animal
{
    public function meow(): void {}
}

class Dog extends Animal
{
    public function bark(): void {}
}

$cats = new Collection([new Cat()]);

/** @param Collection<Animal> $animals */
function addDog($animals): void
{
    $animals->add(new Dog());
}

addDog($cats); // Collection<Cat> _is_ a Collection<Animal>

foreach ($cats->iterate() as $cat) {
    $cat->meow(); // Runtime error, Dog cannot meow
}
Psalm output (using commit 3a24488):

ERROR: InvalidTemplateParam - 11:16 - Template param T of Collection is marked covariant and cannot be used here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants