-
Notifications
You must be signed in to change notification settings - Fork 439
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
Type projections, part 3: call-site restrictions #2485
Conversation
You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, before I dive into this, I'd love NonAcceptingNeverType as a separate PR before this one. If someone uses literal never
in the PHPDoc, we could resolve it to NonAcceptingNeverType, which would solve phpstan/phpstan#9133 nicely.
4d89998
to
2e913ba
Compare
c77c864
to
adfb2bc
Compare
adfb2bc
to
0ba5090
Compare
Some tests failing :) |
Right. I don't yet understand why though, I'll need to take a deeper look |
0ba5090
to
0389ac8
Compare
Don't worry, the failures are not your fault :) I'm gonna review this as soon as time allows :) |
Thanks :) the tests that were failing originally were just minor oversights on my side. But at least they served a purpose of highlighting the one thing I'm the most uncertain of in this PR: the implementation duplicates logic. As I've already mentioned above, variance composition rules are now in two places, and as it turns out, the type traversal is now in three places 😟 |
Can you please link the places with duplicate logic? So I can see it side by side and decide if it's harmful or not :) |
Basically in every So if any specific implementation has special rules (such as "if array's key and value types are both |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions:
- The whole logic replacing parameter types with
never
so that the method cannot be called is done inGenericObjectType::traverseWithVariance
? :) - Maybe I should have asked this in a previous PR: So now we can type things like
Box<covariant B>
in function signatures. Would it make sense to also allow this in@extends
/@implements
/@use
? :) Because that is also a usage of a generic class that might benefit from that.
@@ -19,6 +20,8 @@ public function getTemplateTypeMap(): TemplateTypeMap; | |||
|
|||
public function getResolvedTemplateTypeMap(): TemplateTypeMap; | |||
|
|||
public function getCallSiteVarianceMap(): TemplateTypeVarianceMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can safely add this method only on ParametersAcceptorWithPhpDocs
. The name of that class should really be ExtendedParametersAcceptor
, the current name is a legacy burden :)
In most places in PHPStan, you can actually rely on ParametersAcceptorWithPhpDocs
being there.
ParametersAcceptorWithPhpDocs
follows ExtendedMethodReflection
- an interface you can call, but cannot implement because a method might be added on it at any time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So try to move this to ParametersAcceptorWithPhpDocs
, I think not a lot of places will break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a list of @api
interfaces where we can add methods: https://github.com/phpstan/phpstan-src/blob/1.11.x/src/Rules/Api/BcUncoveredInterface.php
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I'll take a look
src/Type/ArrayType.php
Outdated
@@ -659,6 +659,24 @@ public function traverse(callable $cb): Type | |||
return $this; | |||
} | |||
|
|||
public function traverseWithVariance(TemplateTypeVariance $variance, callable $cb): Type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
traverseWithVariance
seems like a combination of getReferencedTemplateTypes
+ traverse
. Could we somehow use that instead of adding a new traverseWithVariance
method on Type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And even if not, it might be possible to somehow deduplicate the logic between traverse
and traverseWithVariance
? It might be hard to keep them in sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like maybe traverse
could just call traverseWithVariance
with a little bit different callback :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
traverseWithVariance
seems like a combination ofgetReferencedTemplateTypes
+traverse
. Could we somehow use that instead of adding a newtraverseWithVariance
method onType
?
Funny you mention that. In one of my early prototypes, I've had it implemented that way. But it felt too fragile because it relied on object identity (TemplateTypeReference
's template type === template type passed into the traverse
callback), so I threw it away...
Like maybe traverse could just call traverseWithVariance with a little bit different callback :)
Hmm, could be, I'll see where it leads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the prototype code in jiripudil#7, I'm curious @ondrejmirtes if you think that could be a way. I have a bad feeling about it because of the reliance on object identity, but it completely eliminates the logic duplication, and apparently it works 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that, can you please simplify this #2485 PR with this approach? :)
And BTW in case ===
would not work for us in the future, we could use Type::equals()
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's the tricky bit. Consider this example:
/** @template T */
class Foo {
/**
* @param callable(T): T $cb
*/
public function doFoo(callable $cb): void { /* ... */ }
}
/** @param Foo<*> */
function bar(Foo $foo) {
$foo->doFoo(/* ... */);
}
Now, at some point in the resolution, PHPStan needs to replace the template types referenced in the type callable(T): T
. The thing is that it's the same template type which happens to be in two positions with different variance, and we need to distinguish between the two occurrences and treat each of them differently.
This simple approach uses object identity for this, relying solely on the fact that PHPStan creates a new instance of the template type for each of the occurrences. The two template types are otherwise equal in all regards, therefore, no, we need to stick with ===
, and the code will only work as long as this prerequisite remains fulfilled.
I like the simplicity of this solution, but I can't shake off the feeling that it's relying on an implementation detail and that it may all break as soon as somebody comes with an optimization of how types are created.
If you're ok with it, I am too, but I want to make sure you have all the relevant information to make the decision :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's okay now and if we need it, we have a solution for that (a new method on Type).
This would make a really nice comment next to the ===
operator 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and done :)
It's all done in
That's specifically forbidden and reported in the previous PR (#2481) – these annotations are part of the declaration, hence no use for call-site variance. |
0389ac8
to
f937c29
Compare
f937c29
to
fc00bb9
Compare
Please fix the tests, otherwise 👍 :) |
fc00bb9
to
8c9d49a
Compare
Please are you up to the task of updating documentation and maybe also writing a separate article about this? :) Thank you very much. Here's a previous comment about all the docs: #2481 (comment) |
preserve array<*NEVER*, *NEVER*> -> array{} conversion
8c9d49a
to
2e8f508
Compare
I think a separate article is a must. It can then be referenced from other doc pages, release notes, etc. I'll take a look later today 🤞 |
Great, thank you very much! |
So, this is the big one that makes call-site variance type-safe 😌
Rationale
As I've mentioned before, I wanted to take a more benevolent approach than simply preventing people from calling methods. The general idea is that when template types are resolved, the call-site variance should be taken into consideration, according to the following rules.
I'll be using this invariant
Box
in the examples:Contravariant projection
A contravariant projection can safely replace covariant usages of the template type with its upper bound. This is safe because the template type's upper bound is the most general supertype of
Dog
that can be used in place ofT
, and therefore the result of$box->get()
is always guaranteed to be its subtype:Covariant projection
By the same token, a covariant projection can safely replace contravariant usages of the template type with its lower bound. This is safe because the lower bound is the most specific subtype of
Animal
, and therefore will be accepted in theput()
method by every possible type instance ofBox
:For now, the lower bound is always implicitly
never
which – as a mere side effect of this resolution – makes the method effectively uncallable. (That's one more reason for #2110, but before that can be resolved, I've worked around it by implementing a separateNonAcceptingNeverType
.)It's noteworthy that there's a feature request for user-specified lower bounds (phpstan/phpstan#5179) which would integrate nicely with this, allowing you to actually call the
put()
method:Star projection
A star projection is bivariant, it stretches into both directions. It can accept any type that's within the entirety of the template type's bounds, and therefore has to impose both of the restrictions:
Implementation notes
Implementation-wise, the main change is that
TemplateTypeHelper::resolveTemplateTypes()
accepts the variance of the evaluated position and the contextual call-site variance map, and uses them to resolve the type projections to the template type's bounds as described above.Two big chunks of changes are necessary to allow that. I've tried to split the commits by these chunks so that it's easier to distinguish where the 🪄 magic happens and what's just a tedious implementation of new interface methods:
The call-site variance map needs to be passed around from the reflection layer, from
ClassReflection
through the many associated types such as property and methods reflections.The resolution needs to traverse the type while recursively composing its positional variance, similarly to
Type::getReferencedTemplateTypes()
. For this, I've added a newVarianceAwareTypeTraverser
along withType::traverseWithVariance()
that combines traversal with the variance composition logic.I'm not too happy about the fact that this logic is now in two places (
getReferencedTemplateTypes()
andtraverseWithVariance()
), so if you have any ideas on how to deduplicate it, I'm all ears.Also, I've only added a high-level integration test that covers the feature as a whole, so I'm open to suggestions on what kinds of tests to add.
I'll be eagerly awaiting your feedback :) not gonna lie, it feels really good to finally get this done. I'm so looking forward to getting this merged and reaping the fruits of this effort!