-
Notifications
You must be signed in to change notification settings - Fork 65
Support @template tag #27
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
Conversation
Thank you, I like it! Perhaps @JanTvrdik can express if there's something wrong but for now it's great :)
In some future PR we should add support for also obtaining tags with prefixes like @phpstan-
and @psalm-
, but that can wait. We can start reading e.g. @psalm-var
and @psalm-param
once generics are fully supported.
@arnaud-lb I just realised we should probably support only bound atomic types, so no unions and intersections, to make our job easier. What do you think?
@arnaud-lb TemplateTagValueNode::$bound
must be nullable to differentiate between implicit and explicit mixed upper bound.
@ondrejmirtes I don't think that it makes much sense to restrict this in parser. If you don't want to support complex types for now, it's better to just ignore them in phpstan than to mark them as syntax error.
Also, as I've said on multiple occasions, @template
is extremely bad name as
- it is incompatible with many open-source (and close-source) libraries, such as sensio/framework-extra-bundle with over 40M installs
- the word "template" is used by C++, while PHP is much closer in semantic to Java/C#/Kotlin/TypeScript which all call this "generics".
As with phpDocs in general, we need to support what's already in the wild... cc @muglug - your thoughts on @template
vs. @generic
?
Of course I plan to read (and maybe even promote) prefixed tag versions first, but for our purposes in the current phase of development, @template
can be used...
@JanTvrdik : I tend to agree on the name of @template
, mainly because it refers C++ templates, and it does not even refers to types.
it is incompatible with many open-source (and close-source) libraries, such as sensio/framework-extra-bundle with over 40M installs
Are we sure of this ? Doctrine/Symfony annotations tend to use classes for annotations, and require either a FQCN or an import (use) of the class. So, @template
would be seen as an annotation of the class \annotation
, which would not collide with \Sensio\Bundle\FrameworkExtraBundle\Configuration
. Also, it might be case-sensitive.
So, I'm less worried regarding collisions with existing annotations.
Ocramius
commented
May 22, 2019
@template
is indeed a problem. The @psalm-template
used in doctrine/collections
was picked exactly because I disabled annotation parsing for anything containing dashes.
As for the syntax, I'd expect @template T of Active|Closed
and @template T of Active&Closed
to both work: is that requiring more work? Does any type declaration work after of
?
@ondrejmirtes regarding union/intersection types on RHS, I don't think they're a necessity, but they enable some nice features e.g.
/** * @template T as int|string */ class C {} /** @param C<int> $c */ function foo(C $c) : void {} /** @param C<string> $c */ function bar(C $c) : void {} /** @param C<object> $c */ -- this is an error function baz(C $c) : void {}
muglug
commented
May 22, 2019
vis-a-vis @template
or @generic
, I'd be happy to switch (but I'd still support @template
for legacy implementations).
Maybe something like @template-type
is unambiguous?
Just to clarify terminology - what you showed is an opposite of atomic type, right? :) Atomic = int, string, basically all non-unions and non-intersections.
I'm for supporting @template
(optionally with psalm-
and phpstan-
prefixes) today - if someone comes up with a better suggestion, we can always add it. Ideally it would be the job for the PSR-5 working group...
@muglug BTW is it @template T of Foo
or @template T as Foo
? is there any difference?
Ocramius
commented
May 22, 2019
Yes, my example was specifically around non-atomic types (valid scenario, IMO)
I'm for supporting @template
Supporting @template
means that phpstan will be incompatible with Symfony (and many others). Using sth like @generic
provides much better compatibility.
Besides the name, the current psalm syntax also has issue with making @template
tags order significant which is something that PHPDoc mostly avoids by design. So it MAY be better from PHPDoc design point-of-view to specify all generic parameters in a single tag instead of multiple, i.e. sth like
/** @generic <K, V> key and value */
This makes it harder to write description for each parameter, but it better respects PHPDoc design principles (from my point-of-view at least).
Atomic = int, string, basically all non-unions and non-intersections.
Assuming we're using terminology from this parser, then all types can be written as atomic. Atomic just requires some types (such as union types) to be wrapped in parentheses (see ABNF grammar).
Supporting
@template
means that phpstan will be incompatible with Symfony (and many others)
Are you talking about @Template
?
How does @template T
cause that to break?
Phan's co-creator originally specced out @generic
, but changed to @template
after @mindplay-dk mentioned his proposal phan/phan#297 (comment).
@template
tags order significant which is something that PHPDoc mostly avoids by design
I don't see the problem making @template
tags order significant, personally.
Also @generic <TKey as Foo, TValue as Bar>
looks a little weird - it has a space before <
, which is not seen in any other annotation.
It also doesn't provide an easy mechanism for listing template params as covariant. You could adopt Hack's convention (Foo<+TKey as Foo, +TValue as Bar>
) but I prefer the explicit @template-covariant TKey as Foo
.
As an aside, you'll also want to support some form of extension/implements/use (@extends
, @implements
and @use
in Psalm) when extending templates.
muglug
commented
May 22, 2019
BTW is it
@template T of Foo
or@template T as Foo
? is there any difference?
No difference, of
was suggested by @Ocramius as an alternative to as
.
Thank you for your feedbacks. I'm working on a generics PoC right now, I would be happy to make a PR to change this annotation if a consensus is reached.
@template is indeed a problem. The @psalm-template used in doctrine/collections was picked exactly because I disabled annotation parsing for anything containing dashes.
Do you have some guidelines on what to avoid in order to not clash with Doctrine's annotation parser ?
Supporting @template means that phpstan will be incompatible with Symfony (and many others).
Oh, it makes sense now: phpstan itself might stumble upon unrelated @template
annotations. This is indeed an issue.
Using sth like @Generic provides much better compatibility.
[...]
So it MAY be better from PHPDoc design point-of-view to specify all generic parameters in a single tag instead of multiple/** @Generic <K, V> key and value */
- It's also closer to what it does, semantically, as it declares the code object itself as generic. @template was seemingly declaring a type alias as a template. In this sense, I prefer your proposal.
- It's less verbose when there are more than one type parameters, and it's closer to the syntax related to generics in other languages.
- The syntax would support bounds without conflicts as well:
/** @generic <K of string|int, V of Foo> */
or
/** @generic <K: string|int, V: Foo> */
However, we lose in clarity here.
Like @muglug I wouldn't mind making a tag order-significant, alternatively.
Thanks for the background on @template
. Do you know the status of this proposal ?
I don't see the problem making @template tags order significant, personally.
+1
As an aside, you'll also want to support some form of extension/implements/use (@extends, @implements and @use in Psalm) when extending templates.
Indeed 👍 I'm planning to support these in the future.
Ocramius
commented
May 22, 2019
Do you have some guidelines on what to avoid in order to not clash with Doctrine's annotation parser ?
Put a -
in the annotation.
muglug
commented
May 22, 2019
I'm working on a generics PoC now
Let me know if you have questions - happy to show you how I dealt with any given problem in Psalm's implementation.
As I told @ondrejmirtes, the most painful thing was adding support for @extends
months after the initial @template
implementation - I recommend baking that in early, to avoid tying yourself in knots later. Support for @extends
& @implements
allows you to implement generics with much less code.
muglug
commented
May 22, 2019
Oh, that’s a smart approach. Carry on!
mindplay-dk
commented
May 26, 2019
Just for reference, here's the original (rejected, PSR-5) proposal for @template
and @inherits
:
https://github.com/mindplay-dk/fig-standards/blob/features/generics/proposed/phpdoc.md#826-template
It had a section with some explanations about type-hints:
https://github.com/mindplay-dk/fig-standards/blob/features/generics/proposed/phpdoc.md#generics
Curious to see what you guys come up with :-)
Uh oh!
There was an error while loading. Please reload this page.
This adds support for the
@template
tag, with the following syntax:Examples:
Related: phpstan/phpstan#1692
Semantically,
@template
declares that the given name referer to a templated type in the scope of the related code object. The actual type is decided by the user of the code; boundType defines a constraint on this type.