Add CommentNode to the AST

[drjayvee]

These nodes have no effect on compiled templates.

Adding these to the TokenStream and AST enables extensions to add logic in node visitors, such as parsing PHPDoc-style @var string name comments.

This in turn is invaluable for static code analysis of Twig templates as briefly discussed in #4003 and in more detail in TwigStan's README.

[drjayvee]

@fabpot and @stof, do you think this can be added in 3.x? Or do you consider it backward breaking, since new nodes are added to the lexer's token stream and the parser's AST?

Please have a look at TwigStan's README for further motivation of the utility of this change.

I really think that TwigStan could be huge boon for Twig users.

[drjayvee]

One of the code style issues is:

-        $nested = $nested || Node::class !== \get_class($node);
+        $nested = $nested || Node::class !== $node::class;

However, the suggested change isn't supported in PHP < 8.0.

[fabpot]

@drjay

One of the code style issues is:

-        $nested = $nested || Node::class !== \get_class($node);
+        $nested = $nested || Node::class !== $node::class;

However, the suggested change isn't supported in PHP < 8.0.

You can ignore fabbot when it suggests stupid changes.

[fabpot]

I'm wondering if it would be better to define a tag instead of using comments.

Instead of:

{# @var user \User #}
{# @var showBadge bool #}

We could have:

{% var user \User %}
{% var showBadge bool %}

which also allows Twig to check that the syntax is ok and would make it available to more than just static analysis tools (IDEs for instance).

[drjayvee]

Bonjour Fabian,

I'm wondering if it would be better to define a tag instead of using comments.

A new tag has the big advantage of not needing any change to Twig itself. An extension can define, parse and otherwise process it in any way it sees fit.

However, @var comments are already supported by PhpStorm + Symfony Support plugin:

would make it available to more than just static analysis tools (IDEs for instance)

Agreed, but at least the most popular IDE (correct me if I'm wrong) already supports comments.

I don't know about other project, but our app has 750+ of these comments, and we're adding more every week to declare and document templates' context variables, as well as macro arguments. It works really well!

It also aligns with phpDoc, which also uses comments. (That's for historical reasons, of course. But still, I'm not aware of plans to adopt Attributes as a newer alternative.)

So what I'm saying is that the proposed change align with what (some in) the community have already adopted.

which also allows Twig to check that the syntax is ok

I'm planning for TwigStan to check the syntax, most likely by using phpDocumentor under the hood. Twig could implement this functionality by itself, of course. All I'm saying is that I don't think this feature is exclusively possible with a new tag.

[fabpot]

What you're describing is the community having worked around the system. Here, we're talking about making this feature official, so I think using a proper tag is the way to go.

[drjayvee]

Fair enough. I'm actually thrilled to see you take this quite seriously! Twig certainly deserves better support for types. ❤️

However, I'm skeptical a new tag will be adopted in practice anytime soon. Specifically, the Symfony Support plugin is poorly maintained (500 open issues), so I doubt this particular project will adapt. (I've reached out to its author, @Haehnchen, a few weeks ago, but haven't received an answer.)

Do you perhaps have contacts at JetBrains (someone like @brendt, perhaps), who could tell us whether PhpStorm would consider adoption of this?

Speaking just for our team, we'd have to duplicate {# @var #} and {% var %} to have proper support in the IDE and static code analysis for the foreseeable future. Not a fun prospect.

So why not support both? The var tag will be the official way to do it, and Twig (itself) will add (deprecated) support for comments as a (temporary) alternative. (Similar to how Routes can be configured in various ways.)

What do you think?

[drjayvee]

Aside from the type declarations, I can imagine the community adding support for more meta-level features, for example to suppress code inspections (similar to @phpstan-ignore-next-line).

To enable the community to add these sorts of features, either Twig needs to add a more generic tag (e.g., {% annotate %} or {% meta %}), or, you know, use comments. 😉

Again, Twig could do both CommentNodes for flexibility and extensibility, and 1st class support for type declarations.

[brendt]

Hey I passed this to the PhpStorm team, let us know if/when this is getting added and we're happy to look into it 👍

Update: I've made an issue on our side about this: https://youtrack.jetbrains.com/issue/WI-76637/Twig-var-tag

[Haehnchen]

Symfony Support plugin is poorly maintained (500 open issues), so I doubt this particular project will adapt

open issues does not reflect project maintenance, number is still less the Symfony itself :)

But just for letting you known as soon as any implemention is on the way, it will be implemented together with a migration inspection.

---

Just some hints

arrays should considered also: {% var user \User[] %}

whereas var doc comment is widely adopted, there are also ways to define entrypoints to collect incoming variables:

{# @controller FooBundle:Bar:index #}
{# @controller Class::method #}

overall i am sceptical about a tag unless it provides more support out of the box. e.g. it would be nice having lint:twig or something else to check for class existence, profiler support / triggering exceptions in dev mode, ...

[stof]

Adding CommentNode in the AST looks weird to me, because it means it would not be an AST anymore but an intermediate between an AST and a CST (a full CST needs to retain all formatting information)

[drjayvee]

But just for letting you known as soon as any implemention is on the way, it will be implemented together with a migration inspection.

Thanks for chiming in. This is great to hear.

arrays should considered also: {% var user \User[] %}

Agreed. I'm hoping for community contributions to TwigStan to extend the inspections.

there are also ways to define entrypoints to collect incoming variables

Can you point me in the direction of some documentation? I don't quite understand what this means / does, but I am intrigued.

This does support my argument that adding support for comments allows for more generic extensibility than the var tag.

Again, maybe the best is to do both, with a @var comment as a (temporary) alternative to the var tag.

it would be nice having lint:twig or something else to check for class existence, profiler support / triggering exceptions in dev mode

Linting, in combination with exceptions (actually, errors/warnings/notices) in dev mode is exactly the point of TwigStan. Type check (invalid class, misspelled primitive, etc) is already a planned inspection.

---

Adding CommentNode in the AST looks weird to me, because it means it would not be an AST anymore but an intermediate between an AST and a CST (a full CST needs to retain all formatting information)

Ah, I wasn't aware of that distinction at all. I do appreciate the desire to keep the compiler parts clean and by the books.

One alternative could be transform comments to node attributes. But the question immediately becomes: which nodes should receive these attributes? Plus this would have to happen in the Lexer, which isn't the appropriate class to look around the token stream (which it's building itself). Therefore, this seems a no-go.

How about making this behavior configurable? It's clear that only strict dev environments (and linting) would want this. I'm not sure whether this makes matters worse, though. Nor does it really solve stof's principled objection.

Is there another way we can enable static code analysis other than comments?

Or do we maybe just add comments to the AST anyway, since so much good can come out of it to balance the bad? 😜

[JoshuaBehrens]

Can you mention mention me, when this is merged? This way I can use them in this PR https://github.com/twigphp/Twig/pull/3928

[drjayvee]

@stof and @fabpot, have you reconsidered your stance on this PR?

To summarize my position: I'm in favor of a first-class var tag. Once the Symfony Support plugin (and PhpStorm) support it, developers can replace their @var comments. In the meantime, however, support for comments would be a great workaround.

Aside from variable types, I do see other potential uses for comments, so I think making these accessible in TwigExtensions would be really useful. Then again, extensions can already easily add custom tags, so comments aren't needed.

I really want to move ahead on TwigStan because it will significantly increase code quality and prevent bugs. Support for variable typing is essential to most inspections, however.

[fabpot]

I haven't changed my mind here. Having a tag is the way to go for having something officially supported by Twig.

[drjayvee]

Très bien allons-y alors!

I'd be more than happy to work on this, but I don't think I can do it all on my own.

We should probably start by writing a spec? The syntax isn't a big deal, but should Twig do anything with this tag, or leave it up to extensions? I guess at the very least, Twig should check whether the type exists.

Fabian, do you have any ideas for a spec?

Should I start with a basic implementation to get the conversation going? Do we create a new issue for that and close this one?

[fabpot]

I've never thought about such a spec, but others have. So, let's bootstrap a spec doc and let's try to involve people interested in this topic. Let's do it in another PR maybe. When bootstrapped, I can then publish something on the Symfony blog to ask for help. Thank you.

[ruudk]

I was curious if there is a specific reason for not supporting comments in the Lexer? It seems a bit odd that these are simply skipped. While I agree that these comments should not become Nodes in the AST, I believe it would be beneficial to have them available as attributes. This would allow for reading all Twig tokens.

This approach is similar to how PHP-Parser handles it, where comments are stored in an attribute. This method enables anyone to read the AST along with all comments and even supports writing the AST back to the original file.

[drjayvee]

Bonjour @fabpot et @stof,

I've created a first draft of the spec for the var tag in #4165. Please review and share your feedback.

[drjayvee]

@stof, how do you feel about @ruudk's alternative of adding comments as attributes to the next node? If that's a 👎 as well, I'll close this PR.

[drjayvee]

I personally don't care about this now that we are (very likely) going to implement types tag.

@fabpot I'd be more than happy to update the PR, but if you're not sure you want this feature in Twig, I'd rather focus on types for now.

[fabpot]

I personally don't care about this now that we are (very likely) going to implement types tag.

@fabpot I'd be more than happy to update the PR, but if you're not sure you want this feature in Twig, I'd rather focus on types for now.

Let's close then.