Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

preserve large arrays with same keys through union #3032

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Type/Constant/ConstantArrayType.php
Expand Up @@ -1557,7 +1557,7 @@ public function isKeysSupersetOf(self $otherArray): bool

public function mergeWith(self $otherArray): self
{
// only call this after verifying isKeysSupersetOf
// only call this after verifying isKeysSupersetOf, or if losing tagged unions is not an issue
$valueTypes = $this->valueTypes;
$optionalKeys = $this->optionalKeys;
foreach ($this->keyTypes as $i => $keyType) {
Expand Down
45 changes: 36 additions & 9 deletions src/Type/TypeCombinator.php
Expand Up @@ -643,7 +643,7 @@ private static function processArrayAccessoryTypes(array $arrayTypes): array
}

/**
* @param Type[] $arrayTypes
* @param list<Type> $arrayTypes
* @return Type[]
*/
private static function processArrayTypes(array $arrayTypes): array
Expand All @@ -669,9 +669,14 @@ private static function processArrayTypes(array $arrayTypes): array

/** @var int|float $nextConstantKeyTypeIndex */
$nextConstantKeyTypeIndex = 1;
$constantArraysMap = array_map(
static fn (Type $t) => $t->getConstantArrays(),
$arrayTypes,
);

foreach ($arrayTypes as $arrayType) {
$isConstantArray = $arrayType->isConstantArray()->yes();
foreach ($arrayTypes as $arrayIdx => $arrayType) {
$constantArrays = $constantArraysMap[$arrayIdx];
$isConstantArray = $constantArrays !== [];
if (!$isConstantArray || !$arrayType->isIterableAtLeastOnce()->no()) {
$filledArrays++;
}
Expand Down Expand Up @@ -708,6 +713,10 @@ private static function processArrayTypes(array $arrayTypes): array
}

if ($generalArrayOccurred && (!$overflowed || $filledArrays > 1)) {
$reducedArrayTypes = self::reduceArrays($arrayTypes, false);
if (count($reducedArrayTypes) === 1) {
return [self::intersect($reducedArrayTypes[0], ...$accessoryTypes)];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, currently the accessory types will always be empty (so I can't test this). But I added them anyway to be safe in case new accessory types are added in the future.

}
$scopes = [];
$useTemplateArray = true;
foreach ($arrayTypes as $arrayType) {
Expand Down Expand Up @@ -740,7 +749,7 @@ private static function processArrayTypes(array $arrayTypes): array
];
}

$reducedArrayTypes = self::reduceArrays($arrayTypes);
$reducedArrayTypes = self::reduceArrays($arrayTypes, true);

return array_map(
static fn (Type $arrayType) => self::intersect($arrayType, ...$accessoryTypes),
Expand Down Expand Up @@ -833,16 +842,21 @@ private static function countConstantArrayValueTypes(array $types): int
}

/**
* @param Type[] $constantArrays
* @return Type[]
* @param list<Type> $constantArrays
* @return list<Type>
*/
private static function reduceArrays(array $constantArrays): array
private static function reduceArrays(array $constantArrays, bool $preserveTaggedUnions): array
{
$newArrays = [];
$arraysToProcess = [];
$emptyArray = null;
foreach ($constantArrays as $constantArray) {
if (!$constantArray->isConstantArray()->yes()) {
// This is an optimization for current use-case of $preserveTaggedUnions=false, where we need
// one constant array as a result, or we generalize the $constantArrays.
if (!$preserveTaggedUnions) {
return $constantArrays;
}
Comment on lines +855 to +859
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm conflicted about this. On one hand, it would feel bad not to have this condition and run a lot of the method uselessly. But on the other hand, the condition depends on the particular usage of $preserveTaggedUnions. Maybe a better name for $preserveTaggedUnions would help, but I didn't come up with anything. So I at least added a comment.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine 👍

$newArrays[] = $constantArray;
continue;
}
Expand Down Expand Up @@ -888,7 +902,8 @@ private static function reduceArrays(array $constantArrays): array
}

if (
$overlappingKeysCount === count($arraysToProcess[$i]->getKeyTypes())
$preserveTaggedUnions
&& $overlappingKeysCount === count($arraysToProcess[$i]->getKeyTypes())
&& $arraysToProcess[$j]->isKeysSupersetOf($arraysToProcess[$i])
) {
$arraysToProcess[$j] = $arraysToProcess[$j]->mergeWith($arraysToProcess[$i]);
Expand All @@ -897,13 +912,25 @@ private static function reduceArrays(array $constantArrays): array
}

if (
$overlappingKeysCount === count($arraysToProcess[$j]->getKeyTypes())
$preserveTaggedUnions
&& $overlappingKeysCount === count($arraysToProcess[$j]->getKeyTypes())
&& $arraysToProcess[$i]->isKeysSupersetOf($arraysToProcess[$j])
) {
$arraysToProcess[$i] = $arraysToProcess[$i]->mergeWith($arraysToProcess[$j]);
unset($arraysToProcess[$j]);
continue 1;
}

if (
!$preserveTaggedUnions
// both arrays have same keys
&& $overlappingKeysCount === count($arraysToProcess[$i]->getKeyTypes())
&& $overlappingKeysCount === count($arraysToProcess[$j]->getKeyTypes())
) {
$arraysToProcess[$j] = $arraysToProcess[$j]->mergeWith($arraysToProcess[$i]);
unset($arraysToProcess[$i]);
continue 2;
}
}
}

Expand Down
3 changes: 3 additions & 0 deletions tests/PHPStan/Analyser/NodeScopeResolverTest.php
Expand Up @@ -1420,6 +1420,9 @@ public function dataFileAsserts(): iterable
yield from $this->gatherAssertTypes(__DIR__ . '/data/trigger-error-php7.php');
}

yield from $this->gatherAssertTypes(__DIR__ . '/data/preserve-large-constant-array.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-9397.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-10080.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/impure-error-log.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/falsy-isset.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/falsey-coalesce.php');
Expand Down
76 changes: 76 additions & 0 deletions tests/PHPStan/Analyser/data/bug-10080.php
@@ -0,0 +1,76 @@
<?php

namespace Bug10080;

/**
* @param array{
* a1?: string,
* a2?: string,
* a3?: string,
* a4?: string,
* a5?: string,
* a6?: string,
* a7?: string,
* a8?: string,
* a9?: string,
* a10?: string,
* a11?: string,
* a12?: string,
* a13?: string,
* a14?: string,
* a15?: string,
* a16?: string,
* a17?: string,
* a18?: string,
* a19?: string,
* a20?: string,
* a21?: string,
* a22?: string,
* a23?: string,
* a24?: string,
* a25?: string,
* a26?: string,
* a27?: string,
* a28?: string,
* a29?: string,
* a30?: string,
* a31?: string,
* a32?: string,
* a33?: string,
* a34?: string,
* a35?: string,
* a36?: string,
* a37?: string,
* a38?: string,
* a39?: string,
* a40?: string,
* a41?: string,
* a42?: string,
* a43?: string,
* a44?: string,
* a45?: string,
* a46?: string,
* a47?: string,
* a48?: string,
* a49?: string,
* a50?: string,
* a51?: string,
* a52?: string,
* a53?: string,
* a54?: string,
* a55?: string,
* a56?: string,
* a57?: string,
* a58?: string,
* a59?: string,
* a60?: string,
* a61?: string,
* a62?: string|string[]|int|float,
* a63?: string
* } $row
*/
function doStuff(array $row): void
{
\PHPStan\Testing\assertType('string', $row['a51'] ?? '');
\PHPStan\Testing\assertType('string', $row['a51'] ?? '');
}
101 changes: 101 additions & 0 deletions tests/PHPStan/Analyser/data/bug-9397.php
@@ -0,0 +1,101 @@
<?php declare(strict_types = 1);

namespace Bug9397;

use function PHPStan\Testing\assertType;

final class Money {
public static function zero(): Money {
return new Money();
}
}


class HelloWorld
{
/**
* @return array<int, array{
* foo1: Money,
* foo2: ?Money,
* foo3: string,
* foo4: string,
* foo5: string,
* foo6: string,
* foo7: string,
* foo8: string,
* foo9: string,
* foo10:string,
* foo11:int,
* foo12:int,
* foo13:int,
* foo14:int,
* foo15:int,
* foo16:int,
* foo17:int,
* foo18:int,
* foo19:int,
* foo20:int,
* foo21:bool,
* foo22:bool,
* foo23:bool,
* foo24:bool,
* foo25:bool,
* foo26:bool,
* foo27:bool,
* foo28:bool,
* foo29:bool,
* foo30:bool,
* foo31:bool,
* foo32:string,
* foo33:string,
* foo34:string,
* foo35:string,
* foo36:string,
* foo37:string,
* foo38:string,
* foo39:string,
* foo40:string,
* foo41:string,
* foo42:string,
* foo43:string,
* foo44:string,
* foo45:string,
* foo46:string,
* foo47:string,
* foo48:string,
* foo49:string,
* foo50:string,
* foo51:string,
* foo52:string,
* foo53:string,
* foo54:string,
* foo55:string,
* foo56:string,
* foo57:string,
* foo58:string,
* foo59:string,
* foo60:string,
* foo61:string,
* foo62:string,
* foo63:string,
* }>
* If the above type has 63 or more properties, the bug occurs
*/
private static function callable(): array {
return [];
}

public function callsite(): void {
$result = self::callable();
foreach ($result as $id => $p) {
assertType(Money::class, $p['foo1']);
assertType(Money::class . '|null', $p['foo2']);
assertType('string', $p['foo3']);

$baseDeposit = $p['foo2'] ?? Money::zero();
assertType(Money::class, $p['foo1']);
assertType(Money::class . '|null', $p['foo2']);
assertType('string', $p['foo3']);
}
}
}
67 changes: 67 additions & 0 deletions tests/PHPStan/Analyser/data/preserve-large-constant-array.php
@@ -0,0 +1,67 @@
<?php declare(strict_types=1);

namespace PreserveLargeConstantArray;

use function PHPStan\Testing\assertType;

/**
* @param array{1: string|null, 2: int|null, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float} $arr
*/
function multiKeys(array $arr): void
{
if ($arr[1] !== null && $arr[2] !== null) {
$val = 1;
} elseif ($arr[1] === null && $arr[2] === null) {
$val = 2;
} else {
return;
}

assertType('array{1: string|null, 2: int|null, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float}', $arr);
echo 1;
}

/**
* @param array{1: string|null, 2: int, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float} $arr
*/
function simpleUnion(array $arr): void
{
$val = $arr[1] !== null
? $arr[1]
: null;
assertType('array{1: string|null, 2: int, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float}', $arr);
echo 1;
}

/**
* @param array{1?: string, 2: int, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float} $arr
*/
function optionalKey(array $arr): void
{
$val = isset($arr[1])
? $arr[1]
: null;
assertType('array{1?: string, 2: int, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float}', $arr);
echo 1;
}

/**
* @param array{1: string, 2: int, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float} $arr
*/
function multipleOptions(array $arr): void
{
if ($arr[1] === 'a') {
$brr = $arr;
$brr[1] = 'b';
} elseif ($arr[1] === 'b') {
$brr = $arr;
$brr[1] = 'c';
} elseif ($arr[1] === 'c') {
$brr = $arr;
$brr[1] = 'd';
} else {
$brr = $arr;
}
assertType('array{1: string, 2: int, 3: bool, 4: string, 5: int, 6: bool, 7: string, 8: int, 9: bool, 10: string, 11: int, 12: bool, 13: string, 14: int, 15: bool, 16: string, 17: int, 18: bool, 19: string, 20: int, 21: bool, 22: string, 23: int, 24: bool, 25: string, 26: int, 27: bool, 28: string, 29: int, 30: bool, 31: string, 32: int, 33: bool, 34: string, 35: int, 36: bool, 37: string, 38: int, 39: bool, 40: string, 41: int, 42: bool, 43: string, 44: int, 45: bool, 46: string, 47: int, 48: bool, 49: string, 50: int, 51: bool, 52: string, 53: int, 54: bool, 55: string, 56: int, 57: bool, 58: string, 59: int, 60: bool, 61: string, 62: int, 63: bool, 64: float}', $brr);
echo 1;
}