Simplify FiniteTypeSet

[staabm]

No description provided.

[Copilot]

Pull request overview

Simplifies FiniteTypeSet by removing class-string member tracking and broadening finite-union intersection optimization.

Changes:

• Removes cached class-string detection.
• Removes the corresponding bailout and tests.
• Allows class-string members through the finite-union fast path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/Type/FiniteTypeSet.php	Removes class-string tracking.
src/Type/TypeCombinator.php	Removes the class-string fast-path guard.
tests/PHPStan/Type/FiniteTypeSetTest.php	Removes class-string detection tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[staabm]

@copilot how does a nsrt test look like which is affected by the mentioned regression?

[staabm]

@SanderMuller could you try to come up with a nsrt test-case which fails with this PR but succeded before?

my feeling is that this PR only violates a halucinated/not relevant unit tests, but does not affect real world logic/projects

[SanderMuller]

Short answer: no, I could not build one, and I looked hard. But the PR does change real behaviour — it turns the phpstan-doctrine extension test job red — so I'd not merge it as it stands. There is a one-line variant that keeps the whole simplification, below.

No nsrt case exists

I instrumented base so that whenever the class-string guard makes finiteUnionMembers() bail while both sides are complete finite sets, it computes what the fast path would have returned, runs the slow path, and logs the two if they differ (fingerprint = describe(value) + generalize(moreSpecific), the only two things the class-string flag changes).

Across tests/PHPStan/Analyser (2902 tests) and tests/PHPStan/Rules (3293 tests): zero divergences. The guard bails 20 times in the whole nsrt suite, and not once does it change the outcome. So the deleted unit test really was pinning the mechanism rather than any behaviour, and removing the guard is not measurably faster either.

I also wrote 11 probe constructs aimed straight at it (in_array narrowing against a [Foo::class, Bar::class] array, === against a ::class union, loop widening, new $narrowed, coalesce). The divergence does fire in real analysis — 3 hits in a 60-line file, all via

TypeCombinator::intersect <- MutatingScope::applySpecifiedTypes <- MutatingScope::filterByTruthyValue <- ExpressionResult::getTruthyScope

with base(slow)='PHPStan\Type…' ## literal-string&non-falsy-string vs pr(fast)='PHPStan\Type\ArrayType'|'PHPStan\Type\ObjectType' ## class-string. But the narrowed type is discarded right after, so no output changes. That is why nothing first-party catches it.

It does change third-party output

extension-tests / phpstan-doctrine (8.0) make tests fails on this PR with a new false positive:

+49: Property PHPStan\Rules\Doctrine\ORM\MyBrokenEntity::$six type mapping mismatch:
    database can contain DateTime but property expects DateTimeImmutable.

That job is green on #6235 and #6236, which share this PR's exact base commit 0d109aab9, and on #6226, #6228 and #6230 from the same window. So it is attributable rather than flaky.

I could not reproduce it locally — running the extension's EntityColumnRule over MyBrokenEntity.php through bin/phpstan reports $six on base as well, so my ad-hoc config differs from the test's conditions (it builds the rule directly with allowNullablePropertyForRequiredField and objectManagerLoader = null). So I can't name the path inside the extension that depends on this. CI on the updated PR is the cheapest way to confirm.

The one-line variant

The guard exists because intersectFiniteUnions() hands back $a's member, and two members sharing a key are equals() but not necessarily identical — ConstantStringType::equals() ignores the class-string flag, while describe(value) and generalize() do not. Take the member from $b instead and the fast path agrees with the general algorithm, so the guard is unnecessary:

 		$common = [];
 		foreach ($membersA as $key => $member) {
 			if (!array_key_exists($key, $membersB)) {
 				continue;
 			}
 
-			$common[] = $member;
+			// Hand back $b's member, which is what the general algorithm below picks: two members
+			// sharing a key are equals() but not necessarily identical - a constant string that is
+			// also a class-string is not interchangeable with a same-valued one that is not - so
+			// the fast path must not swap one for the other.
+			$common[] = $membersB[$key];
 		}

With that on top of your branch:

• intersect(plain, flagged) and intersect(flagged, plain) both return exactly what base returns (checked in both argument orders, plus the two remove() variants).
• Instrumenting the fast path to compare every hit against the slow path: 0 divergences over the same 6195 Analyser + Rules tests.
• Full suite green: 21321 tests, 96894 assertions.
• The probe file that produces 3 divergences with $member produces 0 with $membersB[$key].

That keeps your -73 lines, and the fast path now also covers the class-string unions it used to bail on.

Two notes:

1. Which flavour survives on base is purely positional — the second operand's member wins — so today the flag survives by accident rather than by design. Worth knowing, but not this PR's problem.
2. If you take the variant, it would be worth replacing the deleted unit test with a behaviour one (intersect two same-valued unions, one from ::class, and assert the result still generalizes to class-string). Otherwise the next simplification pass turns $membersB[$key] back into $member and nothing first-party notices.

[staabm]

But the PR does change real behaviour — it turns the phpstan-doctrine extension test job red — so I'd not merge it as it stands.

re-running the job made it pass, without further changes

[phpstan-bot]

This pull request has been marked as ready for review.

[staabm]

I think the PR is fine, because we did not have a separate class-string case before introducing FiniteTypeSet either