Limit documentation comments to direct declarations

Author: fabpot

doc/documentation_comments.rst

@@ -48,9 +48,9 @@ closing marker or the symmetric ``##}`` marker:
 Documenting Variable Bindings
 -----------------------------
 
-Inside a tag or expression, an inline documentation comment starts with ``##``
-and continues until the end of the line. It describes the variable binding that
-starts on the next line.
+Inside a tag, an inline documentation comment starts with ``##`` and continues
+until the end of the line. It describes the variable binding that starts on the
+next line.
 
 Type Declarations
 ~~~~~~~~~~~~~~~~~
@@ -93,28 +93,6 @@ Each target in a multiple assignment can have its own documentation:
         = user.first_name, user.last_name
     %}
 
-The same syntax works with the assignment operator, including sequence and
-object destructuring:
-
-.. code-block:: twig
-
-    {{ (## The normalized result.
-        result = normalize(value)) }}
-
-    {{ ([
-        ## The first coordinate.
-        x,
-        ## The second coordinate.
-        y,
-    ] = coordinates) }}
-
-    {{ ({
-        name: ## The user's display name.
-            display_name,
-        ## The user's email address.
-        email,
-    } = user) }}
-
 Loop Targets
 ~~~~~~~~~~~~
 
@@ -145,32 +123,16 @@ Documentation comments can describe individual macro arguments:
         name,
         ## The initial field value.
         value = null,
-        ## Extra HTML attributes.
-        ...attributes,
     ) %}
         ...
     {% endmacro %}
 
-Arrow Function Arguments
-~~~~~~~~~~~~~~~~~~~~~~~~
-
-Documentation comments can also describe arrow function arguments:
-
-.. code-block:: twig
-
-    {% set formatter = (
-        ## The value to format.
-        value,
-        ## The requested locale.
-        locale
-    ) => value|format(locale) %}
-
 Attachment Rules
 ----------------
 
-A documentation comment applies to the next supported construct or variable
-binding. Consecutive documentation comments are combined and separated by
-newlines:
+A documentation comment is considered for the construct or variable binding
+that immediately follows it and attaches only when that position is supported.
+Consecutive documentation comments are combined and separated by newlines:
 
 .. code-block:: twig
 
@@ -189,9 +151,12 @@ construct or variable must therefore start on a later line:
         page = 1
     %}
 
-Documentation comments in unsupported positions remain comments and do not
-attach metadata. In particular, they do not document ordinary variable reads,
-mapping keys, function arguments or named call arguments.
+Documentation comments are attached on a best-effort basis where Twig can
+associate them directly with a construct or declaration. Comments in other
+positions remain regular comments and expose no metadata. In particular, they
+do not document ordinary variable reads, mapping keys, function arguments,
+named call arguments, assignment operators, destructuring assignments, arrow
+function arguments or variadic macro arguments.
 
 Reading Documentation from Nodes
 --------------------------------
@@ -207,5 +172,5 @@ metadata is stored on the semantic node represented by the source:
 * variable-binding documentation is stored on the node representing its
   assignment target.
 
-Documentation is discarded when an optimization or a node visitor replaces a
-supported node with an unsupported node.
+Documentation metadata belongs to its node and is not preserved when an
+optimization or a node visitor replaces that node.

src/ExpressionParser.php

@@ -24,7 +24,6 @@
 use Twig\Node\Expression\Variable\AssignContextVariable;
 use Twig\Node\Expression\Variable\ContextVariable;
 use Twig\Node\Node;
-use Twig\Node\NodeDocumentation;
 use Twig\Node\Nodes;
 
 /**
@@ -311,9 +310,7 @@ public function parseAssignmentExpression()
             } else {
                 $stream->expect(Token::NAME_TYPE, null, 'Only variables can be assigned to');
             }
-            $target = new AssignContextVariable($token->getValue(), $token->getLine());
-            NodeDocumentation::set($target, $token);
-            $targets[] = $target;
+            $targets[] = new AssignContextVariable($token->getValue(), $token->getLine());
 
             if (!$stream->nextIf(Token::PUNCTUATION_TYPE, ',')) {
                 break;

src/ExpressionParser/Infix/AssignmentExpressionParser.php

@@ -21,7 +21,6 @@
 use Twig\Node\Expression\Binary\SetBinary;
 use Twig\Node\Expression\Variable\AssignContextVariable;
 use Twig\Node\Expression\Variable\ContextVariable;
-use Twig\Node\NodeDocumentation;
 use Twig\Parser;
 use Twig\Token;
 
@@ -53,9 +52,7 @@ public function parse(Parser $parser, AbstractExpression $left, Token $token): A
         if ($left instanceof ArrayExpression) {
             foreach ($left->getKeyValuePairs() as $i => $pair) {
                 if ($pair['value'] instanceof ContextVariable && !$pair['value'] instanceof AssignContextVariable) {
-                    $target = new AssignContextVariable($pair['value']->getAttribute('name'), $pair['value']->getTemplateLine());
-                    NodeDocumentation::copy($pair['value'], $target);
-                    $left->setNode(2 * $i + 1, $target);
+                    $left->setNode(2 * $i + 1, new AssignContextVariable($pair['value']->getAttribute('name'), $pair['value']->getTemplateLine()));
                 }
             }
 

src/ExpressionParser/Prefix/GroupingExpressionParser.php

@@ -19,7 +19,6 @@
 use Twig\Node\Expression\ListExpression;
 use Twig\Node\Expression\Variable\AssignContextVariable;
 use Twig\Node\Expression\Variable\ContextVariable;
-use Twig\Node\NodeDocumentation;
 use Twig\Parser;
 use Twig\Token;
 
@@ -38,7 +37,7 @@ public function parse(Parser $parser, Token $token): AbstractExpression
                 return $expr->setExplicitParentheses();
             }
 
-            return new ListExpression([$this->toAssignContextVariable($expr)], $token->getLine());
+            return new ListExpression([self::toAssignContextVariable($expr)], $token->getLine());
         }
 
         // determine if we are parsing an arrow function arguments
@@ -53,32 +52,23 @@ public function parse(Parser $parser, Token $token): AbstractExpression
             }
             $stream->expect(Token::PUNCTUATION_TYPE, ',');
             $token = $stream->expect(Token::NAME_TYPE);
-            $name = new AssignContextVariable($token->getValue(), $token->getLine());
-            NodeDocumentation::set($name, $token);
-            $names[] = $name;
+            $names[] = new ContextVariable($token->getValue(), $token->getLine());
         }
 
         if (!$stream->test(Token::OPERATOR_TYPE, '=>')) {
             throw new SyntaxError('A list of variables must be followed by an arrow.', $stream->getCurrent()->getLine(), $stream->getSourceContext());
         }
 
-        return new ListExpression(array_map($this->toAssignContextVariable(...), $names), $token->getLine());
+        return new ListExpression(array_map(self::toAssignContextVariable(...), $names), $token->getLine());
     }
 
-    private function toAssignContextVariable(AbstractExpression $expr): AssignContextVariable
+    private static function toAssignContextVariable(AbstractExpression $expr): AssignContextVariable
     {
         if (!$expr instanceof ContextVariable) {
             throw new SyntaxError('A list must only contain variables.', $expr->getTemplateLine(), $expr->getSourceContext());
         }
 
-        if ($expr instanceof AssignContextVariable) {
-            return $expr;
-        }
-
-        $target = new AssignContextVariable($expr->getAttribute('name'), $expr->getTemplateLine());
-        NodeDocumentation::copy($expr, $target);
-
-        return $target;
+        return $expr instanceof AssignContextVariable ? $expr : new AssignContextVariable($expr->getAttribute('name'), $expr->getTemplateLine());
     }
 
     public function getName(): string

src/ExpressionParser/Prefix/LiteralExpressionParser.php

@@ -22,7 +22,6 @@
 use Twig\Node\Expression\ConstantExpression;
 use Twig\Node\Expression\EmptyExpression;
 use Twig\Node\Expression\Variable\ContextVariable;
-use Twig\Node\NodeDocumentation;
 use Twig\Parser;
 use Twig\Token;
 
@@ -209,7 +208,6 @@ private function parseMappingExpression(Parser $parser)
                 // {a} is a shortcut for {a:a}
                 if ($stream->test(Token::PUNCTUATION_TYPE, [',', '}'])) {
                     $value = new ContextVariable($key->getAttribute('value'), $key->getTemplateLine());
-                    NodeDocumentation::set($value, $token);
                     $node->addElement($value, $key);
                     continue;
                 }

src/Node/Expression/ArrowFunctionExpression.php

@@ -16,7 +16,6 @@
 use Twig\Node\Expression\Variable\AssignContextVariable;
 use Twig\Node\Expression\Variable\ContextVariable;
 use Twig\Node\Node;
-use Twig\Node\NodeDocumentation;
 
 /**
  * Represents an arrow function.
@@ -28,13 +27,7 @@ class ArrowFunctionExpression extends AbstractExpression
     public function __construct(AbstractExpression $expr, Node $names, $lineno)
     {
         if ($names instanceof ContextVariable) {
-            if (!$names instanceof AssignContextVariable) {
-                $target = new AssignContextVariable($names->getAttribute('name'), $names->getTemplateLine());
-                NodeDocumentation::copy($names, $target);
-                $names = $target;
-            }
-
-            $names = new ListExpression([$names], $lineno);
+            $names = new ListExpression([new AssignContextVariable($names->getAttribute('name'), $names->getTemplateLine())], $lineno);
         }
 
         if (!$names instanceof ListExpression) {

src/Node/Expression/Binary/SetBinary.php

@@ -16,7 +16,6 @@
 use Twig\Node\Expression\Variable\AssignContextVariable;
 use Twig\Node\Expression\Variable\ContextVariable;
 use Twig\Node\Node;
-use Twig\Node\NodeDocumentation;
 
 /**
  * @author Fabien Potencier <fabien@symfony.com>
@@ -33,11 +32,7 @@ public function __construct(Node $left, Node $right, int $lineno)
         if (!\is_string($name)) {
             throw new \LogicException('The "name" attribute must be a string.');
         }
-        if (!$left instanceof AssignContextVariable) {
-            $target = new AssignContextVariable($name, $left->getTemplateLine());
-            NodeDocumentation::copy($left, $target);
-            $left = $target;
-        }
+        $left = new AssignContextVariable($name, $left->getTemplateLine());
 
         parent::__construct($left, $right, $lineno);
     }

src/Node/MacroNode.php

@@ -42,7 +42,7 @@ class MacroNode extends Node
      * @param BodyNode        $body
      * @param ArrayExpression $arguments
      */
-    public function __construct(string $name, Node $body, Node $arguments, int $lineno, ?LocalVariable $variadic = null)
+    public function __construct(string $name, Node $body, Node $arguments, int $lineno, ?string $variadicName = null)
     {
         if (!$body instanceof BodyNode) {
             trigger_deprecation('twig/twig', '3.12', \sprintf('Not passing a "%s" instance as the "body" argument of the "%s" constructor is deprecated ("%s" given).', BodyNode::class, static::class, $body::class));
@@ -58,7 +58,6 @@ public function __construct(string $name, Node $body, Node $arguments, int $line
             $arguments = $args;
         }
 
-        $variadicName = null === $variadic ? null : $this->stripReservedPrefix($variadic->getAttribute('name'));
         $seen = [];
         foreach ($arguments->getKeyValuePairs() as $pair) {
             $argName = $pair['key']->getAttribute('name');
@@ -74,12 +73,7 @@ public function __construct(string $name, Node $body, Node $arguments, int $line
             $seen[$argName] = true;
         }
 
-        $nodes = ['body' => $body, 'arguments' => $arguments];
-        if (null !== $variadic) {
-            $nodes['variadic'] = $variadic;
-        }
-
-        parent::__construct($nodes, ['name' => $name], $lineno);
+        parent::__construct(['body' => $body, 'arguments' => $arguments], ['name' => $name, 'variadic_name' => $variadicName], $lineno);
     }
 
     public function compile(Compiler $compiler): void
@@ -99,14 +93,14 @@ public function compileMacroFactory(Compiler $compiler): void
         // "...$bucket" parameter and a context entry; a non-variadic macro must NOT emit
         // "...$varargs" anymore (so extra arguments raise an error), and the implicit
         // "varargs" context entry and VARARGS_NAME handling below go away.
-        $variadic = $this->hasNode('variadic') ? $this->getNode('variadic') : null;
-        if (null === $variadic) {
+        $variadicName = $this->getAttribute('variadic_name');
+        if (null === $variadicName) {
             // Legacy implicit "varargs" bucket: to be removed in 4.0.
             $bucketName = self::VARARGS_NAME;
             $bucketVar = self::VARARGS_NAME;
         } else {
-            $bucketVar = $variadic->getAttribute('name');
-            $bucketName = $this->stripReservedPrefix($bucketVar);
+            $bucketName = $variadicName;
+            $bucketVar = \in_array($variadicName, TempNameExpression::RESERVED_NAMES, true) ? TempNameExpression::RESERVED_NAME_PREFIX.$variadicName : $variadicName;
         }
 
         /** @var ArrayExpression $arguments */
@@ -173,7 +167,7 @@ public function compileMacroFactory(Compiler $compiler): void
 
         $compiler
             ->repr($signature)
-            ->raw(', '.(null !== $variadic ? 'true' : 'false').')')
+            ->raw(', '.(null !== $variadicName ? 'true' : 'false').')')
         ;
     }
 

src/Node/Node.php

@@ -142,19 +142,11 @@ public function getDocumentation(): ?string
     /**
      * @internal
      */
-    public function setDocumentation(string $documentation): void
+    public function setDocumentation(?string $documentation): void
     {
         $this->documentation = $documentation;
     }
 
-    /**
-     * @internal
-     */
-    public function removeDocumentation(): void
-    {
-        $this->documentation = null;
-    }
-
     /**
      * @internal
      */

src/Node/NodeDocumentation.php

@@ -35,18 +35,11 @@ public static function set(Node $node, Token ...$tokens): void
         }
     }
 
-    public static function copy(Node $source, Node $target): void
-    {
-        if (null !== $documentation = $source->getDocumentation()) {
-            self::prepend($target, $documentation);
-        }
-    }
-
     public static function move(Node $source, Node $target): void
     {
         if (null !== $documentation = $source->getDocumentation()) {
             self::prepend($target, $documentation);
-            $source->removeDocumentation();
+            $source->setDocumentation(null);
         }
     }