Skip to content
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

Support @template tag #27

Merged
merged 1 commit into from May 21, 2019
Merged

Conversation

arnaud-lb
Copy link
Contributor

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

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.

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

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

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

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

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

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

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 22, 2019 via email

@muglug
Copy link

muglug commented May 22, 2019

@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

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

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

@ondrejmirtes
Copy link
Member

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

@Ocramius
Copy link

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

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

@muglug
Copy link

muglug commented May 22, 2019

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

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

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

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 22, 2019 via email

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

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 22, 2019 via email

@muglug
Copy link

muglug commented May 22, 2019

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

@mindplay-dk
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 :-)

@ondrejmirtes
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants