Redesign macro calls and argument handling

Author: fabpot

CHANGELOG

@@ -3,6 +3,13 @@
  * Render backed enums using their backing value in the `html_attr` function
  * Fix empty Markup values being treated as truthy in and, or, xor, not, ternary, and elvis expressions
  * Fix a PHP 8.5 `chr()` deprecation when decoding an octal string escape sequence larger than `\377` (such as `"\777"`)
+ * Deprecate calling a macro with a name whose case differs from its definition; macro names will be case-sensitive in 4.0
+ * Deprecate calling a macro without a value for an argument that has no default value; the argument will be required in 4.0
+ * Deprecate passing extra or unknown arguments to a macro that does not declare a variadic argument; it will throw in 4.0
+ * Add support for declaring an explicit variadic macro argument (`{% macro foo(a, ...rest) %}`)
+ * Compile macros as closures stored in a per-template registry instead of `macro_`-prefixed PHP methods
+ * Mark `Twig\Node\MacroNode` as `@final`; it will be final in Twig 4.0
+ * Change `MacroReferenceExpression` to take the bare macro name instead of a `macro_`-prefixed method name
  * Mark `Twig\Markup` as `@final`; it will be final in Twig 4.0
  * Reduce memory usage and speed up the context restoration compiled at the end of `for` loops
  * Allow calling a macro with a dynamic name via the dot operator (`macros.(name)(args)`)

doc/deprecated.rst

@@ -11,6 +11,9 @@ Classes
 * The ``Twig\Markup`` class is considered final as of Twig 3.28 and will be
   final in Twig 4.0. Use ``Twig\Markup`` directly instead of extending it.
 
+* The ``Twig\Node\MacroNode`` class is considered final as of Twig 3.28 and
+  will be final in Twig 4.0.
+
 Functions
 ---------
 
@@ -306,6 +309,26 @@ Templates
   deprecated as of Twig 3.27 and will throw in Twig 4.0. These tags have a
   global effect on the template and must be declared at the root of its body.
 
+Macros
+------
+
+* Passing more arguments to a macro than it declares is deprecated as of Twig
+  3.28 and will throw in Twig 4.0. Declare an explicit variadic argument
+  (``{% macro foo(a, ...rest) %}``) to accept extra positional and named
+  arguments instead of relying on the implicit ``varargs`` variable.
+
+* Passing an unknown named argument to a macro is deprecated as of Twig 3.28 and
+  will throw in Twig 4.0. Declare an explicit variadic argument to accept it.
+
+* Calling a macro without a value for an argument that has no default value is
+  deprecated as of Twig 3.28; such an argument will be required in Twig 4.0
+  (today it silently defaults to ``null``). To keep an argument optional, give
+  it an explicit default value (e.g. ``{% macro input(name, value = null) %}``).
+
+* Calling a macro with a name whose case differs from its definition (e.g.
+  calling ``input`` as ``INPUT``) is deprecated as of Twig 3.28; macro names
+  will be case-sensitive in Twig 4.0. Use the name exactly as defined.
+
 Filters
 -------
 

doc/tags/macro.rst

@@ -11,26 +11,62 @@ via macros (called ``forms.twig``):
 
 .. code-block:: html+twig
 
-    {% macro input(name, value, type = "text", size = 20) %}
-        <input type="{{ type }}" name="{{ name }}" value="{{ value|e }}" size="{{ size }}"/>
+    {% macro input(name, value = "", type = "text", size = 20) %}
+        <input
+            type="{{ type }}"
+            name="{{ name }}"
+            value="{{ value|e }}"
+            size="{{ size }}"
+        />
     {% endmacro %}
 
-    {% macro textarea(name, value, rows = 10, cols = 40) %}
-        <textarea name="{{ name }}" rows="{{ rows }}" cols="{{ cols }}">{{ value|e }}</textarea>
+    {% macro textarea(name, value = "", rows = 10, cols = 40) %}
+        <textarea
+            name="{{ name }}"
+            rows="{{ rows }}"
+            cols="{{ cols }}"
+        >{{ value|e }}</textarea>
     {% endmacro %}
 
-Each macro argument can have a default value (here ``text`` is the default value
+A macro argument can have a default value (here ``text`` is the default value
 for ``type`` if not provided in the call).
 
-Macros differ from native PHP functions in a few ways:
+As with PHP function arguments, a macro argument is required unless it declares
+a default value. Here, ``name`` is required while ``value``, ``type``, and
+``size`` are optional.
 
-* Arguments of a macro are always optional.
+.. deprecated:: 3.28
 
-* If extra positional arguments are passed to a macro, they end up in the
-  special ``varargs`` variable as a list of values.
+    Calling a macro without a value for an argument that has no default value is
+    deprecated as of Twig 3.28; the argument will be required in Twig 4.0 (until
+    then, it defaults to ``null``). Give every optional argument an explicit
+    default value.
 
-But as with PHP functions, macros don't have access to the current template
-variables.
+To accept an arbitrary number of extra arguments, declare an explicit variadic
+argument as described below.
+
+Note that macros don't have access to the current template variables.
+
+A macro can declare an explicit variadic argument to collect any extra
+positional and named arguments into a named variable, using the same ``...``
+notation as PHP:
+
+.. code-block:: html+twig
+
+    {% macro tag(element, ...attributes) %}
+        <{{ element }}
+        {%- for key, value in attributes %} {{ key }}="{{ value }}"{% endfor -%}
+        >
+    {% endmacro %}
+
+    {{ _self.tag("input", type: "text", name: "username") }}
+
+The variadic argument must be the last one and cannot have a default value.
+
+.. versionadded:: 3.28
+
+    Support for declaring an explicit variadic macro argument was added in Twig
+    3.28.
 
 .. tip::
 
@@ -110,8 +146,13 @@ via the ``from`` tag:
 
         <p>{{ _self.input('password', '', 'password') }}</p>
 
-        {% macro input(name, value, type = "text", size = 20) %}
-            <input type="{{ type }}" name="{{ name }}" value="{{ value|e }}" size="{{ size }}"/>
+        {% macro input(name, value = "", type = "text", size = 20) %}
+            <input
+                type="{{ type }}"
+                name="{{ name }}"
+                value="{{ value|e }}"
+                size="{{ size }}"
+            />
         {% endmacro %}
 
 Macros Scoping
@@ -162,3 +203,16 @@ readability (the name after the ``endmacro`` word must match the macro name):
     {% macro input() %}
         ...
     {% endmacro input %}
+
+Deprecating a Macro
+-------------------
+
+Use the :doc:`deprecated <deprecated>` tag at the top of a macro to deprecate
+it; a deprecation notice is triggered whenever the macro is called:
+
+.. code-block:: html+twig
+
+    {% macro input(name, value = "") %}
+        {% deprecated 'The "input" macro is deprecated, use "field" instead.' %}
+        <input name="{{ name }}" value="{{ value|e }}"/>
+    {% endmacro %}

src/ExpressionParser/Infix/ArgumentsTrait.php

@@ -14,6 +14,7 @@
 use Twig\Error\SyntaxError;
 use Twig\Node\Expression\ArrayExpression;
 use Twig\Node\Expression\Binary\SetBinary;
+use Twig\Node\Expression\ConstantExpression;
 use Twig\Node\Expression\Unary\SpreadUnary;
 use Twig\Node\Expression\Variable\ContextVariable;
 use Twig\Node\Expression\Variable\LocalVariable;
@@ -23,11 +24,11 @@
 
 trait ArgumentsTrait
 {
-    private function parseCallableArguments(Parser $parser, int $line, bool $parseOpenParenthesis = true): ArrayExpression
+    private function parseCallableArguments(Parser $parser, int $line, bool $parseOpenParenthesis = true, bool $preserveNames = false): ArrayExpression
     {
         $arguments = new ArrayExpression([], $line);
         foreach ($this->parseNamedArguments($parser, $parseOpenParenthesis) as $k => $n) {
-            $arguments->addElement($n, new LocalVariable($k, $line));
+            $arguments->addElement($n, \is_int($k) || !$preserveNames ? new LocalVariable($k, $line) : new ConstantExpression($k, $line));
         }
 
         return $arguments;

src/ExpressionParser/Infix/DotExpressionParser.php

@@ -60,24 +60,24 @@ public function parse(Parser $parser, AbstractExpression $expr, Token $token): A
             }
         }
 
-        if ($stream->test(Token::OPERATOR_TYPE, '(')) {
-            $type = Template::METHOD_CALL;
-            $arguments = $this->parseCallableArguments($parser, $token->getLine());
-        }
-
         $isMacroTarget = $expr instanceof NameExpression
             && (
                 null !== $parser->getImportedSymbol('template', $expr->getAttribute('name'))
                 || '_self' === $expr->getAttribute('name')
             );
 
+        if ($stream->test(Token::OPERATOR_TYPE, '(')) {
+            $type = Template::METHOD_CALL;
+            $arguments = $this->parseCallableArguments($parser, $token->getLine(), preserveNames: $isMacroTarget);
+        }
+
         if (
             $isMacroTarget
             && $attribute instanceof ConstantExpression
             && \is_string($name = $attribute->getAttribute('value'))
             && preg_match('#^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$#D', $name)
         ) {
-            return new MacroReferenceExpression(new TemplateVariable($expr->getAttribute('name'), $expr->getTemplateLine()), 'macro_'.$name, $arguments, $expr->getTemplateLine());
+            return new MacroReferenceExpression(new TemplateVariable($expr->getAttribute('name'), $expr->getTemplateLine()), $name, $arguments, $expr->getTemplateLine());
         }
 
         if ($isMacroTarget && !$attribute instanceof ConstantExpression) {

src/ExpressionParser/Infix/FunctionExpressionParser.php

@@ -42,8 +42,12 @@ public function parse(Parser $parser, AbstractExpression $expr, Token $token): A
 
         $name = $expr->getAttribute('name');
 
+        // To be removed in 4.0 ("from -> import" unification, see FromTokenParser):
+        // a bare call to a macro imported via "from" is resolved here through the
+        // dedicated "function" imported symbol. In 4.0 this branch goes away once
+        // "from" registers a regular "template" import symbol like "import" does.
         if (null !== $alias = $parser->getImportedSymbol('function', $name)) {
-            return new MacroReferenceExpression($alias['node']->getNode('var'), $alias['name'], $this->parseCallableArguments($parser, $line, false), $line);
+            return new MacroReferenceExpression($alias['node']->getNode('var'), $alias['name'], $this->parseCallableArguments($parser, $line, false, true), $line);
         }
 
         $args = $this->parseNamedArguments($parser, false);

src/Extension/CoreExtension.php

@@ -1360,18 +1360,7 @@ public static function capitalize(string $charset, $string): string
      */
     public static function callMacro(Template $template, string $method, array $args, int $lineno, array $context, Source $source)
     {
-        if (!method_exists($template, $method)) {
-            $parent = $template;
-            while ($parent = $parent->getParent($context)) {
-                if (method_exists($parent, $method)) {
-                    return $parent->$method(...$args);
-                }
-            }
-
-            throw new RuntimeError(\sprintf('Macro "%s" is not defined in template "%s".', substr($method, \strlen('macro_')), $template->getTemplateName()), $lineno, $source);
-        }
-
-        return $template->$method(...$args);
+        return $template->callMacro(substr($method, \strlen('macro_')), $args, $context, $lineno, $source);
     }
 
     /**

src/Node/Expression/MacroReferenceExpression.php

@@ -26,23 +26,15 @@ class MacroReferenceExpression extends AbstractExpression implements SupportDefi
     use SupportDefinedTestTrait;
 
     /**
-     * @param string|AbstractExpression $name A static macro method name (e.g. "macro_foo") or, for a dynamic
-     *                                        call, an expression resolving to the macro name (without the
-     *                                        "macro_" prefix, which is added at runtime)
+     * @param string|AbstractExpression $name The bare macro name (a static identifier) or, for a dynamic
+     *                                        call, an expression resolving to the macro name
      */
     public function __construct(TemplateVariable $template, string|AbstractExpression $name, AbstractExpression $arguments, int $lineno)
     {
         $nodes = ['template' => $template, 'arguments' => $arguments];
         $attributes = ['name' => null];
 
         if (\is_string($name)) {
-            // The name is emitted as raw PHP in compile() via "->{$name}(...)",
-            // so it must be a valid PHP method identifier. Reject anything else
-            // as a defense-in-depth against accidental PHP code injection from
-            // a caller that forgot to validate user-controlled input.
-            if (!preg_match('#^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$#D', $name)) {
-                throw new \LogicException(\sprintf('Macro name "%s" is not a valid PHP identifier.', $name));
-            }
             $attributes['name'] = $name;
         } else {
             $nodes['name'] = $name;
@@ -63,74 +55,41 @@ public function __clone()
 
     public function compile(Compiler $compiler): void
     {
-        if ($this->hasNode('name')) {
-            $this->compileDynamic($compiler);
-
-            return;
-        }
+        $compiler->subcompile($this->getNode('template'));
 
         if ($this->definedTest) {
-            $compiler
-                ->subcompile($this->getNode('template'))
-                ->raw('->hasMacro(')
-                ->repr($this->getAttribute('name'))
-                ->raw(', $context')
-                ->raw(')')
-            ;
+            $compiler->raw('->hasMacro(');
+            $this->compileName($compiler);
+            $compiler->raw(', $context)');
 
             return;
         }
 
+        $compiler->raw('->callMacro(');
+        $this->compileName($compiler);
         $compiler
-            ->subcompile($this->getNode('template'))
-            ->raw('->getTemplateForMacro(')
-            ->repr($this->getAttribute('name'))
+            ->raw(', ')
+            ->subcompile($this->getNode('arguments'))
             ->raw(', $context, ')
             ->repr($this->getTemplateLine())
             ->raw(', $this->getSourceContext())')
-            ->raw(\sprintf('->%s', $this->getAttribute('name')))
-            ->raw('(...')
-            ->subcompile($this->getNode('arguments'))
-            ->raw(')')
         ;
     }
 
     public function getStringCoercedChildNames(): array
     {
-        // Dynamic macro names are prefixed via PHP string concatenation at runtime.
+        // Dynamic macro names are string-coerced at runtime.
         return $this->hasNode('name') ? ['name'] : [];
     }
 
-    private function compileDynamic(Compiler $compiler): void
+    private function compileName(Compiler $compiler): void
     {
-        // The macro method name is resolved at runtime from a context value;
-        // prefixing it with "macro_" constrains the dynamic method call to the
-        // template's macro methods only, and getTemplateForMacro()/hasMacro()
-        // validate that the method actually exists.
-        $var = $compiler->getVarName();
-
-        if ($this->definedTest) {
-            $compiler
-                ->subcompile($this->getNode('template'))
-                ->raw('->hasMacro(\'macro_\'.')
-                ->subcompile($this->getNode('name'))
-                ->raw(', $context)')
-            ;
-
-            return;
+        // A dynamic macro name is resolved at runtime from a context value and
+        // string-coerced before the registry lookup.
+        if ($this->hasNode('name')) {
+            $compiler->raw('(string) ')->subcompile($this->getNode('name'));
+        } else {
+            $compiler->repr($this->getAttribute('name'));
         }
-
-        $compiler
-            ->subcompile($this->getNode('template'))
-            ->raw(\sprintf('->getTemplateForMacro($%s = \'macro_\'.', $var))
-            ->subcompile($this->getNode('name'))
-            ->raw(', $context, ')
-            ->repr($this->getTemplateLine())
-            ->raw(', $this->getSourceContext())')
-            ->raw(\sprintf('->{$%s}', $var))
-            ->raw('(...')
-            ->subcompile($this->getNode('arguments'))
-            ->raw(')')
-        ;
     }
 }

src/Node/Expression/MethodCallExpression.php

@@ -34,11 +34,11 @@ public function compile(Compiler $compiler): void
     {
         if ($this->definedTest) {
             $compiler
-                ->raw('method_exists($macros[')
+                ->raw('$macros[')
                 ->repr($this->getNode('node')->getAttribute('name'))
-                ->raw('], ')
-                ->repr($this->getAttribute('method'))
-                ->raw(')')
+                ->raw(']->hasMacro(')
+                ->repr(substr($this->getAttribute('method'), \strlen('macro_')))
+                ->raw(', $context)')
             ;
 
             return;

src/Node/Expression/TempNameExpression.php

@@ -16,6 +16,9 @@
 
 class TempNameExpression extends AbstractExpression
 {
+    // 4.0: re-evaluate "varargs" here once the implicit macro varargs bucket is removed
+    // (see MacroNode::VARARGS_NAME); the other names map to compiled variables ($context,
+    // $macros, $blocks, $this) and must stay.
     public const RESERVED_NAMES = ['varargs', 'context', 'macros', 'blocks', 'this'];
 
     public function __construct(string|int|null $name, int $lineno)