Add check the array is not empty

[ossinkine]

Closes phpstan/phpstan#3155

I made this the same way as identical compare works (count($foo) !== 0) but I don't understand what is context and why the checks needed such $context->null(), $context->truthy() and why changing context may be needed $newContext = $newContext->negate().

[ossinkine]

Also I've changed bug-2648.php which tests unset function and looks like no changes should be here, type should be invalidated

[ondrejmirtes]

For correct $context handling look at these additional conditions: https://github.com/phpstan/phpstan-src/blob/7dccb9b13014b14776fbd179e190739980452a2d/src/Analyser/TypeSpecifier.php#L198-L217

What they do:

1. $context->null() is for cases when there's a separate statement. So it can be used for calls that look like this:

$this->checkType($arg); // throws exception

But $context->true() and false() is for cases in conditions, like if ($this->isType($arg)).

[ondrejmirtes]

So it looks like this handles X < count($arg) and X <= count($arg). And makes the mistake of handling 0 <= count($arg) also as non-empty which is wrong. (There's the $offset variable a few lines above that can be used for this purpose.)

[ossinkine]

Fixed

[ondrejmirtes]

It's important to replicate everything from https://github.com/phpstan/phpstan-src/blob/7dccb9b13014b14776fbd179e190739980452a2d/src/Analyser/TypeSpecifier.php#L198-L217

[ondrejmirtes]

Why PHPStan fails in the build with Comparison operation ">" between 0 and 0 is always false. is exactly why this needs to be done :)

[ondrejmirtes]

Can you verify that it solves phpstan/phpstan#3558 as well?

[ondrejmirtes]

And also phpstan/phpstan#3631

[ossinkine]

What they do:

1. $context->null() is for cases when there's a separate statement. So it can be used for calls that look like this:

$this->checkType($arg); // throws exception 

But $context->true() and false() is for cases in conditions, like if ($this->isType($arg)).

@ondrejmirtes I'm sorry but I don't understand what you mean. I've tried to remove this conditions to check which tests become failed, but all tests are passes without this conditions. Could you please explain which cases covered by this conditions?

[ossinkine]

phpstan/phpstan#3558 and phpstan/phpstan#3631 are not solved, looks like they does not relate to this PR

[dktapps]

It's also unhandled for <, <=, == and !=. Not sure if you want to look at those ..

[ondrejmirtes]

I'm sorry but I don't understand what you mean

It means that if you don't check the $context and have a code like this:

count($arg) > 0;

Then the array in $arg would be considered non-empty after this line although it doesn't make sense in PHP.

@dktapps Inverted conditions are handled:

phpstan-src/src/Analyser/TypeSpecifier.php

Lines 412 to 416 in 0632865

	} elseif ($expr instanceof Node\Expr\BinaryOp\Greater) {
	return $this->specifyTypesInCondition($scope, new Expr\BinaryOp\Smaller($expr->right, $expr->left), $context, $defaultHandleFunctions);
	} elseif ($expr instanceof Node\Expr\BinaryOp\GreaterOrEqual) {
	return $this->specifyTypesInCondition($scope, new Expr\BinaryOp\SmallerOrEqual($expr->right, $expr->left), $context, $defaultHandleFunctions);

[ossinkine]

@ondrejmirtes As I understood the following test should fail if !$context->null() condition missed

count($ints) > 0; assertType('int|false', max($ints));

But test is passed.

Also I don't understend what this code doing

if ($context->truthy() || $leftType->getValue() === 0) { $newContext = $context; if ($leftType->getValue() === 0) { $newContext = $newContext->negate();	} // ... }

Why the context changed and why this way?

I can't write the tests until I understend what the code doing 😃

[ondrejmirtes]

The linked code handles these cases:

if (count($arg) === 0) { // array is empty } else { // array is not empty }

And:

if (count($arg) !== 0) { // array is not empty } else { // array is empty }

Because when the typeSpecifier->create() is called with $context->true(), the passed type is intersected with the expression type, hence array&nonEmpty. But when it's called with $context->false(), the passed type is removed from the expression type. array minus nonEmpty = empty array.

Your code currently doesn't work and fails because:

if (count($arrayAccessoryTypes) > 1) {	$arrayAccessoryTypesToProcess = array_values(array_intersect_key(...$arrayAccessoryTypes)); } else {	// PHPStan thinks $arrayAccessoryTypes is empty but in reality it can contain one element } 

That's why PHPStan fails in the build.

Also, I'm just realizing we should cover this scenario:

if (count($arrayAccessoryTypes) >= 1) { } else { // array is empty }

And:

if (count($arrayAccessoryTypes) < 1) { // array is empty } else { // array is not empty }

[ossinkine]

After night of debugging I got how it works 😄
@ondrejmirtes I've added more tests, please look at this

[ossinkine]

@ondrejmirtes Also please look at this test

phpstan-src/tests/PHPStan/Analyser/data/bug-2648.php

Lines 13 to 21 in 0632865

	public function doFoo(array $list): void
	{
	if (count($list) > 1) {
	assertType('int<2, max>', count($list));
	unset($list['fooo']);
	assertType('array<bool>', $list);
	assertType('int', count($list));
	}
	}

After this fix $list has nonEmpty and it isn't invalidated, but must to be

[ondrejmirtes]

Awesome, thank you very much, just merged it! Fixed the failing test with this: e96481c

Now that you're fluent in PHPStan's internals, you could help me out by looking at other bugs, I'd really appreciate it :) Issues from this list should be fairly easy to fix: https://github.com/phpstan/phpstan/milestone/8 If you decide to work on something and realize it's more than 50 lines of code to fix it, please reach out to me so we can work on it together. (It's possible that I' wrong and it's not an easy fix, or that you approached it from a different angle.).

Thank you once more :)

[ossinkine]

It's said loudly I'm a fluent in PHPStan's internals 😆

The project definitely requires decreasing class sizes and more detailed commenting the code.

Anyway I'll glad to help you.