Skip to content

Sort transitive tips instead of ksorting a list - #426

Merged
janedbal merged 1 commit into
shipmonk-rnd:masterfrom
ondrejmirtes:fix-transitive-tips-ordering
Aug 28, 2026
Merged

Sort transitive tips instead of ksorting a list#426
janedbal merged 1 commit into
shipmonk-rnd:masterfrom
ondrejmirtes:fix-transitive-tips-ordering

Conversation

@ondrejmirtes

Copy link
Copy Markdown
Contributor

buildError() collects transitive tips into a list:

$tips = [];

foreach (array_slice($blackMembersGroup, 1) as $transitivelyDeadMember) {
    $tips[] = $this->buildTransitiveErrorMessages($transitivelyDeadMember) . $exclusionMessage;
    // ...
}

$builder->metadata($metadata);

ksort($tips);

Since $tips is appended to with $tips[], its keys are already 0..n-1 in ascending order, so ksort() is a no-op. The tips are emitted in insertion order, which is the order of getTransitiveDeadCalls() → the insertion order of $usageGraph → the order PHPStan hands over collected data. That last one is not stable across parallel runs.

I noticed this while profiling the extension on a ~16k-file codebase: two runs of identical code produced different tip order for the same error, so diffing analysis output between runs showed spurious changes.

The existing test expectations show the same thing — Grouping\Example::boo and Grouping\Example::foo have the same three transitively dead members, but the fixture encoded them in two different orders:

'Unused Grouping\Example::boo',
"• Thus Grouping\Example::bag is transitively unused\n" .
"• Thus Grouping\Example::bar is transitively unused\n" .
'• Thus Grouping\Example::TRANSITIVELY_UNUSED_CONST is transitively unused',

'Unused Grouping\Example::foo',
"• Thus Grouping\Example::bar is transitively unused\n" .   // <- flipped
"• Thus Grouping\Example::bag is transitively unused\n" .
'• Thus Grouping\Example::TRANSITIVELY_UNUSED_CONST is transitively unused',

sort($tips) gives the deterministic order the ksort() call was clearly reaching for. I updated the three affected expectations; the full suite passes (858 tests), as do phpstan and phpcs.

I also opened phpstan/phpstan#15126 asking for a rule that reports ksort() on a list, since PHPStan already reports the analogous array_values() on a list and would have caught this.

$tips is built with $tips[] = ..., so its keys are already 0..n-1 in
ascending order and ksort() does nothing. The tip order therefore
followed the insertion order of $usageGraph, which follows the order
PHPStan hands over collected data - and that varies between parallel
runs. On a large project two runs of the same code produced different
tip order for the same error.

The two grouping expectations that encoded the old order contained the
same three members in two different orders, which is the same symptom.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G7xhBWRXCTYyc336QVj13s
@janedbal

Copy link
Copy Markdown
Member

Good catch, thanks! This is a regression: originally fixed in #146 by keying the tips by member ref (making the ksort() meaningful), then broken in #281 which switched to plain $tips[] = ... appends while keeping the ksort(). Your sort() fix looks good.

@janedbal
janedbal merged commit 92b5681 into shipmonk-rnd:master Aug 28, 2026
33 checks passed
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.

2 participants