fix return type of parent calls for SplHeap

[schlndh]

This is an attempt to fix phpstan/phpstan#9867

The issue happened because the return type for SplHeap methods ignored the phpdoc (as you can see from the change the previous code only considered phpdoc from stubs). I discovered that similar code uses the TypehintHelper in these situations, so I used it as well.

But it broke a few tests due to benevolent unions in the functionaMap.php. So I added a workaround for it. I couldn't think of any issues, since I haven't found any code uses benevolent unions and which would need a phpdoc as well (e.g. due to generics).

---

Previously, it worked for SplMinHeap because it took a different branch in PhpClassReflectionExtension::createMethod:

• Since SplMinHeap has the methods duplicated in phpstorm-stubs, the $declaringClassName is SplMinHeap.
• Then $this->signatureMapProvider->hasMethodSignature($declaringClassName, $methodReflection->getName()) returns false, because the functionMap.php only has the methods for SplHeap. Since this is the branch that contains the incorrect code, the issue was avoided for SplMinHeap.
• For SplMaxHeap and SplHeap the declaring class is SplHeap (which is in the functionMap.php) and thus the branch was taken and the issue happened.

---

Here are a few tests that didn't work without the change to TypehintHelper::decideType (there were a few more):

• DateTimeModifyReturnTypes.php
• pdo-prepare.php
• bug-6609.php

[schlndh]

This broke memcache-get.php test. The explanation is here: #2323 (comment)

[schlndh]

This is not (easily) possible to fix via functionMap.php. There are two variants in function map: one which returns mixed and one which returns array|false. And as you can see, the phpdoc return type in phpstorm-stubs is string|array|false.

decideType(mixed, string|array|false) = string|array|false, which doesn't seem like something that could be changed.

Perhaps if decideType also considered where the two types come from. E.g. in this case we know that the first type comes from the function map. Since string|array|false could have been used in the function map, we could reach the conclusion that we used the broader type intentionally. However, there are issues in the function map as well, so it's though to say whether such approach would be more feasible.

[ondrejmirtes]

What about disregarding PhpStorm stubs type completely if functionMap has multiple variants? But the "golden test" for Reflection will come in handy in that scenario :)

[schlndh]

Nice, that worked (for memcace::get). I limited it specifically to the return type. The stubs are still useful/necessary for positional parameter names, @throws, ...

But unfortunately, it doesn't cover the ReflectionPropety patch.

[ondrejmirtes]

I'm gonna take a look at this soon but can you please look at this? phpstan/phpstan#9849 (reply in thread)

I list two very common and very annoying issues, they're somewhat in the same area as this bug, and I think you're more than capable of addressing them 😊 Thank you very much.

[ondrejmirtes]

Can we solve this a different way instead of patching jetbrains/phpstorm-stubs? Like adding some entries to functionMap.php? Thanks!

[schlndh]

Note to self: This might also get affected/fixed: phpstan/phpstan#10057

2) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD DOMDocument::load" ('METHOD DOMDocument::load', 'Has side-effects: Maybe\nVisi... mixed')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
     /**
      * @param string $filename
      * @param int $options
-     * @return mixed
+     * @return bool|DOMDocument
      */
     function load(string $filename, int $options = 0): mixed'

https://github.com/schlndh/phpstan-src/actions/runs/6586745404/job/17895674524?pr=1

[schlndh]

I removed two of the patches (the SplHeap patch was accepted to upstream, and Memcache was fixed by your idea).

I checked the differences in the reflection golden test - they seem mostly OK (though a lot of them are in niche, undocumented functions/methods, so I can't be 100% sure). Here are some of the wrong changes that I noticed:

12) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD DOMDocument::load" ('METHOD DOMDocument::load', 'Is internal: Yes\nHas side-ef... mixed')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
     /**
      * @param string $filename
      * @param int $options
-     * @return mixed
+     * @return bool|DOMDocument
      */
     function load(string $filename, int $options = 0): mixed'

/home/schlndh/devel/custom/phpstan-src/tests/PHPStan/Reflection/ReflectionProviderGoldenTest.php:69

13) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD DOMDocument::loadXML" ('METHOD DOMDocument::loadXML', 'Is internal: Yes\nHas side-ef... mixed')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
     /**
      * @param string $source
      * @param int $options
-     * @return mixed
+     * @return bool|DOMDocument
      */
     function loadXML(string $source, int $options = 0): mixed'

It seems that these methods can return DOMDocument only when called statically, which doesn't seem to be possible since PHP 8 (this result is from 8.2).

62) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD MongoDB::createCollection" ('METHOD MongoDB::createCollection', 'Is internal: Yes\nHas side-ef... mixed')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
      * @param bool $options
      * @param int $size
      * @param int $max
-     * @return MongoCollection
+     * @return MongoCollection<p>
      */
     function createCollection(mixed $name, mixed $options, mixed $size, mixed $max): mixed'

There are several cases like this. The problem is this: @return MongoCollection <p>Returns a collection object representing the new collection.</p>. I'm not sure if the phpdoc parser is supposed to ignore the space between the type and the <p>. I found 5 such cases (all of them mongo related), so it's probably not a big deal for now.

107) PHPStan\Reflection\ReflectionProviderGoldenTest::test with data set "METHOD ReflectionUnionType::getTypes" ('METHOD ReflectionUnionType::getTypes', 'Is internal: Yes\nVisibility:... array')
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
 Visibility: public
 Variants: 1
     /**
-     * @return array
+     * @return array<ReflectionIntersectionType|ReflectionNamedType>
      */
     function getTypes(): array'

I guess this should be just array<ReflectionType>.

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

As for the Mongo @return MongoCollection <p>Returns a collection object representing the new collection.</p> problem:

There's some logic in phpstan/phpdoc-parser that should avoid this problem: https://github.com/phpstan/phpdoc-parser/blob/c2b8bbfa971e25976e9c64ea30a4e944e2688cb4/src/Parser/TypeParser.php#L367-L395

Can you please try to write a failing test in the repository and maybe modify the logic so that it works for Mongo PHPDocs too? Thanks.

[ondrejmirtes]

Thank you!