Answer UnionType comparisons from an identity-keyed FiniteTypeSet instead of scanning every member

[phpstan-bot]

Summary

Comparing a union of constant strings against another one cost one isSuperTypeOf() call per
pair of members. Membership among such values is exact-value equality, so it is a set lookup:
this PR keys a union's members by value identity once, caches that on the union, and answers
isSuperTypeOf(), isSubTypeOf(), accepts(), isAcceptedBy(), equals() and tryRemove()
from the map. O(n*m) member comparisons become O(n+m), and a single-value lookup becomes O(1).

The issue asked for a trie. A trie earns its keep on prefix or pattern queries — a router
matching URIs — and none of these comparisons are that (ConstantStringType::isSuperTypeOf()
on another constant string is value === value). As noted in the issue's comment thread, an
identity-keyed hash is the structure this actually wants, and it answers the same question for
constant ints, bools, null and enum cases, so all of them are keyed, not just strings.

Measured on 400-member unions (UnionType API directly, before → after):

operation	identical unions	disjoint unions
isSuperTypeOf	204 ms → 8 ms	575 ms → 6 ms
isSubTypeOf	203 ms → 6 ms	575 ms → 6 ms
accepts	2139 ms → 7 ms	2355 ms → 13 ms
isAcceptedBy	1504 ms → 6 ms	1731 ms → 12 ms
isSuperTypeOf(single value)	240 ms → 2 ms

End to end, analysing tests/bench/data/big-constant-string-union.php goes from 1.46 s to
0.76 s. PHPStan's own self-analysis is unchanged (55.8 s → 55.4 s), so ordinary code pays
nothing for the map.

Changes

• src/Type/FiniteTypeSet.php (new) — a union's members indexed by value identity. Keys
  null, constant scalars other than floats, and bare enum cases; everything else goes to
  $others, so a single object type next to fifty constant strings does not defeat the
  optimization, it only means callers still consult those few members the slow way. Also
  exposes containedIn() for union-vs-union answers and hasClassStringMember() for callers
  that hand a member back rather than only comparing values.
• src/Type/UnionType.php — builds the set lazily once per instance
  (getFiniteTypeSet()) and uses it in isSuperTypeOf(), isSubTypeOf(), accepts(),
  isAcceptedBy(), equals() and tryRemove().
• src/Type/TypeCombinator.php — finiteUnionMembers() reuses the shared, cached set
  instead of keying the union again on every intersect(). Its class-string bail is kept: the
  class-string flag is part of a constant string's representation but not of its value, and
  intersect() hands a member back.
• build/PHPStan/Build/SkipTestsWithRequiresPhpAttributeRule.php — the rule demanded an
  inverse comparison operator before checking that the comparison was about PHP_VERSION_ID
  at all, so a test method whose first statement is an if on any other binary operator (a
  || chain, say) crashed the analysis with ShouldNotHappenException. The new test file hit
  it. The PHP_VERSION_ID check now comes first.
• tests/bench/data/big-constant-string-union.php (new) — benchmark input.

Root cause

UnionType answered every comparison by walking $types and asking each member, and the
callers that pass a union walk it again — so union-vs-union was O(n*m) virtual calls. For
members that stand for one concrete value that scan is pure waste: two of them are
interchangeable iff they are equals(), and any two that are not equals() are disjoint. A
key derived from the value settles both questions with one hash lookup.

The same shape repeated across the whole comparison family, which is why the fix does too:

• isSuperTypeOf(value) — yes if the map holds it; otherwise every keyed member answers no,
  so only unkeyed members are left to ask.
• isSubTypeOf(union) / equals(union) — set containment between the two maps.
• accepts(value) — only the positive answer is value identity; a member that does not hold
  the value can still accept it through scalar coercion. So the miss is not simply no, but it
  is the same for every member of a kind (all constant strings behave alike as acceptors, and
  so do all cases of one enum), and consulting one member per kind is enough.
• accepts(union) — a member standing for a single value never accepts more than one of the
  other union's values, so the member-by-member or() cannot come out yes and its result is
  discarded for the compound answer anyway; go straight to isAcceptedBy().
• tryRemove(value) — drop the one member under that key instead of running
  TypeCombinator::remove() against every member.

Floats are the one finite scalar left out on purpose: equals() does not agree with value
identity for them (-0.0 === 0.0, NAN !== NAN). Template types are excluded outright, their
comparison semantics are not value identity, and a type that merely contains a value — an
intersection with an accessory type, a whole single-case enum, a conditional type resolving to
a constant — is excluded by an equals() check against its own constant scalar type.

Test

tests/PHPStan/Type/FiniteTypeSetTest.php. The map is an optimization, so the property worth
testing is that it never disagrees with comparing member by member: each test runs the real
operation against a reference implementation that spells out the loop UnionType used before,
over a matrix of 17 unions × 18 query types (and 17 × 17 union pairs) — 2397 cases covering
constant strings, ints, bools, null, enum cases, mixed-kind unions, unions diluted with an
object / a float / a general string / a class-string constant, and BenevolentUnionType.
accepts() additionally asserts that no member attaches a reason to its answer, which is what
lets one member of a kind stand in for the rest without changing error messages.

The reference implementations caught two real mistakes while writing them (the accepts()
compound-delegation tail and BenevolentUnionType::isAcceptedBy()'s or-semantics), and
mutating the fix afterwards — inverting the accepts() union shortcut, dropping the
completeness guard in isSuperTypeOf(), dropping the equals() guard in
FiniteTypeSet::key() — fails 254, 6 and 12 cases respectively.

Being an optimization, none of this fails without the fix; the benchmark data file and the
numbers above are what show the change did something. make tests (20029 tests), make phpstan and make cs are green. make name-collision fails identically with and without the
change (tests/PHPStan/Build/data/final-class-rule-pipe.php uses PHP 8.5 pipe syntax that the
detector's parser rejects on this PHP version).

Probed and deliberately left alone:

• TypeCombinator::union() already collapses constant scalars and enum cases through
  describe-keyed maps, so it is O(n) — no change needed.
• TypeCombinator::doRemove()'s finite-type loop is O(n*m), but for a union of values it is
  unreachable: isSuperTypeOf() settles the removal before it, and tryRemove() now handles
  the hit.
• IntersectionType, the composite sibling, gets nothing: its members are not
  mutually-disjoint values, so identity keying does not apply.
• UnionType::getFiniteTypes() dedupes by describe(VerbosityLevel::cache()). Switching it to
  identity keys would collapse a class-string constant with a same-valued plain one, which
  describe() keeps apart — left as is.

Fixes phpstan/phpstan#15008
Fixes phpstan/phpstan#14978

[SanderMuller]

Nice work. It follows the issue thread faithfully (identity-keyed map, not a trie, extended to the sibling operations) and the benchmarks look right. I checked the correctness-critical parts and they hold up: the six fast paths reproduce the slow-path results (all-in yes, none-in no, mixed maybe), isComplete()/$others keeps mixed unions on the slow path, and the accepts() representatives-of-other-kinds path is sound (I verified the numeric-string coercion case that could break it).

What keeps it from green, all outside the optimization:

1. Self-analysis: 15 "Class ManyCasesTestEnum not found" errors (and old-PHPUnit 7.4). The test imports the enum fixtures with use + ::class, forcing resolution under the <8.1 phpVersion analysis. UnionTypeTest avoids this by referencing them as string literals (new EnumCaseObjectType('PHPStan\Fixture\ManyCasesTestEnum', 'A')); matching that should clear both jobs.
2. The SkipTestsWithRequiresPhpAttributeRule change looks out of scope, only there so the test's version guard does not trip the rule. Better split out or restructured away.
3. Mutation testing is red: the subtle branches (representative loop, containedIn thresholds, $others gating) want tests that pin them directly.

(Rector/shipmonk are the repo-wide flakes, not this PR.) The algorithm and edge handling are solid; these are all peripheral.

[phpstan-bot]

Pushed. All three review points addressed, one commit each.

1. Self-analysis: 15 class.notFound errors (90b4a0a)

Confirmed and reproduced first — analysing the test file with phpVersion: 80000 and build/enums.neon (which excludes the three enum fixture files below PHP 8.1) gave exactly 15 Class PHPStan\Fixture\ManyCasesTestEnum not found. Switched to string literals as UnionTypeTest and EnumCaseObjectTypeTest do, and left a note in the class doc saying why, so it doesn't get tidied back into an import. Re-run under the same simulated config: 0.

2. SkipTestsWithRequiresPhpAttributeRule (abed2ab)

Reverted to the pre-PR state, byte-identical. I checked before reverting whether the test still needs it: with the rule restored, bin/phpstan analyse -c build/phpstan.neon tests/PHPStan/Type/FiniteTypeSetTest.php is clean. The reordering was carried over from an earlier draft that skipped on a PHP_VERSION_ID guard; the test uses #[RequiresPhp] now. The latent crash it fixed is real but unrelated to keying unions, so it belongs in its own change with its own test.

3. Mutation testing (4c0234d)

Worth flagging: infection is configured with --source-directory='build/PHPStan/Build', so the only mutable source in this PR was the rule file — the red MSI came from the untested lines the reordering added, and reverting it in #2 removes the mutants. Your underlying point still stands, though: the matrix reached those branches only sideways. So I pinned them head on — containedIn() at all three thresholds including both boundaries, the single-kind representative case, the four isComplete() gates, and hasClassStringMember().

I verified these are load-bearing rather than decorative by mutating the source ten ways and confirming each dies against the new tests alone (filtered to just them):

mutation	killed
containedIn no-threshold off $other's count	✓
containedIn swapped array_diff_key() args	✓
containedIn === 0 → >= 0	✓
representative loop without the kind continue	✓
hasClassStringMember without the isClassString() guard	✓
isComplete() → true	✓
isSuperTypeOf / tryRemove / finiteTypeSetContainedIn / equals without their gate	✓ (4)

Sources restored to pristine afterwards (git diff src/ empty).

make tests (20348, 97 skipped — same as before), make phpstan and make cs are green.

[staabm]

how is the impact of this change on phpstan/phpstan#15004?

[phpstan-bot]

Pushed as 032c1284a. Here's the answer to the review question.

Impact on phpstan/phpstan#15004

I measured it rather than estimated it. Reproducer is the issue's own shape — a 1|2|…|N parameter narrowed by a chain of N !== clauses — run through analyse -l 8:

N	before this PR	PR as you reviewed it	+ the commit I just pushed
50	1.30 s	1.20 s	1.20 s
100	3.11 s	2.70 s	2.41 s
200	15.02 s	12.92 s	10.42 s
300	46.27 s	39.57 s	30.95 s

The PR as you reviewed it helped #15004 by about 15%. That is less than I'd have guessed, so I profiled instead of leaving it there.

What the profile showed

Counting calls at N=100: 79 674 TypeCombinator::doRemove() calls costing 0.92 s, and that 0.92 s splits almost evenly into UnionType::isSubTypeOf() at 0.40 s and UnionType::tryRemove() at 0.47 s.

tryRemove() is the one this PR already keyed. isSubTypeOf() was not — doRemove() opens with $typeToRemove->isSuperTypeOf($fromType), which lands on UnionType::isSubTypeOf() through the CompoundType delegation, and finiteTypeSetContainedIn() only ever compared two unions. So a union of values asked about one value still walked every member. Roughly half the remaining hot path was left on the table by the PR's own logic.

What I pushed

A single value is a one-member set, so the map answers it: the union is under that value only if it has nothing else in it, and disjoint from it if the key is missing. FiniteTypeSet::containedInKey() is containedIn() for that case; the helper now treats a non-union other type as the set holding just that value. That's the third column above — 1.5× versus main at N=300, and it's the same identity-map idea, not a new mechanism.

isAcceptedBy() shares the helper and keeps its yes-only guard. Worth flagging honestly: no constant value accepts another one — not a numeric string an int, in either direction — so that guard provably cannot change an answer today, and mutating it away leaves all tests green. I kept it and said so in a comment, because that's a fact about the current value types rather than something the map guarantees.

What it does not do

This is a constant factor. It is not the fix #15004 wants. Growth stays at roughly O(N^2.7) — base scales 3.08× from N=200→300, mine 2.97×. Both isSubTypeOf() and tryRemove() are inherently O(n) per call (the latter has to build an (n−1)-member union), against an O(n) chain of O(n) removals. Only the issue's Option B — not making the removals at all — changes the exponent. A 400-clause case will still not be single-digit seconds.

Verification

make tests 21039 tests / 97 skipped (same skips as before), make phpstan and make cs green. The reference-implementation matrix now runs isSubTypeOf() and isAcceptedBy() over the union/non-union pairs too, and asserts no member attaches a reason to an answer the map now gives on its behalf; four mutations of the new code fail 399, 1, 92 and 36 cases.

Two measurement notes, since both nearly misled me:

• I first read a 15% regression on big-constant-string-union.php and a 30% one on self-analysis. Both were artifacts — the first from measuring immediately after a git stash pop (cold), the second from a background test suite. Re-measured clean, six runs each: bench file 2.81 s either way, self-analysis 1m23/1m24 with the change against 1m25/1m25 without.
• I deliberately did not add a tests/bench/data/bug-15004.php. Sized to run in CI (N≈60, 1.3 s) the effect is ~7% and sits in the harness's ±20% noise band; sized to show it (N≥200) it costs >10 s per rev and the suite runs every file six times.

[staabm]

putting a pre-existing key into $others seems to be not necessary for any test before this PR.

do we really need it?

[phpstan-bot]

Pushed as fa8f9cec7.

Answer: yes, it's needed — and now it's pinned

I checked by removing the branch ($members[$key] = $type unconditionally) and running the suite. Two real breakages, both on a union that holds one value twice:

	with the branch	without it
('a'|'a')->equals('a'|'b')	false	true
('a'|'a')->tryRemove('a')	*NEVER*	ShouldNotHappenException: Cannot create UnionType with:

UnionType::equals() guards on member count first, so two two-member unions get past it; if both maps collapse to {s:a} and {s:a, s:b}, containedIn() says yes and two different types come out equal. And tryRemove() drops the single member under the key, leaving new UnionType([]), which the constructor rejects.

Why no test caught it before

You're right that nothing pinned it — the branch was only reachable sideways, through testSubclassOfAShortcutClassIsKeyedTheSameWay's count assertions. That's because TypeCombinator::union() never produces such a union: it dedupes constant scalars by describe(VerbosityLevel::cache()), and 'a' and a class-string 'a' describe identically, so the combinator already folds them. But UnionType::__construct() is @api and does not dedupe, so a directly-built union can hold the same value twice — and before this PR it answered both operations correctly via the member-by-member loop. Regressing that would be a behaviour change in @api territory for a case the optimization has no reason to touch.

What I added

testUnionRepeatingOneValueIsNotAnsweredFromTheMap, built on the shape that actually arises — 'a' and 'a' with the class-string flag, which are equals() (the flag isn't part of the value) and therefore must share a key. It asserts the equals() answer and the tryRemove() result first, then the set shape, so the failure it produces is the behavioural one: mutating the branch away now fails on assertFalse($union->equals(...)) rather than on a count. Also spelled the reason out in the create() docblock, which previously only said the set "never claims a union has fewer members than it does" without naming what goes wrong.

make tests (21169, 97 skipped), make phpstan and make cs green. Rebased onto b0d8cc432, which landed while I was working.

[staabm]

this if-case seems unnecessary?

[phpstan-bot]

Done. Pushed as b0d8cc432.

The isString() guard in hasClassStringMember() — removed

You're right, it couldn't change the answer, and it wasn't buying anything either.

Why it's unreachable as a filter. Every member in $members is one of exactly five classes. The shortcut in key() names four of them outright, and the general path can't widen that: it requires $scalarTypes[0]->equals($type), and ConstantScalarTypeTrait::equals() is $type instanceof self && $this->value === $type->value — so a keyed member is always an instance of the constant class its own value came from. Plus EnumCaseObjectType. Of those:

member	isClassString()	source
ConstantIntegerType, ConstantBooleanType	No	JustNullableTypeTrait
NullType	No	NullType:332
EnumCaseObjectType	No	ObjectTypeTrait
ConstantStringType	the real answer	ConstantStringType:95

So isClassString()->no() already skips every non-string member on its own.

And it didn't save the reflection lookup the doc comment cites. That lookup is ReflectionProviderStaticAccessor::getInstance()->hasClass() inside ConstantStringType::isClassString() — i.e. it fires for precisely the members the guard let through. For everything else isClassString() is a constant createNo(), the same cost as the isString() call being spent to avoid it.

I extended the no strings at all fixture from int|null to cover every non-string kind including an enum case — whose class name genuinely does name a class, so it's the one that would bite if ObjectTypeTrait ever stopped answering no.

On the $others question from your other comment

I'd measured this before your fa8f9cec7 landed, and it agrees with your commit: dropping the duplicate makes ('a'|'a')->tryRemove('a') take the fast path down to zero remaining members and throw ShouldNotHappenException: Cannot create UnionType with:. Your test covers it better than the one I'd written — the class-string-flag pair is the realistic way two equals() members show up, and it pins the equals() consequence too — so I dropped mine rather than adding a second one.

make tests (21169, 97 skipped), make phpstan and make cs green on the merged state.

[staabm]

I'd measured this before your fa8f9ce landed, and it agrees with your commit: dropping the duplicate makes ('a'|'a')->tryRemove('a') take the fast path down to zero remaining members and throw ShouldNotHappenException: Cannot create UnionType with:. Your test covers it better than the one I'd written — the class-string-flag pair is the realistic way two equals() members show up, and it pins the equals() consequence too — so I dropped mine rather than adding a second one.

add new nsrt tests which succeed before this PR and would fail without this change.

[phpstan-bot]

I processed this review but have nothing to report.

[phpstan-bot]

I processed this review but have nothing to report.

[staabm]

remove inline ignores. should be baselined instead

[staabm]

use Type::getEnumCaseObject instead

[phpstan-bot]

Pushed as a75b5a8b6.

Inline ignores → baseline

Both // @phpstan-ignore phpstanApi.instanceofType comments are gone — key() and kind() now carry a bare instanceof EnumCaseObjectType — and the two errors they suppressed are recorded in phpstan-baseline.neon instead, matching how every other phpstanApi.instanceofType in src/ is handled (20 such entries already):

-
	rawMessage: 'Doing instanceof PHPStan\Type\Enum\EnumCaseObjectType is error-prone and deprecated. Use Type::getEnumCases() instead.'
	identifier: phpstanApi.instanceofType
	count: 2
	path: src/Type/FiniteTypeSet.php

Entry hand-written rather than regenerated, so the diff is only the six added lines and nothing unrelated moves; it is inserted in the file's path order, between src/Type/FileTypeMapper.php and src/Type/FloatType.php.

I confirmed the entry is load-bearing rather than decorative: with the ignores removed and before adding it, bin/phpstan analyse src/Type/FiniteTypeSet.php reported exactly those two errors at lines 140 and 179, and the message string above is copied from that output.

make phpstan, make tests (21168 tests, 97 skipped — the same skips as before) and make cs are green. I rebased onto your fa8f9cec7 and re-ran make phpstan plus FiniteTypeSetTest (3408 tests) afterwards, both still green.

Two things from the same review round that this commit does not touch, in case they were meant for it rather than left for later:

• kind() should use Type::getEnumCaseObject() — still open. It would remove one of the two baselined instanceofs (the key() one is a different question: it needs the member to be a bare EnumCaseObjectType, which is what equals() compares, and getEnumCaseObject() also answers for $this & Enum::C and single-case enums). Say the word and I'll do it and drop the baseline count to 1.
• Pre-existing key → $others — you covered this yourself in fa8f9cec7.

[phpstan-bot]

I'm waiting for the background make tests run to finish before splitting the commits and pushing.

[staabm]

add a new bench for

<?php
/** @param 999|1|2|3|4|5|6|7|8|9|10|11|12|13|14|15|16|17|18|19|20|21|22|23|24|25|26|27|28|29|30|31|32|33|34|35|36|37|38|39|40|41|42|43|44|45|46|47|48|49|50|51|52|53|54|55|56|57|58|59|60|61|62|63|64|65|66|67|68|69|70|71|72|73|74|75|76|77|78|79|80 $x */
function g(int $x): void {
 if ($x !== 1 && $x !== 2 && $x !== 3 && $x !== 4 && $x !== 5 && $x !== 6 && $x !== 7 && $x !== 8 && $x !== 9 && $x !== 10 && $x !== 11 && $x !== 12 && $x !== 13 && $x !== 14 && $x !== 15 && $x !== 16 && $x !== 17 && $x !== 18 && $x !== 19 && $x !== 20 && $x !== 21 && $x !== 22 && $x !== 23 && $x !== 24 && $x !== 25 && $x !== 26 && $x !== 27 && $x !== 28 && $x !== 29 && $x !== 30 && $x !== 31 && $x !== 32 && $x !== 33 && $x !== 34 && $x !== 35 && $x !== 36 && $x !== 37 && $x !== 38 && $x !== 39 && $x !== 40 && $x !== 41 && $x !== 42 && $x !== 43 && $x !== 44 && $x !== 45 && $x !== 46 && $x !== 47 && $x !== 48 && $x !== 49 && $x !== 50 && $x !== 51 && $x !== 52 && $x !== 53 && $x !== 54 && $x !== 55 && $x !== 56 && $x !== 57 && $x !== 58 && $x !== 59 && $x !== 60 && $x !== 61 && $x !== 62 && $x !== 63 && $x !== 64 && $x !== 65 && $x !== 66 && $x !== 67 && $x !== 68 && $x !== 69 && $x !== 70 && $x !== 71 && $x !== 72 && $x !== 73 && $x !== 74 && $x !== 75 && $x !== 76 && $x !== 77 && $x !== 78 && $x !== 79 && $x !== 80) { echo "none"; }
}

[VincentLanglet]

Two types standing for the same value are not merged: the second one goes to $others
	 * so that the set never claims a union has fewer members than it does. TypeCombinator
	 * never builds such a union, but the UnionType constructor is @api and does not dedupe,
	 * and a union holding one value twice is not the union holding it once - merging them
	 * would let equals() call 'a'|'a' and 'a'|'b' the same type, and leave tryRemove() with
	 * no member to build a union from.

is unclear/smelly to me

[VincentLanglet]

ALso is @internal needed, I thought every not @api is internal

[ondrejmirtes]

Yes, everything not-@api is @internal, but if you're already inside an @api and want to make something "internal", that's the only way.

[staabm]

Two types standing for the same value are not merged: the second one goes to $others
	 * so that the set never claims a union has fewer members than it does. TypeCombinator
	 * never builds such a union, but the UnionType constructor is @api and does not dedupe,
	 * and a union holding one value twice is not the union holding it once - merging them
	 * would let equals() call 'a'|'a' and 'a'|'b' the same type, and leave tryRemove() with
	 * no member to build a union from.

is unclear/smelly to me

the bots arguments are: new UnionType(new ConstantStringType("a"), new ClassStringType("a")) is not the same as new UnionType(new ConstantStringType("a")) because they are not yet normalized.

I was not sure either whether its a artificial limitation because types should usually be constructed with TypeCombinator::union() or whether its a real thing.

@ondrejmirtes wdyt?

[SanderMuller]

On the Test (PHP 8.5) RegressionBench reds: these are the baseline-drift failure mode bench.yml itself warns about, not a code regression. The variant runs on the PR runner and is compared against the committed tests/bench/storage/baseline-8.5.xml, recorded on a different runner, and the workflow comment notes "GitHub runners differ in speed by up to 2x". A slower runner then shows a broad, consistent offset across every bench, which is what the +20-40% (with tight per-run variance) is.

Measuring base vs this PR head on one machine, which removes the cross-runner variable:

• The 999|1|...|N union narrowed by an N-clause !== chain (N=200): base 7.32s, this PR 5.74s. About 21% faster, which is the intended win.
• impure-call-columns.php (one of the flagged benches), boot-subtracted analysis time: base 0.90s vs PR 0.94s, so +4.4%, against the +26% CI reports.
• The natural suspect, dropping the get_class() fast path in key(), is neutral: the head before it and the current head measure 5.77s vs 5.74s on that shape.

Same thing happened earlier on this PR with or-chain-resolve-type-blowup at +64.95%, which was 0% on a direct comparison.

One honest caveat: there is a small real overhead, about 4% on impure-call-columns, from building the FiniteTypeSet lazily on unions that do not benefit. The PR's self-analysis numbers were flat, so it is workload-specific, but it is there.