Skip to content

Commit

Permalink
Improve TypeCombinator::reduceArrays() perf with retained type comple…
Browse files Browse the repository at this point in the history
…teness
  • Loading branch information
rvanvelzen authored and ondrejmirtes committed Sep 23, 2022
1 parent 3bfe2e6 commit 7190fb7
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 57 deletions.
95 changes: 39 additions & 56 deletions src/Type/TypeCombinator.php
Expand Up @@ -637,96 +637,79 @@ private static function processArrayTypes(array $arrayTypes, array $accessoryTyp
return [
self::intersect(new ArrayType(
self::union(...$keyTypesForGeneralArray),
self::union(...self::optimizeConstantArrays($valueTypesForGeneralArray)),
self::union(...$valueTypesForGeneralArray),
), ...$accessoryTypes),
];
}

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

return array_map(
static fn (Type $arrayType) => self::intersect($arrayType, ...$accessoryTypes),
self::optimizeConstantArrays($reducedArrayTypes),
self::reduceArrays($arrayTypes),
);
}

/**
* @param Type[] $types
* @param Type[] $constantArrays
* @return Type[]
*/
private static function optimizeConstantArrays(array $types): array
private static function reduceArrays(array $constantArrays): array
{
$constantArrayValuesCount = self::countConstantArrayValueTypes($types);

if ($constantArrayValuesCount > ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT) {
$results = [];
foreach ($types as $type) {
$results[] = TypeTraverser::map($type, static function (Type $type, callable $traverse): Type {
if ($type instanceof ConstantArrayType) {
return $type->generalize(GeneralizePrecision::moreSpecific());
}
$newArrays = [];
$arraysToProcess = [];
$emptyArray = null;
foreach ($constantArrays as $constantArray) {
if (!$constantArray instanceof ConstantArrayType) {
$newArrays[] = $constantArray;
continue;
}

return $traverse($type);
});
if ($constantArray->isEmpty()) {
$emptyArray = $constantArray;
continue;
}

return $results;
$arraysToProcess[] = $constantArray;
}

return $types;
}
if ($emptyArray !== null) {
$newArrays[] = $emptyArray;
}

/**
* @param Type[] $types
*/
private static function countConstantArrayValueTypes(array $types): int
{
$constantArrayValuesCount = 0;
foreach ($types as $type) {
if ($type instanceof ConstantArrayType) {
$constantArrayValuesCount += count($type->getValueTypes());
$arraysToProcessPerKey = [];
foreach ($arraysToProcess as $i => $arrayToProcess) {
foreach ($arrayToProcess->getKeyTypes() as $keyType) {
$arraysToProcessPerKey[$keyType->getValue()][] = $i;
}
}

TypeTraverser::map($type, static function (Type $type, callable $traverse) use (&$constantArrayValuesCount): Type {
if ($type instanceof ConstantArrayType) {
$constantArrayValuesCount += count($type->getValueTypes());
}
$eligibleCombinations = [];

return $traverse($type);
});
foreach ($arraysToProcessPerKey as $arrays) {
for ($i = 0, $arraysCount = count($arrays); $i < $arraysCount - 1; $i++) {
for ($j = $i + 1; $j < $arraysCount; $j++) {
$eligibleCombinations[$arrays[$i]][$arrays[$j]] = $arrays[$j];
}
}
}
return $constantArrayValuesCount;
}

/**
* @param Type[] $constantArrays
* @return Type[]
*/
private static function reduceArrays(array $constantArrays): array
{
$newArrays = [];
$arraysToProcess = [];
foreach ($constantArrays as $constantArray) {
if (!$constantArray instanceof ConstantArrayType) {
$newArrays[] = $constantArray;
foreach ($eligibleCombinations as $i => $other) {
if (!array_key_exists($i, $arraysToProcess)) {
continue;
}

$arraysToProcess[] = $constantArray;
}
foreach ($other as $j) {
if (!array_key_exists($j, $arraysToProcess)) {
continue;
}

for ($i = 0, $arraysToProcessCount = count($arraysToProcess); $i < $arraysToProcessCount; $i++) {
for ($j = $i + 1; $j < $arraysToProcessCount; $j++) {
if ($arraysToProcess[$j]->isKeysSupersetOf($arraysToProcess[$i])) {
$arraysToProcess[$j] = $arraysToProcess[$j]->mergeWith($arraysToProcess[$i]);
array_splice($arraysToProcess, $i--, 1);
$arraysToProcessCount--;
unset($arraysToProcess[$i]);
continue 2;

} elseif ($arraysToProcess[$i]->isKeysSupersetOf($arraysToProcess[$j])) {
$arraysToProcess[$i] = $arraysToProcess[$i]->mergeWith($arraysToProcess[$j]);
array_splice($arraysToProcess, $j--, 1);
$arraysToProcessCount--;
unset($arraysToProcess[$j]);
continue 1;
}
}
Expand Down
14 changes: 13 additions & 1 deletion tests/PHPStan/Analyser/AnalyserIntegrationTest.php
Expand Up @@ -931,7 +931,19 @@ public function testBug7581(): void
public function testBug7903(): void
{
$errors = $this->runAnalyse(__DIR__ . '/data/bug-7903.php');
$this->assertNoErrors($errors);
$this->assertCount(6, $errors);
$this->assertSame('Comparison operation ">" between 0 and 0 is always false.', $errors[0]->getMessage());
$this->assertSame(212, $errors[0]->getLine());
$this->assertSame('Comparison operation ">" between 0 and 0 is always false.', $errors[1]->getMessage());
$this->assertSame(213, $errors[1]->getLine());
$this->assertSame('Comparison operation ">" between 0 and 0 is always false.', $errors[2]->getMessage());
$this->assertSame(214, $errors[2]->getLine());
$this->assertSame('Comparison operation ">" between 0 and 0 is always false.', $errors[3]->getMessage());
$this->assertSame(215, $errors[3]->getLine());
$this->assertSame('Comparison operation ">" between 0 and 0 is always false.', $errors[4]->getMessage());
$this->assertSame(229, $errors[4]->getLine());
$this->assertSame('Comparison operation ">" between 0 and 0 is always false.', $errors[5]->getMessage());
$this->assertSame(230, $errors[5]->getLine());
}

public function testBug7901(): void
Expand Down
1 change: 1 addition & 0 deletions tests/PHPStan/Analyser/NodeScopeResolverTest.php
Expand Up @@ -1041,6 +1041,7 @@ public function dataFileAsserts(): iterable
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-7987.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-7963-three.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-8017.php');
yield from $this->gatherAssertTypes(__DIR__ . '/data/bug-8004.php');
}

/**
Expand Down
114 changes: 114 additions & 0 deletions tests/PHPStan/Analyser/data/bug-8004.php
@@ -0,0 +1,114 @@
<?php

namespace Bug8004;

use function PHPStan\Testing\assertType;

class Importer
{
/**
* @param array{questions: array<int, array{question: string|null, answer_1: string|\DateTimeInterface|null, answer_2: string|\DateTimeInterface|null, answer_3: string|\DateTimeInterface|null, answer_4: string|\DateTimeInterface|null, right_answer: int|float|string|\DateTimeInterface|null, right_answer_explanation: string|null}>} $importQuiz
*
* @return list<array{line: int, type: QuizImportErrorType::*, value: int|float|string|\DateTimeInterface|null}>
*/
public function getErrorsOnInvalidQuestions(array $importQuiz, int $key): array
{
$errors = [];

foreach ($importQuiz['questions'] as $index => $question) {
if (empty($question['question']) && empty($question['answer_1']) && empty($question['answer_2']) && empty($question['answer_3']) && empty($question['answer_4']) && empty($question['right_answer']) && empty($question['right_answer_explanation'])) {
continue;
}

if (empty($question['question'])) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::EMPTY_QUESTION, 'value' => $index + 1];
} elseif (255 < mb_strlen($question['question'])) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_QUESTION_TOO_LONG, 'value' => $index + 1];
}

if (null === $question['answer_1'] || '' === $question['answer_1'] || null === $question['answer_2'] || '' === $question['answer_2']) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::EMPTY_ANSWER, 'value' => $index + 1];
}

if (null !== $question['answer_1'] && '' !== $question['answer_1']) {
if (\is_string($question['answer_1']) && 150 < mb_strlen($question['answer_1'])) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_1_TOO_LONG, 'value' => $index + 1];
} elseif ($question['answer_1'] instanceof \DateTimeInterface) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_1_TYPE, 'value' => $index + 1];
}
}

if (null !== $question['answer_2'] && '' !== $question['answer_2']) {
if (\is_string($question['answer_2']) && 150 < mb_strlen($question['answer_2'])) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_2_TOO_LONG, 'value' => $index + 1];
} elseif ($question['answer_2'] instanceof \DateTimeInterface) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_2_TYPE, 'value' => $index + 1];
}
}

$hasQuestion3 = isset($question['answer_3']) && null !== $question['answer_3'] && '' !== $question['answer_3'];

if ($hasQuestion3) {
if (\is_string($question['answer_3']) && 150 < mb_strlen($question['answer_3'])) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_3_TOO_LONG, 'value' => $index + 1];
} elseif ($question['answer_3'] instanceof \DateTimeInterface) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_3_TYPE, 'value' => $index + 1];
}
}

$hasQuestion4 = isset($question['answer_4']) && null !== $question['answer_4'] && '' !== $question['answer_4'];

if ($hasQuestion4) {
if (\is_string($question['answer_4']) && 150 < mb_strlen($question['answer_4'])) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_4_TOO_LONG, 'value' => $index + 1];
} elseif ($question['answer_4'] instanceof \DateTimeInterface) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_ANSWER_4_TYPE, 'value' => $index + 1];
}
}

$answerCount = 2 + ($hasQuestion3 ? 1 : 0) + ($hasQuestion4 ? 1 : 0);

if (empty($question['right_answer']) || !is_numeric($question['right_answer']) || $question['right_answer'] <= 0 || (int) $question['right_answer'] > $answerCount) {
$errors[] = ['line' => $key, 'type' => QuizImportErrorType::INVALID_RIGHT_ANSWER, 'value' => $index + 1];
}
}

assertType("array<int, array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_answer_2_type'|'invalid_answer_3_too_long'|'invalid_answer_3_type'|'invalid_answer_4_too_long'|'invalid_answer_4_type'|'invalid_question_too_long'|'invalid_right_answer', value: int}>", $errors);

return $errors;
}
}

class QuizImportErrorType
{
public const EMPTY_ANSWER = 'empty_answer';
public const EMPTY_BONUS_DURATION = 'empty_bonus_duration';
public const EMPTY_BONUS_POINTS = 'empty_bonus_points';
public const EMPTY_COLLECTION = 'empty_collection';
public const EMPTY_INTRODUCTION = 'empty_introduction';
public const EMPTY_QUESTION = 'empty_question';
public const EMPTY_TITLE = 'empty_title';
public const INVALID_ANSWER_1_TOO_LONG = 'invalid_answer_1_too_long';
public const INVALID_ANSWER_2_TOO_LONG = 'invalid_answer_2_too_long';
public const INVALID_ANSWER_3_TOO_LONG = 'invalid_answer_3_too_long';
public const INVALID_ANSWER_4_TOO_LONG = 'invalid_answer_4_too_long';
public const INVALID_ANSWER_1_TYPE = 'invalid_answer_1_type';
public const INVALID_ANSWER_2_TYPE = 'invalid_answer_2_type';
public const INVALID_ANSWER_3_TYPE = 'invalid_answer_3_type';
public const INVALID_ANSWER_4_TYPE = 'invalid_answer_4_type';
public const INVALID_AUTHOR = 'invalid_author';
public const INVALID_BONUS_DURATION = 'invalid_bonus_duration';
public const INVALID_BONUS_POINTS = 'invalid_bonus_points';
public const INVALID_CLOSING_DATE = 'invalid_closing_date';
public const INVALID_COLLECTION = 'invalid_collection';
public const INVALID_NEWS_FEED = 'invalid_news_feed';
public const INVALID_PARTICIPATION_POINTS = 'invalid_participation_points';
public const INVALID_PERFECT_POINTS = 'invalid_perfect_points';
public const INVALID_PUBLICATION_DATE = 'invalid_publication_date';
public const INVALID_QUESTION_TOO_LONG = 'invalid_question_too_long';
public const INVALID_RESPONSE_TIME = 'invalid_response_time';
public const INVALID_RIGHT_ANSWER = 'invalid_right_answer';
public const INVALID_RIGHT_ANSWER_POINTS = 'invalid_right_answer_points';
public const INVALID_TITLE_TOO_LONG = 'invalid_title_too_long';
public const INVALID_WRONG_ANSWER_POINTS = 'invalid_wrong_answer_points';
}

0 comments on commit 7190fb7

Please sign in to comment.