Fix array + array inference#4944
Conversation
2d48e86 to
c65ac7f
Compare
|
This pull request has been marked as ready for review. |
| if ($leftCount > 0) { | ||
| // Use the first constant array as a reference to list potential offsets. | ||
| // We only need to check the first array because we're looking for offsets that exist in ALL arrays. | ||
| $constantArray = $leftConstantArrays[0]; |
There was a problem hiding this comment.
I am not sure why this only works on index 0.
There was a problem hiding this comment.
This is something you could try for array_merge too I think.
Since we're looking only for certainty of hasOffsetValueType YES the key has to be in all the constant arrays, so checking only one constant array is enough.
- only keys in the first constant array can have YES certainty
- key which are not in the first contant array will at most have the MAYBE certainty
There was a problem hiding this comment.
do you think it would make sense to adjust array_merge() and array_replace() with this PR, so we have it in sync over all implementations?
There was a problem hiding this comment.
Honestly I'm not sure about it.
Those implementations seems more complicated, especially because there is an iteration
foreach ($argTypes as $argType) {
Also the logic seems to be different since there is some looking at code like
TrinaryLogic::createFromBoolean($argType->hasOffsetValueType($keyType)->yes());
or
if ($hasOffsetValue->yes()) {
// the last string-keyed offset will overwrite previous values
$hasOffsetType = new HasOffsetValueType(
$keyType,
$offsetType,
);
} elseif ($hasOffsetValue->maybe()) {
$hasOffsetType = new HasOffsetType(
$keyType,
);
} else {
continue;
}
which I don't have here.
So I would prefer avoid to delay with PR (with the risk of introducing a regression) and fixing the existing bug, and we could try a dedicated PR with the refacto for array_merge/replace ?
Closes phpstan/phpstan#13561