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

fixed count() type after array_pop/array_shift #1691

Closed
wants to merge 1 commit into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 4, 2022

Comment on lines 1843 to 1849
$validCount = IntegerRangeType::fromInterval(0, null);
if ($validCount->isSuperTypeOf($decrementedCountType)->yes()) {
$scope = $scope->assignExpression(
$countExpr,
$decrementedCountType
);
}

Copy link
Contributor Author

@staabm staabm Sep 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just an initial idea on how this could be implemented. I am not sure its the right way to fix it, so figured I would submit it for review and further discussion.

I already realized that with this PR applied the following does not yet work:

/** @param array<int, string> $headers */
function pop1(array $headers): void
{
	if (count($headers) >= 4) {
		assertType('int<4, max>', count($headers));
		array_pop($headers);
		assertType('int<3, max>', sizeof($headers));
	}
}

/** @param array<int, string> $headers */
function foo(array $headers): void
{
	if (sizeof($headers) >= 4) {
		\PHPStan\dumpType(sizeof($headers));
		array_pop($headers);
		\PHPStan\dumpType(sizeof($headers));
	}
}

could be fixed - I guess - in a similar way as we already do for count here. but maybe there is a more elegant way?


array_push and array_unshift could be handled in a similar way.. will leave it for a separate PR though

@@ -1817,6 +1819,10 @@ function (MutatingScope $scope) use ($expr, $nodeCallback, $context): Expression
) {
$arrayArg = $expr->getArgs()[0]->value;
$arrayArgType = $scope->getType($arrayArg);

$countExpr = new FuncCall(new Name('count'), [$expr->getArgs()[0]]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atm this line depends on whether the code beeing analyzed is in a namespace or not

@ondrejmirtes
Copy link
Member

We can't do this precisely without having some accessory type that would carry how many items are there in the array. Right now we only have NonEmptyArrayType which tells when it's int<0, max> and when it's int<1, max>. I don't want to do the count() computation anywhere but CountFunctionReturnTypeExtension.

I don't think this is worth fixing now, I removed the "Easy fixes" milestdone.

@ondrejmirtes
Copy link
Member

See #1828 (comment)

@staabm staabm deleted the bug7804 branch October 13, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type inference after using array_pop could be improved.
2 participants