Try to fix issue 8087

[VincentLanglet]

Closes phpstan/phpstan#8087

[VincentLanglet]

The change provided by this PR is fixing the issue phpstan/phpstan#8087 (comment)
I'll try to make a TLDR:

Some tests are failing because.

	public function doFoo(int $i)
	{
		$array = [];

		$array[$i]['bar'] = 1;
		$array[$i]['baz'] = 2;

		assertType('non-empty-array<int, array{bar: 1, baz: 2}>', $array);
	}

is now understood as

	public function doFoo(int $i)
	{
		$array = [];

		$array[$i]['bar'] = 1;
		$array[$i]['baz'] = 2;

		assertType('non-empty-array<int, array{bar: 1, baz?: 2}>', $array);
	}

Currently the following tests:

	public function doFoo(int $i)
	{
		$array = [];

		$array[$i]['bar'] = 1;
                 $i++;
		$array[$i]['baz'] = 2;

		assertType('non-empty-array<int, array{bar: 1, baz: 2}>', $array);
	}

and

	public function doFoo(int $i, int $j)
	{
		$array = [];

		$array[$i]['bar'] = 1;
		$array[$j]['baz'] = 2;

		assertType('non-empty-array<int, array{bar: 1, baz: 2}>', $array);
	}

but they shouldn't IMHO.

But I'm not sure what is possible to do with PHPStan, and what is wanted.
I'm open to (and certainly need) suggestion @staabm, @ondrejmirtes, @herndlm. :)

[VincentLanglet]

I just saw that #1823 has added a new $i === 0 check, so I wonder if I'm in the right direction

[VincentLanglet]

phpstan/phpstan#6173 seems related

[VincentLanglet]

Instead of doing 9acd633 (which was failing some tests)
I can do 89d446e

When setting a value for a specific constant key, it shouldn't override the whole array value type.
So I made a special check for ConstantStringType and ConstantIntegerType ; I dunno if this change make sens.

But now, all the tests (old and new ones) are green. Is this ok this way ?

[VincentLanglet]

Wdyt about this solution @ondrejmirtes ?

[ondrejmirtes]

I don't love this fix, what sense does it make? Why do you choose to union the values even for !$unionValues?

[VincentLanglet]

When you have the following kind of array

non-empty-array<string, mixed>&hasOffsetValue('uses', array)

And when you're doing

$array['uses'][] = '';

Phpstan is trying to do

$valueToWrite = $offsetValueType->setOffsetValueType($offsetType, $valueToWrite, $i === 0);

where $i is 1, so unionValues is false.

setOffsetValueType is called on an intersection Type so it's applied on every types
hasOffsetValue('uses', array) become hasOffsetValue('uses', non-empty-array)
and non-empty-array<string, mixed> become non-empty-array<string, non-empty-array> (which is wrong)

I wanted that non-empty-array<string, mixed> stay like it was. So I thought that when you're calling setOffsetValue with a specific constant key, the generic array type should still keep the union strategy. (in this case mixed will stay mixed).

But maybe something should be done to the IntersectionType::offsetValue method.
Something like, if there is hasOffsetValueType, the ArrayType must not call setOffsetValueType...

[VincentLanglet]

In fact the issue is that

$array['uses'][] = '';

and

$array['uses'] = [...$array['uses'], ''];

doesn't behave the same way.

[VincentLanglet]

So I rewrite the setOffsetValueType code by merging together all the specific code to the ConstantStringType/ConstantIntegerType, (without changing anything) it's

if ($offsetType instanceof ConstantStringType || $offsetType instanceof ConstantIntegerType) {
     if ($offsetType->isSuperTypeOf($this->keyType)->yes()) {
          $builder = ConstantArrayTypeBuilder::createEmpty();
          $builder->setOffsetValueType($offsetType, $valueType);
          return $builder->getArray();
     }

     return TypeCombinator::intersect(
          new self(
               TypeCombinator::union($this->keyType, $offsetType),
               $unionValues ? TypeCombinator::union($this->itemType, $valueType) : $valueType,
          ),
          new HasOffsetValueType($offsetType, $valueType),
          new NonEmptyArrayType()
     );
}

return TypeCombinator::intersect(
     new self(
          TypeCombinator::union($this->keyType, $offsetType),
          $unionValues ? TypeCombinator::union($this->itemType, $valueType) : $valueType,
     ),
     new NonEmptyArrayType()
);

And now, it makes more sens to me to remove the $unionsValues check in case $offsetType instanceof ConstantStringType || $offsetType instanceof ConstantIntegerType. The code become

     return TypeCombinator::intersect(
          new self(
               TypeCombinator::union($this->keyType, $offsetType),
               TypeCombinator::union($this->itemType, $valueType),
          ),
          new HasOffsetValueType($offsetType, $valueType),
          new NonEmptyArrayType()
     );

Since we're intersecting with new HasOffsetValueType($offsetType, $valueType), we shouldn't override the existing item types of the array.

[VincentLanglet]

I rewrote the commit:

• ac1c086 is doing the refacto of the method without changing anything to the existent logic.
• cfa2d20 is the failing test
• 22a2c7c is the fix I propose.

[ondrejmirtes]

Alright, let's try this. Thank you!

[herndlm]

🎉 nice that you figured out a simplified fix

[VincentLanglet]

🎉 nice that you figured out a simplified fix

Yeah, it's satisfying. I feel like this was the most complex PR I had to make so far. :)

Now I just need to find what to do with #1795 in order to allow me to bump phpstan dependencies on my work project. 🤞

[ondrejmirtes]

PHPStan bug is rarely blocking, you can almost always ignore the error and update anyway.

[mad-briller]

PHPStan bug is rarely blocking, you can almost always ignore the error and update anyway.

no point holding yourself back from finding other potential issues elsewhere

and i do love it when you composer update and phpstan tells you that ignored errors don't need to be ignored

[VincentLanglet]

I agree on solo project or on a project with other phpstan-lovers.

But when it's a project with some phpstan-beginners or complaining people

• Ignoring these errors means "showing them how to ignore an error". And sometimes, that's something I try to avoid because they may ignore real error with the phpstan-ignore or baseline tips. (True story, already seen)
• If they write new code with this bug, they won't understand what to do / they will complain about it.

PHPStan is a great tool, and more and more people appreciate working with it, but sometimes it require time before ^^'

[mad-briller]

And sometimes, that's something I try to avoid because they may ignore real error with the phpstan-ignore or baseline tips. (True story, already seen)

you can say that again 😅 , i have experienced this myself introducing phpstan to my org aswell, people love the path of least resistance, but that should definitely be questioned in code review