Detect function variadic-ness within top-level function_exists()

[staabm]

closes phpstan/phpstan#11559
closes phpstan/phpstan#4753

[phpstan-bot]

This pull request has been marked as ready for review.

[ondrejmirtes]

I feel like this code is too complicated. We could simply traverse through everything similarly to how we do here:

phpstan-src/src/Type/FileTypeMapper.php

Lines 758 to 772 in ad32861

	if ($node instanceof Node) {
	$callbackResult = $nodeCallback($node);
	if ($callbackResult === self::SKIP_NODE) {
	return;
	}
	foreach ($node->getSubNodeNames() as $subNodeName) {
	$subNode = $node->{$subNodeName};
	$this->processNodes($subNode, $nodeCallback, $endNodeCallback);
	}
	$endNodeCallback($node, $callbackResult);
	} elseif (is_array($node)) {
	foreach ($node as $subNode) {
	$this->processNodes($subNode, $nodeCallback, $endNodeCallback);
	}
	}

What's more important to also solve here is to answer the question: What if we find the same function multiple times? Is it enough if one of them is variadic, or all of them need to be variadic?

Also we might find multiple functions and some of them might be variadic natively (...$params) and some only through func_get_args(). The code needs to account for that.

And last but not least, do not forget to bump the version in the cache key here :)

[staabm]

I feel like this code is too complicated

from the pre-existing code I got the impression its on purpose filtering only the smallest possible AST, therefore I went with this ugly if-else analysis. adjusted it now to analyze the whole AST.

Also we might find multiple functions and some of them might be variadic natively (...$params) and some only through func_get_args(). The code needs to account for that.

I added tests for the cases, I think it works as expected now.

And last but not least, do not forget to bump the version in the cache key here :)

great catch

[ondrejmirtes]

I think I'm missing looking at Param::$variadic here. Now it looks like it treats function foo($bar) { } and function foo(...$bar) { } the same and passes the function as variadic in either case. I think it should be more strict.

[staabm]

native variadic is already checked in the outer scope.

for native variadic we don't call into callsFuncGetArgs at all

[ondrejmirtes]

We can have a situation when $isNativelyVariadic = $this->reflection->isVariadic(); returns false and still one of the function declarations in the file might have function foo(...$bar) .

[ondrejmirtes]

Please reproduce this situation and fix it, thanks.

[staabm]

I was not able to reproduce it initially because of the caching involved.

After temporarily using different cache keys locally the case was showing up :-)

[ondrejmirtes]

Alright, this works for me :) Thank you!