Skip to content

Conversation

VincentLanglet
Copy link
Contributor

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

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

@VincentLanglet
Copy link
Contributor Author

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
Copy link
Contributor

staabm commented Oct 3, 2025

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
Copy link
Contributor Author

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.

Comment on lines 217 to 222
$optionalKeysCombinations = [
[],
array_slice($this->optionalKeys, 0, 1, true),
array_slice($this->optionalKeys, -1, 1, true),
$this->optionalKeys,
];
Copy link
Contributor Author

@VincentLanglet VincentLanglet Oct 3, 2025

Choose a reason for hiding this comment

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

When an array only have optionalKey AND is non-empty, the result of getAllArray intersected with NonEmptyArray will give an array with all the keys and no key will be reported with

$innerType->hasOffsetValueType($innerDimType)->no()

So I have to

  • add a smallest array possible to report every optional key (except the first one)
  • add another smallest array possible to report the first optional key

If wanted, these two array could be added only if
count($this->keyTypes) === count($this->optionalKeys)

@VincentLanglet
Copy link
Contributor Author

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...

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.

Union of optional keyed arrays doesn't mean both keys exists at same time
2 participants