check variance of template types in properties#2314
check variance of template types in properties#2314ondrejmirtes merged 3 commits intophpstan:1.10.xfrom
Conversation
|
Please fix the build, I'll review it once PHPStan-on-PHPStan is green :) |
src/Rules/Generics/VarianceCheck.php
Outdated
There was a problem hiding this comment.
Honestly, I'm not entirely sure if this part is correct. Could you give me an example of a property whose readable and writable types differ?
There was a problem hiding this comment.
Native property will always have the same read/write type.
There was a problem hiding this comment.
You can probably simplify this code by typehinting PhpPropertyReflection only (it's the return type of ClassReflection::getNativeProperty). And remove things that aren't possible (and thus untestable) with native properties.
|
Please rebase - conflicts in config :) |
22dd356 to
47cc3a2
Compare
|
Rebased :) |
|
Really nice, thank you! |
|
@jiripudil This PR introduced phpstan/phpstan#9195 . Why property is unsafe to be covariant when intentionally declared so? |
|
Properties (with the exception of readonly) can be both read from and written to, which makes them invariant. Consider this function: /**
* @param Magic<Fruit> $magic
*/
function doMagic(Magic $magic): void
{
}The function could assign an /**
* @param Magic<Fruit> $magic
*/
function doMagic(Magic $magic): void
{
$magic->property = new Apple();
}But by making the /** @var Magic<Orange> */
$magic = new Magic();
doMagic($magic);You have just mixed apples with oranges :) |
|
Thank you - I got a little confused with the |
Picked from #2075. To sum up:
Properties are treated as invariant. The only exception is native readonly properties: I believe we can safely consider them covariant because PHPStan already makes sure that they are initialized in the constructor and only read ever since.
Similarly to methods, private properties are ignored because they do not affect subtyping in any way.
For the time being, this check also covers static properties. I still think static properties should not be allowed to reference class template types at all, but that turns out to be difficult to do the right way (#2232), so I say let's leave it unresolved for now. I doubt people are using template types in static properties anyway, because they are an immediate footgun.