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

phpstan 1.8.5 reports weird failures about the returned array type when returning a list of shapes #8004

Closed
stof opened this issue Sep 13, 2022 · 6 comments · Fixed by phpstan/phpstan-src#2137
Labels

Comments

@stof
Copy link
Contributor

stof commented Sep 13, 2022

Bug report

Code snippet that reproduces the problem

<?php

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];
            }
        }

        return $errors;
    }
}

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

This code snippet currently breaks the REPL on phpstan.org (the API returns a 502 bad gateway error)

Expected output

no error (as was the case in 1.8.1).

Note that adding this phpdoc removes the reported error:

+        /**
+         * @var list<array{line: int, type: QuizImportErrorType::*, value: int|float|string|\DateTimeInterface|null}>
+         */
         $errors = [];

Did PHPStan help you today? Did it make you happy in any way?

@mergeable
Copy link

mergeable bot commented Sep 13, 2022

This bug report is missing a link to reproduction at phpstan.org/try.

It will most likely be closed after manual review.

@VincentLanglet
Copy link
Contributor

Could you add the local error message since the code snippet breaks phpstan.org ?

I would say it's similar to #7963, too complicated array are now simplified when the type is computed by phpstan (which explain why there is no error with the @var annotation).

@stof
Copy link
Contributor Author

stof commented Sep 14, 2022

Method Importer::getErrorsOnInvalidQuestions() should return array<int, array{line: int, type: 'empty_answer'|
'empty_bonus_duration'|'empty_bonus_points'|'empty_collection'|'empty_introduction'|'empty_question'|
'empty_title'|'invalid_answer_1…'|'invalid_answer_1…'|'invalid_answer_2…'|'invalid_answer_2…'|
'invalid_answer_3…'|'invalid_answer_3…'|'invalid_answer_4…'|'invalid_answer_4…'|'invalid_author'|
'invalid_bonus…'|'invalid_bonus_points'|'invalid_closing_date'|'invalid_collection'|'invalid_news_feed'|
'invalid…'|'invalid_perfect…'|'invalid_publication…'|'invalid_question…'|'invalid_response…'|
'invalid_right_answer'|'invalid_right…'|'invalid_title_too…'|'invalid_wrong…',
value: DateTimeInterface|float|int|string|null}> 
but returns array<int, array<literal-string&non-falsy-string, int|(literal-string&non-falsy-string)>>.

@VincentLanglet
Copy link
Contributor

Definitely related then. The array shape is "just" simplified when inferred by PHPStan.

@ondrejmirtes
Copy link
Member

/cc @rvanvelzen This one is definitely interesting, I thought that your Arnaud-inspired array collapse improvements would fix this but it doesn't. But I don't understand the behaviour.

When I analyse this locally, the type of $errors before return is:

array<int, array<literal-string&non-falsy-string, int|(literal-string&non-falsy-string)>>

When I increase the limit in optimizeConstantArrays from ConstantArrayTypeBuilder::ARRAY_COUNT_LIMIT to like 2048 (so that the optimization is skipped!), the analysis passes and the dumped type is:

array<int, array{line: int, type:
         'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_a
         nswer_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}>

Which is quite nice and small. And I don't understand why I need to increase the optimization limit when the resulting array type is actually this small!?

When I revert to the original optimization limit and dump the types that are optimized, they look like this:

click for more
string(574) "array{0: array{line: int, type: 'invalid_right_answer', value: int}, 1: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long', value: int}, 2?: array{line: int, type: 'empty_answer'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 3?: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 4?: array{line: int, type: 'invalid_answer_2_too_long', value: int}}"
string(807) "array{0: array{line: int, type: 'invalid_answer_4_too_long'|'invalid_answer_4_type', value: int}, 1: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 2?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long', value: int}, 3?: array{line: int, type: 'empty_answer'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 4?: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 5?: array{line: int, type: 'invalid_answer_2_too_long', value: int}}"
string(1063) "array{0: array{line: int, type: 'invalid_answer_3_too_long'|'invalid_answer_3_type', value: int}, 1: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_answer_4_too_long'|'invalid_answer_4_type'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 2?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 3?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long', value: int}, 4?: array{line: int, type: 'empty_answer'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 5?: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 6?: array{line: int, type: 'invalid_answer_2_too_long', value: int}}"
string(1372) "array{0: array{line: int, type: 'invalid_answer_2_too_long'|'invalid_answer_2_type', value: int}, 1?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'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}, 2?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_answer_4_too_long'|'invalid_answer_4_type'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 3?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 4?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long', value: int}, 5?: array{line: int, type: 'empty_answer'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 6?: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 7?: array{line: int, type: 'invalid_answer_2_too_long', value: int}}"
string(1703) "array{0: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type', value: int}, 1: 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}, 2?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'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}, 3?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_answer_4_too_long'|'invalid_answer_4_type'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 4?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 5?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long', value: int}, 6?: array{line: int, type: 'empty_answer'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 7?: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 8?: array{line: int, type: 'invalid_answer_2_too_long', value: int}}"
string(1998) "array{0: array{line: int, type: 'empty_answer', value: int}, 1: 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}, 2?: 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}, 3?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'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}, 4?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_answer_4_too_long'|'invalid_answer_4_type'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 5?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 6?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long', value: int}, 7?: array{line: int, type: 'empty_answer'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 8?: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 9?: array{line: int, type: 'invalid_answer_2_too_long', value: int}}"
string(2361) "array{0: array{line: int, type: 'empty_question'|'invalid_question_too_long', value: int}, 1: 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}, 2?: 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}, 3?: 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}, 4?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'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}, 5?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_answer_4_too_long'|'invalid_answer_4_type'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 6?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long'|'invalid_right_answer', value: int}, 7?: array{line: int, type: 'empty_answer'|'empty_question'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long'|'invalid_question_too_long', value: int}, 8?: array{line: int, type: 'empty_answer'|'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 9?: array{line: int, type: 'invalid_answer_1_too_long'|'invalid_answer_1_type'|'invalid_answer_2_too_long', value: int}, 10?: array{line: int, type: 'invalid_answer_2_too_long', value: int}}"

What I don't understand:

  1. Why the intermediary type is so messy?
  2. What reduces the type to something neat and small later? Because I don't think it's reduceArrays(). The method optimizeConstantArrays() is already called with the input that went through reduceArrays().

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants