Skip to content

[6.x] Fix parallel scan crash on PHP-native intersection types#11841

Open
alies-dev wants to merge 1 commit into
vimeo:6.xfrom
alies-dev:6.x-fix-intersect-scan-race
Open

[6.x] Fix parallel scan crash on PHP-native intersection types#11841
alies-dev wants to merge 1 commit into
vimeo:6.xfrom
alies-dev:6.x-fix-intersect-scan-race

Conversation

@alies-dev
Copy link
Copy Markdown
Contributor

Scanning a codebase that uses PHP-native intersection types in function signatures (e.g. Map&ResultInterface&stdClass&Vector from azjezz/psl) intermittently crashed parallel scan workers with Could not get class storage for stdclass (or various other class names) thrown from ClassLikeStorageProvider::get.

The crash path is FunctionLikeNodeScanner::startgetTranslatedFunctionParamTypeHintResolver::resolve (intersection branch) → Type::intersectUnionTypes. Inside intersectUnionTypes there is a per-atomic-pair narrowing pass and a final union-level narrowing pass. The per-atomic pass already wraps UnionTypeComparator::isContainedBy in try { ... } catch (InvalidArgumentException) { /* Ignore non-existing classes during initial scan */ } because this exact scenario was anticipated. The final union-level narrowing block at line 808 was missing the same guard, so it bubbled the throw out of the worker.

That block only runs when $intersection_performed is false after the cross-product loop, which happens for pairs of TNamedObject where mayHaveIntersection returns false on at least one side (a final class with storage already populated). Whether a given parallel worker has the storage for the final class depends on the order in which it scanned files — Vector and Map in psl are both final readonly class, so workers that processed those files before intersection.php hit the crash, and workers that didn't processed it via the defensive mayHaveIntersection=true path. Hence the ~50% crash rate. Confirmed via debug logging: when mhi1=0 mhi2=1 (Vector storage present, ResultInterface storage absent) the unwrapped narrowing block ran and threw on the missing class.

The fix wraps the union-level narrowing block in the same try/catch already used at lines 928 and 988 of the same function. When containment can't be determined because storage is missing, the narrowing optimization is skipped and the un-narrowed type is returned, which is exactly what the user-visible "lighter-weight intersection that doesn't need containment data" should look like.

Verification:

  • 30+30+20 parallel runs of psl (Psalm 6.x with this fix) on a host that previously crashed ~50% of the time: zero crashes.
  • 20 runs of psl with --scan-threads=1: zero crashes, deterministic 3127 errors (matches single-thread baseline).
  • Full paratest suite: 21115 tests, 76 failures, 78 skipped — identical to the pre-fix baseline. The earlier draft of this fix that guarded classExtends directly broke 8 of those tests because InternalCallMapHandlerTest::assertTypeValidity explicitly catches InvalidArgumentException from that path as a "skip this assertion when storage isn't loaded" signal; that contract is preserved here.
  • Real-app benchmark across 14 apps (laravel-framework, symfony, drupal, sylius, rector, php-cs-fixer, monica, bagisto, etc.) shows the psl crash is gone and 13 of 14 apps are at exactly the same issue count as the pre-fix baseline. Two divergences worth noting: php-cs-fixer goes from 7,638 to 7,642 because the previous wrong-type narrowing was hiding four real DocblockTypeContradiction/NullableReturnStatement issues in a getBlockType function whose ?int native return contradicts its @return Tokens::BLOCK_TYPE_* docblock; psl goes from 3,473 to 3,474 because the test file packages/type/tests/static-analysis/intersection.php (designed specifically to exercise Map&ResultInterface&stdClass&Vector) now consistently flags that the docblock and native parsers produce different intersection representations for the same syntax. The +1 in psl is a side effect of intersectUnionTypes returning null when narrowing fails, which TypeHintResolver then converts to Type::getNever(); the previous behavior was to either crash or non-deterministically hit the defensive mayHaveIntersection=true path. A follow-up could refine the null-degradation path to preserve the cross-product, but that is a separate, pre-existing issue.

Bench summary table (relevant column = 6.16.1-12-32347e114):

App 6.16.1-11-6ff4aa2b4 6.16.1-12 (this fix) Delta
psl 3,473 3,474 +1 (test file)
php-cs-fixer 7,638 7,642 +4 (real bugs surfaced)
all 12 others unchanged unchanged 0

Type::intersectUnionTypes was crashing during parallel scan when a worker
encountered a PHP-native intersection type like `Map&ResultInterface&stdClass&Vector`
and one of the named types was a final class with storage already populated
in the worker, while another required-for-comparison class was not.

The per-atomic-pair branches inside intersectAtomicTypes already handle
this scenario by catching InvalidArgumentException with the comment
"Ignore non-existing classes during initial scan". The post-foreach
narrowing block running UnionTypeComparator::isContainedBy on the union
types as a whole was missing the same guard, so it bubbled the throw out
to the parallel worker and aborted the scan with messages like
"Could not get class storage for stdclass".

The narrowing block only runs when intersection_performed stayed false,
which happens for pairs of TNamedObject where mayHaveIntersection returns
false for at least one side (final class with storage available). When
storage is missing for a class needed downstream by the comparator, the
correct behavior is the same as for the per-atomic case: skip the
narrowing optimization and let the un-narrowed type stand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant