Attach documentation comments to nodes by fabpot · Pull Request #4871 · twigphp/Twig

fabpot · 2026-07-25T14:30:49Z

Alternatives to #4870

To avoid BC breaks, I have another idea, using ## as a new syntax, a bit like /** */ in PHP vs /* */.

"Documentation" is attached as metadata to the next relevant node:

{## The main content displayed on the page #}
{% block content %}
    ...
{% endblock %}

Documentation comments can also describe variables declared with the types tag:

{% types {
    ## The unique identifier of the article
    id: 'string',

    ## Whether the article should be highlighted
    featured?: 'boolean',
} %}

Node visitors can access this metadata through Node::getDocumentation(), allowing IDEs, static analyzers, and documentation generators to consume it without affecting template rendering. Documentation is preserved when visitors or optimizations replace nodes.

Closes #4768
Closes #4870

Kocal · 2026-07-25T14:37:24Z

Will it works for {% props %} tag from UX TwigComponent?

Also, I forgot to close #4768, but it was already resolved with our @prop and @block annotations (which are also properly highlighted in PHPStorm, thanks to the Symfony plugin).

Since my initial issue is closed, do we really need this new feature here?

It would requires UX to change again, and the Symfony PHPStorm plugin should be modified as well.

upsun-dispatch · 2026-07-25T14:40:20Z

📋 PR Summary

This incremental change narrows the documentation-comment feature: inline ## documentation is no longer attached to ordinary expressions, assignment operators, destructuring targets, arrow-function arguments, block/macro names, or variadic macro arguments. Documentation is no longer copied across node replacements (NodeTraverser stops propagating it; the DocumentationNodeVisitor no longer strips it in leaveNode and now runs at priority 512). The MacroNode constructor's variadic argument changes from a ?LocalVariable node to a ?string $variadicName, and the helper methods Node::removeDocumentation() and NodeDocumentation::copy() are removed.

Changes

Layer / File(s)	Summary
documentation semantics narrowing
`doc/documentation_comments.rst`	Removes documentation of assignment/destructuring/arrow/variadic doc-comment support and states documentation is not preserved across node replacement.
parser doc-attachment simplification
`src/ExpressionParser.php`	Drops `NodeDocumentation::set` on assignment targets and the unused import.
`src/ExpressionParser/Infix/AssignmentExpressionParser.php`	Removes documentation copying when wrapping destructuring targets in AssignContextVariable.
`src/ExpressionParser/Prefix/GroupingExpressionParser.php`	Makes `toAssignContextVariable` static and stops copying documentation; arrow-arg names built as ContextVariable.
`src/ExpressionParser/Prefix/LiteralExpressionParser.php`	Drops documentation attachment on mapping shorthand values.
`src/Parser.php`	Removes expression-level documentation attachment in parseExpression; PrintNode/tag documentation still set in subparse.
`src/TokenParser/BlockTokenParser.php`	Stops attaching documentation from the block-name token; block documentation now comes from the leading comment only.
expression-node doc handling
`src/Node/Expression/ArrowFunctionExpression.php`	Stops preserving documentation when normalizing a single name into a ListExpression.
`src/Node/Expression/Binary/SetBinary.php`	Always rebuilds the left AssignContextVariable without copying documentation.
macro variadic model change
`src/Node/MacroNode.php`	Constructor takes `?string $variadicName` stored as an attribute; compile derives bucket names via TempNameExpression reserved-name handling.
`src/TokenParser/MacroTokenParser.php`	Parses the variadic as a plain (prefix-stripped) name string passed to MacroNode; no longer builds a variadic node with documentation.
doc metadata lifecycle
`src/Node/Node.php`	Makes setDocumentation accept null and removes removeDocumentation().
`src/Node/NodeDocumentation.php`	Removes the copy() helper; move() clears the source via setDocumentation(null).
`src/NodeTraverser.php`	Removes copyDocumentation propagation across enterNode/leaveNode replacements.
`src/NodeVisitor/DocumentationNodeVisitor.php`	Drops expression documentation removal in leaveNode; changes priority from -512 to 512.
tests
`tests/Node/MacroTest.php`	Removes the test asserting the variadic argument is a node.
`tests/ParserTest.php`	Removes tests for expression/arrow/destructuring/variadic documentation attachment and node-replacement preservation; adjusts remaining assertions.
`tests/TokenParser/BlockTokenParserTest.php`	Removes cases for inline documentation inside the block tag.

fabpot · 2026-07-25T15:26:17Z

Will it works for {% props %} tag from UX TwigComponent?

Also, I forgot to close #4768, but it was already resolved with our @prop and @block annotations (which are also properly highlighted in PHPStorm, thanks to the Symfony plugin).

Since my initial issue is closed, do we really need this new feature here?

It would requires UX to change again, and the Symfony PHPStorm plugin should be modified as well.

This is generic, so it should work just fine.

Kocal · 2026-07-25T15:39:31Z

Cool, thanks!

When merged/released, I will work on UX & Symfony PHPStorm plugin to re-align things with this PR.

upsun-dispatch

📋 Upsun Dispatch Review: incremental · 18 files reviewed · no new issues · 2 still open

Outstanding from earlier reviews:

#3650339795 — src/Node/TypesNode.php:32: Two divergent construction sites for the same nodes invite line-number drift and maintenance errors.
#3650339797 — src/Node/Node.php:145: Ambiguous @internal marking on a method the feature relies on causes API-stability confusion.

upsun-dispatch

📋 Upsun Dispatch Review: full · 17 files reviewed · 🔵 1 info · 2 still open

Outstanding from earlier reviews:

#3650339795 — src/Node/TypesNode.php:32: Two divergent construction sites for the same nodes invite line-number drift and maintenance errors.
#3650339797 — src/Node/Node.php:145: Ambiguous @internal marking on a method the feature relies on causes API-stability confusion.

Kocal · 2026-07-25T18:28:58Z

Opened Haehnchen/idea-php-symfony2-plugin#2836, UX PR will follows when this one get merged/released.

upsun-dispatch · 2026-07-26T08:02:38Z


-        return new TypesNode($types, $token->getLine());
+        $node = new TypesNode($types, $token->getLine());
+        foreach ($nodes as $name => $type) {


🔵 Info — Redundant construction and duplicated type state make the model wasteful and easy to desynchronize.

TypesNode's constructor already builds a TypeNode child for every entry of the mapping (TypesNode.php: foreach ($types as $name => $type) { $nodes[$name] = new TypeNode(...) }), and this loop then immediately overwrites each of those children via setNode() with a second TypeNode that carries the documentation and the correct per-name line number. The constructor-built nodes are discarded on every parse, and the type data now lives in three copies (the mapping attribute plus two sets of TypeNodes). A visitor mutating a TypeNode will silently diverge from the mapping attribute compiled elsewhere.

upsun-dispatch

📋 Upsun Dispatch Review: incremental · 26 files reviewed · 🔵 2 infos · 1 still open

Outstanding from earlier reviews:

#3652057652 — src/TokenParser/TypesTokenParser.php:40: Redundant construction and duplicated type state make the model wasteful and easy to desynchronize.

upsun-dispatch · 2026-07-26T19:32:16Z

                $stream->expect(Token::NAME_TYPE, null, 'Only variables can be assigned to');
            }
-            $targets[] = new AssignContextVariable($token->getValue(), $token->getLine());
+            $target = new AssignContextVariable($token->getValue(), $token->getLine());


🔵 Info — Duplicated conversion logic across parsers is easy to desynchronize as documentation handling evolves.

The removed Parser::createAssignContextVariable/convertToAssignContextVariable helpers are now re-implemented as near-duplicate blocks in at least five places: this inline new AssignContextVariable(...) + NodeDocumentation::set(...), an identical block in AbstractTokenParser::parseAssignmentExpression, a private createAssignContextVariable in GroupingExpressionParser, a new AssignContextVariable(...) + NodeDocumentation::promote(...) variant in AssignmentExpressionParser and ArrowExpressionParser, and manual getDocumentation()/setDocumentation() copies in SetBinary and ArrowFunctionExpression::toAssignContextVariable. These copies already diverge in how they carry documentation (set vs promote vs direct copy), so a future change to conversion semantics must be replicated in every site.

upsun-dispatch · 2026-07-26T19:32:16Z

+final class NodeDocumentation
+{
+    /** @var \WeakMap<Node, string>|null */
+    private static ?\WeakMap $pending = null;


🔵 Info — Global mutable static state is harder to test and reason about than the instance state it replaced.

$pending and $targets are process-global static \WeakMaps, replacing the former per-Parser instance state (pendingDocumentation/documentationTargets) that was reset at the start and end of each parse(). The state is now shared across all Parser instances and never reset; entries only clear when the keyed Node is garbage-collected. This is safe for single-threaded parsing (nodes are distinct objects per parse and WeakMap keys are weak), but it makes the stage→promote / set→setTarget lifecycle harder to test and reason about than the scoped instance state it replaced.

upsun-dispatch

📋 Upsun Dispatch Review: incremental · 26 files reviewed · no new issues · 3 still open

Outstanding from earlier reviews:

#3652057652 — src/TokenParser/TypesTokenParser.php:40: Redundant construction and duplicated type state make the model wasteful and easy to desynchronize.
#3653256238 — src/ExpressionParser.php:314: Duplicated conversion logic across parsers is easy to desynchronize as documentation handling evolves.
#3653256239 — src/Node/NodeDocumentation.php:22: Global mutable static state is harder to test and reason about than the instance state it replaced.

upsun-dispatch

📋 Upsun Dispatch Review: full · 67 files reviewed · 🔵 3 infos · 2 still open

⚠️ correctness reviewer unavailable — coverage may be partial

🔵 Info (P2 — suggestions) — outside diff range

src/ExpressionParser.php · design

parseExpression() unconditionally calls NodeDocumentation::set($expr, $documentationToken) on every expression it parses, so the leading token's documentation is attached to arbitrary expression nodes and correctness depends entirely on DocumentationNodeVisitor::leaveNode() later stripping it from every AbstractExpression that is not an AssignContextVariable/LocalVariable. If that visitor is unregistered or reordered, or a new documented-target expression type is introduced, documentation silently leaks onto (or is wrongly removed from) nodes.

Review details

Commit: Commit fc2ffbe
Model: claude-opus-4-8
Panel: security · correctness · robustness · design

Outstanding from earlier reviews:

#3652057652 — src/TokenParser/TypesTokenParser.php:40: Redundant construction and duplicated type state make the model wasteful and easy to desynchronize.
#3653256238 — src/ExpressionParser.php:314: Duplicated conversion logic across parsers is easy to desynchronize as documentation handling evolves.

upsun-dispatch · 2026-07-27T17:32:02Z

+            $this->modules[] = $node;
+        }
+
+        $module = end($this->modules);


🔵 Info — Undocumented root-node assumption makes the visitor unsafe to reuse on subtrees.

$module = end($this->modules) returns false when the stack is empty, and the following BlockReferenceNode/MacroDeclarationNode branches immediately dereference it with $module->getNode('blocks')/$module->getNode('macros'). The visitor assumes traversal always begins at a ModuleNode (so one is pushed before any block/macro node is entered); traversing a subtree not rooted at a ModuleNode would call ->getNode() on false and fatal, with nothing documenting or enforcing that assumption.

upsun-dispatch · 2026-07-27T17:32:02Z

+        }
+
+        if ($node instanceof ModuleNode) {
+            array_pop($this->modules);


🔵 Info — Stale AST references accumulate across compilations when traversal throws.

The $this->modules stack is pushed in enterNode and popped here in leaveNode, but the visitor instance is reused across compilations. If traversal aborts after a ModuleNode was pushed (e.g. another visitor or a leaveNode throws), this array_pop never runs, so the aborted ModuleNode and the AST it references stay on the stack and are never released; repeated failed compilations accumulate them.

upsun-dispatch

📋 Upsun Dispatch Review: incremental · 59 files reviewed · no new issues · 3 still open

Outstanding from earlier reviews:

#3652057652 — src/TokenParser/TypesTokenParser.php:40: Redundant construction and duplicated type state make the model wasteful and easy to desynchronize.
#3659503446 — src/NodeVisitor/DocumentationNodeVisitor.php:35: Undocumented root-node assumption makes the visitor unsafe to reuse on subtrees.
#3659503454 — src/NodeVisitor/DocumentationNodeVisitor.php:53: Stale AST references accumulate across compilations when traversal throws.

stof · 2026-07-28T08:41:52Z

@fabpot is it expected that this PR contains commits from #4851 ?

stof · 2026-07-28T08:59:30Z

+ * @author Fabien Potencier <fabien@symfony.com>
+ */
+#[YieldReady]
+final class TypeNode extends Node


should this class (or at least its constructor) be @internal if it is only meant to be instantiated by TypesNode ?

stof · 2026-07-28T09:02:21Z

+use Twig\Token;
+
+/**
+ * @internal


should this be @internal ? Looking at the implementation of token parsers, it looks like custom token parsers provided by extensions might need to use it as well if they want to support documentation on their nodes.

stof · 2026-07-28T09:05:23Z


-        $this->moveCursor(substr($this->code, $this->cursor, $match[0][1] - $this->cursor).$match[0][0]);
+        $comment = substr($this->code, $this->cursor, $match[0][1] - $this->cursor);
+        if (str_starts_with($comment, '#')) {


how does the ## marker combine with whitespace control markers ?

fabpot mentioned this pull request Jul 25, 2026

Add a docs option to the types and block tags #4870

Closed

upsun-dispatch Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread src/Node/TypesNode.php

Comment thread src/Node/Node.php Outdated

fabpot force-pushed the documented-comments branch from fcbb983 to 12bb3aa Compare July 25, 2026 15:46

upsun-dispatch Bot reviewed Jul 25, 2026

View reviewed changes

fabpot force-pushed the documented-comments branch from 12bb3aa to d864497 Compare July 25, 2026 16:01

upsun-dispatch Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread src/Lexer.php Outdated

Kocal mentioned this pull request Jul 25, 2026

Rework UX component prop/block docs for Twig 3.29 comment syntax Haehnchen/idea-php-symfony2-plugin#2836

Open

fabpot force-pushed the documented-comments branch from d864497 to 4636611 Compare July 26, 2026 07:53

upsun-dispatch Bot reviewed Jul 26, 2026

View reviewed changes

fabpot force-pushed the documented-comments branch from 37205b4 to 099455d Compare July 26, 2026 19:37

upsun-dispatch Bot reviewed Jul 26, 2026

View reviewed changes

fabpot added 8 commits July 27, 2026 18:59

Redesign macro calls and argument handling

203335d

Represent macro declarations explicitly in the AST

ed46ee7

Attach documentation comments to nodes

d6ebe0f

Resolve documentation with a node visitor

ba0ebfb

Resolve declaration documentation semantically

4913312

Narrow documentation visitor state

f019365

Use normalized destructuring targets

d6765e9

Inline arrow argument normalization

fc2ffbe

fabpot force-pushed the documented-comments branch from 099455d to fc2ffbe Compare July 27, 2026 17:17

upsun-dispatch Bot reviewed Jul 27, 2026

View reviewed changes

fabpot added 2 commits July 27, 2026 19:45

Resolve documentation after other visitors

f6c6828

Limit documentation comments to direct declarations

57acc0a

Kocal mentioned this pull request Jul 27, 2026

[Toolkit] Enforce single-line props declarations with a linter symfony/ux#3739

Closed

upsun-dispatch Bot reviewed Jul 27, 2026

View reviewed changes

stof reviewed Jul 28, 2026

View reviewed changes

Haehnchen mentioned this pull request Jul 28, 2026

docs(storefront): document Symfony UX Toolkit prop and block format shopware/shopware#18418

Closed

5 tasks

Uh oh!

Uh oh!

Conversation

fabpot commented Jul 25, 2026

Uh oh!

Kocal commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsun-dispatch Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabpot commented Jul 25, 2026

Uh oh!

Kocal commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsun-dispatch Bot left a comment

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Kocal commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsun-dispatch Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot left a comment

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot left a comment

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot left a comment

Choose a reason for hiding this comment

🔵 Info (P2 — suggestions) — outside diff range

Uh oh!

upsun-dispatch Bot Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

upsun-dispatch Bot left a comment

Choose a reason for hiding this comment

Uh oh!

stof commented Jul 28, 2026

Uh oh!

stof Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

stof Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

stof Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Kocal commented Jul 25, 2026 •

edited

Loading

upsun-dispatch Bot commented Jul 25, 2026 •

edited

Loading

Kocal commented Jul 25, 2026 •

edited

Loading

Kocal commented Jul 25, 2026 •

edited

Loading