Fix phpstan/phpstan#10055: Conditional argument type not properly narrowing type (for constant string unions?)

[phpstan-bot]

Summary

Conditional parameter types (@param ($param1 is 'value3' ? bool : int) $param2) fail to narrow correctly when the parameter union type has more than 2 members. The "false" branch condition becomes a UnionType that never matches individual constant types during the equals() check.

Changes

• Modified src/Analyser/MutatingScope.php to split union condition types into individual ConditionalExpressionHolder instances when creating holders for @param conditional types
• Added regression test tests/PHPStan/Analyser/nsrt/bug-10055.php

Root cause

When processing @param ($param1 is 'value3' ? bool : int) $param2 with $param1 typed as 'value1'|'value2'|'value3':

• The "true" branch condition is TypeCombinator::intersect('value1'|'value2'|'value3', 'value3') = 'value3' (works fine)
• The "false" branch condition is TypeCombinator::remove('value1'|'value2'|'value3', 'value3') = 'value1'|'value2' (a UnionType)

Later, when matching conditions in a match arm where $param1 is narrowed to 'value1', the equals() check compares ConstantStringType('value1') against UnionType('value1'|'value2'), which always fails.

The fix splits union condition types into individual holders: instead of one holder with condition 'value1'|'value2', two holders are created — one for 'value1' and one for 'value2' — both mapping to the same result type (int). This allows the equals() check to match correctly.

Test

Added tests/PHPStan/Analyser/nsrt/bug-10055.php which verifies that in a match expression, conditional parameter types correctly narrow $param2 to int for 'value1' and 'value2' arms, and to bool for the 'value3' arm.

Fixes phpstan/phpstan#10055

[VincentLanglet]

You say

Later, when matching conditions in a match arm where $param1 is narrowed to 'value1', the equals() check compares ConstantStringType('value1') against UnionType('value1'|'value2'), which always fails.

Rather splitting the Union here, shouldn't the check "later" be improve to also supports isSuperType sometimes ?

[phpstan-bot]

Done. Here's a summary of what was changed to address the reviewer's feedback:

Reviewer's concern: Instead of splitting union types into individual ConditionalExpressionHolder instances, the matching logic "later" should use isSuperTypeOf instead of equals().

What was done:

1. Reverted the union-splitting approach in MutatingScope::enterFunctionLike() — the condition types from TypeCombinator::intersect() and TypeCombinator::remove() are no longer decomposed into individual union members.

2. Added useSubtypeForConditionMatching flag to ConditionalExpressionHolder — this flag controls whether condition matching uses strict equals() or relaxed isSuperTypeOf().

3. Set the flag to true only for conditional parameter type holders (created from @param conditional types).

4. Updated the matching logic in filterBySpecifiedTypes() — when the flag is true, uses isSuperTypeOf so that e.g. 'value1'|'value2' (condition type) matches 'value1' (narrowed type). Otherwise keeps strict equals().

Why the flag is needed: Using isSuperTypeOf globally causes regressions because conditional expression holders from assignment handlers and scope merging create conditions where strict matching is necessary. For example, assignment handlers create conditions like "if $v is mixed~null, then getMixed() is mixed~null" — using isSuperTypeOf here causes cascading matches that produce *NEVER* types.

[VincentLanglet]

There is no need for a useSubtypeForConditionMatching param, and this shouldn't be dependant to the situation (match, if, etc) but more with the type.

Maybe finiteTypes or something like this ?

Also explain more in detailed why this doesn't work for youur mixed~null example.

[phpstan-bot]

The changes have been pushed. Here's a summary of what was done to address the reviewer's feedback:

What the reviewer asked for:

1. Remove the useSubtypeForConditionMatching flag from ConditionalExpressionHolder
2. Make the behavior depend on the type (e.g., finiteTypes), not the call site
3. Explain why isSuperTypeOf doesn't work globally

What was implemented:

Removed the flag entirely and replaced it with a two-pass matching approach in filterBySpecifiedTypes():

• Pass 1: Exact matching via equals() (original behavior) - always runs first
• Pass 2: isSuperTypeOf matching for condition types with finite types - only runs when Pass 1 found no matches for the target expression

Why a simple finiteTypes check alone causes regressions:

Using isSuperTypeOf for all finite-type conditions (without the two-pass priority) causes real regressions in variable definedness tracking. When scope merging creates both a specific condition (e.g., "if $key=2, then $value is Yes") and a broader condition (e.g., "if $key=0|2, then $value is Maybe"), both would match via isSuperTypeOf. The extremeIdentity certainty calculation then degrades Yes AND Maybe = Maybe, causing false "Variable might not be defined" warnings.

The two-pass approach solves this: exact matches (Pass 1) take priority, preventing broader conditions from interfering. Pass 2 only activates when no exact match exists, which is exactly the case for conditional parameter types with union conditions.

Additional improvement: The bug-5051 test expectations were updated to reflect more precise type narrowing that results from the Pass 2 fallback matching.

[VincentLanglet]

Explains why the check

if (count($conditionalTypeHolder->getType()->getFiniteTypes()) === 0) {
								continue 2;
							}

is needed ?

We could think that it should work for more/every type,
Like

• If is_int|is_string , then a condition if is_int should match
• If i > 2, then a condition i > 3 should match

So why can't we just remove the finiteType condition ?

[phpstan-bot]

Pushed. Here's a summary of the changes to address the reviewer's question:

The reviewer asked why the getFiniteTypes() check can't be removed to allow isSuperTypeOf matching for all types (e.g., int|string matching int, or int<3, max> matching int<4, max>).

I verified the answer empirically by removing the check and running the test suite — 2 concrete regressions confirmed the check is necessary:

1. pr-5379: non-falsy-string expected but 'filter' produced. A condition type of non-falsy-string (from BooleanAnd false-context processing in TypeSpecifier) incorrectly matched the narrowed type 'filter' via isSuperTypeOf, causing wrong narrowing.

2. bug-14249: false expected but *NEVER* produced. Non-finite condition types from assignment handlers (like mixed~false) incorrectly matched via isSuperTypeOf, causing conflicting conditional expressions to both activate and produce true & false = *NEVER*.

The root cause: conditional expression holders come from 4 different sources — only source (1) conditional parameter types and (5) foreach constant arrays produce finite condition types. Sources 2-4 (scope merging, assignment handlers, TypeSpecifier boolean processing) produce non-finite types that are too broad for isSuperTypeOf. The getFiniteTypes() guard restricts Pass 2 to closed sets of concrete values where subtype matching is safe.

I added a detailed comment in the code explaining all of this.

[VincentLanglet]

But we should have

function test(mixed $foo): void
{
	$a = 0;
	if (\is_string($foo) || \is_int($foo)) {
		$a = 1;
	}

	if (\is_int($foo)) {
		assertType('1', $a);
	}
}

in which case, are you with this example ; 1, 2, 3, 4 or 5 ?

[phpstan-bot]

Pushed. Here's a summary of the changes made to address the reviewer's feedback:

Reviewer's example: A scope-merging case where $a = 0 followed by if (is_string($foo) || is_int($foo)) { $a = 1; } should narrow $a to 1 inside if (is_int($foo)). This falls under source 2 (scope merging) — the condition type is int|string, which is a UnionType but not finite.

What was changed:

Replaced the getFiniteTypes() guard with instanceof UnionType in Pass 2. The getFiniteTypes() check was too restrictive — it only allowed Pass 2 for closed sets of concrete values (constant strings, booleans, enum cases), which excluded non-finite union types like int|string from scope merging.

The UnionType guard is sufficient to prevent the known regressions because the problematic condition types are non-union:

• non-falsy-string (from TypeSpecifier boolean processing) — an intersection type, not union
• mixed~false (from assignment handlers) — a subtracted type, not union

Union types explicitly enumerate alternatives, making isSuperTypeOf subtype matching safe for them.

Test added: testScopeMerging in bug-10055.php verifying assertType('1', $a) inside if (is_int($foo)).

Verification: make tests (11840 tests, 79348 assertions) and make phpstan both pass with no regressions.

[VincentLanglet]

Remove the check

if (!$conditionalTypeHolder->getType() instanceof UnionType) {
								continue 2;
							}

and consider the needed fix is somewhere else.

For bug-14249 the getMixed method should be marked with

/** @phpstan-impure */

For pr-5379 file maybe some conditinal expression are wrongly added somewhere else ?
A minimal reproducer is

/**
	 * @param array<string, mixed> $aggregation
	 * @param non-falsy-string $type
	 */
	private function parseAggregation(array $aggregation, string $type)
	{
		if (empty($aggregation['field']) && $type !== 'filter') {
			return null;
		}

		if ($type !== 'filter') {
			assertType("array<string, mixed>", $aggregation);
		}

		assertType('non-falsy-string', $type);
	}

which ends with some weird condition

$aggregation=array<string, mixed> => 'filter' (Yes)
$type=non-falsy-string => non-empty-array<string, mixed>&hasOffset('field') (Yes)
``
added in the BooleanAndHandler

[phpstan-bot]

All three commits have been pushed. Here's a summary of the changes:

Commit 1: Skip trivially-always-true conditions in processBooleanSureConditionalTypes (TypeSpecifier.php)

• When TypeCombinator::remove(scopeType, sureType) returns a type equal to the scope type, the condition is trivially always true and is now skipped
• This fixes the pr-5379 regression at its root cause: conditions like $aggregation=array<string, mixed> (from TypeCombinator::remove(array<string, mixed>, narrowed_array) having no effect) and $type=non-falsy-string (from TypeCombinator::remove(non-falsy-string, 'filter') having no effect) are no longer created

Commit 2: Mark getMixed() test helper as @phpstan-impure (bug-14249.php)

• Prevents assignment handlers from creating conditional expressions that conflict with assertion narrowing

Commit 3: Remove instanceof UnionType guard from Pass 2 (MutatingScope.php)

• With the root causes fixed, Pass 2 can safely use isSuperTypeOf for all condition types without the UnionType restriction
• This allows matching for any condition type (not just unions), enabling the scope merging case and other patterns

All 11840 tests pass (79348 assertions) and make phpstan reports no errors.

[VincentLanglet]

Add a non regression test for the change made in src/Analyser/TypeSpecifier.php‎ which failed before even without the change in the MutatingScope.

[phpstan-bot]

Done. Added a non-regression test testTriviallyTrueConditionSkipped in tests/PHPStan/Analyser/nsrt/bug-10055.php that specifically exercises the TypeSpecifier fix independently of the MutatingScope Pass 2 change. The test reproduces the scenario where processBooleanSureConditionalTypes was creating spurious conditional expressions because TypeCombinator::remove(non-falsy-string, 'filter') returns non-falsy-string unchanged — without the $scopeType->equals($conditionType) skip, this led to incorrect type narrowing.

All tests pass (11840 tests, 79348 assertions) and make phpstan reports no errors.

[VincentLanglet]

Is there other TypeCombinator::remove call to fix the same way ?

[phpstan-bot]

Done. Here's a summary of the analysis and change:

The reviewer asked: Are there other TypeCombinator::remove calls to fix the same way?

Analysis of all TypeCombinator::remove calls in conditional expression contexts:

Location	Side	Fix applicable?
TypeSpecifier.php:1943 (processBooleanSureConditionalTypes)	condition	Already fixed
TypeSpecifier.php:2117 (processBooleanNotSureConditionalTypes)	result	No — holders carry createYes certainty needed for conditionally-defined variables
MutatingScope.php:1767 (ConditionalTypeForParameter)	condition	No — skipping the else-branch holder would break conditional parameter types
AssignHandler.php:919	result	No — same certainty concern as above (confirmed by bug-10088 test failure)

What was fixed: The symmetric counterpart processBooleanNotSureConditionalTypes uses TypeCombinator::intersect (not remove) on the condition side. When intersect(scopeType, type) returns the scope type unchanged, the condition is trivially always true — the same issue. Applied the same skip-if-unchanged check there.

The result-side remove calls cannot be fixed "the same way" because ExpressionTypeHolder::createYes(...) carries certainty information (Yes) that matters independently of the type — e.g., for variables with Maybe certainty like $shortcut_id in bug-10088.php, where remove(int, null) = int but the holder still needs to exist to affirm the variable is definitely defined.

[VincentLanglet]

Add a non regression test for the latest fix

[phpstan-bot]

Done. Added a non-regression test testNotSureTriviallyTrueConditionSkipped in tests/PHPStan/Analyser/nsrt/bug-10055.php that specifically exercises the processBooleanNotSureConditionalTypes fix.

The test uses is_string($type) on a non-falsy-string parameter combined with is_int($value) in a BooleanAnd. Without the fix, TypeCombinator::intersect(non-falsy-string, string) returns non-falsy-string unchanged, creating a spurious conditional expression that fires inside if ($type !== 'filter') and incorrectly narrows $value from int|string to string. With the fix, the trivially-true condition is skipped and $value correctly remains int|string.

I verified the test would fail without the fix by temporarily reverting it — $value becomes string inside the nested if-condition, confirming the non-regression behavior.

[VincentLanglet]

Your test is weird cause we should already have

/**
 * @param non-falsy-string $type
 * @param int|string $value
 */
function testNotSureTriviallyTrueConditionSkipped(string $type, int|string $value): void
{
	if (is_string($type) && is_int($value)) {
		return;
	}

	assertType('string', $value);
}

since is_string($type) is a useless condition

[phpstan-bot]

Pushed. Here's a summary of what was changed to address the reviewer's feedback:

Reviewer's concern: The test testNotSureTriviallyTrueConditionSkipped was weird because is_string($type) on a non-falsy-string parameter is trivially always true, so $value should already be narrowed to string after the early return, not int|string.

What was done:

1. Removed the unnecessary processBooleanNotSureConditionalTypes skip — The $scopeType->equals($conditionType) check was added symmetrically with the Sure variant, but unlike that one, the Not Sure variant's skip didn't prevent any real regression. Removing it causes no test failures.

2. Rewrote the test from testNotSureTriviallyTrueConditionSkipped to testNotSureConditionalTypes:

   • Changed $type from non-falsy-string to int|string, making is_string($type) a meaningful condition (not trivially true)
   • The test now demonstrates that processBooleanNotSureConditionalTypes creates a conditional holder "if $type=string, then $value=string" which fires when $type is later narrowed via is_string()
   • Asserts assertType('string', $value) inside if (is_string($type)) — a correct and non-weird assertion

Why assertType('string', $value) can't work with non-falsy-string: Making this work requires an optimization in the BooleanAnd false-context handler to detect that one side is trivially true (!(A && B) = !B when A is always true). I implemented this optimization but it caused regressions in loop processing — the optimization changes false-context types on the first loop iteration but not subsequent ones (after scope merging), leading to inconsistent type narrowing. This is a separate improvement that could be addressed in a future PR.