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

only fuse array & Traversable with phpstorm generics #6689

Merged
merged 3 commits into from
Oct 21, 2021

Conversation

orklah
Copy link
Collaborator

@orklah orklah commented Oct 18, 2021

This should fix #6682

The change is pretty weird though but I couldn't make sense of it otherwise. It seems this piece of code will only merge Array&Traversable into Iterable if we're not allowing PHPStorm generics. In fact, allowing them fix the example: https://psalm.dev/r/ba98d80cff

So I guess it was just a mistake. Seems like no test was covering this neither.

I'm very much thinking about deprecating the "allow PHPStorm generics" and removing it in Psalm 5. PHPStorm now supports standard generics notation so it makes little sense to have this now but this will come back to bite us when we'll try working with intersections. With #6672, it's the second fix I make to this config in a week.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/ba98d80cff
<?php declare(strict_types=1);

/**
 * Copied from https://github.com/doctrine/doctrine-laminas-hydrator/blob/2.2.x/src/DoctrineObject.php#L638
 */
class DoctrineObject
{
    /**
     * Verifies if a provided identifier is to be considered null
     *
     * @param  mixed $identifier
     * @return bool
     */
    private function isNullIdentifier($identifier)
    {
        if (null === $identifier) {
            return true;
        }

        if ($identifier instanceof \Traversable || is_array($identifier)) {
            $nonNullIdentifiers = array_filter(
                ArrayUtils::iteratorToArray($identifier),
                function ($value) {
                    return null !== $value;
                }
            );

            return empty($nonNullIdentifiers);
        }

        return false;
    }
}
    
/**
 * Copied from https://github.com/laminas/laminas-stdlib/blob/3.7.x/src/ArrayUtils.php#L218
 */
class ArrayUtils 
{
    /**
     * Convert an iterator to an array.
     *
     * Converts an iterator to an array. The $recursive flag, on by default,
     * hints whether or not you want to do so recursively.
     *
     * @param  array|\Traversable  $iterator     The array or Traversable object to convert
     * @param  bool                $recursive    Recursively check all nested structures
     * @throws \InvalidArgumentException If $iterator is not an array or a Traversable object.
     * @return array
     */
    public static function iteratorToArray($iterator, $recursive = true)
    {
        return [];
    }
}
Psalm output (using commit f7a6336):

No issues!

@weirdan
Copy link
Collaborator

weirdan commented Oct 18, 2021

I'm very much thinking about deprecating the "allow PHPStorm generics" and removing it in Psalm 5

👍 on this. Mind adding it to #4700 ?

@weirdan
Copy link
Collaborator

weirdan commented Oct 18, 2021

it seems this piece of code will only merge Array&Traversable into Iterable if we're not allowing PHPStorm generics.

I think the idea was to prevent treating Traversable|float[] as iterable<mixed, mixed>.

@orklah
Copy link
Collaborator Author

orklah commented Oct 19, 2021

I think the idea was to prevent treating Traversable|float[] as iterable<mixed, mixed>.

It will actually do the opposite if I'm not mistaken. But only if you don't use PHPStorm generics. If you do, Traversable|float[] will stay as is

@orklah orklah merged commit 2d90631 into vimeo:master Oct 21, 2021
@weirdan weirdan added the release:fix The PR will be included in 'Fixes' section of the release notes label Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Providing iterable to array|\Traversable triggers an InvalidArgument error
2 participants