Type projections, part 2: enforce elemental rules #2481
Conversation
| $typeProjectionIsNotAllowedMessage, | ||
| TypeProjectionHelper::describe($ancestorType->getTypes()[$index], $typeVariance, VerbosityLevel::typeOnly()), | ||
| $ancestorType->describe(VerbosityLevel::typeOnly()), | ||
| ))->identifier('generics.typeProjectionNotAllowed')->build(); |
There was a problem hiding this comment.
Now that you want this in 1.10.x, is adding an identifier here ok, or should I remove it and add it afterwards only to 1.11.x?
There was a problem hiding this comment.
This is okay, I might change the identifier though :)
| if ($templateType instanceof TemplateType && !$genericTypeVariance->invariant()) { | ||
| if ($genericTypeVariance->equals($templateType->getVariance())) { | ||
| $messages[] = RuleErrorBuilder::message(sprintf( | ||
| $typeProjectionIsRedundantMessage, |
There was a problem hiding this comment.
This is not technically a problem. I feel like it should be just a warning, but afaik PHPStan doesn't know that concept, so I've included it here along with a tip on how to fix it
jiripudil
force-pushed
the
type-projections-2
branch
from
9e28fc9
to
a6607d8
Compare
|
Before we continue, I'd also like you to think about documentation. Right now there are these pages about generics:
And also maybe we should write a completely new article explaining this whole issue and new posibilities with call-site variance. The source code for all of this is in https://github.com/phpstan/phpstan/tree/1.11.x/website/src so feel free to send PRs there too :) Thank you. I'll review your existing PRs as soon as possible but it might be a bit challenging, I'm travelling for the next two weeks but hopefully I'll find some time :) |
Yep, I'm thinking about that, and was mainly thinking along the lines of "maybe we should write a completely new article explaining this whole issue and new posibilities with call-site variance" :)
Absolutely no worries, I'll actually be off (and possibly offline) too for some time within the next two weeks |
| @@ -84,6 +84,15 @@ public function testRule(): void | |||
| 'PHPDoc tag @mixin contains non-object type int.', | |||
| 92, | |||
| ], | |||
| [ | |||
| 'Type projection contravariant MixinRule\Foo in generic type MixinRule\Adipiscing<contravariant MixinRule\Foo> in PHPDoc tag @mixin is conflicting with variance of template type T of class MixinRule\Adipiscing.', | |||
There was a problem hiding this comment.
Two things:
- Should we call this "type projection"? What exactly is the projection? Is
MixinRule\Adipiscing<Foo>(the common syntax we use today) also some kind of projection? I like "call-site variance" much better as opposed to "declaration variance". - The second part of the message "conflicting with variance of template type T" could specify the exact variance, so it could say:
"conflicting with covariance of template type T""conflicting with invariance of template type T""conflicting with contravariance of template type T"
There was a problem hiding this comment.
Good point, I've reworded the messages. Different sources use the term "type projection" rather vaguely, so it's for the best to use "call-site variance", hopefully it should clear any confusion about what the error refers to.
jiripudil
force-pushed
the
type-projections-2
branch
from
a6607d8
to
9b64cbc
Compare
jiripudil
force-pushed
the
type-projections-2
branch
2 times, most recently
from
5315178
to
2bea05b
Compare
ondrejmirtes
force-pushed
the
type-projections-2
branch
from
2bea05b
to
50397fc
Compare
|
Thank you! I believe we should do 3 more things:
Thank you very much. |
This PR should be straightforward. It prohibits type projections in incompatible places (e.g.
@extends) and checks that the call-site variance is not in conflict with the declaration-site variance.