#4997 added more precise type inference for count() returning 0 or positive-int on known arrays

[Ocramius]

Fixes #4997

Note: I only provided the stub, but am unsure about where tests for stubs (if there are any) are applicable.

Questions open:

• @psalm-flow - unclear whether this is needed, seems related to taint analysis, so probably not necessary
• @psalm-pure - unclear whether this is to be declared. Purity depends on the given argument: perhaps @azjezz knows more here? See #4997 added more precise type inference for count() returning 0 or positive-int on known arrays #4999 (comment)

[orklah]

Seems like current CallMap expect Countable|array|SimpleXMLElement|ResourceBundle as parameters. SimpleXMLElement is not countable nor array but can be counted through voodoo. Not sure about ResourceBundle though...

[orklah]

@psalm-flow is related to the continuity of taint though the function. It shouldn't be needed for count.

[azjezz]

@psalm-pure - unclear whether this is to be declared. Purity depends on the given argument: perhaps @azjezz knows more here?

we can't really declare count pure as it depends on the argument type ( #4145 ) ( unless we can do function overrides in stubs )

If $value is array, SimpleXMLElement, or ResourceBundle, then count is pure.
If $value is Countable and ( $value is immutable or count(): int is declared with @psalm-mutation-free ), then count is pure
otherwise, count is not pure.

[Ocramius]

EDIT: solved ✔️ (no need to refactor)

This block can probably also be refactored away somehow:

psalm/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallReturnTypeFetcher.php

Lines 286 to 326 in 1afce4d

	case 'count':
	if (($first_arg_type = $statements_analyzer->node_data->getType($call_args[0]->value))) {
	$atomic_types = $first_arg_type->getAtomicTypes();
	if (count($atomic_types) === 1) {
	if (isset($atomic_types['array'])) {
	if ($atomic_types['array'] instanceof Type\Atomic\TCallableArray
	|| $atomic_types['array'] instanceof Type\Atomic\TCallableList
	|| $atomic_types['array'] instanceof Type\Atomic\TCallableKeyedArray
	) {
	return Type::getInt(false, 2);
	}
	if ($atomic_types['array'] instanceof Type\Atomic\TNonEmptyArray) {
	return new Type\Union([
	$atomic_types['array']->count !== null
	? new Type\Atomic\TLiteralInt($atomic_types['array']->count)
	: new Type\Atomic\TInt
	]);
	}
	if ($atomic_types['array'] instanceof Type\Atomic\TNonEmptyList) {
	return new Type\Union([
	$atomic_types['array']->count !== null
	? new Type\Atomic\TLiteralInt($atomic_types['array']->count)
	: new Type\Atomic\TInt
	]);
	}
	if ($atomic_types['array'] instanceof Type\Atomic\TKeyedArray
	&& $atomic_types['array']->sealed
	) {
	return new Type\Union([
	new Type\Atomic\TLiteralInt(count($atomic_types['array']->properties))
	]);
	}
	}
	}
	}
	break;

The new stubs don't seem to contradict it, but I'm not sure how well tested it is right now, nor if the stubs override it.

[Ocramius]

EDIT: solved ✔️

@orklah ResourceBundle was indeed added to the signature, but downstream it fails in real-world projects:

ERROR: UndefinedClass - src/Framework/MockObject/Rule/ConsecutiveParameters.php:117:18 - Class or interface ResourceBundle does not exist (see https://psalm.dev/019)
        foreach ($parameters as $i => $parameter) {

https://app.circleci.com/pipelines/github/vimeo/psalm/4824/workflows/6f0caadd-ba64-414d-ad4e-d453e9cc6dab/jobs/21060, git@github.com:muglug/phpunit.git

Perhaps I should allow object instead? Or perhaps we should really just stop allowing SimpleXMLElement and ResourceBundle :X

[azjezz]

we should really just stop allowing SimpleXMLElement and ResourceBundle :X

👍 for that

[Ocramius]

EDIT: solved ✔️

Mutation modifier seems to be computed in psalm's internals:

psalm/src/Psalm/Internal/Codebase/Functions.php

Lines 401 to 421 in 1afce4d

	if ($function_id === 'count' && isset($args[0]) && $type_provider) {
	$count_type = $type_provider->getType($args[0]->value);
	if ($count_type) {
	foreach ($count_type->getAtomicTypes() as $atomic_count_type) {
	if ($atomic_count_type instanceof TNamedObject) {
	$count_method_id = new MethodIdentifier(
	$atomic_count_type->value,
	'count'
	);
	try {
	$method_storage = $codebase->methods->getStorage($count_method_id);
	return $method_storage->mutation_free;
	} catch (\Exception $e) {
	// do nothing
	}
	}
	}
	}
	}

I think we're OK as-is, if that is still taken into account whilst looking at the stubs.

I'd gladly write test scenarios for these cases, just need someone to point me at an appropriate location where to put them :-)

[orklah]

I believe FunctionCallTest.php is the appropriate location for those. I added tests here in the past (for example for parse_url)

[Ocramius]

@orklah thanks for pushing me in the right direction: I've added detailed tests around positive-int and 0 return types of count(), as well as its purity properties.

This ended up being much simpler than expected, given that the pre-existing code needed very minor adjustments to operate as expected, so we should be good to review and hopefully merge if the solution is acceptable 👍

[muglug]

Thanks!