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

fix iterator_to_array, iterator_count for PHP 8.2 #2625

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented Sep 17, 2023

Fixes phpstan/phpstan#9793 (and the example given in phpstan/phpstan#7760, though that issue is specified more broadly).

The problem happens here:

if ($this->functionSignatureMapProvider->hasFunctionSignature($functionName)) {
. We get the correct signature from php8-stubs, but the signature in functionMap.php was outdated and it overwrote the the correct signature.

@ondrejmirtes
Copy link
Member

We could have done this, but it feels just like a workaround.

The facts:

This is why we need the actual fix in the logic and not a workaround :) Because more functions might be impacted by this bug, if not now, then certainly in the future.

@ondrejmirtes
Copy link
Member

Thinking about the actual reason, I think the problem might be that functionMap has Traversable but the correct type is iterable. But iterable->isSuperTypeOf(Traversable) is yes(). At the same time PHPDoc types can actually be used to narrow down the type. Think of native int with PHPDoc's positive-int.

iterable is actually array|Traversable, but in PHPStan there's a special IterableType implementation.

So maybe we need to special-case this problem in TypehintHelper. If we have iterable in the native type and Traversable in the PHPDoc, iterable should still win.

@schlndh
Copy link
Contributor Author

schlndh commented Sep 18, 2023

@ondrejmirtes Ok, thanks for the pointers. I'll try look into the underlying problem. But I guess it wouldn't hurt to fix the functionMap.php regardless.

@ondrejmirtes
Copy link
Member

I'm looking forward to this one! The reflection golden test should make this a breeze, we'll see all the implication of the fix :)

@schlndh
Copy link
Contributor Author

schlndh commented Nov 16, 2023

I tried adding a hack to decideType instead of fixing the signature map. According the the reflection golden test the only two currently affected native functions/methods are these two. So it doesn't seem worth it to me to add such a weird edge-case to decideType. Also, the change broke existing test - i.e. it could have negative impact on non-native code.

You can see the experiment here: schlndh#3

@ondrejmirtes ondrejmirtes merged commit a9851db into phpstan:1.10.x Nov 16, 2023
418 of 424 checks passed
@ondrejmirtes
Copy link
Member

Awesome, thank you! It's great we can now do these changes in informed way.

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