Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

check variance of template types in properties #2314

Merged
merged 3 commits into from Apr 3, 2023

Conversation

jiripudil
Copy link
Contributor

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.

@ondrejmirtes
Copy link
Member

Please fix the build, I'll review it once PHPStan-on-PHPStan is green :)


$errors = [];

if ($propertyReflection->isReadable()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native property will always have the same read/write type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ondrejmirtes
Copy link
Member

Please rebase - conflicts in config :)

@jiripudil
Copy link
Contributor Author

Rebased :)

@ondrejmirtes
Copy link
Member

Really nice, thank you!

@ondrejmirtes ondrejmirtes merged commit 164241d into phpstan:1.10.x Apr 3, 2023
377 checks passed
@jiripudil jiripudil deleted the property-variance-check branch April 3, 2023 13:13
@mvorisek
Copy link
Contributor

@jiripudil This PR introduced phpstan/phpstan#9195 . Why property is unsafe to be covariant when intentionally declared so?

@jiripudil
Copy link
Contributor Author

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 Apple into the object's property, and the property would happily accept it, because the type of the property in this piece of code is just Fruit:

/**
 * @param Magic<Fruit> $magic
 */
function doMagic(Magic $magic): void
{
    $magic->property = new Apple();
}

But by making the Magic's type parameter covariant, you are saying that Magic<Orange> is a subtype of Magic<Fruit>, and therefore will be accepted by the function:

/** @var Magic<Orange> */
$magic = new Magic();

doMagic($magic);

You have just mixed apples with oranges :)

@mvorisek
Copy link
Contributor

Thank you - I got a little confused with the ...but occurs in invariant position in property error - maybe the error can be reworded or phpstan doc added? If I understand this correctly, there is nothing like "invariant position in property", but simply phpstan forces the user to not use covariant template in property phpdoc as it might be easily accepted by weaker template type - https://phpstan.org/r/8dbb1732-a361-46be-b771-113f1ef11de2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants