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

@template annotation syntax issues #2527

Closed
arnaud-lb opened this issue Oct 21, 2019 · 31 comments
Closed

@template annotation syntax issues #2527

arnaud-lb opened this issue Oct 21, 2019 · 31 comments

Comments

@arnaud-lb
Copy link
Contributor

I would like to relaunch the discussion around the @template annotation, since multiple issues have been raised around its syntax that may justify a change. Such a change would be relatively easy before the feature is shipped in phpstan-stable.

Here is a summary of the issues that have been raised in the past:

Semantic mismatch (vs C++)

The name template refers to C++-style generics, whose semantics do not match PHPStan or Psalm semantics (who are closer to Java-style generics). It can be misleading and give wrong expectations.

Naming conflict

The name template might conflict with existing projects already using an annotation named like this (for example, Symfony uses an annotation with that name do indicate which HTML template should be used to render a controller). Because of this, PHPStan/Psalm might be confused when analyzing these projects, and these projects might be confused when importing phpstan/psalm-annotated code.

Doctrine parser

The Doctrine annotation parser will attempt to load any annotation whose name looks like a valid class name (template does). Because of this, projects that use this parser may fail when using phpstan/psalm-annotated code. (Although it is possible to tell the parser to ignore some annotations. The ignore list is case-sensitive, so it is possible to ignore phpstan's template while not ignoring Template.)

This issue actually exists for all non-standard phpDoc annotations.

of / as keywords

It has been suggested to replace the of and as keyword used for declaring an upper bound for types, by extends.

Previous discussions

Here are previous discussions I can remember:

cc @mindplay-dk @muglug @Ocramius @ondrejmirtes @JanTvrdik @hrach @teohhanhui

@Ocramius
Copy link
Contributor

Overall, I absolutely endorse having a - in the annotation. Using @type-template would suffice to work around all the weirdness with doctrine/annotations. I explicitly use @psalm-* annotations in all my projects to completely circumvent the problem, and I'm happy that way.

As for the naming "template", I'm not super worried about it, but the community does indeed call them "generics".

@arnaud-lb
Copy link
Contributor Author

arnaud-lb commented Oct 21, 2019

Here is a suggestion that attempts to address the 4 mentioned issues:

"@type-param" name ["extends" type] [description]

Examples:

@type-param KeyT

@type-param T extends FooInterface

@type-param KeyT This key type of this collection

@type-param T extends FooInterface The FooInterface implementation

Example on a class:

/**
 * The Collection class
 *
 * @type-param KeyT   The key type
 * @type-param ValueT The type of the values used in this collection
 */
interface Collection
{
     /**
      * @param array<KeyT, ValueT> $values
      */
     public function __construct($values)
     {

Pros:

  • It matches Java/Kotlin terminology ("Type parameter")
  • It doesn't relate to C++, which has very different semantics
  • It might be a more intuitive name
  • It doesn't appear to be used as an annotation name in the wild
  • The Doctrine parser ignores it (because of the -)

@muglug
Copy link
Contributor

muglug commented Oct 21, 2019

In the PHP world I think @type-param could be easily confused with @param types.

How about @generic-type TKey?

cc @TysonAndre who works on Phan.

@arnaud-lb
Copy link
Contributor Author

Thanks for adding Tyson.

I like @generic-type as well, although I wonder if it's correct to say that a type parameter is a generic type itself (the annotated class/method/function is generic, but is the type parameter ifself generic ?)

A synonym for "parameter type" is "type variable". This gives us an other possible alternative: @type-variable T

@TysonAndre
Copy link

The largest issue is doctrine - Symfony can be taken care of by being case sensitive for non-standard tags (requiring @template instead of @Template)

The same issue with @template would also apply to any custom phpdoc that was implemented (e.g. @immutable (Phan/Psalm went with namespaced versions with different meanings), @override, @inherits, etc.)

I like @template-type (This is also used for function templates, not just on classes), but don't have a strong preference (Having a name with template in it would avoid confusion with other tags and be easier to search)


Another way to solve this would be to add a common prefix that all static analyzers (or IDEs) could use, if a syntax and meaning could be agreed on:

  • e.g. @x-template instead of @template, x-inherits, x-override, x-assert instead of psalm-assert/phan-assert/etc, x-seal-methods/x-forbid-undeclared-magic-methods, etc.
    This assumes nothing is using @x-

@ondrejmirtes
Copy link
Member

What about @generic T? Meaning "this class/method/function below this annotation is generic". I also like @type-variable which is the actual name for this thing in Java.

@Ocramius
Copy link
Contributor

type-variable also matches the FP typed langs: 👍

@TysonAndre
Copy link

What about @generic T? Meaning "this class/method/function below this annotation is generic". I also like @type-variable which is the actual name for this thing in Java.

generic has the same problem as template does - the lack of a hyphen.

The @type-variable also sounds fine - a small initial amount of confusion for users with with PHP variables and @var is possible, but that doesn't prevent going with that.

The Doctrine annotation parser will attempt to load any annotation whose name looks like a valid class name (template does).

@arnaud-lb
Copy link
Contributor Author

Another way to solve this would be to add a common prefix that all static analyzers (or IDEs) could use, if a syntax and meaning could be agreed on:
e.g. @x-template instead of @template, x-inherits, x-override, x-assert instead of psalm-assert/phan-assert/etc, x-seal-methods/x-forbid-undeclared-magic-methods, etc.

Provided that we absolutely need hyphens in order to escape the Doctrine annotation parser, this could be a solution. The prefix could be optional, so that both @x-foo and @foo are allowed.

@arnaud-lb
Copy link
Contributor Author

arnaud-lb commented Oct 21, 2019

I also like @generic, in the form @generic <K, V> (in this form, it declares that the annotated type itself (the type related to the docblock) is generic). One pro is that the < ... > might look familiar. One con is that the ability to describe/document the type variables is reduced.

/**
 * @generic <K, V>
 */
class Collection
{

@ondrejmirtes
Copy link
Member

The one-line @generic has these advantages:

  1. The order of type variables is now explicit. This is a problem of @template - the definition is dependent on the order of phpDoc tags which is unprecedented.
  2. There are < - > - familiarity.

You can still write something like @generic<K is Foo, V is Bar> to get the bounds.

The only disadvantage I can think of is that it condenses a lot of information into a single line in case of more complex definitions. And that there is no - although I don't think it's a big problem - it only surfaced in the past because Symfony interpreted @template.

@iluuu1994
Copy link
Contributor

@generic<...> is only feasible if it can be broken down into multiple lines (IMO):

/**
 * @generic<
 *     T is \Foo\Bar\Baz,
 *     U is \Foo\Bar\Baz,
 * >
 */
class Foo {}

Or we'd could do something like @generic-constraint:

/**
 * @generic<T, U>
 * @generic-constraint T is \Foo\Bar\Baz
 * @generic-constraint U is \Foo\Bar\Baz
 */
class Foo {}

which is something Swift and Rust do for readability:

fn foo<T, U>() -> T
where
    T: Bar + Baz,
    U: Baz,
{}

@arnaud-lb
Copy link
Contributor Author

I would like to add that we also need to support variance modifiers in the syntax. I personally like Kotlin's in/out modifiers, which is inspired from C# (see https://kotlinlang.org/docs/reference/generics.html).

With @generic, the syntax may look like this:

@generic <out T, U>
@generic <in T> 

The out modifier means that the parameterized type will be covariant on T (with the restriction that T can only be used as return value).

The in modifier means that the parameterized type will be contravariant on T (with the restriction that T can only be used as parameter value).

@iluuu1994
Copy link
Contributor

Heads up: Psalm already implements covariant parameters using @template-covariant.

@teohhanhui
Copy link
Contributor

If we end up with a whole family of @template / @template- tags, it'll be really confusing. I really like the idea of having just 1 @generic tag, but it should respect the PHPDoc syntax, i.e. with a space after @generic.

in/out modifiers

Not sure this makes any sense to PHP developers... Is it really necessary for the user to specify this? Does it even make sense for the user to have control over whether to allow covariant / contravariant types? IIUC the return type is always covariant, and the parameter type is always contravariant: https://wiki.php.net/rfc/covariant-returns-and-contravariant-parameters

@muglug
Copy link
Contributor

muglug commented Nov 6, 2019

I mentioned this to Ondrej a couple of weeks ago, but another reason I think a single line @generic is unnecessary is that it doesn't align with how developers write @param annotations.

i.e. they don't write @param (int $a, string $b, Foo<Bar> $collection = null), rather specifying each param individually.

And it's very rare that there are more than two template params for a given class. I've never heard of users being tripped up by the order of them.

Re covariance:

Not sure this makes any sense to PHP developers... Is it really necessary for the user to specify this?

Yes. It avoids a whole class of bugs - see vimeo/psalm#1603

I'm less worried about template contravariance (Psalm doesn't yet support it).

@ondrejmirtes
Copy link
Member

We've decided to try out @phpstan-generic<K ,V> - all other existing syntax will also be supported, but we'll try this out and see if it sticks. No other way to see what'd people prefer...

@teohhanhui
Copy link
Contributor

i.e. they don't write @param (int $a, string $b, Foo $collection = null), rather specifying each param individually.

Oops... I didn't read properly. What I'm suggesting is to rename @template to @generic, and use 1 @generic tag for each type. No @template- / @generic- tags. The complete definition of each generic type is self-contained in a single tag.

Yes. It avoids a whole class of bugs - see vimeo/psalm#1603

Can't the static analyzer tell that it's type narrowing (thus not allowed) when you try to pass Collection<Dog> to a parameter which expects Collection<Animal>? I don't see why it needs to be in the @generic tag?

@muglug
Copy link
Contributor

muglug commented Nov 6, 2019

No other way to see what'd people prefer

You could use a Twitter poll.

@ondrejmirtes
Copy link
Member

The idea with @phpstan-generic<K, V> is to get as close to native syntax as possible. Let's see in practice how it works out :)

@ondrejmirtes
Copy link
Member

You could use a Twitter poll.

  1. Not all PHPStan users are my followers / aren't even on Twitter.
  2. Asking to press a button after 5 seconds of thinking will lead to different results than using something in real source code for months.

@muglug
Copy link
Contributor

muglug commented Nov 6, 2019

Not all PHPStan users are my followers / aren't even on Twitter.

Absolutely, but there's no reason to assume the sample would be skewed one way or the other - as long as the sample is representative there shouldn't be a problem.

Asking to press a button after 5 seconds of thinking will lead to different results than using something in real source code for months.

Asking people to choose between two different ways of expressing the same thing in their codebase (and explaining that via documentation) is much worse, IMO, than using the results of a poll.

Presumably you'll document the use of generics/templates, and your examples of templated behaviour will choose one over the other. That's very likely the one your users will use.

@ondrejmirtes
Copy link
Member

I just believe the correct feedback loop is only established by releasing something for real instead of just asking/trying out something artificially...

@muglug
Copy link
Contributor

muglug commented Nov 6, 2019

I just believe the correct feedback loop is only established by releasing something for real instead of just asking/trying out something artificially

But users will just use whichever one is preferred by the documentation. People don't like to have to think about that sort of stuff.

I'm unaware of a time TypeScript ever launched a feature and told their users "we're giving you the choice of syntax – if you don't use version A we'll deprecate it in the future".

Either you introducing @phpstan-generic<TKey, TValue as Foo> as a supported annotation for all time (in which case I'll add Psalm support for the same) or you're not introducing it at all. I don't see a feasible middle-ground.

@ondrejmirtes
Copy link
Member

Either you introducing @phpstan-generic<TKey, TValue as Foo> as a supported annotation for all time (in which case I'll add Psalm support for the same) or you're not introducing it at all. I don't see a feasible middle-ground.

Yes, thanks :) We will introduce it but it's possible that people won't like it and will stay with @template which is going to be the feedback I'm looking for. Or they will find some major flaw in @phpstan-generic which we will need to address. That's why we start with a prefixed one. It also includes - so that current tools aren't broken.

We'll also probably use is instead of as or of.

@muglug
Copy link
Contributor

muglug commented Nov 6, 2019

We'll also probably use is instead of as or of.

Well now you've gone too far ;)

@muglug
Copy link
Contributor

muglug commented Nov 6, 2019

Will @phpstan-generic<TKey as Collection<Foo|Bar, Baz>, TValue as Collection2<Baz>> also be supported?

@ondrejmirtes
Copy link
Member

We currently only support class names as bounds but we want to expand that to object and possibly other types as well.

@arnaud-lb
Copy link
Contributor Author

@teohhanhui

Can't the static analyzer tell that it's type narrowing (thus not allowed) when you try to pass Collection to a parameter which expects Collection? I don't see why it needs to be in the @Generic tag?

It could, but it would result in unstable APIs

@ondrejmirtes
Copy link
Member

Hi, we'd like to do @phpstan-generic<TKey, TValue is Foo> annotation, but because of time and other constraints, we're sticking with the current @template TValue of Foo syntax for 0.12.0. Thanks for understanding.

@lock
Copy link

lock bot commented Jan 1, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants