Try to fix issue 8087

[VincentLanglet]

Closes https://github.com/phpstan/phpstan/issues/8087

[VincentLanglet]

The change provided by this PR is fixing the issue https://github.com/phpstan/phpstan/issues/8087#issue-1392517306 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 https://github.com/phpstan/phpstan-src/pull/1823 has added a new $i === 0 check, so I wonder if I'm in the right direction

[VincentLanglet]

https://github.com/phpstan/phpstan/issues/6173 seems related

[VincentLanglet]

Instead of doing https://github.com/phpstan/phpstan-src/pull/1808/commits/9acd633e8b89d90d69e3ad3595c74d0a5f528001 (which was failing some tests) I can do https://github.com/phpstan/phpstan-src/pull/1808/commits/89d446ec10919ffb7470f36097cd2dd662717681

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 ?

[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:

• https://github.com/phpstan/phpstan-src/pull/1808/commits/ac1c086712e2ebe3476de98bd8661e10929f191d is doing the refacto of the method without changing anything to the existent logic.
• https://github.com/phpstan/phpstan-src/pull/1808/commits/cfa2d209c50ffd55128ec54994d010ab0a024969 is the failing test
• https://github.com/phpstan/phpstan-src/pull/1808/commits/22a2c7c9ac16c70e153db0131dc0cba5e5433f4c 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 https://github.com/phpstan/phpstan-src/pull/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 :sweat_smile: , 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