Add CommentNode to the AST #4009

drjayvee · 2024-03-16T13:27:48Z

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.

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 [twigphp#4003](twigphp#4003) and in more detail in [TwigStan](https://github.com/alisqi/twig-stan/)'s README.

drjayvee · 2024-03-16T13:36:37Z

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

See twigphp/Twig#4009 for more details

drjayvee · 2024-03-16T13:47:50Z

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 · 2024-03-17T16:48:19Z

@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 · 2024-03-17T16:52:02Z

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 · 2024-03-18T10:45:11Z

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 · 2024-03-18T11:23:26Z

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 · 2024-03-18T14:51:25Z

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 · 2024-03-18T15:00:35Z

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 · 2024-03-19T05:02:02Z

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 · 2024-03-20T20:01:50Z

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 · 2024-03-21T08:31:05Z

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 · 2024-03-21T10:26:11Z

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 · 2024-03-24T00:46:43Z

Can you mention mention me, when this is merged? This way I can use them in this PR #3928

drjayvee · 2024-03-28T09:06:44Z

@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 · 2024-07-05T07:44:50Z

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

drjayvee · 2024-07-05T13:49:06Z

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 · 2024-07-05T15:46:54Z

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 · 2024-07-13T16:38:13Z

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 · 2024-07-29T13:19:42Z

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 · 2024-08-22T11:22:17Z

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

fabpot

Even if supporting comment nodes in core is simple as demonstrated by this PR, I'm still a bit worried about how people are going to use this new "feature".

You should also add a note in CHANGELOG.

fabpot · 2024-08-23T07:45:39Z

src/Lexer.php

+        $text = substr($this->code, $this->cursor, $match[0][1] - $this->cursor).$match[0][0];
+        $this->pushToken(/* Token::COMMENT_TYPE */ 14, trim(substr($text, 0, strrpos($text, '#}'))));
+        $this->moveCursor($text);


Suggested change

$text = substr($this->code, $this->cursor, $match[0][1] - $this->cursor).$match[0][0];

$this->pushToken(/* Token::COMMENT_TYPE */ 14, trim(substr($text, 0, strrpos($text, '#}'))));

$this->moveCursor($text);

$text = substr($this->code, $this->cursor, $match[0][1] - $this->cursor);

$this->pushToken(Token::COMMENT_TYPE, trim($text));

$this->moveCursor($text.$match[0][0]);

fabpot · 2024-08-23T07:46:08Z

src/Node/CommentNode.php

+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ * (c) Armin Ronacher


Can be removed

fabpot · 2024-08-23T07:46:48Z

src/Node/CommentNode.php

+
+    public function compile(Compiler $compiler): void
+    {
+        // skip comments in compilation


Suggested change

// skip comments in compilation

// ignore comments in generated code

fabpot · 2024-08-23T07:48:54Z

tests/LexerTest.php

@@ -353,6 +353,26 @@ public function testUnterminatedBlock()
        $lexer->tokenize(new Source($template, 'index'));
    }

+    public function testCommentValues()
+    {
+        $template = '{# comment #}some text{#another one#}';


Suggested change

$template = '{# comment #}some text{#another one#}';

$template = "{# comment #}some text{#another one#}{# \n \nwhitespace\n #}";

fabpot · 2024-08-23T07:50:13Z

tests/LexerTest.php

+    public function testCommentValues()
+    {
+        $template = '{# comment #}some text{#another one#}';
+        $lexer = new Lexer(new Environment($this->createMock(LoaderInterface::class)));


Suggested change

$lexer = new Lexer(new Environment($this->createMock(LoaderInterface::class)));

$lexer = new Lexer(new Environment(new ArrayLoader()));

As this is how it's done in 4.x.

fabpot · 2024-08-23T07:52:52Z

tests/LexerTest.php

+        self::assertEquals(
+            'another one', // assert that comment is parsed
+            $stream->expect(Token::COMMENT_TYPE)->getValue()
+        );


Suggested change

);

);

self::assertEquals(

'whitespace',

$stream->expect(Token::COMMENT_TYPE)->getValue()

);

drjayvee · 2024-08-26T09:36:59Z

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 · 2024-08-26T09:48:08Z

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.

drjayvee mentioned this pull request Mar 16, 2024

Add static code analysis and/or linting to improve code quality #4003

Closed

drjayvee added a commit to alisqi/TwigQI that referenced this pull request Mar 16, 2024

Add information re: comment nodes to README

3219510

See twigphp/Twig#4009 for more details

Adjust code style

4d41a2c

drjayvee mentioned this pull request Mar 16, 2024

Mark implicit macro argument default values as such with an attribute in AST #4010

Closed

moetelo mentioned this pull request Apr 9, 2024

Add @var type hinting support moetelo/twiggy#14

Closed

drjayvee mentioned this pull request Jul 29, 2024

Add tag to declare variable types #4165

Closed

fabpot requested changes Aug 23, 2024

View reviewed changes

fabpot closed this Aug 26, 2024

drjayvee mentioned this pull request Aug 30, 2024

Add support for new types tag Haehnchen/idea-php-symfony2-plugin#2396

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CommentNode to the AST #4009

Add CommentNode to the AST #4009

drjayvee commented Mar 16, 2024

drjayvee commented Mar 16, 2024

drjayvee commented Mar 16, 2024

fabpot commented Mar 17, 2024

fabpot commented Mar 17, 2024

drjayvee commented Mar 18, 2024 •

edited

Loading

fabpot commented Mar 18, 2024

drjayvee commented Mar 18, 2024

drjayvee commented Mar 18, 2024

brendt commented Mar 19, 2024 •

edited

Loading

Haehnchen commented Mar 20, 2024

stof commented Mar 21, 2024 •

edited

Loading

drjayvee commented Mar 21, 2024 •

edited

Loading

JoshuaBehrens commented Mar 24, 2024

drjayvee commented Mar 28, 2024

fabpot commented Jul 5, 2024

drjayvee commented Jul 5, 2024 •

edited

Loading

fabpot commented Jul 5, 2024

ruudk commented Jul 13, 2024

drjayvee commented Jul 29, 2024

drjayvee commented Aug 22, 2024

fabpot left a comment •

edited

Loading

fabpot Aug 23, 2024

fabpot Aug 23, 2024

fabpot Aug 23, 2024

fabpot Aug 23, 2024

fabpot Aug 23, 2024

fabpot Aug 23, 2024

drjayvee commented Aug 26, 2024

fabpot commented Aug 26, 2024

	// skip comments in compilation
	// ignore comments in generated code

	$template = '{# comment #}some text{#another one#}';
	$template = "{# comment #}some text{#another one#}{# \n \nwhitespace\n #}";

	$lexer = new Lexer(new Environment($this->createMock(LoaderInterface::class)));
	$lexer = new Lexer(new Environment(new ArrayLoader()));

Add CommentNode to the AST #4009

Add CommentNode to the AST #4009

Conversation

drjayvee commented Mar 16, 2024

drjayvee commented Mar 16, 2024

drjayvee commented Mar 16, 2024

fabpot commented Mar 17, 2024

fabpot commented Mar 17, 2024

drjayvee commented Mar 18, 2024 • edited Loading

fabpot commented Mar 18, 2024

drjayvee commented Mar 18, 2024

drjayvee commented Mar 18, 2024

brendt commented Mar 19, 2024 • edited Loading

Haehnchen commented Mar 20, 2024

stof commented Mar 21, 2024 • edited Loading

drjayvee commented Mar 21, 2024 • edited Loading

JoshuaBehrens commented Mar 24, 2024

drjayvee commented Mar 28, 2024

fabpot commented Jul 5, 2024

drjayvee commented Jul 5, 2024 • edited Loading

fabpot commented Jul 5, 2024

ruudk commented Jul 13, 2024

drjayvee commented Jul 29, 2024

drjayvee commented Aug 22, 2024

fabpot left a comment • edited Loading

Choose a reason for hiding this comment

fabpot Aug 23, 2024

Choose a reason for hiding this comment

fabpot Aug 23, 2024

Choose a reason for hiding this comment

fabpot Aug 23, 2024

Choose a reason for hiding this comment

fabpot Aug 23, 2024

Choose a reason for hiding this comment

fabpot Aug 23, 2024

Choose a reason for hiding this comment

fabpot Aug 23, 2024

Choose a reason for hiding this comment

drjayvee commented Aug 26, 2024

fabpot commented Aug 26, 2024

drjayvee commented Mar 18, 2024 •

edited

Loading

brendt commented Mar 19, 2024 •

edited

Loading

stof commented Mar 21, 2024 •

edited

Loading

drjayvee commented Mar 21, 2024 •

edited

Loading

drjayvee commented Jul 5, 2024 •

edited

Loading

fabpot left a comment •

edited

Loading