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

Possible regression with Doctrine since psalm version 5.10.0 #9947

Closed
CReimer opened this issue Jun 22, 2023 · 11 comments
Closed

Possible regression with Doctrine since psalm version 5.10.0 #9947

CReimer opened this issue Jun 22, 2023 · 11 comments

Comments

@CReimer
Copy link

CReimer commented Jun 22, 2023

My team and I can't upgrade beyond psalm 5.9.0 because newer version keep flooding us with these error messages

ERROR: InvalidArgument - src/Entities/Article.php:382:39 - Argument 1 of Doctrine\Common\Collections\ArrayCollection::contains expects TMaybeContained:fn-doctrine\common\collections\readablecollection::contains as mixed, but PHPW\Entities\Aufgabe provided (see https://psalm.dev/004)
        if ($this->tasks->contains($element)) {

Here's a small example of this error

<?php

use Doctrine\Common\Collections\ArrayCollection;

require_once __DIR__ . '/../vendor/autoload.php';

$data = [
    'data1',
    'data2',
    'data3',
];

$collection = new ArrayCollection($data);

if ($collection->contains('data2')) {
    echo 'HELLO WORLD' . PHP_EOL;
}

Of which psalm 5.12.0 returns this error message

ERROR: InvalidArgument - tool/psalmErrorExample.php:15:27 - Argument 1 of Doctrine\Common\Collections\ArrayCollection::contains expects TMaybeContained:fn-doctrine\common\collections\readablecollection::contains as mixed, but 'data2' provided (see https://psalm.dev/004)
if ($collection->contains('data2')) {

PHP version: 8.2.7
Doctrine/ORM version: 2.15.2
Psalm version: 5.12.0

@psalm-github-bot
Copy link

Hey @CReimer, can you reproduce the issue on https://psalm.dev ?

@CReimer
Copy link
Author

CReimer commented Jun 22, 2023

Actually, I can.

Here's the link: https://psalm.dev/r/a98fd8bfec

@psalm-github-bot
Copy link

I found these snippets:

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

use Doctrine\Common\Collections\Selectable;

/**
 * @psalm-template TKey of array-key
 * @template-covariant T
 */
interface ReadableCollection {
    /**
     * Checks whether an element is contained in the collection.
     * This is an O(n) operation, where n is the size of the collection.
     *
     * @param mixed $element The element to search for.
     * @psalm-param TMaybeContained $element
     *
     * @return bool TRUE if the collection contains the element, FALSE otherwise.
     * @psalm-return (TMaybeContained is T ? bool : false)
     *
     * @template TMaybeContained
     */
    public function contains(mixed $element);
}

/**
 * The missing (SPL) Collection/Array/OrderedMap interface.
 *
 * A Collection resembles the nature of a regular PHP array. That is,
 * it is essentially an <b>ordered map</b> that can also be used
 * like a list.
 *
 * A Collection has an internal iterator just like a PHP array. In addition,
 * a Collection can be iterated with external iterators, which is preferable.
 * To use an external iterator simply use the foreach language construct to
 * iterate over the collection (which calls {@link getIterator()} internally) or
 * explicitly retrieve an iterator though {@link getIterator()} which can then be
 * used to iterate over the collection.
 * You can not rely on the internal iterator of the collection being at a certain
 * position unless you explicitly positioned it before. Prefer iteration with
 * external iterators.
 *
 * @psalm-template TKey of array-key
 * @psalm-template T
 * @template-extends ReadableCollection<TKey, T>
 */
interface Collection extends ReadableCollection {

}

/**
 * An ArrayCollection is a Collection implementation that wraps a regular PHP array.
 *
 * Warning: Using (un-)serialize() on a collection is not a supported use-case
 * and may break when we change the internals in the future. If you need to
 * serialize a collection use {@link toArray()} and reconstruct the collection
 * manually.
 *
 * @psalm-template TKey of array-key
 * @psalm-template T
 * @template-implements Collection<TKey,T>
 * @psalm-consistent-constructor
 */
class ArrayCollection implements Collection {
    /**
     * An array containing the entries of this collection.
     *
     * @psalm-var array<TKey,T>
     *
     * @var mixed[]
     */
    private array $elements = [];

    /**
     * Initializes a new ArrayCollection.
     *
     * @psalm-param array<TKey,T> $elements
     */
    public function __construct(array $elements = []) {
        $this->elements = $elements;
    }

    /**
     * {@inheritDoc}
     *
     * @template TMaybeContained
     */
    public function contains(mixed $element) {
        return in_array($element, $this->elements, true);
    }
}

$data = [
    'data1',
    'data2',
    'data3',
];

$collection = new ArrayCollection($data);

if ($collection->contains('data2')) {
    echo 'HELLO WORLD' . PHP_EOL;
}
Psalm output (using commit bb28c5a):

ERROR: InvalidArgument - 100:27 - Argument 1 of ArrayCollection::contains expects TMaybeContained:fn-readablecollection::contains as mixed, but 'data2' provided

@orklah
Copy link
Collaborator

orklah commented Jun 22, 2023

Seems to fix itself as long as you repeat the phpdoc in the signature of contains: https://psalm.dev/r/1049462d2e

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/1049462d2e
<?php

use Doctrine\Common\Collections\Selectable;

/**
 * @psalm-template TKey of array-key
 * @template-covariant T
 */
interface ReadableCollection {
    /**
     * Checks whether an element is contained in the collection.
     * This is an O(n) operation, where n is the size of the collection.
     *
     * @param mixed $element The element to search for.
     * @psalm-param TMaybeContained $element
     *
     * @return bool TRUE if the collection contains the element, FALSE otherwise.
     * @psalm-return (TMaybeContained is T ? bool : false)
     *
     * @template TMaybeContained
     */
    public function contains(mixed $element);
}

/**
 * The missing (SPL) Collection/Array/OrderedMap interface.
 *
 * A Collection resembles the nature of a regular PHP array. That is,
 * it is essentially an <b>ordered map</b> that can also be used
 * like a list.
 *
 * A Collection has an internal iterator just like a PHP array. In addition,
 * a Collection can be iterated with external iterators, which is preferable.
 * To use an external iterator simply use the foreach language construct to
 * iterate over the collection (which calls {@link getIterator()} internally) or
 * explicitly retrieve an iterator though {@link getIterator()} which can then be
 * used to iterate over the collection.
 * You can not rely on the internal iterator of the collection being at a certain
 * position unless you explicitly positioned it before. Prefer iteration with
 * external iterators.
 *
 * @psalm-template TKey of array-key
 * @psalm-template T
 * @template-extends ReadableCollection<TKey, T>
 */
interface Collection extends ReadableCollection {

}

/**
 * An ArrayCollection is a Collection implementation that wraps a regular PHP array.
 *
 * Warning: Using (un-)serialize() on a collection is not a supported use-case
 * and may break when we change the internals in the future. If you need to
 * serialize a collection use {@link toArray()} and reconstruct the collection
 * manually.
 *
 * @psalm-template TKey of array-key
 * @psalm-template T
 * @template-implements Collection<TKey,T>
 * @psalm-consistent-constructor
 */
class ArrayCollection implements Collection {
    /**
     * An array containing the entries of this collection.
     *
     * @psalm-var array<TKey,T>
     *
     * @var mixed[]
     */
    private array $elements = [];

    /**
     * Initializes a new ArrayCollection.
     *
     * @psalm-param array<TKey,T> $elements
     */
    public function __construct(array $elements = []) {
        $this->elements = $elements;
    }

    /**
     * @param mixed $element The element to search for.
     * @psalm-param TMaybeContained $element
     *
     * @return bool TRUE if the collection contains the element, FALSE otherwise.
     * @psalm-return (TMaybeContained is T ? bool : false)
     *
     * @template TMaybeContained
     */
    public function contains(mixed $element) {
        return in_array($element, $this->elements, true);
    }
}

$data = [
    'data1',
    'data2',
    'data3',
];

$collection = new ArrayCollection($data);

if ($collection->contains('data2')) {
    echo 'HELLO WORLD' . PHP_EOL;
}
Psalm output (using commit bb28c5a):

No issues!

@ygottschalk
Copy link
Contributor

Also fixed by removing the extra template annotation: https://psalm.dev/r/97b80a50b6

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/97b80a50b6
<?php

use Doctrine\Common\Collections\Selectable;

/**
 * @psalm-template TKey of array-key
 * @template-covariant T
 */
interface ReadableCollection {
    /**
     * Checks whether an element is contained in the collection.
     * This is an O(n) operation, where n is the size of the collection.
     *
     * @param mixed $element The element to search for.
     * @psalm-param TMaybeContained $element
     *
     * @return bool TRUE if the collection contains the element, FALSE otherwise.
     * @psalm-return (TMaybeContained is T ? bool : false)
     *
     * @template TMaybeContained
     */
    public function contains(mixed $element);
}

/**
 * The missing (SPL) Collection/Array/OrderedMap interface.
 *
 * A Collection resembles the nature of a regular PHP array. That is,
 * it is essentially an <b>ordered map</b> that can also be used
 * like a list.
 *
 * A Collection has an internal iterator just like a PHP array. In addition,
 * a Collection can be iterated with external iterators, which is preferable.
 * To use an external iterator simply use the foreach language construct to
 * iterate over the collection (which calls {@link getIterator()} internally) or
 * explicitly retrieve an iterator though {@link getIterator()} which can then be
 * used to iterate over the collection.
 * You can not rely on the internal iterator of the collection being at a certain
 * position unless you explicitly positioned it before. Prefer iteration with
 * external iterators.
 *
 * @psalm-template TKey of array-key
 * @psalm-template T
 * @template-extends ReadableCollection<TKey, T>
 */
interface Collection extends ReadableCollection {

}

/**
 * An ArrayCollection is a Collection implementation that wraps a regular PHP array.
 *
 * Warning: Using (un-)serialize() on a collection is not a supported use-case
 * and may break when we change the internals in the future. If you need to
 * serialize a collection use {@link toArray()} and reconstruct the collection
 * manually.
 *
 * @psalm-template TKey of array-key
 * @psalm-template T
 * @template-implements Collection<TKey,T>
 * @psalm-consistent-constructor
 */
class ArrayCollection implements Collection {
    /**
     * An array containing the entries of this collection.
     *
     * @psalm-var array<TKey,T>
     *
     * @var mixed[]
     */
    private array $elements = [];

    /**
     * Initializes a new ArrayCollection.
     *
     * @psalm-param array<TKey,T> $elements
     */
    public function __construct(array $elements = []) {
        $this->elements = $elements;
    }

    /**
     * {@inheritDoc}
     */
    public function contains(mixed $element) {
        return in_array($element, $this->elements, true);
    }
}

$data = [
    'data1',
    'data2',
    'data3',
];

$collection = new ArrayCollection($data);

if ($collection->contains('data2')) {
    echo 'HELLO WORLD' . PHP_EOL;
}

$_false = $collection->contains(123);
/** @psalm-trace $_false */
Psalm output (using commit a0a9c27):

INFO: Trace - 103:28 - $_false: false

@CReimer
Copy link
Author

CReimer commented Jun 26, 2023

Does that mean it's an upstream doctrine/collection issue? Should I report it there then?

@orklah
Copy link
Collaborator

orklah commented Jun 26, 2023

Yeah, please do, let's hear what they think about this

@CReimer
Copy link
Author

CReimer commented Jun 26, 2023

It looks like I don't need to. It's already fixed upstream. Unfortunately no release yet.
I searched for an issue on the doctrine/orm Github page. It turns out the problem can be found on a separate doctrine/collections page.

doctrine/collections#368 (Report)
doctrine/collections#369 (Pull Request)
doctrine/collections@71a25d7 (Fix)

Thank you all. I'm sorry for wasting your time.

@CReimer CReimer closed this as completed Jun 26, 2023
@orklah
Copy link
Collaborator

orklah commented Jun 26, 2023

No worries! Feedback is great :)

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

No branches or pull requests

3 participants