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

LoopAnalyzer crash on undefined $this->property key in $loop_scope->loop_parent_context->vars_in_scope #5848

Closed
Ocramius opened this issue May 28, 2021 · 6 comments

Comments

@Ocramius
Copy link
Contributor

Ocramius commented May 28, 2021

I get a crash in LoopAnalyzer when analyzing code like https://psalm.dev/r/3c9f6e100f (some of it simplified for the sake of excluding external factors):

lots of code:
<?php

declare(strict_types=1);

interface ApiClient {
    public function get(string $endpoint, array $criteria): array;
}

abstract class Criteria
{
    public ?int $limit = null;
    public ?int $page = null;

    /**
     * @psalm-return array{
     *     limit?: int,
     *     page?: int,
     * }
     */
    public function toArray(): array
    {
        return array_filter([
            'limit' => $this->limit,
            'page' => $this->page,
        ]);
    }
}


final class BatchIterator
{
    private ApiClient $client;
    /** @psalm-var non-empty-string */
    private string $endpoint;
    private Criteria $requestedCriteria;
    /** @psalm-var callable(array): array $collectionParser */
    private $collectionParser;

    /**
     * @psalm-param non-empty-string $endpoint
     * @psalm-param callable(array): list<array> $collectionParser
     */
    public function __construct(
        ApiClient $client,
        string $endpoint,
        Criteria $criteria,
        callable $collectionParser,
    ) {
        $this->client = $client;
        $this->endpoint = $endpoint;
        $this->requestedCriteria = clone $criteria;
        $this->collectionParser = $collectionParser;
    }

    /**
     * @template T
     * @psalm-param callable(array): T $itemParser
     * @psalm-param positive-int $batchSize
     * @psalm-return iterable<T>
     */
    public function getIterable(callable $itemParser, int $batchSize = 25): iterable
    {
        $batchCriteria = clone $this->requestedCriteria;

        $currentPage = $batchCriteria->page = $batchCriteria->page ?? 1;

        // If the requested limit is smaller than the batch size we can optimize the response by only fetching the requested amount.
        $batchCriteria->limit = ($this->requestedCriteria->limit && $this->requestedCriteria->limit < $batchSize)
            ? $this->requestedCriteria->limit
            : $batchSize;

        $fetchedItems = 0;

        do {
            $response = $this->client->get($this->endpoint, $batchCriteria->toArray());

            $this->requestedCriteria->limit = $this->requestedCriteria->limit ?? 0;

            /** @psalm-var list<array> $items */
            $items = ($this->collectionParser)($response);

            if ([] === $items) {
                break;
            }

            foreach ($items as $item) {
                if ($this->requestedCriteria->limit <= $fetchedItems) {
                    break;
                }
                yield $itemParser($item);
                ++$fetchedItems;
            }

            $batchCriteria->page = ++$currentPage;
        } while ($this->requestedCriteria->limit > $fetchedItems);
    }
}

Overall, the issue doesn't seem to be reproducible with psalm.dev, but the trace looks like this:

❯ ./vendor/bin/psalm --no-cache
Scanning files...
Analyzing files...

░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░   60 / 1293 (4%)
░░░░░░░░░░░Uncaught Exception: PHP Error: Undefined array key "$this->requestedCriteria" in vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php:428
Stack trace in the forked worker:
#0 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php(428): Psalm\Internal\ErrorHandler::Psalm\Internal\{closure}()
#1 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php(97): Psalm\Internal\Analyzer\Statements\Block\LoopAnalyzer::analyze()
#2 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php(498): Psalm\Internal\Analyzer\Statements\Block\DoAnalyzer::analyze()
#3 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php(172): Psalm\Internal\Analyzer\StatementsAnalyzer::analyzeStatement()
#4 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php(432): Psalm\Internal\Analyzer\StatementsAnalyzer->analyze()
#5 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/ClassAnalyzer.php(1780): Psalm\Internal\Analyzer\FunctionLikeAnalyzer->analyze()
#6 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/ClassAnalyzer.php(403): Psalm\Internal\Analyzer\ClassAnalyzer->analyzeClassMethod()
#7 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/FileAnalyzer.php(213): Psalm\Internal\Analyzer\ClassAnalyzer->analyze()
#8 vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(343): Psalm\Internal\Analyzer\FileAnalyzer->analyze()
#9 vendor/vimeo/psalm/src/Psalm/Internal/Fork/Pool.php(193): Psalm\Internal\Codebase\Analyzer->Psalm\Internal\Codebase\{closure}()
#10 vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(409): Psalm\Internal\Fork\Pool->__construct()
#11 vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(272): Psalm\Internal\Codebase\Analyzer->doAnalysis()
#12 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php(636): Psalm\Internal\Codebase\Analyzer->analyzeFiles()
#13 vendor/vimeo/psalm/src/psalm.php(700): Psalm\Internal\Analyzer\ProjectAnalyzer->check()
#14 vendor/vimeo/psalm/src/psalm.php(884): Psalm\{closure}()
#15 vendor/vimeo/psalm/psalm(2): require_once('...')
#16 {main} in vendor/vimeo/psalm/src/Psalm/Internal/Fork/Pool.php:365
Stack trace:
#0 vendor/vimeo/psalm/src/Psalm/Internal/Fork/Pool.php(399): Psalm\Internal\Fork\Pool->readResultsFromChildren()
#1 vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(480): Psalm\Internal\Fork\Pool->wait()
#2 vendor/vimeo/psalm/src/Psalm/Internal/Codebase/Analyzer.php(272): Psalm\Internal\Codebase\Analyzer->doAnalysis()
#3 vendor/vimeo/psalm/src/Psalm/Internal/Analyzer/ProjectAnalyzer.php(636): Psalm\Internal\Codebase\Analyzer->analyzeFiles()
#4 vendor/vimeo/psalm/src/psalm.php(700): Psalm\Internal\Analyzer\ProjectAnalyzer->check()
#5 vendor/vimeo/psalm/src/psalm.php(884): Psalm\{closure}()
#6 vendor/vimeo/psalm/psalm(2): require_once('...')
#7 {main}
(Psalm 4.7.3@38c452ae584467e939d55377aaf83b5a26f19dd1 crashed due to an uncaught Throwable)

Looking at the crash in more detail, I inspected the contents of PhpParser\Node\Stmt\Do_ in

} elseif ($stmt instanceof PhpParser\Node\Stmt\Do_) {
DoAnalyzer::analyze($statements_analyzer, $stmt, $context);
} elseif ($stmt instanceof PhpParser\Node\Stmt\Const_) {
by adding a try-catch:

        } elseif ($stmt instanceof PhpParser\Node\Stmt\Do_) {
            try {
                DoAnalyzer::analyze($statements_analyzer, [$stmt], $context);
            } catch (\Throwable $e) {
                \error_log((new PhpParser\PrettyPrinter\Standard())->prettyPrint($stmt));
                throw $e;
            }

The actual code in $stmt there is:

do {
    $response = $this->client->get($this->endpoint, $batchCriteria->toArray());
    $this->requestedCriteria->limit = $this->requestedCriteria->limit ?? 0;
    /** @psalm-var list<array> $items */
    $items = ($this->collectionParser)($response);
    if ([] === $items) {
        break;
    }
    foreach ($items as $item) {
        if ($this->requestedCriteria->limit <= $fetchedItems) {
            break;
        }
        (yield $itemParser($item));
        ++$fetchedItems;
    }
    $batchCriteria->page = ++$currentPage;
} while ($this->requestedCriteria->limit > $fetchedItems);

The problem seems to occur because $this->requestedCriteria is modified within the loop, when a break; occurs, but I'm unsure how to reproduce the issue in isolation 🤔

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/3c9f6e100f
<?php

declare(strict_types=1);

interface ApiClient {
    public function get(string $endpoint, array $criteria): array;
}

abstract class Criteria
{
    public ?int $limit = null;
    public ?int $page = null;

    /**
     * @psalm-return array{
     *     limit?: int,
     *     page?: int,
     * }
     */
    public function toArray(): array
    {
        return array_filter([
            'limit' => $this->limit,
            'page' => $this->page,
        ]);
    }
}


final class BatchIterator
{
    private ApiClient $client;
    /** @psalm-var non-empty-string */
    private string $endpoint;
    private Criteria $requestedCriteria;
    /** @psalm-var callable(array): array $collectionParser */
    private $collectionParser;

    /**
     * @psalm-param non-empty-string $endpoint
     * @psalm-param callable(array): list<array> $collectionParser
     */
    public function __construct(
        ApiClient $client,
        string $endpoint,
        Criteria $criteria,
        callable $collectionParser,
    ) {
        $this->client = $client;
        $this->endpoint = $endpoint;
        $this->requestedCriteria = clone $criteria;
        $this->collectionParser = $collectionParser;
    }

    /**
     * @template T
     * @psalm-param callable(array): T $itemParser
     * @psalm-param positive-int $batchSize
     * @psalm-return iterable<T>
     */
    public function getIterable(callable $itemParser, int $batchSize = 25): iterable
    {
        $batchCriteria = clone $this->requestedCriteria;

        $currentPage = $batchCriteria->page = $batchCriteria->page ?? 1;

        // If the requested limit is smaller than the batch size we can optimize the response by only fetching the requested amount.
        $batchCriteria->limit = ($this->requestedCriteria->limit && $this->requestedCriteria->limit < $batchSize)
            ? $this->requestedCriteria->limit
            : $batchSize;

        $fetchedItems = 0;

        do {
            $response = $this->client->get($this->endpoint, $batchCriteria->toArray());

            $this->requestedCriteria->limit = $this->requestedCriteria->limit ?? 0;

            /** @psalm-var list<array> $items */
            $items = ($this->collectionParser)($response);

            if ([] === $items) {
                break;
            }

            foreach ($items as $item) {
                if ($this->requestedCriteria->limit <= $fetchedItems) {
                    break;
                }
                yield $itemParser($item);
                ++$fetchedItems;
            }

            $batchCriteria->page = ++$currentPage;
        } while ($this->requestedCriteria->limit > $fetchedItems);
    }
}
Psalm output (using commit b259296):

No issues!

@Ocramius
Copy link
Contributor Author

Modifying the code as follows seems to make it work:

         $currentPage = $batchCriteria->page = $batchCriteria->page ?? 1;
+        $limit = $this->requestedCriteria->limit;
+        $collectionParser = $this->collectionParser;
 
         // If the requested limit is smaller than the batch size we can optimize the response by only fetching the requested amount.
         $batchCriteria->limit = ($this->requestedCriteria->limit && $this->requestedCriteria->limit < $batchSize)
@@ -73,17 +75,16 @@ final class BatchIterator
         do {
             $response = $this->client->get($this->endpoint, $batchCriteria->toArray());
 
-            $this->requestedCriteria->limit = $this->requestedCriteria->limit ?? 0;
+            $limit = $limit ?? 0;
 
            /** @psalm-var list<array> $items */
-            $items = ($this->collectionParser)($response);
+            $items = $collectionParser($response);
 
             if ([] === $items) {
                 break;
             }
 
             foreach ($items as $item) {
-                if ($this->requestedCriteria->limit <= $fetchedItems) {
+                if ($limit <= $fetchedItems) {
                     break;
                 }
                 yield $itemParser($item);

In practice, any $this->var has been removed from the do { ... } while (...) loop, and all is fine. Note that $this->collectionParser also wasn't recognized, indicating a potential issue in how parent scope is being populated.

@weirdan weirdan added the bug label May 28, 2021
@muglug
Copy link
Collaborator

muglug commented May 28, 2021

Are you running any plugins that manipulate the Context object?

@Ocramius
Copy link
Contributor Author

I'm running with @azjezz' psl plugin, as well as the phpunit plugin: will try removing it and re-running 👍

@Ocramius
Copy link
Contributor Author

Ocramius commented May 31, 2021

I just verified that this occurs also without plugins

@Ocramius
Copy link
Contributor Author

Closing here: I no longer have access to the codebase that leads to the crash, so this cannot be currently reproduced, sorry :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants