Do not treat mixed type arguments as compatible both ways when both generic types are parameterized - #6398
Conversation
… generic types are parameterized - `TemplateTypeVariance::isValidVariance()` gained a `$strict` flag. Without it the behaviour is unchanged: `mixed` and `BenevolentUnionType` type arguments answer "yes" in both directions, which is what keeps unparameterized class names (whose type arguments are resolved to the template bounds) compatible with parameterized ones. - With `$strict` the gradual-typing shortcuts are skipped, so covariance and contravariance are evaluated normally, and invariance only keeps `Foo<mixed>` a supertype of `Foo<int>` while the opposite direction becomes `maybe`. - `GenericObjectType::isSuperTypeOfInternal()` turns the strict mode on outside of the accepts context when the compared type is itself a `GenericObjectType`, i.e. when both sides carry explicit type arguments. - This makes `isSuperTypeOf()` antisymmetric again, so `TypeCombinator::union()` and `TypeCombinator::intersect()` no longer produce a different result depending on the order of the operands. - Analogous cases probed and fixed by the same change: `@template-covariant`, `@template-contravariant`, generic interfaces reached through `@implements`/`@extends`, `class-string<Foo<mixed>>`, and intersections. Array/iterable/callable/object-shape type arguments and unparameterized subclasses (`None extends Option<mixed>`) were probed and were already order-independent. - Extended `TemplateTypeVarianceTest` to assert both the default and the strict result in both directions.
ondrejmirtes
left a comment
There was a problem hiding this comment.
Add regression test for phpstan/phpstan#11935
Pins the gradual-typing leniency the strict variance mode has to keep: `accepts()` stays symmetric for a `mixed` type argument, while `isSuperTypeOf()` only answers `yes` when the `mixed` is on the supertype side. See phpstan/phpstan#11935 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed as What I added
Verification
One thing worth flaggingIssue #11935's own snippet — plain |
|
did this PR close phpstan/phpstan#11935 ? |
Summary
Assigning
new Foo($mixed)in atryblock andnew Foo(123)in the correspondingcatchblock made PHPStan inferFoo<int>for the merged variable — theFoo<mixed>branch was silently dropped.The union of
Foo<mixed>andFoo<int>was order-dependent:Foo<mixed>|Foo<int>collapsed toFoo<mixed>whileFoo<int>|Foo<mixed>collapsed toFoo<int>.GenericObjectType::isSuperTypeOf()answeredyesin both directions, soTypeCombinator::union()absorbed whichever type happened to come first.Changes
src/Type/Generic/TemplateTypeVariance.php—isValidVariance()gained abool $strict = falseparameter. In the default (gradual) mode nothing changes. In the strict mode themixed/BenevolentUnionTypeshortcuts that answeryesin both directions are skipped; covariance and contravariance then fall through to the normalisSuperTypeOf()computation, and invariance is relaxed only in one direction (mixedon the supertype side →yes, on the subtype side →maybe).src/Type/Generic/TemplateType.php,src/Type/Generic/TemplateTypeTrait.php— the new optional parameter threaded throughTemplateType::isValidVariance().src/Type/Generic/GenericObjectType.php—isSuperTypeOfInternal()enables the strict mode outside of the accepts context when the compared type is itself aGenericObjectType.tests/PHPStan/Type/Generic/TemplateTypeVarianceTest.php— now asserts the result in both directions for both the default and the strict mode.phpstan-baseline.neon— bumped the ignoredphpstanApi.instanceofTypecount forGenericObjectType.php(3 → 4).Analogous cases that were broken the same way and are fixed by this change (all covered by the regression test):
@template-covariant:CovariantFoo<int>|CovariantFoo<mixed>used to collapse toCovariantFoo<int>.@template-contravariant:ContravariantFoo<mixed>|ContravariantFoo<int>used to collapse toContravariantFoo<mixed>instead ofContravariantFoo<int>.@implements/@extends:FooInterface<int>|FooInterface<mixed>.GenericClassStringType:class-string<Foo<mixed>>|class-string<Foo<int>>used to collapse toclass-string<Foo<int>>.TypeCombinator::intersect():Foo<mixed>&Foo<int>used to produceFoo<mixed>instead ofFoo<int>.Probed and found to be already order-independent, so left alone:
array<mixed>/list<mixed>,iterable<mixed>,array{a: mixed}shapes,object{a: mixed}shapes, andClosure(mixed): voidparameter types.Root cause
TemplateTypeVariance::isValidVariance()short-circuited toyeswhenever either of the compared type arguments wasmixed(or aBenevolentUnionType), regardless of which side it was on. That is the right answer foraccepts()— gradual typing means an unparameterized ormixed-parameterized value is usable wherever a specific one is expected and vice versa — but it madeisSuperTypeOf()a symmetric relation.TypeCombinator::union()decides which of two types to drop by askingisSuperTypeOf()on$afirst and on$bsecond, so a symmetricyesmeans the surviving type is decided by argument order. Scope merging (if/else,try/catch,switch, loops) feeds the branch types into that union, which is why thecatchbranch won in the reported case.The shortcut still has to apply when the other side is a class name written without type arguments — those are resolved to the template bounds, so their
mixedmeans "not parameterized" rather than "the type argument ismixed". Making them strict would have broken sealed-class exhaustiveness (tests/PHPStan/Rules/Comparison/data/bug-14412.php) andNone extends Option<mixed>narrowing (tests/PHPStan/Analyser/nsrt/bug-11430.php). The strict mode is therefore only turned on when the compared type carries explicit type arguments.Test
tests/PHPStan/Analyser/nsrt/bug-15198.phpcontains the reproducer from the playground link (try/catchwithnew Foo($mixed)vsnew Foo(123)) plus:if/elsein both orders,Foo<mixed>|Foo<int>,CovariantFoo,ContravariantFoo,FooInterfaceandclass-string<Foo<…>>unions,Foo<mixed>&Foo<int>intersection,None|Option<string>in both orders, guarding the leniency that has to stay for unparameterized class names.Every new assertion except the last group fails without the source change.
TemplateTypeVarianceTestcovers the flag itself at the unit level.Fixes phpstan/phpstan#15198