Check reserved names in TempNameExpression

src/ExpressionParser.php

@@ -538,7 +538,7 @@ class ExpressionParser
             return $node;
         }
 
-        $args = $this->parseArguments(true);
+        $args = $this->parseOnlyArguments();
         $function = $this->getFunction($name, $line);
 
         if ($function->getParserCallable()) {
@@ -660,7 +660,7 @@ class ExpressionParser
             if (!$this->parser->getStream()->test(Token::PUNCTUATION_TYPE, '(')) {
                 $arguments = new EmptyNode();
             } else {
-                $arguments = $this->parseArguments(true);
+                $arguments = $this->parseOnlyArguments();
             }
 
             $filter = $this->getFilter($token->getValue(), $token->getLine());
@@ -689,20 +689,24 @@ class ExpressionParser
     /**
      * Parses arguments.
      *
-     * @param bool $namedArguments Whether to allow named arguments or not
-     * @param bool $definition     Whether we are parsing arguments for a function (or macro) definition
-     *
      * @return Node
      *
      * @throws SyntaxError
      */
-    public function parseArguments($namedArguments = false, $definition = false)
+    public function parseArguments()
     {
+        $namedArguments = false;
+        $definition = false;
         if (func_num_args() > 2) {
             trigger_deprecation('twig/twig', '3.15', 'Passing a third argument ($allowArrow) to "%s()" is deprecated.', __METHOD__);
         }
-        if (!$namedArguments) {
-            trigger_deprecation('twig/twig', '3.15', 'Passing "false" for the first argument ($namedArguments) to "%s()" is deprecated.', __METHOD__);
+        if (func_num_args() > 1) {
+            trigger_deprecation('twig/twig', '3.15', 'Passing a second argument ($definition) to "%s()" is deprecated.', __METHOD__);
+            $definition = func_get_arg(1);
+        }
+        if (func_num_args() > 0) {
+            trigger_deprecation('twig/twig', '3.15', 'Passing a first argument ($namedArguments) to "%s()" is deprecated.', __METHOD__);
+            $namedArguments = func_get_arg(0);
         }
 
         $args = [];
@@ -819,7 +823,7 @@ class ExpressionParser
 
         $arguments = null;
         if ($stream->test(Token::PUNCTUATION_TYPE, '(')) {
-            $arguments = $this->parseArguments(true);
+            $arguments = $this->parseOnlyArguments();
         } elseif ($test->hasOneMandatoryArgument()) {
             $arguments = new Nodes([0 => $this->getPrimary()]);
         }
@@ -917,6 +921,7 @@ class ExpressionParser
     }
 
     // checks that the node only contains "constant" elements
+    // to be removed in 4.0
     private function checkConstantExpression(Node $node): bool
     {
         if (!($node instanceof ConstantExpression || $node instanceof ArrayExpression
@@ -945,10 +950,55 @@ class ExpressionParser
     private function createArguments(int $line): ArrayExpression
     {
         $arguments = new ArrayExpression([], $line);
-        foreach ($this->parseArguments(true) as $k => $n) {
+        foreach ($this->parseOnlyArguments() as $k => $n) {
             $arguments->addElement($n, new TempNameExpression($k, $line));
         }
 
         return $arguments;
     }
+
+    public function parseOnlyArguments()
+    {
+        $args = [];
+        $stream = $this->parser->getStream();
+        $stream->expect(Token::PUNCTUATION_TYPE, '(', 'A list of arguments must begin with an opening parenthesis');
+        $hasSpread = false;
+        while (!$stream->test(Token::PUNCTUATION_TYPE, ')')) {
+            if ($args) {
+                $stream->expect(Token::PUNCTUATION_TYPE, ',', 'Arguments must be separated by a comma');
+
+                // if the comma above was a trailing comma, early exit the argument parse loop
+                if ($stream->test(Token::PUNCTUATION_TYPE, ')')) {
+                    break;
+                }
+            }
+
+            if ($stream->nextIf(Token::SPREAD_TYPE)) {
+                $hasSpread = true;
+                $value = new SpreadUnary($this->parseExpression(), $stream->getCurrent()->getLine());
+            } elseif ($hasSpread) {
+                throw new SyntaxError('Normal arguments must be placed before argument unpacking.', $stream->getCurrent()->getLine(), $stream->getSourceContext());
+            } else {
+                $value = $this->parseExpression();
+            }
+
+            $name = null;
+            if (($token = $stream->nextIf(Token::OPERATOR_TYPE, '=')) || ($token = $stream->nextIf(Token::PUNCTUATION_TYPE, ':'))) {
+                if (!$value instanceof NameExpression) {
+                    throw new SyntaxError(\sprintf('A parameter name must be a string, "%s" given.', \get_class($value)), $token->getLine(), $stream->getSourceContext());
+                }
+                $name = $value->getAttribute('name');
+                $value = $this->parseExpression();
+            }
+
+            if (null === $name) {
+                $args[] = $value;
+            } else {
+                $args[$name] = $value;
+            }
+        }
+        $stream->expect(Token::PUNCTUATION_TYPE, ')', 'A list of arguments must be closed by a parenthesis');
+
+        return new Nodes($args);
+    }
 }

src/Node/Expression/TempNameExpression.php

@@ -12,6 +12,7 @@
 namespace Twig\Node\Expression;
 
 use Twig\Compiler;
+use Twig\Error\SyntaxError;
 
 class TempNameExpression extends AbstractExpression
 {
@@ -19,6 +20,11 @@ class TempNameExpression extends AbstractExpression
 
     public function __construct(string|int $name, int $lineno)
     {
+        // All names supported by ExpressionParser::parsePrimaryExpression() should be excluded
+        if (\in_array(strtolower($name), ['true', 'false', 'none', 'null'])) {
+            throw new SyntaxError(\sprintf('You cannot assign a value to "%s".', $name), $lineno);
+        }
+
         if (is_int($name) || ctype_digit($name)) {
             $name = (int) $name;
         } elseif (in_array($name, self::RESERVED_NAMES)) {

src/Node/MacroNode.php

@@ -14,6 +14,7 @@ namespace Twig\Node;
 use Twig\Attribute\YieldReady;
 use Twig\Compiler;
 use Twig\Error\SyntaxError;
+use Twig\Node\Expression\ArrayExpression;
 use Twig\Node\Expression\TempNameExpression;
 
 /**
@@ -27,7 +28,8 @@ class MacroNode extends Node
     public const VARARGS_NAME = 'varargs';
 
     /**
-     * @param BodyNode $body
+     * @param BodyNode        $body
+     * @param ArrayExpression $arguments
      */
     public function __construct(string $name, Node $body, Node $arguments, int $lineno)
     {
@@ -35,13 +37,19 @@ class MacroNode extends Node
             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));
         }
 
-        foreach ($arguments as $argumentName => $argument) {
-            if (self::VARARGS_NAME === $argumentName) {
-                throw new SyntaxError(\sprintf('The argument "%s" in macro "%s" cannot be defined because the variable "%s" is reserved for arbitrary arguments.', self::VARARGS_NAME, $name, self::VARARGS_NAME), $argument->getTemplateLine(), $argument->getSourceContext());
+        if (!$arguments instanceof ArrayExpression) {
+            trigger_deprecation('twig/twig', '3.15', \sprintf('Not passing a "%s" instance as the "arguments" argument of the "%s" constructor is deprecated ("%s" given).', ArrayExpression::class, static::class, $arguments::class));
+
+            $args = new ArrayExpression([], $arguments->getTemplateLine());
+            foreach ($arguments as $name => $default) {
+                $args->addElement($default, new TempNameExpression($name, $default->getTemplateLine()));
             }
-            if (in_array($argumentName, TempNameExpression::RESERVED_NAMES)) {
-                $arguments->setNode('_'.$argumentName.'_', $argument);
-                $arguments->removeNode($argumentName);
+            $arguments = $args;
+        }
+
+        foreach ($arguments->getKeyValuePairs() as $pair) {
+            if ('_'.self::VARARGS_NAME.'_' === $pair['key']->getAttribute('name')) {
+                throw new SyntaxError(\sprintf('The argument "%s" in macro "%s" cannot be defined because the variable "%s" is reserved for arbitrary arguments.', self::VARARGS_NAME, $name, self::VARARGS_NAME), $pair['value']->getTemplateLine(), $pair['value']->getSourceContext());
             }
         }
 
@@ -55,21 +63,15 @@ class MacroNode extends Node
             ->write(\sprintf('public function macro_%s(', $this->getAttribute('name')))
         ;
 
-        $count = \count($this->getNode('arguments'));
-        $pos = 0;
-        foreach ($this->getNode('arguments') as $name => $default) {
+        foreach ($this->getNode('arguments')->getKeyValuePairs() as $pair) {
+            $name = $pair['key'];
+            $default = $pair['value'];
             $compiler
-                ->raw('$'.$name.' = ')
+                ->subcompile($name)
+                ->raw(' = ')
                 ->subcompile($default)
+                ->raw(', ')
             ;
-
-            if (++$pos < $count) {
-                $compiler->raw(', ');
-            }
-        }
-
-        if ($count) {
-            $compiler->raw(', ');
         }
 
         $compiler
@@ -82,11 +84,13 @@ class MacroNode extends Node
             ->indent()
         ;
 
-        foreach ($this->getNode('arguments') as $name => $default) {
+        foreach ($this->getNode('arguments')->getKeyValuePairs() as $pair) {
+            $name = $pair['key'];
             $compiler
                 ->write('')
-                ->string(trim($name, '_'))
-                ->raw(' => $'.$name)
+                ->string(trim($name->getAttribute('name'), '_'))
+                ->raw(' => ')
+                ->subcompile($name)
                 ->raw(",\n")
             ;
         }

src/TokenParser/MacroTokenParser.php

@@ -14,8 +14,15 @@ namespace Twig\TokenParser;
 use Twig\Error\SyntaxError;
 use Twig\Node\BodyNode;
 use Twig\Node\EmptyNode;
+use Twig\Node\Expression\ArrayExpression;
+use Twig\Node\Expression\ConstantExpression;
+use Twig\Node\Expression\NameExpression;
+use Twig\Node\Expression\TempNameExpression;
+use Twig\Node\Expression\Unary\NegUnary;
+use Twig\Node\Expression\Unary\PosUnary;
 use Twig\Node\MacroNode;
 use Twig\Node\Node;
+use Twig\Node\Nodes;
 use Twig\Token;
 
 /**
@@ -34,8 +41,7 @@ final class MacroTokenParser extends AbstractTokenParser
         $lineno = $token->getLine();
         $stream = $this->parser->getStream();
         $name = $stream->expect(Token::NAME_TYPE)->getValue();
-
-        $arguments = $this->parser->getExpressionParser()->parseArguments(true, true);
+        $arguments = $this->parseDefinition();
 
         $stream->expect(Token::BLOCK_END_TYPE);
         $this->parser->pushLocalScope();
@@ -64,4 +70,56 @@ final class MacroTokenParser extends AbstractTokenParser
     {
         return 'macro';
     }
+
+    private function parseDefinition(): ArrayExpression
+    {
+        $arguments = new ArrayExpression([], $this->parser->getCurrentToken()->getLine());
+        $stream = $this->parser->getStream();
+        $stream->expect(Token::PUNCTUATION_TYPE, '(', 'A list of arguments must begin with an opening parenthesis');
+        while (!$stream->test(Token::PUNCTUATION_TYPE, ')')) {
+            if (count($arguments)) {
+                $stream->expect(Token::PUNCTUATION_TYPE, ',', 'Arguments must be separated by a comma');
+
+                // if the comma above was a trailing comma, early exit the argument parse loop
+                if ($stream->test(Token::PUNCTUATION_TYPE, ')')) {
+                    break;
+                }
+            }
+
+            $token = $stream->expect(Token::NAME_TYPE, null, 'An argument must be a name');
+            $name = new TempNameExpression($token->getValue(), $this->parser->getCurrentToken()->getLine());
+            if ($token = $stream->nextIf(Token::OPERATOR_TYPE, '=')) {
+                $default = $this->parser->getExpressionParser()->parseExpression();
+            } else {
+                $default = new ConstantExpression(null, $this->parser->getCurrentToken()->getLine());
+                $default->setAttribute('is_implicit', true);
+            }
+
+            if (!$this->checkConstantExpression($default)) {
+                throw new SyntaxError('A default value for an argument must be a constant (a boolean, a string, a number, a sequence, or a mapping).', $token->getLine(), $stream->getSourceContext());
+            }
+            $arguments->addElement($default, $name);
+        }
+        $stream->expect(Token::PUNCTUATION_TYPE, ')', 'A list of arguments must be closed by a parenthesis');
+
+        return $arguments;
+    }
+
+    // checks that the node only contains "constant" elements
+    private function checkConstantExpression(Node $node): bool
+    {
+        if (!($node instanceof ConstantExpression || $node instanceof ArrayExpression
+            || $node instanceof NegUnary || $node instanceof PosUnary
+        )) {
+            return false;
+        }
+
+        foreach ($node as $n) {
+            if (!$this->checkConstantExpression($n)) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }

tests/Fixtures/tags/macro/argument_reserved_names.test

@@ -0,0 +1,12 @@
+--TEST--
+"macro" tag
+--TEMPLATE--
+{% import _self as macros %}
+
+{% macro input(true, false, null) %}
+    {{ true }}
+{% endmacro %}
+--DATA--
+return []
+--EXCEPTION--
+Twig\Error\SyntaxError: You cannot assign a value to "true" in "index.twig" at line 4.

tests/Node/MacroTest.php

@@ -14,8 +14,10 @@ namespace Twig\Tests\Node;
 use Twig\Environment;
 use Twig\Loader\ArrayLoader;
 use Twig\Node\BodyNode;
+use Twig\Node\Expression\ArrayExpression;
 use Twig\Node\Expression\ConstantExpression;
 use Twig\Node\Expression\NameExpression;
+use Twig\Node\Expression\TempNameExpression;
 use Twig\Node\MacroNode;
 use Twig\Node\Nodes;
 use Twig\Node\TextNode;
@@ -26,7 +28,7 @@ class MacroTest extends NodeTestCase
     public function testConstructor()
     {
         $body = new BodyNode([new TextNode('foo', 1)]);
-        $arguments = new Nodes([new NameExpression('foo', 1)], 1);
+        $arguments = new ArrayExpression([new NameExpression('foo', 1), new ConstantExpression(null, 1)], 1);
         $node = new MacroNode('foo', $body, $arguments, 1);
 
         $this->assertEquals($body, $node->getNode('body'));
@@ -36,9 +38,11 @@ class MacroTest extends NodeTestCase
 
     public static function provideTests(): iterable
     {
-        $arguments = new Nodes([
-            'foo' => new ConstantExpression(null, 1),
-            'bar' => new ConstantExpression('Foo', 1),
+        $arguments = new ArrayExpression([
+            new TempNameExpression('foo', 1),
+            new ConstantExpression(null, 1),
+            new TempNameExpression('bar', 1),
+            new ConstantExpression('Foo', 1),
         ], 1);
 
         $body = new BodyNode([new TextNode('foo', 1)]);

tests/ParserTest.php

@@ -194,12 +194,11 @@ EOF
             ->getNode('arguments')
         ;
 
-        $this->assertTrue($argumentNodes->getNode('po')->hasAttribute('is_implicit'));
-        $this->assertTrue($argumentNodes->getNode('po')->getAttribute('is_implicit'));
-        $this->assertNull($argumentNodes->getNode('po')->getAttribute('value'));
+        $this->assertTrue($argumentNodes->getNode(1)->hasAttribute('is_implicit'));
+        $this->assertNull($argumentNodes->getNode(1)->getAttribute('value'));
 
-        $this->assertFalse($argumentNodes->getNode('lo')->hasAttribute('is_implicit'));
-        $this->assertTrue($argumentNodes->getNode('lo')->getAttribute('value'));
+        $this->assertFalse($argumentNodes->getNode(3)->hasAttribute('is_implicit'));
+        $this->assertTrue($argumentNodes->getNode(3)->getAttribute('value'));
     }
 
     protected function getParser()