-
Notifications
You must be signed in to change notification settings - Fork 60
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
Type projections #138
Conversation
= TokenAngleBracketOpen GenericTypeArgument *(TokenComma GenericTypeArgument) TokenAngleBracketClose | ||
|
||
GenericTypeArgument | ||
= [TokenContravariant / TokenCovariant] 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.
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 |
The TL; DR is that 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 |
My mind-dump (aka explanation) here https://hrach.dev/posts/variance/ - star projection is syntax sugar for covariant use-site type projection. |
|
src/Ast/Type/TypeProjectionNode.php
Outdated
public $type; | ||
|
||
/** @var 'covariant'|'contravariant' */ | ||
public $variance; |
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.
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.
Currently:
I'm still kinda interested in this feature ; but so far I found an hacky solution:
I like the syntax
Since it basically says "I accept any Box" but I just add |
I am in favor of |
Might be worth to ping psalm maintainer @orklah to check if this feature can also be handled by psalm. |
👍 |
TypeScript has a good explanation: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#optional-variance-annotations-for-type-parameters
|
src/Ast/Type/TypeProjectionNode.php
Outdated
|
||
use PHPStan\PhpDocParser\Ast\NodeAttributes; | ||
|
||
class TypeProjectionNode implements 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.
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 Sure, one day we can move on and use |
Does the following example will be valid:
?
? |
You can't call |
ok, so it will be less permissive than my hacky solution
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 |
I don't either, there's no |
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.
Are there any downsides for for the latter ? |
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.
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
Well, I think for 99.99 % of users, |
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 Thank you! |
Is this said talk public? :-) |
@staabm It is, but in Czech https://www.youtube.com/watch?v=-tOCEtrQPww |
32417d9
to
ace9e07
Compare
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 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 |
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
$genericTypes = []; | ||
|
||
foreach ($this->genericTypes as $index => $type) { | ||
$variance = $this->variances[$index]; |
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.
} elseif ($tokens->tryConsumeTokenValue('covariant')) { | ||
$variance = Ast\Type\GenericTypeNode::VARIANCE_COVARIANT; | ||
} else { | ||
$variance = Ast\Type\GenericTypeNode::VARIANCE_INVARIANT; |
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:
|
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 🤔