Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
ondrejmirtes merged 1 commit into phpstan:master from arnaud-lb:template-tag
May 21, 2019
Merged

Conversation

Copy link
Contributor

@arnaud-lb arnaud-lb commented May 21, 2019
edited
Loading

This adds support for the @template tag, with the following syntax:

@template name [of boundType] [description]

Examples:

@template T
@template T of DateTime the value type
@template T of DateTime
@template T the value type

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.

Copy link
Member

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.

@ondrejmirtes ondrejmirtes merged commit 847540a into phpstan:master May 21, 2019
Copy link
Member

@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?

Copy link
Collaborator

@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.

Copy link
Collaborator

Also, as I've said on multiple occasions, @template is extremely bad name as

  1. it is incompatible with many open-source (and close-source) libraries, such as sensio/framework-extra-bundle with over 40M installs
  2. the word "template" is used by C++, while PHP is much closer in semantic to Java/C#/Kotlin/TypeScript which all call this "generics".
hrach, zmitic, and VasekPurchart reacted with thumbs up emoji

Copy link
Member

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...

Copy link
Contributor Author

@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.

Copy link

@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?

Copy link
Member

ondrejmirtes commented May 22, 2019 via email

Right now the parser accepts it but I’m not sure if it won’t raise more questions when implementing it in phpstan/phpstan...
On 2019年5月22日 at 17:13, Marco Pivetta ***@***.***> wrote: @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? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27?email_source=notifications&email_token=AAAZTOA5ENTDRTISJJN3SS3PWVPI5A5CNFSM4HOK7NVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV7METQ#issuecomment-494846542>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZTOE6GPXURNTWIUN7HCTPWVPI5ANCNFSM4HOK7NVA> .
-- Ondřej Mirtes

Copy link

muglug commented May 22, 2019
edited
Loading

@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 {}

see https://psalm.dev/r/0ef5e86bce

Copy link

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?

Copy link
Member

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...

Copy link
Member

@muglug BTW is it @template T of Foo or @template T as Foo? is there any difference?

Copy link

Yes, my example was specifically around non-atomic types (valid scenario, IMO)

Copy link
Collaborator

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).

Copy link

muglug commented May 22, 2019
edited
Loading

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.

Copy link

muglug commented May 22, 2019

@ondrejmirtes

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.

Copy link
Contributor Author

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.

@Ocramius

@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 ?

@JanTvrdik

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.

@muglug

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.

Copy link

Do you have some guidelines on what to avoid in order to not clash with Doctrine's annotation parser ?

Put a - in the annotation.

Copy link
Member

ondrejmirtes commented May 22, 2019 via email

Is the one in Symfony @template? What about making our parser case-sensitive?
On 2019年5月22日 at 21:37, Marco Pivetta ***@***.***> wrote: Do you have some guidelines on what to avoid in order to not clash with Doctrine's annotation parser ? Put a - in the annotation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27?email_source=notifications&email_token=AAAZTOBNU45CYW6BUVZ3WWTPWWOIDA5CNFSM4HOK7NVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWACZCY#issuecomment-494939275>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZTOC2XN3AO7HELYLJFWLPWWOIDANCNFSM4HOK7NVA> .
-- Ondřej Mirtes

Copy link

muglug commented May 22, 2019

@arnaud-lb

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.

Copy link
Member

ondrejmirtes commented May 22, 2019 via email

We’re starting with implemeting method and function level generics for now, to dip are toes in it. Once we have that working, we’ll deal with all the complexities of class level generics. Thanks!
On 2019年5月22日 at 21:57, Matthew Brown ***@***.***> wrote: @arnaud-lb <https://github.com/arnaud-lb> 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 <https://github.com/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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27?email_source=notifications&email_token=AAAZTOH6KJTFQGEVLSTHAFLPWWQQZA5CNFSM4HOK7NVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWAEWXQ#issuecomment-494947166>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAZTOEGZBUEGNNYCTTMYZTPWWQQZANCNFSM4HOK7NVA> .
-- Ondřej Mirtes

Copy link

muglug commented May 22, 2019

Oh, that’s a smart approach. Carry on!

Copy link

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 :-)

Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /