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

Add tag to declare variable types #4165

Closed
drjayvee opened this issue Jul 29, 2024 · 90 comments
Closed

Add tag to declare variable types #4165

drjayvee opened this issue Jul 29, 2024 · 90 comments

Comments

@drjayvee
Copy link
Contributor

drjayvee commented Jul 29, 2024

See #4003 and #4009 for some context. In short, since the PHP language itself as well as its community are moving to (optional) stricter typing, it makes sense for Twig to support this as well by adding a types tag.

@fabpot said he'd like to bootstrap a spec. This is the first draft. Everything is up for debate, obviously. In addition, I've put in italics parts of the spec that I have doubts about myself.

Given the discussions below (thanks all for contributing), I think the spec is now crystallizing. Consider it a beta.

Syntax

The types tag contains a mapping of variable name to constant type string (no expression or interpolation):

{% types {variable: 'Type'} %}

The reason for quoting the type is that several characters (e.g., \) are currently not supported by the Lexer. It also allows analyzer implementations to freely specify the syntax.

The variable name must not be quoted. This is to ensure that only valid names can be used.

Types

The syntax and content of the types are not part of this specification. This provides complete freedom to extensions that inspect and analyze the tags. It also keeps the implementation in Twig very simple and robust.

(In practice, it's likely that implementations will rely on widely-used specifications like PHPDoc / PHPStan types.)

Examples

Here are two examples:

{% types {foo: 'bool'} %}

{% types {
  bar: "list<int>",
  baz: '?\Name\Space\ClassName'
} %}

Behavior

This specification does require any behavior other than validating the syntax for the tag itself. The tag does not lead to generated PHP code.

Usage

This section describes possible implementations to demonstrate the utility of this addition.

It is up to extensions developed by the community to implement any of this.

Development time

The types can be useful during development by enabling IDEs to auto-complete code, issue warnings, and more.

This is already implemented by the Symfony Support plugin for PHPStorm when using comments to declare variable types (e.g., {# @var id int #}).

These tags also provide precious documentation about the variables expected in a template's context.

Compile time

During compilation, a Twig extension can:

  1. Check the type and throw an appropriate Exception if it is not valid.
  2. For objects, check whether properties and more methods exist, and throw an appropriate Exception if there is none.
  3. Add run-time checks (see below).

Run time

During run time, behavior may depend on strict_variables environment option:

  • If disabled (likely in production environments), a PHP warning is triggered.
  • If enabled, (development and testing environments), an Exception is thrown.

Possible extensions

@JoshuaBehrens
Copy link

Would you mind linking

Use property access or method call for objects instead of twig_get_attribute() to improve performance

with #3928 so we can see, when this is unblocked by this issue? :)

@ruudk
Copy link
Contributor

ruudk commented Jul 29, 2024

If I may suggest: instead of deciding the possible var type values, why not accept any valid PHPDoc type. Together with PHPStan's PHPDoc Parser we can parse them into PhpDocNode's that then can be transformed to PHPStan types. From there, it can be used to build compile time assertions. Doing it from scratch feels like reinventing the wheel 😅

We could still only accept a subset from the possible types, and let others improve it over time via PR's.

@ericmorand
Copy link
Contributor

Give it enough time and Twig will end up being PHTML, the format it was supposed to replace.

@drjayvee
Copy link
Contributor Author

drjayvee commented Jul 30, 2024

If I may suggest: instead of deciding the possible var type values, why not accept any valid PHPDoc type

Thanks, that's a good suggestion. I was mindful of not reinventing the wheel and hoped that we could re-use parts of PHPStan to parse and evaluate the types. Should we perhaps go all the way and support any PHPStan type? I'll add a sentence in italics to that section for now.

I'll add support for nullable types, which I forgot. But the more gets added, the more sense it makes to just go all the way

@willrowe
Copy link
Contributor

willrowe commented Aug 2, 2024

I'm not sure if this would be the best place to implement this, but a lot of times I have a flag in a template to allow certain markup to be turned on or off, but need to retain previous functionality for other templates that are already including it. I usually have to add a set tag to check if the variable exists and set it to a default value if not. I already include a comment at the top of the template denoting what variables are used in the template, but it would be nice to be able to provide a default value for any optional variables in the same place that I denote the type, replacing the need for a bunch of set tags.

@ruudk
Copy link
Contributor

ruudk commented Aug 15, 2024

{% var variable Type %}

I think that the order should be the same as with inline PHPDocs:

{% var Type variable %}

@willrowe
Copy link
Contributor

willrowe commented Aug 15, 2024

I agree with @ruudk on the order. It would also match how class properties and argument types are defined in PHP.

Also, going along with that and clarifying what I was proposing in #4165 (comment), this is what it would look like to specify a default value:

{% var Type variable = 'default_value' %}
{% var bool flag = false %}

@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 16, 2024

I think that the order should be the same as with inline PHPDocs:

Agreed. I've updated the OP. (While I prefer the original order, we should follow existing conventions.)

We could also consider an optional description. phpDocumentor supports this, but PHPStan doesn't seem to.

{% var bool foo to foo or not to foo %}

Then again, this makes the tag less readable, plus there's nothing Twig itself would do with this. The alternative is to just add a comment (before or afterwards) for humans:

{% var bool foo %} {# to foo or not to foo #}

@ruudk
Copy link
Contributor

ruudk commented Aug 16, 2024

We could also consider an optional description. phpDocumentor supports this, but PHPStan doesn't seem to.

What's the point of providing a description there?
The reason why PHPStan doesn't support it at that position is that the @var is already inside a comment. So you can add a comment above the @var to explain the why.

Same can be done with Twig:

{# I need to explain this somehow #}
{% var 'bool' foo %}

PS: The Type should be a string (ConstantStringExpression).

The variable could be a NameExpression or ConstantStringExpression.

@drjayvee
Copy link
Contributor Author

it would be nice to be able to provide a default value for any optional variables in the same place that I denote the type, replacing the need for a bunch of set tags.

I personally actually disagree on principle: declaring the type and any value should not be mixed. This does bring up two interesting questions, though.

First, should we add undefined as a possible type? Or should template developers ensure variables are always set (possibly using null)? Since var is all about stricter typing, I would prefer the latter.

Second, what do we do about variables defined using set? Should users add var afterwards? Or should Twig try to auto-detect the type? That would be very difficult (or outright infeasible) since set can use an expression (using literals and even function calls).

@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 16, 2024

What's the point of providing a description there? The reason why PHPStan doesn't support it at that position is that the @var is already inside a comment. So you can add a comment above the @var to explain the why.

Exactly. I was just thinking out loud.

PS: The Type should be a string (ConstantStringExpression).

The variable could be a NameExpression or ConstantStringExpression.

Why should the type be quoted? It's not an expression (although I confess not to know the definition of "expression"). It also looks really weird to me and is inconsistent with PHPStan.

Do you think the implementation could be tricky if the type isn't quoted?

Update: the implementation does indeed run into issues. Therefore, I've amended the spec to quote types.

@willrowe
Copy link
Contributor

I personally actually disagree on principle: declaring the type and any value should not be mixed.

@drjayvee can you explain more?

@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 16, 2024

I personally actually disagree on principle: declaring the type and any value should not be mixed.

@drjayvee can you explain more?

@willrowe, absolutely! I should have done that straight away.

But first off, I want to point out that this is just my personal opinion. I'd love to hear what other people think.

One reason I dislike your proposal is that Twig already has three ways of setting values:

  1. injecting them into the template's context (in PHP)
  2. using the set tag
  3. defining a macro argument

I feel like adding a fourth way leads to bloat. (Or worse, conflicts.)

Macros arguments already have support for default values. (In fact, they're never even undefined, since they get an implicit default of null. I'm actually proposing to make this distinction clear in the AST: #4010)

The set tag already supports your use case just fine:

{% set foo = foo ?? 'bar' %}

{# or, in case null must be kept as-is, more strictly: #}
{% set foo = foo is defined ? foo : 'bar' %}

The second problem is that it feels like mixing concerns. I'm envisioning the var tag to be analogous to PHPStan's @var annotation, not PHP's native type declarations (e.g., for class properties and function arguments). Another way of looking at var is that it only affects template code (and therefore provide value) after the tag. You can read it like "from here on out, foo is guaranteed to be of this type".

However, I'm not super convinced by my own argument here, since both PHP and Typescript (as just two examples) do let you declare types and default values in one go in many places (e.g., function default values and class properties).

A third problem is that var is currently envisioned to have no effect on compiled templates. I think this is a sensible design goal is prevent compatibility issues and make the change more palatable to maintainers and users. If var does support default values, it will have to affect compilation output.

I hope I explained clearly. I'm sincerely curious as to you what you think now you know my reasoning.

@ericmorand
Copy link
Contributor

@drjayvee

{% var ?\Name\Space\ClassName baz %}

What does it mean? What is \Name\Space\ClassName and how is such a type supposed to be declared?

@willrowe
Copy link
Contributor

@drjayvee those are all great points and illustrate how you see this feature being utilized.

Where we differ is that I think about this as more of a dockblock/argument list at the top of the template file (similar to the extends tag) defining what variables should be available in the context of the template file. As if the file is a function and the var tags are its arguments. That's why it makes sense to me to have support for default value, since arguments can have default values.

Keeping the above in mind:

One reason I dislike your proposal is that Twig already has three ways of setting values:

  1. injecting them into the template's context (in PHP)

I see this as augmenting the template context specifically, so if a var is missing from the template context it will be initialized with the default value, if one has been specified.

  1. using the set tag

Since I am thinking of this as a single definition at the top, it would be different than set. The set tag can be used multiple times throughout the template to change a variable value at runtime.

  1. defining a macro argument

I think macros are separate from what I am talking about.

I feel like adding a fourth way leads to bloat.

I don't see it as an additional way to set variable values within the template if it were to only affect the context as it comes into the template. It would be overridden by any values provided by the user.

Macros arguments already have support for default values. (In fact, they're never even undefined, since they get an implicit default of null.

I'm not talking about macros, just template file context.

The set tag already supports your use case just fine

As I said in my original comment, I already do this, but am looking for a way to easily combine the documentation of variables within a template file and a default value.

The second problem is that it feels like mixing concerns. I'm envisioning the var tag to be analogous to PHPStan's @var annotation, not PHP's native type declarations (e.g., for class properties and function arguments). Another way of looking at var is that it only affects template code (and therefore provide value) after the tag. You can read it like "from here on out, foo is guaranteed to be of this type".

This is where we are thinking about the tag differently. It seems like adding the ability specify a type to the existing set tag would accomplish what you are describing here.

A third problem is that var is currently envisioned to have no effect on compiled templates. I think this is a sensible design goal is prevent compatibility issues and make the change more palatable to maintainers and users. If var does support default values, it will have to affect compilation output.

I think it would be much more powerful to have the variable type information available after compiling, since that would allow introspection of what variables a template file expects to be passed as context, among other things. Compiling the types could also possibly allow Twig to leverage built-in PHP type checks so that checks don't necessarily need to be run manually at runtime, but would rather be implicitly checked.

All this being said, I think it comes down to whether it would be used as a way to define variable types once for the whole file, or inline.

@drjayvee
Copy link
Contributor Author

@drjayvee

{% var ?\Name\Space\ClassName baz %}

What does it mean? What is \Name\Space\ClassName and how is such a type supposed to be declared?

This tag declares that variable baz must be an object of class with FQN \Name\Space\ClassName or null (due to the ? prefix).

The PHP equivalent for a function argument would be

function bar(?\Name\Space\ClassName $baz) {}

@drjayvee
Copy link
Contributor Author

@willrowe thanks for your detailed response. It does indeed appear we have slightly different use cases in mind.

Documenting / specifying the template's context variables similar to function arguments is an explicit goal of this proposal.
I still maintain that setting default values would be supported just fine with a combination of set and var (to declare type).

The current proposal is more generic: you can use var in macros and anywhere inside a template.

Consider the following example:

{% if user.isLoggedIn() %}
  {% set userName = user.fullName %} {# Class property is ?string but guaranteed to be string if user is logged in #}
{% else %}
  {% set userName = 'Anonymous' %}
{% endif %}

{% var string userName %}

The var tag on the last line serves as documentation and safety net. It guarantees that, userName is always string, even though user.fullName itself is nullable.

That being said, I very much would like the community to weigh in here.

Speaking of: @fabpot, you mentioned you wanted to involve some interested parties, no? I'd also like to know your thoughts on the proposal so far.

@ericmorand
Copy link
Contributor

@drjayvee thanks. How do you declare to the twig compiler that this class exist? Said differently, at compile-time, how is the compiler expected to check that this type exist?

@drjayvee
Copy link
Contributor Author

@drjayvee thanks. How do you declare to the twig compiler that this class exist? Said differently, at compile-time, how is the compiler expected to check that this type exist?

Simply by using class_exists(), right? I guess the only potential issue is that autoloading must be enabled. But who doesn't have that.

Do you see any other problem? Or am I overlooking something here?

@ericmorand
Copy link
Contributor

@drjayvee you are explaining how the php implementation would work. But how would the other implementations work? More generally, from a specification point of view, how would \Foo\Bar be resolved?

@ericmorand
Copy link
Contributor

ericmorand commented Aug 19, 2024

@drjayvee also it is fundamental to establish how type checking would work? Let's say the a variable x of type \Foo\Bar is used this way:

{{ x.y.z }}

What would be the algorithm that compilers are supposed to implement to check that x.y.z is a valid member access?

As I said earlier in this thread in an ironic manner, I'm concerned that twig would become a proper typed programming language, which means that it would also come with a complexity in both specification and implementation that the TwigPHP team may not be ready to face. I recommend that you have a look at how typed languages are specified and how their compilers are implemented before making a decision about this feature: this is very very complex topic.

@willrowe
Copy link
Contributor

I still maintain that setting default values would be supported just fine with a combination of set and var (to declare type).

  • As I said, I already use set and that is absolutely a way that the same thing can be accomplished. Having both the type and default value in the same place would be easier to maintain and less error prone.

Documenting / specifying the template's context variables similar to function arguments is an explicit goal of this proposal.
The current proposal is more generic: you can use var in macros and anywhere inside a template.
The var tag on the last line serves as documentation and safety net. It guarantees that, userName is always string, even though user.fullName itself is nullable.

I think there are two different things we are talking about here in regards to a template's context variables:

  1. I was talking about the context variables that are passed in when rendering or including a template, so they are like "arguments".
  2. You are talking about the more general context variables which encompass both those that are initially passed (that I was talking about) and the context variables as they may change during runtime via set, etc.

I think it is important to distinguish between the two. Using your example to expand on what I mentioned in my last comment, I think it would be more clear to augment the existing set tag since you are creating a new context variable:

{% set string userName = (
    user.isLoggedIn()
        ?
    user.fullName
        :
    'Anonymous'
) %}

If that variable could be passed to the template instead, this could be done:

{% var string userName = 'Anonymous' %}

@drjayvee
Copy link
Contributor Author

@drjayvee you are explaining how the php implementation would work. But how would the other implementations work? More generally, from a specification point of view, how would \Foo\Bar be resolved?

At compile time, Twig checks whether the type is valid. For classes, that would simply be done using class_exists(). For scalars, that's even simpler. Then again, we might let PHPStan do these checks.

For runtime (i.e., in the compiled template's PHP code), a check is added to assert the type.

For example:

{% var int number %}
{% var \User user %}
{{ user.name }} has {{ number }}

Would compile to something like the following, if (and only if) strict_variables is enabled:

if (!is_int($context['int'])) {
  trigger_error('Invalid type for variable "int"'); // or throw an Exception
}
if (! $context['user'] instanceof \User) {
  trigger_error('Invalid type for variable "user"'); // idem dito
}

Do you see any problem here so far? Or significant limitations which render this whole feature moot?

also it is fundamental to establish how type checking would work? Let's say the a variable x of type \Foo\Bar is used this way:
{{ x.y.z }}
What would be the algorithm that compilers are supposed to implement to check that x.y.z is a valid member access?

That's an interesting question. I'd hope to leverage PHPStan to do those chained checks. At the very least, though, we could check whether y is a valid property method of class x (if x's type is declared to be a class at all). That should be simple enough, and will already provide significant value.

However, even if properties/methods aren't checked at all, there's still value.

Consider this example:

{% var \Foo\Bar foo %}
{% set baz = foo.bar.getBaz() %}
{% var string baz %}

At compile time, we can ensure that the type is valid. The var tag also serves as (standardized) type documentation, as discussed in the OP.

At runtime, we can trigger warnings/errors or throw Exceptions if foo is not an instance of \Foo\Bar, and if baz is not a string. That alone will add a significant safety net to developers.

The goal of var is not to match PHPStan's level 9. Just adding what I'm proposing will add significant value. var is also entirely optional, so it doesn't have to support complicated use cases - certainly not in the first iteration.

@drjayvee
Copy link
Contributor Author

@willrowe

I think there are two different things we are talking about here in regards to a template's context variables:

Oh, do I understand correctly that you'd want the default value in var to apply exclusively to injected variables, not ones created using set and macro arguments?

In that case, I think this would get too complicated, both in the implementation, as well as for users to understand.

I think it would be more clear to augment the existing set tag since you are creating a new context variable

I guess I wouldn't be opposed to adding optional types to set.

However, set can define (the documentation reads "assigned") multiple values, and be used "to 'capture' chunks of text". This might make it more complicated to implement and support.

{% set string foo, int bar = 'foo', 1337 %} {# should work fine #}

{% set string baz %} {# pointless, as captured chunks will always be guaranteed to be string (right?) #}
    Baz
{% endset %}

Moreover, set declarations have limited scope in loops (see documentation), so the implementation would need to take that into account.

I guess that could all work. But you'd need typing support in set and default values in var, right?

It's an interesting idea for sure.

My gut still says it's better separate those concerns. But that's just one opinion.

@ericmorand
Copy link
Contributor

@drjayvee, what about other compiler implementations like twig.js or Twing? What would be the logic that they need to implement?

@willrowe
Copy link
Contributor

@ericmorand what do you currently do for constant?

@ericmorand
Copy link
Contributor

@willrowe since there was no specification, we made an arbitrary call:

https://twing.nightlycommit.com/specifics.html#constant

But constant is an easy case to handle since the value is picked from the data passed to the context.

Types are another story and need a proper specification to be supported by non-PHP implementation.

@willrowe
Copy link
Contributor

@ericmorand do you have a specific question about how it should function when a template is rendered?

You keep asking about the specification and how it should be implemented outside of PHP, but it doesn't seem like that is something to be decided here in this issue or even in this project. You may need to decide for yourself how you are going to implement it in your own project so it matches TwigPHP in a way that is consistent with how you have implemented all the other features.

@drjayvee
Copy link
Contributor Author

@ericmorand

@drjayvee, what about other compiler implementations like twig.js or Twing? What would be the logic that they need to implement?

I honestly wasn't aware of these! It's interesting that there are already two.

Not to be disrespectful, but I wouldn't want to limit the capability or complicate the spec to cater to implementations other than TwigPHP, especially ones in other languages. (Given the difficulties described here, this might already be the stance of the Twig maintainers.)

To answer your question directly: the easiest path is to simply ignore var. Alternatively, Twing (and any other implementation) could chose their own specification and implementation of the var tag.

Twing's documentation already reads:

Twig specification being intimately bound to the TwigPHP implementation, some parts of it don’t make sense outside of the context of a PHP runtime. Below is the list of those parts that Twing interprets differently from its TwigPHP counterpart.

var would be very similar to constant: either chose a different specification and implementation, or just don't support it.

@stof
Copy link
Member

stof commented Aug 26, 2024

only enable those if strict_types is enabled. That's probably only used in dev environments, where the overhead is less important than code quality.

I can tell you that this assumption is wrong.
I found out that enabling strict_types in prod was actually useful:

  • it has no overhead compared to disabling strict_types
  • the fallback to null does not help:
    • many filters or functions will fail when receiving null as input, but the stack trace will not allow you to find the root cause of the issue as it won't show where the null comes from
    • some functions or filters will work but produce unexpected results when getting null. For instance, the date filter (and its siblings from twig/intl-extra) convert null to new \DateTimeImmutable('now'), which might not be what you expect.

@drjayvee
Copy link
Contributor Author

many filters or functions will fail when receiving null as input, but the stack trace will not allow you to find the root cause of the issue as it won't show where the null comes from

That's the very problem that lead me to investigate static analysis for twig. I had renamed a variable (in a macro) and forgot to change one of the uses. It broke the whole page in production. This is no longer possible with our extension.

More generally though, we could reconsider the utility for strict_variables (not strict_types)? But that's an issue for another time.

@drjayvee
Copy link
Contributor Author

In PR #4235, the documentation for types includes this note:

The syntax for and contents of type strings are intentionally left out of scope.

@fabpot added the following in his review (merci beaucoup pour ca!)

Thinking out loud here: maybe we can document a default subset that should always be supported by extensions, the basic types used by Twig: boolean, number (maybe integer, float), mapping, sequence, string/stringable, object, iterable?

I think this is a good idea, but I have some reservations. Let me also think out loud here.

It's weird that Twig core specifies should/what must be supported, while explicitly leaving validation and support for more types to said extensions.

Then again, it would be great if the basics are specified to give the tag more weight and possibly direction.

I'd propose bool, string, float, int, array, object and mixed. (null is pointless, right?)

While I like number, I prefer int and float because those are the actual types in PHP, plus the distinction conveys relevant information.

Next, while I like mapping and sequence, I'm afraid those will need their own detailed specifications and implementations in extensions. Most likely is that implementations will use PHPStan's types, as those are most widely used and supported by other tools, including IDEs.

What's more, specifying mapping but not its keys and values' types, and specifying sequence without the items' type(s) adds very little over array.

Lastly, mixed is necessary unless we support union types (e.g., int|string), which goes way to far for core. Without mixed you can't document that a variable is required (unless it's of one of the other types.)

I would also really like support for nullable by using the ? prefix to the type.

I predict we'll see lots of different opinions. Which isn't to say we shouldn't have it and stick to just agnosticism. Should we consider this a separate issue? I guess we should either try to merge the tag with a basic list of types, or decide to remain completely agnostic.

@stof
Copy link
Member

stof commented Aug 27, 2024

Documenting a default subset would mean that extensions wanting to validate type would be interoperable on the meaning of this default subset, making things more useful. See for instance bool vs boolean which would hurt interoperability if 2 extensions decide to use different names for the boolean type.
However, we could also let extensions collaborate between themselves as a start to see how things go (for instance, in the PHP land, psalm and phpstan tend to harmonize the types they support for the same reason)

@JoshuaBehrens
Copy link

JoshuaBehrens commented Aug 27, 2024

Regarding get_attribute optimization: I would hate to work with mixed just because someone has an "object|array kind of situation" because their code wants to support 1) a "pagination"-object they get or 2) someone fakes the object by passing an array in the include … with { pagination: }. Then everyone will just use mixed. And therefore I cannot predict to go for array access or method call check :/

Do I get it right, that any variable is expected to be nullable? So there is no need to write string|null?

The pagination DTO is just something to make it less abstract. Insert any case you have in your mind, where a template requires certain data to be used.

@willrowe
Copy link
Contributor

Do I get it right, that any variable is expected to be nullable? So there is no need to write string|null?

I would like the ability to explicitly mark variables as accepting a null value or completely nullable. To give some examples, I currently document variables that are optional and are not required to be passed to the template context like so:

{% types {
    optional_variable: '?boolean',
} %}

Usually, I will then set that variable to a default value if it has not been passed. The ? denotes that it is not required. This is distinct from allowing a null value to be passed to a variable though:

{% types {
    optional_variable: 'string|null',
} %}

In this case, if some text should be displayed in an h1 tag, then it should be passed as a string. If the h1 tag should be omitted altogether then null should be passed explicitly.

@fabpot
Copy link
Contributor

fabpot commented Aug 27, 2024

Can we say that a variable is optional if there is a is defined test or a default filter? The more tools can deduce the better IMHO.

@willrowe
Copy link
Contributor

@fabpot that's a great question. I'm only thinking in terms of what is passed in the template context.

@ruudk
Copy link
Contributor

ruudk commented Aug 27, 2024

If you think of the context as an array, in PHP you would type optional array keys as follow:

array{key: 'string', optionalKey?: 'string'}

See https://phpstan.org/writing-php-code/phpdoc-types#array-shapes

It would be great if we could write:

{% types {
  key: 'string',
  optionalKey?: 'string'
} %}

Also, if we want to describe what are valid values for types, I would say: any type you would write in PHPDoc.

See https://phpstan.org/writing-php-code/phpdoc-types

Let's keep it simple and see what analyzers do with it. We can standardize things depending on use cases in the wild. After things are proven to be useful.

@willrowe
Copy link
Contributor

@ruudk that's an interesting approach. I'm not picky about the syntax, I would just like a way to denote it that is valid and doesn't conflict with anything else.

@smnandre
Copy link
Contributor

I'm thinking too the base types of twig should be "documented" to encourage compatibility.

For the rest, i kinda feel this could be solved by usage pretty quickly, as users will do what they need, tools too, and thus can be standardized in a future step.

@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 28, 2024

Optional variables

Do I get it right, that any variable is expected to be nullable? So there is no need to write string|null?

My idea with the ? prefix is to only use it as shorthand for |null, just like in PHP. By default, variables are not considered nullable. (Again, that's my proposal.)

I would really encourage everyone (who's interested in types and code quality) to ensure variables are always defined. But that might be too opinionated an approach for Twig core. Alternatively, users can just leave optional variables out of types, of course, but that creates a gap in what can and cannot be declared. :/

Can we say that a variable is optional if there is a is defined test or a default filter? The more tools can deduce the better IMHO.

I prefer ruud's proposal (i.e., {name?: 'type'}) to declare which variables are optionally defined. Otherwise, template developers would have to go looking for name is defined in the entire template, so there's way less clarity.

(Edit: I just tried adding the implementation for name? to my PR branch, and it turns out to be trivial. 🚀)

(Extensions can trigger a warning if an optional variable is used without is defined - at least in theory. They can also trigger a warning for optional variables and advise to ensure variables are defined.)

Do we all agree that we do want support for optional variables in types? (I do!)

Here's an example:

{% types {
  requiredInt: 'int',
  optionalInt?: 'int',
  requiredNullableInt: '?int',
  optionalNullableInt?: '?int',
} %}

Types
It's good to see people agree that defining a base set of types is useful.

So do you prefer matching PHP's concrete types (e.g., 'string, array`), or Fabian's more high-level ones (e.g., 'stringable, iterable')?

Again, extensions can add types and validate types as they see fit. I'm okay with extension developers figuring out interoperability amongst each other. Most likely, extensions will settle on PHPStan/Psalm.

@stof
Copy link
Member

stof commented Aug 28, 2024

I like the ? suffix on the variable name to indicate optional variables. That's a nice syntax, and it is consistent with the syntax for array shapes used in phpstan and psalm.

For the basic types, I would say that the concrete PHP types need to be part of the list, but this list might include other types. For instance, iterable makes perfect sense. But anyway, this can happen later compared to adding the {% types %} as it is purely documentation aimed at type-checking extensions. The priority is introducing the tag so that those extensions can start consuming it.
And I indeed expect those extensions to use types matching the phpstan/psalm ones (especially as it can allow them to actually use phpstan/psalm in their implementation). This will be easier for devs that won't need to learn 2 different ways to describe types. But let's see how those extensions evolve.

@ruudk
Copy link
Contributor

ruudk commented Aug 28, 2024

For instance, iterable makes perfect sense.

And that's perfectly valid according to PHPStan's documentation

@ruudk
Copy link
Contributor

ruudk commented Aug 28, 2024

My idea with the ? prefix is to only use it as shorthand for |null, just like in PHP. By default, variables are not considered nullable. (Again, that's my proposal.)
{% types {
requiredInt: 'int',
optionalInt?: 'int',
requiredNullableInt: '?int',
optionalNullableInt?: '?int',
} %}

That's not a valid type in PHPDoc. In PHPDoc you write string|null.

So it would become:

{% types {
  requiredInt: 'int',
  optionalInt?: 'int',
  requiredNullableInt: 'int|null',
  optionalNullableInt?: 'int|null',
} %}

I think to support the ? at the end of a key we need to modify Twig\ExpressionParser::parseMappingExpression to support it. Maybe we can create a new one that parses static mappings. So that means that keys can end with ?. And that values should always be a string (not an expression).

Twig also supports shorthands like { name, other: 'value' } where name is an existing variable that is then written as name: name. Just like modern JavaScript/TypeScript. In the case of this types mapping, it should not be possible.

@fabpot
Copy link
Contributor

fabpot commented Aug 28, 2024

To give us more time to experiment, maybe we could mark the new tag as being "experimental" like what we do on Symfony. That will let us change the details after we gain enough experience using it.

@drjayvee
Copy link
Contributor Author

drjayvee commented Aug 28, 2024

[?int] is not a valid type in PHPDoc.

Right, but it is valid PHP. 🤕 I actually kind of prefer |null anyway because it's more explicit.

I think to support the ? at the end of a key we need to modify Twig\ExpressionParser::parseMappingExpression to support it.

Nope. My current implementation doesn't use that method because it supports way more than types should, such as the {key} shorthand for {key: key}, expressions for values, and more.

Adding support for name? is trivial:

diff --git a/src/TokenParser/TypesTokenParser.php b/src/TokenParser/TypesTokenParser.php
index 2bbe183f..b252b848 100644
--- a/src/TokenParser/TypesTokenParser.php
+++ b/src/TokenParser/TypesTokenParser.php
@@ -68,7 +68,10 @@ final class TypesTokenParser extends AbstractTokenParser

             $nameToken = $stream->expect(Token::NAME_TYPE);
             $nameExpression = new NameExpression($nameToken->getValue(), $nameToken->getLine());
+            
+            $isOptional = $stream->nextIf(Token::PUNCTUATION_TYPE, '?') !== null;
+            $nameExpression->setAttribute('is_optional', $isOptional);
             $stream->expect(Token::PUNCTUATION_TYPE, ':', 'A mapping key must be followed by a colon (:)');

             $valueToken = $stream->expect(Token::STRING_TYPE);

To give us more time to experiment, maybe we could mark the new tag as being "experimental" like what we do on Symfony. That will let us change the details after we gain enough experience using it.

Sure, I'm totally fine with that.

@ruudk
Copy link
Contributor

ruudk commented Aug 28, 2024

My current implementation doesn't use that method because it supports way more than types should, such as the {key} shorthand for {key: key}, expressions for values, and more.

Adding support for name? is trivial:

That's great 👏

drjayvee added a commit to alisqi/Twig that referenced this issue Aug 28, 2024
This also adds support for optional keys as discussed in twigphp#4165.

I really don't like using a static method, but I couldn't figure out how
else to call the method in tests. See the notes on the method itself.

Any help is appreciated!
fabpot added a commit that referenced this issue Aug 29, 2024
This PR was squashed before being merged into the 3.x branch.

Discussion
----------

Add types tag

The is a draft implementation for #4165.

Commits
-------

67dabd8 Add types tag
@drjayvee drjayvee changed the title Add tag to declare variable types [WIP] Add tag to declare variable types Aug 30, 2024
@drjayvee
Copy link
Contributor Author

And it's merged! Thank you all for contributing.

Do we still want to continue discussing the base types that should be supported? Do we continue in a new issue and close this one? Or do we wait and see what the community does with this tag?

I still think it would be valuable to have the basics covered and part of the official spec, but I'd prefer continuing this discussion in a new issue.

Lastly, I wanted to ask you, @Haehnchen, whether you're going to add support for types in the Symfony Support plugin. I've also asked the JetBrains PHPStorm team to consider adding 1st-party support.

@fabpot
Copy link
Contributor

fabpot commented Aug 30, 2024

Thank you all for your great discussion here. Let’s open another issue for the basic types.

@drjayvee
Copy link
Contributor Author

I'll close this issue. See you in #4256!

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

No branches or pull requests

8 participants