Check inner union types before delegating to isSubTypeOf for LateResolvableType in UnionType::isSuperTypeOf()#5645
Conversation
staabm
left a comment
There was a problem hiding this comment.
needs another test which reflects 1:1 the snippet from phpstan/phpstan#10942
|
Done. I updated the test to include the exact snippet from the issue's playground link as the primary test case (classes A and B with |
…esolvableType` in `UnionType::isSuperTypeOf()` - When a `ConditionalTypeForParameter` (or other `LateResolvableType`) was inside a `UnionType`, `isSuperTypeOf()` immediately delegated to `isSubTypeOf()`, which resolved the conditional and lost precision (anding with Maybe for non-resolvable types) - Now the inner types of the union are checked first via `isSuperTypeOf()`, which allows matching conditional types to find each other directly (via the `$type instanceof self` branch in `ConditionalTypeForParameter::isSuperTypeOf()`) before falling back to the resolve-based `isSubTypeOf()` path - This fixes false positive "should be contravariant/covariant" errors when a child method overrides a parent method with the exact same conditional parameter/return type in a union
The existing test used a simplified snippet. Add the exact code from the issue's playground link (with inherited conditional param type and no repeated PHPDoc in the child class) as the primary test case, and keep the original explicit-PHPDoc variant as a secondary case. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b900e03 to
559d805
Compare
VincentLanglet
left a comment
There was a problem hiding this comment.
Without changing the code, explain in a github comment, step by step how the type method are used/resolved in order to produce the false positive should be contravariant and why this change fix it.
|
The token doesn't have write permissions to the upstream No code changes are needed — the reviewer explicitly said "without changing the code, explain in a github comment." Here's the explanation: @VincentLanglet Here is the step-by-step walkthrough of how the false positive was produced and why this change fixes it. SetupConsider the reproducer from the issue: class Step 1: How the variance check starts
$parameterType->isSuperTypeOf($parentParameterType)Both The call becomes: Step 2: UnionType::isSuperTypeOf() dispatches parentUnionSince if ($otherType instanceof self && !$otherType instanceof TemplateUnionType) {
return $otherType->isSubTypeOf($this);
}This calls Step 3: UnionType::isSubTypeOf() iterates inner types
return IsSuperTypeOfResult::extremeIdentity(
...array_map(fn (Type $innerType) => $otherType->isSuperTypeOf($innerType), $this->types)
);This makes two calls:
Step 4: UnionType::isSuperTypeOf() with a ConditionalTypeForParameter — the bugNow Before the fix, the early-return block at line 268 included: || ($otherType instanceof LateResolvableType && $otherType instanceof CompoundType && !$otherType instanceof TemplateType)This matched, so PHPStan immediately returned Step 5: LateResolvableTypeTrait::isSubTypeOf() resolves and loses precision
Since return $otherType->isSuperTypeOf($result); // childUnion->isSuperTypeOf(IntegerType)The child union if (!$this->isResolvable()) {
$isSuperType = $isSuperType->and(IsSuperTypeOfResult::createMaybe());
}
Step 6: The result propagates as Maybe → false positiveBack in step 4, the early return gives Maybe for Back in step 3, Back in step 1, the parameter result is The same flow applies to return type covariance (step is Why the fix worksAfter the fix, the foreach ($this->types as $innerType) {
$result = $innerType->isSuperTypeOf($otherType);
if ($result->yes()) {
return $result;
}
$results[] = $result;
}When if ($type instanceof self) {
return $this->if->isSuperTypeOf($type->if)
->and($this->else->isSuperTypeOf($type->else));
}Since both conditionals have identical The I wasn't able to post this comment directly to the PR — the token doesn't have write access to |
VincentLanglet
left a comment
There was a problem hiding this comment.
Shouldn't the fix be inside LateResolveTypeTrait instead ?
Let's have a ConditionalType,
ConditionalType(...) should be subTypeOf Union(ConditionalType(...), Foo)
|
All 12,062 tests pass and Here's the summary for the PR response: The fix can't be moved to Why the trait can't call Every type that uses the standard if ($type instanceof CompoundType) {
return $type->isSubTypeOf($this);
}So Why a That approach requires removing the Why It's the only place that can both:
This is the same pattern already used for |
Summary
When a child method overrides a parent method with the exact same conditional parameter type in a union (e.g.,
string|($x is int ? float : bool)), PHPStan incorrectly reported "should be contravariant" for parameters and "should be covariant" for return types. The issue was thatUnionType::isSuperTypeOf()did not give the inner types a chance to directly matchLateResolvableTypemembers before delegating to the resolve-basedisSubTypeOf()path.Changes
UnionType::isSuperTypeOf()insrc/Type/UnionType.phpto check inner types first when comparing against aLateResolvableType && CompoundType. If any inner type directly confirms it is a supertype (e.g., a matchingConditionalTypeForParametercomparing via its$type instanceof selfbranch), the method returnsYesimmediately. Otherwise it falls back to the existingisSubTypeOf()delegation.tests/PHPStan/Rules/Methods/data/bug-10942.phpcovering both parameter contravariance and return type covariance with conditional types in unions.Root cause
UnionType::isSuperTypeOf()had an early return forLateResolvableType && CompoundTypetypes that immediately delegated to$otherType->isSubTypeOf($this). This method (fromLateResolvableTypeTrait) resolves the conditional type to the union of its if/else branches, then checks if that resolved type fits the target. For non-resolvable types likeConditionalTypeForParameter, the result is downgraded to at mostMaybevia$isSuperType->and(IsSuperTypeOfResult::createMaybe()).The inner type loop (which would have found a matching
ConditionalTypeForParametervia theif ($type instanceof self)branch returningYes) was never reached because the early return ran first.The fix separates the
LateResolvableTypecase from the other early returns, first trying the inner type loop to find direct matches before falling back toisSubTypeOf.Analogous cases probed
ConditionalType(template-based conditionals): Has the sameisSuperTypeOfstructure with$type instanceof selfbranch. The fix covers this case too, though it's less likely to appear in practice becauseTemplateTypeHelper::resolveToBounds()usually makesConditionalTyperesolvable before the comparison.LateResolvableTypeimplementations (KeyOfType,ValueOfType,OffsetAccessType,NewObjectType,ClassConstantAccessType,GetTemplateTypeType): These use the defaultisSuperTypeOfDefault()from the trait without a same-type shortcut. The fix still tries them first but they'd returnMaybefor non-resolvable cases. In practice, these types are typically resolvable afterresolveToBounds(), so the issue doesn't manifest.IntersectionType::isSuperTypeOf(): Does not have the same early return pattern forLateResolvableType— it usesextremeIdentityover inner types directly, so it's not affected.isSuperTypeOf()is used for both parameter contravariance and return type covariance checks, so the fix addresses both directions.Test
tests/PHPStan/Rules/Methods/data/bug-10942.php: Tests that overriding a method with identicalstring|($x is int ? float : bool)parameter and return types produces no errors. Verified the test fails without the fix (reports both contravariance and covariance errors).Fixes phpstan/phpstan#10942