Phpstan: issue possibly with phpstan ^0.9, possibly with phpdoc blocks on php-cs-fixer ^2.8

[SignpostMarv]

On my php-cs-fixer ruleset I'm getting the following issue:

 ------ -----------------------------------------------------------------------
  Line   src\Config.php
 ------ -----------------------------------------------------------------------
  101    Parameter #1 $finder of method PhpCsFixer\Config::setFinder() expects
         iterable<string>, PhpCsFixer\Finder given.

I'm not sure if this is an issue relating to the changes between 0.8 & 0.9, or if it's specifically a phpdoc block issue with php-cs-fixer ?

[ondrejmirtes]

How does the method PhpCsFixer\Config::setFinder() look like?

[mhujer]

Like this:

    /**
     * @param iterable|string[]|\Traversable $finder
     *
     * @return self
     */
    public function setFinder($finder);

[ondrejmirtes]

I think the problem is with the setFinder method - it should accept PhpCsFixer\Finder as well, which is kind of obvious from its name 😊 PHPStan's behaviour is correct here, it correctly infers the return type of the array_reduce call.

[kubawerlos]

It does, PhpCsFixer\Finder extends Symfony\Component\Finder\Finder which implements IteratorAggregate which extends Traversable.

[ondrejmirtes]

But the phpDoc does not say so.

[SignpostMarv]

@ondrejmirtes

namespace PhpCsFixer;

use Symfony\Component\Finder\Finder as BaseFinder;
class Finder extends BaseFinder
{

which extends from

namespace Symfony\Component\Finder;
class Finder implements \IteratorAggregate, \Countable
{

as @mhujer pasted & @kubawerlos said, PhpCsFixer\ConfigInterface::setFinder() is type-hinted as returning \Traversable

thus sincePhpCsFixer\ConfigInterface::getFinder() returns Traversable, IteratorAggregate implements Traversable, and Symfony\Component\Finder\Finder::append(), Symfony\Component\Finder\Finder::in(), & Symfony\Component\Finder\Finder::ignoreUnreadableDirs() all type-hint @return $this, it seems array_reduce() should be accepted as passing the Traversable option ?

Please also note:

diff --git a/src/Config.php b/src/Config.php
index 588f550..2e30f1b 100644
--- a/src/Config.php
+++ b/src/Config.php
@@ -96,9 +96,16 @@ public function __construct(array $inDirs)
         /**
         * @var DefaultFinder $finder
         */
-        $finder = $this->getFinder();
+        $finder = static::ReduceFinder($this->getFinder(), ...$inDirs);

-        $this->setFinder(array_reduce(
+        $this->setFinder($finder);
+    }
+
+    protected static function ReduceFinder(
+        DefaultFinder $finder,
+        string ...$inDirs
+    ) : DefaultFinder {
+        return array_reduce(
             $inDirs,
             (
                 function (
@@ -113,7 +120,7 @@ function (
                 }
             ),
             $finder->ignoreUnreadableDirs()
-        ));
+        );
     }

     /**

results in:

 ------ ---------------------------------------------------------------------------------------------------------------------------------
  Line   src\Config.php
 ------ ---------------------------------------------------------------------------------------------------------------------------------
  99     Parameter #1 $finder of static method SignpostMarv\CS\Config::ReduceFinder() expects PhpCsFixer\Finder, iterable<string> given.
  101    Parameter #1 $finder of method PhpCsFixer\Config::setFinder() expects iterable<string>, PhpCsFixer\Finder given.
 ------ ---------------------------------------------------------------------------------------------------------------------------------

 [ERROR] Found 2 errors

so it looks like phpstan is taking iterable|string[]|\Traversable and going iterable<string> ?

[JanTvrdik]

so it looks like phpstan is taking iterable|string[]|\Traversable and going iterable<string> ?

Yes, string[] is a magic type which is resolved by heuristics, e.g.

• string[] | Foo is resolved as iterable<string> & Foo if Foo is iterable and
• string[] | Bar is resolved as array<string> | Bar if Bar is not iterable

In this case it's resolved as iterable<string> | (Traversable & iterable<string>) which is further simplified to just iterable<string>. This may or may not be what the author of the comment intended which is precisely why using Xyz[] types is very problematic.

[SignpostMarv]

@JanTvrdik so is this a problem with the heuristics then?

[JanTvrdik]

Based on a quick glance I think that the problem is that setFinder() requires iterable<string> but Symfony\Component\Finder\Finder returns in getIterator() iterable<SplFileInfo>.

So either setFinder() method should be updated to accept iterable<SplFileInfo> or Finder must return iterable<string>.

[keradus]

no. setFinder requires iterable (of anything) or string[] or Traversable.
described heuristics didn't work well for this case.

[JanTvrdik]

@keradus See, I'm a human a even I don't understand what was iterable|string[]|\Traversable supposed to mean. We're not yet at the age when programs are smarter than human.

setFinder requires iterable (of anything) or string[] or Traversable

Oh, now I see. In that case can't write iterable|string[]|\Traversable, because iterable|string[] means iterable of string. It's an idiotic syntax but that's what PHP community decided it should mean.

Luckily for you, if what you really want is to say that the parameter is of type „iterable (of anything) or array (of string) or Traversable (of anything)“, then you can just say that the parameter is iterable. It means the same.

[keradus]

almost. it was added in times when iterable was still an rfc and IDEs were not fully supporting those ;)

[keradus]

It's an idiotic syntax but that's what PHP community decided it should mean.

just for my curiosity. is it written down in some standard, like PHPDoc ?

[JanTvrdik]

just for my curiosity. is it written down in some standard, like PHPDoc ?

No, unfortunately not yet. The standard, if that's what we choose to call phpDocumentor documentation, technically it's not a standard, is very outdated.

• It does not mention iterable type at all.
• It does not mention variadic parameters at all.
• It assumes that Collection|Product[] mean Collection instance or array of Product, which does not match real-world usage.

[SignpostMarv]

No, unfortunately not yet. The standard, if that's what we choose to call phpDocumentor documentation, technically it's not a standard, is very outdated.

This is why we see some parsers taking issue with syntax like (int|string)[] being considered invalid in some cases, but not in other parsers (it's taken to mean an array that can only contain strings & ints)

p.s. ; PSR-5 is vacant

[SignpostMarv]

@JanTvrdik it gets worse :D

diff --git a/src/Config.php b/src/Config.php
index 588f550..462b5b6 100644
--- a/src/Config.php
+++ b/src/Config.php
@@ -113,7 +113,7 @@ function (
                 }
             ),
             $finder->ignoreUnreadableDirs()
-        ));
+        )->getIterator());
     }

     /**

results in:

 ------ -----------------------------------------------------------------------
  Line   src\Config.php
 ------ -----------------------------------------------------------------------
  101    Parameter #1 $finder of method PhpCsFixer\Config::setFinder() expects
         iterable<string>,
         iterable<Symfony\Component\Finder\SplFileInfo>&Iterator given.
 ------ -----------------------------------------------------------------------

Because SplFileInfo hasn't been imported into the scope of the file, the phpdoc parser (correctly?) assumes SplFileInfo exists in the namespace of the class the method being docblocked.

[SignpostMarv]

@JanTvrdik I'm going to ahead and say the heuristics are currently broken.

    /**
     * @param iterable|string[]|\Traversable|Finder $finder
     *
     * @return self
     */
    public function setFinder($finder);

• explicitly slapping Finder onto the param types does not resolve the issue
• using boolean notation, iterable|string[]|\Traversable should be interpreted as accepting either iterable<mixed>, array<string>, or Traversable<mixed>

[JanTvrdik]

Because SplFileInfo hasn't been imported into the scope of the file, the phpdoc parser (correctly?) assumes SplFileInfo exists in the namespace of the class the method being docblocked.

Yes, that's definitely bug in the PHPDoc. SplFileInfo must either be prefixed with \ or it must be imported.

using boolean notation, iterable|string[]|\Traversable should be interpreted as accepting either iterable<mixed>, array<string>, or Traversable<mixed>

Trust me, I've tried for many hours to make it work like this, but it produces significantly worse results with real world code. The current algorithms matches how humans use the notation much better.

explicitly slapping Finder onto the param types does not resolve the issue

There is actually one think we may be able to do to handle this case better. Currently Collection|string[] is resolved as Collection of strings when Collection is iterable. We may try to slightly modify this condition to resolve it in this way only when iterable value type of Collection is supertype of string.

For example if Collection implements Iterator interface and its current method returns int. We would then resolve Collection|string[] as Collection | array<string> because int is not a supertype of string.

[JanTvrdik]

Test with the modified heuristics (#650)
https://phpstan.org/r/61fcd487e45f1c59c9768149566fb54f

[keradus]

Yes, that's definitely bug in the PHPDoc.

No, it is not. Symfony\Component\Finder can create iterable of
Symfony\Component\Finder\SplFileInfo that extends \SplFileInfo

The current algorithms matches how humans use the notation much better.

issue here is lack of standard I would say...

[JanTvrdik]

@keradus You're right.

[SignpostMarv]

@JanTvrdik have made a pull request on php-cs-fixer

[Sevavietl]

As I understand the problem still remains. @SignpostMarv, was there any luck with your PR?

[SignpostMarv]

@Sevavietl docs were changed in FriendsOfPHP/PHP-CS-Fixer/pull/3415