-
Notifications
You must be signed in to change notification settings - Fork 65
Type projections #138
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 #138
Conversation
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.
nit: tabs vs. spaces
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.
But which one is correct? It's pretty inconsistent across this whole file 😃
/cc @VincentLanglet @hrach who were interested in this in the past.
Also /cc @rvanvelzen @JanTvrdik for an opinion on the syntax :)
I would much more prefer the in
/ out
syntax used by Kotlin and TypeScript. It's both shorter and easier to understand.
The TL; DR is that @param Collection<covariant Animal>
will allow you to pass Collection<Dog>
there even in case of @template T
(and not @template-covariant
). But you won't be able to call methods that have T
in the parameter position.
More info about current behaviour here: https://phpstan.org/blog/whats-up-with-template-covariant
Also /cc @arnaud-lb for an opinion :)
I don't understand the in
/out
syntax, I don't know which one is which. Any mnemonic help for that?
hrach
commented
Jun 20, 2022
My mind-dump (aka explanation) here https://hrach.dev/posts/variance/ - star projection is syntax sugar for covariant use-site type projection.
Any mnemonic help for that?
in
is for parameters (inputs, writing "into" collection) and out
is for return types (outputs, reading "out of" collection).
src/Ast/Type/TypeProjectionNode.php
Outdated
@hrach
hrach
Jun 20, 2022
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.
Shouldn't be here also invariant variance and be the default?
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 maybe use class string constants instead of literal strings so it can be nicely checked in user code :)
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.
Correct me if I'm wrong but I thought invariance does not create a type projection? Currently, the parser simply parses the naked type if there is no covariant
/contravariant
(or in
/out
) and it doesn't get any special treatment further on.
I think if we included invariant here, this class would have to become TypeArgumentNode
which is either invariant (=> naked type), or contravariant or covariant (=> type projection). But then what about star projection? It is only an idiomatic shortcut for out mixed
/ in never
and could be resolved to either one, but I can see the value in distinguishing it explicitly.
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.
What about doing a similar thing I suggested in phpstan-src and make the variance simply an additional property of GenericTypeNode instead?
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.
@jiripudil sorry, you're right, I missed the 359's early return. Anyway, wouldn't you rather consider something like GenericTypeParameterNode($type, $variance)
(and ~ GenericStarParameterNode
) -- i.e. the parser describes what it parses, not what is the semantic.
@hrach
hrach
Jun 20, 2022
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.
(but that's BC break :/ if done through composition)
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.
What about doing a similar thing I suggested in phpstan-src and make the variance simply an additional property of GenericTypeNode instead?
I haven't yet had time to investigate this direction properly. I guess it could work. I'm still unsure about how to approach the star projection, though. It is equivalent to covariant mixed
and I believe that's how people usually think of it, but IMO it deserves an explicit representation, even if only to preserve the appearance in code in the error output.
/cc @VincentLanglet @hrach who were interested in this in the past.
The TL; DR is that
@param Collection<covariant Animal>
will allow you to passCollection<Dog>
there even in case of@template T
(and not@template-covariant
). But you won't be able to call methods that haveT
in the parameter position.More info about current behaviour here: https://phpstan.org/blog/whats-up-with-template-covariant
Currently:
- https://phpstan.org/r/d496e8d3-2aac-4a56-b88b-af37a9332bd1 reports an error since the template is not declared
- https://phpstan.org/r/8ce15174-c2c5-46d5-9bec-36d1b44707d8 reports an error because of the covariance
I'm still kinda interested in this feature ; but so far I found an hacky solution:
- https://phpstan.org/r/a859d377-52ba-46a1-93f0-914469d35607
It's maybe stronger than the solution proposed by this PR since - https://phpstan.org/r/30b20739-5d6b-4deb-913a-b25b10592645 is reported as an error
- but https://phpstan.org/r/6e829e83-21bb-4bfe-a310-aba9862ed3b5 is fully working (and I'm not sure this PR will support this)
And it works with psalm https://psalm.dev/r/c566a5d99a
I like the syntax
@param Box<*>
Since it basically says "I accept any Box" but I just add <*>
to avoid a Phpstan level 6 error.
I am in favor of in
/out
too, but above all, I'd prefer the language to be consistent, so if we go that way, I think we should also support @template-out
(and eventually @template-in
). WDYT?
Might be worth to ping psalm maintainer @orklah to check if this feature can also be handled by psalm.
hrach
commented
Jun 20, 2022
I think we should also support @template-out (and eventually @template-in). WDYT?
👍
vhenzl
commented
Jun 20, 2022
I don't understand the
in
/out
syntax, I don't know which one is which. Any mnemonic help for that?
TypeScript has a good explanation: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#optional-variance-annotations-for-type-parameters
in
is for setters with contravariant parameters, out
for getters with a covariant return type.
src/Ast/Type/TypeProjectionNode.php
Outdated
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.
Ideally TypeProjectionNode
should not implement TypeNode
, because it is not a type and cannot be used in places where type is expected. It's only allowed in place of generic argument.
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.
It is roughly equivalent to CallableTypeParameterNode
which implements just Node
instead of TypeNode
.
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 made a suggestion here: #138 (comment)
What's most important to me is consistency. It's easy to explain "To solve this problem, you either need to write covariant
at declaration or at call-site". It's much harder to grasp "you need to write covariant
at declaration or in
at call-site".
Sure, one day we can move on and use in/out
instead everywhere but we'd have to do that in sync with Psalm maintainers and PhpStorm developers at minimum.
Does the following example will be valid:
/**
* @param Collection<covariant mixed> $collection
*/
public function compute(Collection $collection): int
{
$item = $collection->get();
if (null !== $item) {
$collection->add($item);
}
return $collection->count();
}
?
or it will only solve
/**
* @param Collection<covariant mixed> $collection
*/
public function compute(Collection $collection): int
{
return $collection->count();
}
?
for the example https://phpstan.org/r/6e829e83-21bb-4bfe-a310-aba9862ed3b5
You can't call $collection->add($item)
on Collection<covariant mixed>
.
You can't call
$collection->add($item)
onCollection<covariant mixed>
.
ok, so it will be less permissive than my hacky solution
/**
* @template T
* @param Collection<T> $collection
*/
public function compute(Collection $collection): int
{
$item = $collection->get();
if (null !== $item) {
$collection->add($item);
}
return $collection->count();
}
but at least it will not be an hacky solution and solve lot of usecase.
Updating the blog article about generics will be useful IMHO for people like me which are not a lot familiar with covariance/contravariance. I currently don't know what will be the use case for Collection<contravariant mixed>
I currently don't know what will be the use case for
Collection<contravariant mixed>
I don't either, there's no @template-contravariant
yet...
I have a preference for out/in as it's easier to remember than covariant/contravariant and also shorter to type/read. But since we already use covariant/contravariant, that it's the way to go in my opinion.
One thing I'm not sure about is whether
Foo<covariant>
should throw a parse error (as currently implemented), or whether it should create an IdentifierTypeNode('covariant'). The former is potentially a BC break if somebody has a class named covariant in their code, and however unlikely it seems, you can always break somebody's build thinking
Are there any downsides for for the latter ?
ok, so it will be less permissive than my hacky solution
I don't think making the function itself generic is hacky, actually. See https://hrach.dev/posts/variance/ linked above, there's a section named 'Breaking the Contract' at the bottom with code very similar to yours which solves the problem exactly that way.
I don't either, there's no @template-contravariant yet...
Honestly, neither do I, all the examples you can find on the Internet look very artificial to me. Maybe we could limit type projections to covariance, for starters? Some of the logic might need to be implemented anyway because star projections somewhat act in both ways, but we could only support covariant projections on the parser level and then introduce contravariant projections together with @template-contravariant
when someone with an actual use case requests them.
Are there any downsides for for the latter ?
Well, I think for 99.99 % of users, Foo<covariant>
is by mistake, and parse error means failing early. But it fails anyway, and with a pretty informative error message too. So no, I don't see any downsides.
8b988e6
to
32417d9
Compare
@jiripudil I just watched your talk on generics on YouTube, and I’m really looking forward to this! I just checked the code again, I think there shouldn’t be class TypeProjectionNode implements TypeNode
. We need to move this onto GenericTypeNode, and in some backward-compatible manner, like an extra property called typeVariances
.
Thank you!
Is this said talk public? :-)
mabar
commented
Nov 27, 2022
@staabm It is, but in Czech https://www.youtube.com/watch?v=-tOCEtrQPww
32417d9
to
ace9e07
Compare
I just watched your talk on generics on YouTube, and I’m really looking forward to this! I just checked the code again, I think there shouldn’t be
class TypeProjectionNode implements TypeNode
. We need to move this onto GenericTypeNode, and in some backward-compatible manner, like an extra property calledtypeVariances
.
Thanks! Yes, I've had that in the works for some time :) I've rebased this onto 1.9.x and changed the implementation to populate GenericTypeNode::$variances
.
I think I've also managed to figure out that damned star projection – it's effectively a bivariant placeholder, so I'm treating it as such. I'm really happy about this because in phpstan-src, we'll be able to represent it the same way as here – add support for this new case of TemplateTypeVariance
, and simply keep a list of use-site variances in GenericObjectType
, as you've suggested :)
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.
Otherwise I'm ready to merge this and tag a new minor release 😊
src/Ast/Type/GenericTypeNode.php
Outdated
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 offset might not exist.
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 unsupported covariance name like bullshitriant
isn't gonna cause a parse error and will silently fall back to invariant, right? Maybe we should address that or at least test it.
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.
It falls back to invariant and proceeds to parse the value as a type. Thus,
Foo<bullshitriant>
creates a generic type withIdentifierTypeNode('bullshitriant')
in it (supposedly failing later due to the unknown class). That seems correct to me, I don't think there's a reliable way to distinguish between a typo and a type on the parser level.Foo<bullshitriant Type>
throws because 'bullshitriant' is parsed as an identifier and the parser expects a comma or a closing angle bracket to follow. I'll add this case to the test.
Thank you very much!
BTW I can imagine at least 3-4 PRs in phpstan-src about this:
- @template-contravariant for completeness sake
- Basic support for this new syntax in the typesystemby reading the new node property, putting it into GenericObjectType etc.
- Doing all the stuff about limiting what method can be called based on call-site variance, this is gonna be some new logic in GenericObjectType
- Checking that the usage of type projection is sane and safe, checking declaration site variance against call-site variance etc. Checking that call-site variance isn't used in a bad place like
@implements
etc.
This PR adds support for parsing type projections (phpstan/phpstan#3290) which I'd like to implement in PHPStan. Type projections are a way of declaring the variance of a type parameter at the site of use rather than at the site of its declaration (via
@template-covariant
). This allows you to have the type parameter in an incompatible position in the generic class, and only prevents you from using it where the projection is used.Type projection can be created in the type argument position of a generic type by using either:
The choice of the variance keywords favours consistency: while Kotlin uses
in
andout
which might arguably be more intent-revealing, PHPStan already uses@template-covariant
in a similar position, so I say let's stick with it.One thing I'm not sure about is whether
Foo<covariant>
should throw a parse error (as currently implemented), or whether it should create anIdentifierTypeNode('covariant')
. The former is potentially a BC break if somebody has a class namedcovariant
in their code, and however unlikely it seems, you can always break somebody's build 🤔