Determine which types to support for types tag

[drjayvee]

Following discussions in #4165, we want to determine which types (if any), Twig wants to suggest or even mandate extensions support.

There seem to be four options:

1. Remain completely agnostic
2. Use PHP's types: string, bool, etc
3. Use more generic types such as stringable, iterable
4. Use PHPStan/Psalm types

[fabpot]

It's also related to the work done here: https://github.com/twigphp/Twig/pull/4252/files#diff-6fd6db196be65abd4dcc8132c2bc70a2e0b232c4a5732f29ea26d0a09676ff74R79-R90

[Haehnchen]

the PhpStorm Symfony integration now supports types: Haehnchen/idea-php-symfony2-plugin#2396

Basically most projects needs a class type hint with array. Thats what is now supported here for know:

{% types {
    foobar: '\App\Entity\ErrorReport',
    foobars: '\App\Entity\ErrorReport[]',
} %}

I am also not sure about the type value itself. imho should be as open as possible but guidlines would be helpful.
In the end we are talking about templates and to supports all the "syntax sugar" from the start looks too heavy:

• array<\App\Entity\ErrorReport>
• array<int|\App\Entity\ErrorReport>
• \App\Entity\ErrorReport|\App\Entity\ErrorReportFoo

[fabpot]

Can we start small with all the types from https://github.com/twigphp/Twig/pull/4362/files + some more precise ones like the above example?

[drjayvee]

I'm working on a types validation inspection for TwigQI, so allow me to add my thoughts here.

First off, I've come around to preferring the higher-level Twig types number and iterable over the actual PHP ones. Maybe integer, float and array should be allowed, but deprecated straight away?

However, I really being able to tell the distinction between lists (e.g., [13, 37]) and maps (e.g., {foo: 13, bar: 37}). I'd be in favor of deprecating the ambiguous iterable in favor of list sequence and mapping. Just my two cents.

💡 What if we support:

• iterable<valueType> for lists sequences
• iterable<keyType, valueType> for maps mappings
• iterable for either

One can use any of these for iterable objects, depending on what the actual keys are.

As for more complicated types, I don't think union types are sensible for Twig. What's a template designer supposed to do with sequence<number|iterable|App\Model\User[]>? If multiple types share an interface, then an actual interface should be used, at least in the view (not that I'm a huge fan of separate view models).

Surely, though, we need at least support for classes and iterables of classes.

Therefore, I'd propose:

• \App\Thing
• iterable<\App\Thing> as opposed to \App\Thing[] for consistency with the Twig types
• iterable<string, \App\Thing> (or any other sensible type for the keys)

[stof]

I don't think we should restrict the types in a strict way. If the backend code passes an array containing multiple kind of values, the analysis with twigstan will be much more useful if we describe a union type than if we fallback to mixed due to the restriction.

iterable is not a replacement for list and map. It is a super type (when you only care about looping). A list<T> is an iterable<int, T> (not that Twig currently uses the naming "sequence" rather than "list", but those are the same thing).

[drjayvee]

I don't think we should restrict the types in a strict way.

Can you elaborate? Do you mean Twig itself should remain completely agnostic and the community should come to an understanding? Or just that Twig should never throw or trigger_error for "invalid" types? (I agree with the latter, not so much with the former.)

the analysis with twigstan

Which one are you referring to? 😊 @ruudk's project, or mine (which was formerly called TwigStan as well)?

I'm trying to keep TwigQI simple and in line with @Haehnchen's PHPStorm plugin. That probably means not supporting the full monty PHPStan/PSalm syntax.

iterable is not a replacement for list and map.

Well, the documentation now distinguishes between "iterable (mapping)" and "iterable (sequence)". The question is how do we make that distinction in {% types %}? I'm proposing syntax that supports all cases including the generic iterable.

Your thoughts?

not[e] that Twig currently uses the naming "sequence" rather than "list"

Thanks for pointing that out. I've updated my previous comment to avoid confusion.

[drjayvee]

Adding support for nullable types is very important (to me, at least).

There are two ways:

1. Union type support: 'string|null'
2. Nullable short-hand: '?string'

I favor the 2nd option because the first leads naturally to full union support (e.g., 'string|\App\Model|number|null'), or some arbitrary limits such. I'm concerned about the complexity for full union.

There is a third option as well, of course: agnosticism.

[stof]

@drjayvee Twig itself has already made the choice of being agnostic of the types being valid or invalid (Twig itself does not validate the content of the string at all), which is what would allow twigstan to leverage the full syntax of phpstan for instance

I'm trying to keep TwigQI simple and in line with @Haehnchen's PHPStorm plugin. That probably means not supporting the full monty PHPStan/PSalm syntax.

Given that PHPStorm supports union types in its type system, are you sure the PHPStorm plugin does not support them in its current @var comment ? Isn't it using the type system of PHPStorm ?

Which one are you referring to?

when talking about twigstan, I'm talking about the project that still has this name today.

Well, the documentation now distinguishes between "iterable (mapping)" and "iterable (sequence)". The question is how do we make that distinction in {% types %}? I'm proposing syntax that supports all cases including the generic iterable.

To me, places expecting only a sequence should use sequence<valueType>, not iterable<valueType>, to make it clear that it expects a sequence (maybe using list<valueType> like in PHP would be easier for the ecosystem than forcing sequence though, to be checked with the twigstan project as well).
However, that does not mean iterable should be deprecated in any way. Many features in Twig expect an iterable, without caring whether it is a list or a mapping. As said before iterable is a super-type of both sequence and mapping in Twig (and some iterables are neither a sequence nor a mapping, as Traversable PHP objects are also valid iterables)

Btw, using iterable<valueType> as the way to describe the type expecting a sequence would be very confusing because both phpstan and Psalm support this single-type generic type for iterable, making mean iterable<anyKeyType, valueType (I don't know what the use exactly to represent this anyKeyType internally, but that's the intent)

And note that iterable<keyType, valueType> will not necessary mean a mapping either. If your keyType is int, a sequence would satisfy this type, because it is indeed an iterable using integers as keys.

[drjayvee]

Unfortunately, my proposal re: interable<> is in conflict with PHPStan (/ PHPDoc / PSR-5 / Psalm), which supports

• General arrays, which include array<Type> and array<int, Type>
• Iterables, which include iterable<Type> and Collection<int, Type>, but not iterable<int, Type>

It looks to me like we need to decide whether to "break" compatibility with PHP.

Consider @fabpot's vision:

Twig tries to abstract PHP types as much as possible and works with a few basic types [...]

I fully agree. Twig is for front end developers, who might not be PHP devs at all.

While I see the value of using PHPStan's enormous capabilities, I think types at least should be kept simple. (Validating logic is another matter, and here, PHPStan might still have a role to play!)

One last thing is whether to support array mapping shapes. We don't have to, to be clear. Developers who do want typing and static analysis can wrap arrays in view models. (I'm warming up to that myself lately.)

I'd love to hear your opinion and plans, @Haehnchen.

[drjayvee]

@stof thanks for your detailed response. I'll reply tomorrow!

[stof]

@drjayvee iterable<int, Type> is supported by phpstan since years (maybe the doc forgot to mention them)

[drjayvee]

Twig itself has already made the choice of being agnostic of the types being valid or invalid (Twig itself does not validate the content of the string at all), which is what would allow twigstan to leverage the full syntax of phpstan for instance

I fully agree on this principle.

However, I'd personally like if the Twig documentation included a basic set of types to establish a community convention.

I can only speak for our (AlisQI's) team, but our Twig templates are mostly written and maintained by developers who are fluent in TypeScript, not PHP. Therefore, I actually don't want to use PHPstan's complicated type system, nor will I try to implement checks in TwigQI. My vision is for templates to use simple types. Instead of array shapes, union/intersection types, etc, use a class (e.g., readonly class SomeViewModel). Or specify a more generic type like iterable or mixed and forgo type documentation and checking, while still being able to document that a variable exists.

By the way, I'm not arguing Twig's types, whatever they may be, should therefore be closer to TypeScript. But I do like Twig having its own (simple) types as opposed to relying on PHP's. Fabian agrees (see quote further up).

Now, I do understand the appeal of TwigStan, don't get me wrong!

However, it looks to me like we're at a crossroads, and have to decide between:

1. Agnosticism
2. Twig types: number, iterable (sequence or mapping), etc
   I would really like to add the distinction between sequence/list and mapping (whether that's an array shape/tuple or iterable with unknown keys)
3. PHP(Stan) types: int(eger), float, array, iterable, Collection, etc

The first option is obviously the most flexible, but it looks like it may split the community's extensions and IDE integration(s). That would be a a bit of a bummer.

Are the 2nd and 3rd really in conflict? Let's explore, in the hope of finding a subset that makes us all happy.

Let me think out loud here. TwigStan already needs to map Twig syntax and semantics (e.g., macro arguments are always optional) to PHP code that PHPStan can inspect. Transpiling number to int|float wouldn't be that big of a deal.

But what about iterable? PHPStan doesn't support "bare" iterable (without value type), but it does support both iterable<valueType> and iterable<keyType, valueType>. (Thanks for pointing that out stof. The docs indeed omit the latter.) TwigStan could transpile bare iterator to iterator<mixed>, right?

So if Twig "officially supported" number, iterable, iterable<Type> and iterable<Type, Type>, it wouldn't be hard for TwigStan to make that work, while keeping the full range of PHPStan types on the table.

To me, places expecting only a sequence should use sequence, not iterable,

Sure, that would be still be possible. Fortunately, iterable<Type> accepts both sequences (e.g., [13, 37]) and mappings (e.g., ['foo' => 1337]).

So it looks like this should fly, right? @ruudk, I'd love to hear your thoughts on all this.

Given that PHPStorm supports union types in its type system, are you sure the PHPStorm plugin does not support them in its current @var comment ? Isn't it using the type system of PHPStorm?

The plugin simply uses the first type. In other words: it does not support unions.

[stof]

2. I would really like to add the distinction between sequence/list and mapping (whether that's an array shape/tuple or iterable with unknown keys)

Twig has 3 types for those: iterable, sequences and mappings. That's not about having either iterable on one hand or sequences and mappings on the other hand.

I can only speak for our (AlisQI's) team, but our Twig templates are mostly written and maintained by developers who are fluent in TypeScript, not PHP. Therefore, I actually don't want to use PHPstan's complicated type system, nor will I try to implement checks in TwigQI. My vision is for templates to use simple types.
By the way, I'm not arguing Twig's types, whatever they may be, should therefore be closer to TypeScript. But I do like Twig having its own (simple) types as opposed to relying on PHP's. Fabian agrees (see quote further up).

The funny thing about that statement is that Typescript actually supports types that are way more complex that what phpstan supports.

Note that Fabien never said he wants to forbid using more complex types in the {% types %} tag. He was the one asking that Twig itself performs no validation at all on the content of the type string, to allow supporting any type we want in specific tools.
There is a difference between the interoperable types (for which we expect all tools to support them) and what a tool can support.
However, as TwigQI is about running checks during the rendering of templates, it should be careful about its handling of types it does not support (think about what would happen for a {% types %} tag in a third-party template).

PHPStan doesn't support "bare" iterable (without value type)

it actually does (that's what it uses when using a PHP native iterable type), treating it as iterable<mixed>. But it also reports an error in level 6+ (similar to what typescript does with noImplicitAny).

To me, places expecting only a sequence should use sequence, not iterable,

Sure, that would be still be possible. Fortunately, iterable<Type> accepts both sequences (e.g., [13, 37]) and mappings (e.g., ['foo' => 1337]).

And that's my point. We have a bunch of places requiring a sequence, not any type of iterable. Documenting those as iterable<Type> would mean that static analysis does not discover

[drjayvee]

2. I would really like to add the distinction between sequence/list and mapping (whether that's an array shape/tuple or iterable with unknown keys)

Twig has 3 types for those: iterable, sequences and mappings. That's not about having either iterable on one hand or sequences and mappings on the other hand.

I meant to distinguish between sequence (a subset of iterable where we don't really care about keys) on the one hand and mapping/iterable (where we do) on the other hand.

Obviously, we could use PHPStan's types and semantics.

I'm merely trying to come up with a simpler subset that matches the Twig documentation, with maximum PHPStan compatibility.

The funny thing about that statement is that Typescript actually supports types that are way more complex that what phpstan supports.

I'm literally saying Twig's types don't have to be like TypeScript. (I'm not a huge fan myself.) I'm trying to explain why it makes sense for Twig not to use PHP's types, and especially not PHPStan's.

Note that Fabien never said he wants to forbid using more complex types in the {% types %} tag.

I'm not claiming he did, plus I'm explicitly saying that I'm not in favor of that either.

This is not a productive conversation, so I'm leaving it at that.

[willrowe]

I have been documenting variable types at the top of Twig files for years using a simple comment. In that time I have refined the types I use into the following most used:

• PHP classes
• int
• float
• string
• bool
• dictionary
• list

These follow the built-in PHP types mostly and then Python types (based on Jinja2) to distinguish between iterables.

1. I think that it makes sense to have a single number type that can accept either int or float. Since Twig is a presentational layer, the thing that matters is the number format used for display and not necessarily the specific number type that the input is provided as.
2. In general I think it is good to have more PHP agnostic naming for the types that makes it clear that we are in a template and not PHP code. The names should be clear and unambiguous. Using boolean instead of bool, for example, is a good call.
3. I use dictionary and list so often, I think there should be a separate type for each, as sub-types of iterable. I have always struggled with how to type variables that could be either, so iterable is very useful as well. So that would be the super-type iterable and then the two sub-types sequence and mapping.
4. Having union types just to be able to denote that null can be passed, would be worth it alone. I use null as a flag a lot to denote whether a section of a template should be omitted.
5. Though not entirely necessary, I almost always include array shapes in my mapping types so I know what keys and values are expected inside a passed array. Since mappings are so common and can hold many different types inside of them, this would be well worth looking into.

[drjayvee]

Thanks for your feedback, @willrowe

I agree with most of your points!

I use dictionary and list so often, I think there should be a separate type for each, as sub-types of iterable.
[...]
Though not entirely necessary, I almost always include array shapes in my mapping types so I know what keys and values are expected inside a passed array. Since mappings are so common and can hold many different types inside of them, this would be well worth looking into

I also thought the difference was really important, but I came around somewhat.

I now strongly favor a simple and PHP-agnostic type system, even if that means not every type can be documented in detail.

I now think wrapping dictionaries/mappings in readonly class is better dev UX, especially to abstract away from how different languages handle this. It also reduces typing (and, therefore, typos) if the type is used multiple times.

However, I have a feeling the community won't come to a consensus on this topic.

Having union types just to be able to denote that null can be passed, would be worth it alone.

TwigQI supports nullable types using a ? prefix. This supports this use case, which I agree is very common, without having to get into limited union support.

Would you agree this is sufficient?

[willrowe]

TwigQI supports nullable types using a ? prefix. This supports this use case, which I agree is very common, without having to get into limited union support.

• I would prefer using a union type with null since the ? suffix on the variable name is already used to denote an optional variable and something like string|null is much more explicit.