Report non existent offset on non empty array

[VincentLanglet]

Closes phpstan/phpstan#7143

I discovered that a maybe not existent offset

• was reported on array{'foo'?: 1, 'bar'?: 2}
• was not reported on non-empty-array{'foo'?: 1, 'bar'?: 2}

It seems like it's because TypeUtils::flattenType does not support Intersection types so

TypeUtiles::flattenType(array{'foo'?: 1, 'bar'?: 2})
// returns [], ['foo' => 1], ['bar' => 2], ['foo' => 1, 'bar' => 2]

but

TypeUtiles::flattenType(non-empty-array{'foo'?: 1, 'bar'?: 2})
// returns `non-empty-array{'foo'?: 1, 'bar'?: 2}`
// instead of ['foo' => 1], ['bar' => 2], ['foo' => 1, 'bar' => 2]

Funny thing, fixing it is breaking some non-regression tests like

• Misinterpreting Whether Regular Expression Piece Exists phpstan#11602: this was supposed to be fixed by RegexArrayShapeMatcher - enforce list-type when no named captures #3899 but wasn't: the array shape was transformed into an intersection list&array shape, that's why the error wasn't reported anymore. (cc @staabm)
  => A way to see this is the fact it's reported with reportPossiblyNonexistentConstantArrayOffset https://phpstan.org/r/42af1687-c88d-4369-8c0b-7d3d1a14fcaa

• Array offset in OR condition can not be resolved. phpstan#6379: this was supposed to be fixed but wasn't too: the array shape was transformed into an intersection non-empty-array&array shape, so the error was not reported anymore.
  => A way to see this is the fact it's reported with reportPossiblyNonexistentConstantArrayOffset https://phpstan.org/r/656d35b8-2ea0-4207-99b7-cd218f2151b9

What should I do in such case ? Removing the non-regression test and you'll open the two issues @ondrejmirtes ?

[VincentLanglet]

What I currently do not fix:

/**
 * @param array{foo?: string, bar?: string, 1?:1, 2?:2, 3?:3, 4?:4, 5?:5, 6?:6, 7?:7, 8?:8, 9?:9}&non-empty-array $arr
 */

Because when there is more than 10 optional keys getAllArrays return [] and an array with all the existing keys ; and then the intersection with non-empty-array removes the empty array...

[staabm]

I think the previous behaviour is intentional, as non-empty-array{'foo'?: 1, 'bar'?: 2} means at least one of the 2 offsets is set (because its non-empty) but we don't know better and therefore play it safe (to not be annoying)

[VincentLanglet]

I think the previous behaviour is intentional, as non-empty-array{'foo'?: 1, 'bar'?: 2} means at least one of the 2 offsets is set (because its non-empty) but we don't know better and therefore play it safe

I don't think it's intentional. The "play it safe" argument is ok on level < 7, but in my mind that's exactly the purpose of "reportMaybe". Here, even if the array is non empty, we cannot be sure which key si non-optional so we have to report the key might not exists.

You have the same issue (as showed by https://phpstan.org/r/a9660963-948d-4d4b-9cb8-166af8806987) with

list{0: non-falsy-string, 1: non-empty-string, 2: numeric-string, 3: string, 4?: string, 5?: numeric-string}

which doesn't report that offset 4 might not exists BECAUSE it's a list, and as soon as you unset the 0 value it's reported
https://phpstan.org/r/c4ad2970-a61a-42a4-8600-22a889a7b737

In phpstan/phpstan#7143 the same can be reproduce with unset($arr['foo']); which starts triggering the might-not-exists error on bar.

[VincentLanglet]

Failure

1) PHPStan\Rules\Arrays\NonexistentOffsetInArrayDimFetchRuleTest::testBug6379
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'
+'16: Offset 'c' might not exist on non-empty-array{cr?: string, c?: string}.
 '

/home/runner/work/phpstan-src/phpstan-src/src/Testing/RuleTestCase.php:179
/home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Rules/Arrays/NonexistentOffsetInArrayDimFetchRuleTest.php:390

2) PHPStan\Rules\Arrays\NonexistentOffsetInArrayDimFetchRuleTest::testBug11602
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'
+'16: Offset 5 might not exist on list{0: non-falsy-string, 1: non-empty-string, 2: numeric-string, 3: string, 4?: string, 5?: numeric-string}.
+19: Offset 4 might not exist on list{0: non-falsy-string, 1: non-empty-string, 2: numeric-string, 3: string, 4?: string, 5?: numeric-string}.

are expected, those bug was never fixed it just stopped to be reported because the array was non-empty.

So we have to remove the tests and reopen the issues...

[VincentLanglet]

I don't think

$type instanceof IntersectionType && $type->isConstantArray()->maybe();

is something doable... so I dunno how to cover the mutant

[VincentLanglet]

Funny thing, fixing it is breaking some non-regression tests like

• Misinterpreting Whether Regular Expression Piece Exists phpstan#11602: this was supposed to be fixed by RegexArrayShapeMatcher - enforce list-type when no named captures #3899 but wasn't: the array shape was transformed into an intersection list&array shape, that's why the error wasn't reported anymore. (cc @staabm)
  => A way to see this is the fact it's reported with reportPossiblyNonexistentConstantArrayOffset phpstan.org/r/42af1687-c88d-4369-8c0b-7d3d1a14fcaa
• Array offset in OR condition can not be resolved. phpstan#6379: this was supposed to be fixed but wasn't too: the array shape was transformed into an intersection non-empty-array&array shape, so the error was not reported anymore.
  => A way to see this is the fact it's reported with reportPossiblyNonexistentConstantArrayOffset phpstan.org/r/656d35b8-2ea0-4207-99b7-cd218f2151b9

Do you agree on reopening those 2 issues @ondrejmirtes @staabm ?

[VincentLanglet]

RoaveBetter new errors are expected

[staabm]

getting this error back would be frustrating for the phpstan user.

what needs to be done, that we can make the error not show up here?
how does the type look like in $matches and what needs to change so we don't get the error back?

[VincentLanglet]

getting this error back would be frustrating for the phpstan user.

Unlike you I never worked on regex with PHPStan. I don't even understand it well.
If I understand correctly, if the third element exists, both the fourth and fifth should exists ?

According to phpstan/phpstan#11602 (comment) you said it would require a tagged union for this specific case. So $matches should be understood as

list{0: non-falsy-string, 1: non-empty-string, 2: numeric-string}|list{0: non-falsy-string, 1: non-empty-string, 2: numeric-string, 3: string, 4: string, 5: numeric-string}

[staabm]

said differently: the fix this PR provides for a maybe not-so-important problem feels less important to me than getting back a regression in PHPStan preg_match() handling.

I don't say this PR is not correct, but I feel we should make sure we don't need to regress at least the preg_match() case before merging.
(this might mean I need to dive into PHPStan regex handling)

thats my personal opinion though.

[VincentLanglet]

the fix this PR provides for a maybe not-so-important problem feels less important to me than getting back a regression in PHPStan preg_match() handling.

That's really opinionated and debated for at least two reason

• "getting back a regression in PHPStan preg_match() handling" => This is not a real regression in preg_match handling, since the preg_match handling is already not fully working as explained you can look at https://phpstan.org/r/a6d39b21-3365-4b4a-bfa8-6b111b6a9c7a the false-error is already reported with reportPossiblyNonexistentConstantArrayOffset (since the preg_match is not correctly handled).

• "maybe not-so-important problem feels less important", I feel like the whole preg_match issue could be solved on developper side by checking if (!isset($matches[5])) instead of $matches[3]. Since you improved the preg_match result as a list, it should imply isset for the key 3 and 4.

Seems like it currently does not https://phpstan.org/r/74ac6bac-5ac9-41b6-987d-281bad7a771d and could be fixed in the next following PR. WDYT ?