Attach documentation comments to nodes

[fabpot]

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]

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]

📋 PR Summary

This incremental change refines the documentation-comment feature: DocumentationNodeVisitor now tracks macro declarations per module so that when a macro is defined more than once only the last declaration's documentation is promoted onto the compiled MacroNode (and stale declarations are cleared), and its priority is lowered to -512 so it runs before the optimizer. Parser::cleanupBodyForChildTemplates now moves documentation from BlockReferenceNodes onto the registered block body before they are dropped in child templates, guarding against unregistered block references. New tests cover child-template preservation, duplicate macros, node-visitor access, and the unregistered-block edge case. I verified the duplicate-macro and child-template flows against the merged code and found the logic consistent with the tests. The former process-global static WeakMap state in NodeDocumentation has been removed in favor of stateless set/move/prepend helpers.

Changes

Layer / File(s)	Summary
documentation attachment
src/NodeVisitor/DocumentationNodeVisitor.php	Adds a per-module stack of macro declarations so duplicate macros keep only the last documentation, clears superseded/mismatched declarations, and lowers the visitor priority to -512 so it runs before optimizations.
src/Parser.php	Preserves documentation on block references in child templates by moving it onto the registered block body before the reference is removed; guards with isset so unregistered block references no longer error, and simplifies set() call to drop the redundant end token.
tests
tests/ParserTest.php	Adds tests for unregistered block references, ignored inline tag documentation, node-visitor documentation access, and duplicate-macro documentation handling, plus supporting helper classes.
tests/TokenParser/BlockTokenParserTest.php	Adds a test asserting block documentation is preserved in child templates.
tests/Node/NodeTest.php	Tightens the documentation string assertion from assertEquals to assertSame.

[fabpot]

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]

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]

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

[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]

🔵 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]

📋 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]

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

[fabpot]

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

Yes, it was needed, but I've just rebased now that the PR has been merged, it's going to make reviewing easier.

[upsun-dispatch]

📋 Upsun Dispatch Review: full · 19 files reviewed · 🔵 1 info · 4 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.
• #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.

[upsun-dispatch]

📋 Upsun Dispatch Review: incremental · 5 files reviewed · no new issues · 5 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.
• #3659503446 — src/NodeVisitor/DocumentationNodeVisitor.php:38: Undocumented root-node assumption makes the visitor unsafe to reuse on subtrees.
• #3659503454 — src/NodeVisitor/DocumentationNodeVisitor.php:65: Stale AST references accumulate across compilations when traversal throws.
• #3682514990 — src/Node/NodeDocumentation.php:21: Misleading name invites incorrect assumptions if new call sites reuse it expecting replacement semantics.