feature #4482 Enforce AbstractBinary for all binary operators (fabpot)

This PR was merged into the 3.x branch. Discussion ---------- Enforce AbstractBinary for all binary operators Commits ------- 4f8ba93 Enforce AbstractBinary for all binary operators
twigphp · Nov 29, 2024 · d1d8e3c · d1d8e3c
2 parents 4e1cbc7 + 4f8ba93
commit d1d8e3c
Show file tree

Hide file tree

Showing 20 changed files with 351 additions and 47 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -4,6 +4,7 @@
 
 # 3.16.0 (2024-11-29)
 
+ * Deprecate `ConditionalExpression` and `NullCoalesceExpression` (use `ConditionalTernary` and `NullCoalesceBinary` instead)
  * Deprecate `InlinePrint`
  * Fix having macro variables starting with an underscore
  * Deprecate not passing a `Source` instance to `TokenStream`

diff --git a/doc/deprecated.rst b/doc/deprecated.rst
@@ -189,7 +189,15 @@ Nodes
   made ready for ``yield``; the ``use_yield`` Environment option can be turned
   on when all nodes use the ``#[\Twig\Attribute\YieldReady]`` attribute.
 
- * The ``InlinePrint`` class is deprecated as of Twig 3.16 with no replacement.
+ * The ``Twig\Node\InlinePrint`` class is deprecated as of Twig 3.16 with no
+   replacement.
+
+ * The ``Twig\Node\Expression\NullCoalesceExpression`` class is deprecated as
+   of Twig 3.16, use ``Twig\Node\Expression\Binary\NullCoalesceBinary``
+   instead.
+
+ * The ``Twig\Node\Expression\ConditionalExpression`` class is deprecated as of
+   Twig 3.16, use ``Twig\Node\Expression\Ternary\ConditionalTernary`` instead.
 
 Node Visitors
 -------------

diff --git a/src/ExpressionParser.php b/src/ExpressionParser.php
@@ -20,11 +20,11 @@
 use Twig\Node\Expression\ArrowFunctionExpression;
 use Twig\Node\Expression\Binary\AbstractBinary;
 use Twig\Node\Expression\Binary\ConcatBinary;
-use Twig\Node\Expression\ConditionalExpression;
 use Twig\Node\Expression\ConstantExpression;
 use Twig\Node\Expression\GetAttrExpression;
 use Twig\Node\Expression\MacroReferenceExpression;
 use Twig\Node\Expression\NameExpression;
+use Twig\Node\Expression\Ternary\ConditionalTernary;
 use Twig\Node\Expression\TestExpression;
 use Twig\Node\Expression\Unary\AbstractUnary;
 use Twig\Node\Expression\Unary\NegUnary;
@@ -269,22 +269,16 @@ private function getPrimary(): AbstractExpression
     private function parseConditionalExpression($expr): AbstractExpression
     {
         while ($this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, '?')) {
-            if (!$this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, ':')) {
-                $expr2 = $this->parseExpression();
-                if ($this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, ':')) {
-                    // Ternary operator (expr ? expr2 : expr3)
-                    $expr3 = $this->parseExpression();
-                } else {
-                    // Ternary without else (expr ? expr2)
-                    $expr3 = new ConstantExpression('', $this->parser->getCurrentToken()->getLine());
-                }
-            } else {
-                // Ternary without then (expr ?: expr3)
-                $expr2 = $expr;
+            $expr2 = $this->parseExpression();
+            if ($this->parser->getStream()->nextIf(Token::PUNCTUATION_TYPE, ':')) {
+                // Ternary operator (expr ? expr2 : expr3)
                 $expr3 = $this->parseExpression();
+            } else {
+                // Ternary without else (expr ? expr2)
+                $expr3 = new ConstantExpression('', $this->parser->getCurrentToken()->getLine());
             }
 
-            $expr = new ConditionalExpression($expr, $expr2, $expr3, $this->parser->getCurrentToken()->getLine());
+            $expr = new ConditionalTernary($expr, $expr2, $expr3, $this->parser->getCurrentToken()->getLine());
         }
 
         return $expr;

diff --git a/src/Extension/CoreExtension.php b/src/Extension/CoreExtension.php
@@ -26,6 +26,7 @@
 use Twig\Node\Expression\Binary\BitwiseXorBinary;
 use Twig\Node\Expression\Binary\ConcatBinary;
 use Twig\Node\Expression\Binary\DivBinary;
+use Twig\Node\Expression\Binary\ElvisBinary;
 use Twig\Node\Expression\Binary\EndsWithBinary;
 use Twig\Node\Expression\Binary\EqualBinary;
 use Twig\Node\Expression\Binary\FloorDivBinary;
@@ -41,6 +42,7 @@
 use Twig\Node\Expression\Binary\MulBinary;
 use Twig\Node\Expression\Binary\NotEqualBinary;
 use Twig\Node\Expression\Binary\NotInBinary;
+use Twig\Node\Expression\Binary\NullCoalesceBinary;
 use Twig\Node\Expression\Binary\OrBinary;
 use Twig\Node\Expression\Binary\PowerBinary;
 use Twig\Node\Expression\Binary\RangeBinary;
@@ -320,6 +322,8 @@ public function getOperators(): array
                 '+' => ['precedence' => 500, 'class' => PosUnary::class],
             ],
             [
+                '?:' => ['precedence' => 5, 'class' => ElvisBinary::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT],
+                '??' => ['precedence' => 300, 'precedence_change' => new OperatorPrecedenceChange('twig/twig', '3.15', 5), 'class' => NullCoalesceBinary::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT],
                 'or' => ['precedence' => 10, 'class' => OrBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT],
                 'xor' => ['precedence' => 12, 'class' => XorBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT],
                 'and' => ['precedence' => 15, 'class' => AndBinary::class, 'associativity' => ExpressionParser::OPERATOR_LEFT],
@@ -351,7 +355,6 @@ public function getOperators(): array
                 'is' => ['precedence' => 100, 'associativity' => ExpressionParser::OPERATOR_LEFT],
                 'is not' => ['precedence' => 100, 'associativity' => ExpressionParser::OPERATOR_LEFT],
                 '**' => ['precedence' => 200, 'class' => PowerBinary::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT],
-                '??' => ['precedence' => 300, 'precedence_change' => new OperatorPrecedenceChange('twig/twig', '3.15', 5), 'class' => NullCoalesceExpression::class, 'associativity' => ExpressionParser::OPERATOR_RIGHT],
             ],
         ];
     }

diff --git a/src/Node/Expression/Binary/ElvisBinary.php b/src/Node/Expression/Binary/ElvisBinary.php
@@ -0,0 +1,50 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\Node\Expression\Binary;
+
+use Twig\Compiler;
+use Twig\Node\Expression\AbstractExpression;
+use Twig\Node\Expression\OperatorEscapeInterface;
+
+final class ElvisBinary extends AbstractBinary implements OperatorEscapeInterface
+{
+    public function __construct(AbstractExpression $left, AbstractExpression $right, int $lineno)
+    {
+        parent::__construct($left, $right, $lineno);
+
+        $this->setNode('test', clone $left);
+        $left->setAttribute('always_defined', true);
+    }
+
+    public function compile(Compiler $compiler): void
+    {
+        $compiler
+            ->raw('((')
+            ->subcompile($this->getNode('test'))
+            ->raw(') ? (')
+            ->subcompile($this->getNode('left'))
+            ->raw(') : (')
+            ->subcompile($this->getNode('right'))
+            ->raw('))')
+        ;
+    }
+
+    public function operator(Compiler $compiler): Compiler
+    {
+        return $compiler->raw('?:');
+    }
+
+    public function getOperandNamesToEscape(): array
+    {
+        return ['left', 'right'];
+    }
+}
diff --git a/src/Node/Expression/Binary/NullCoalesceBinary.php b/src/Node/Expression/Binary/NullCoalesceBinary.php
@@ -0,0 +1,84 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\Node\Expression\Binary;
+
+use Twig\Compiler;
+use Twig\Node\EmptyNode;
+use Twig\Node\Expression\AbstractExpression;
+use Twig\Node\Expression\BlockReferenceExpression;
+use Twig\Node\Expression\NameExpression;
+use Twig\Node\Expression\OperatorEscapeInterface;
+use Twig\Node\Expression\Test\DefinedTest;
+use Twig\Node\Expression\Test\NullTest;
+use Twig\Node\Expression\Unary\NotUnary;
+use Twig\TwigTest;
+
+final class NullCoalesceBinary extends AbstractBinary implements OperatorEscapeInterface
+{
+    public function __construct(AbstractExpression $left, AbstractExpression $right, int $lineno)
+    {
+        parent::__construct($left, $right, $lineno);
+
+        if (!$left instanceof NameExpression) {
+            $left = clone $left;
+            $test = new DefinedTest($left, new TwigTest('defined'), new EmptyNode(), $left->getTemplateLine());
+            // for "block()", we don't need the null test as the return value is always a string
+            if (!$left instanceof BlockReferenceExpression) {
+                $test = new AndBinary(
+                    $test,
+                    new NotUnary(new NullTest($left, new TwigTest('null'), new EmptyNode(), $left->getTemplateLine()), $left->getTemplateLine()),
+                    $left->getTemplateLine(),
+                );
+            }
+
+            $this->setNode('test', $test);
+        } else {
+            $left->setAttribute('always_defined', true);
+        }
+    }
+
+    public function compile(Compiler $compiler): void
+    {
+        /*
+         * This optimizes only one case. PHP 7 also supports more complex expressions
+         * that can return null. So, for instance, if log is defined, log("foo") ?? "..." works,
+         * but log($a["foo"]) ?? "..." does not if $a["foo"] is not defined. More advanced
+         * cases might be implemented as an optimizer node visitor, but has not been done
+         * as benefits are probably not worth the added complexity.
+         */
+        if ($this->hasNode('test')) {
+            $compiler
+                ->raw('((')
+                ->subcompile($this->getNode('test'))
+                ->raw(') ? (')
+                ->subcompile($this->getNode('left'))
+                ->raw(') : (')
+                ->subcompile($this->getNode('right'))
+                ->raw('))')
+            ;
+
+            return;
+        }
+
+        parent::compile($compiler);
+    }
+
+    public function operator(Compiler $compiler): Compiler
+    {
+        return $compiler->raw('??');
+    }
+
+    public function getOperandNamesToEscape(): array
+    {
+        return $this->hasNode('test') ? ['left', 'right'] : ['right'];
+    }
+}
diff --git a/src/Node/Expression/ConditionalExpression.php b/src/Node/Expression/ConditionalExpression.php
@@ -13,11 +13,15 @@
 namespace Twig\Node\Expression;
 
 use Twig\Compiler;
+use Twig\Node\Expression\OperatorEscapeInterface;
+use Twig\Node\Expression\Ternary\ConditionalTernary;
 
-class ConditionalExpression extends AbstractExpression
+class ConditionalExpression extends AbstractExpression implements OperatorEscapeInterface
 {
     public function __construct(AbstractExpression $expr1, AbstractExpression $expr2, AbstractExpression $expr3, int $lineno)
     {
+        trigger_deprecation('twig/twig', '3.16', \sprintf('"%s" is deprecated; use "%s" instead.', __CLASS__, ConditionalTernary::class));
+
         parent::__construct(['expr1' => $expr1, 'expr2' => $expr2, 'expr3' => $expr3], [], $lineno);
     }
 
@@ -42,4 +46,9 @@ public function compile(Compiler $compiler): void
                 ->raw('))');
         }
     }
+
+    public function getOperandNamesToEscape(): array
+    {
+        return ['expr2', 'expr3'];
+    }
 }
diff --git a/src/Node/Expression/Filter/DefaultFilter.php b/src/Node/Expression/Filter/DefaultFilter.php
@@ -16,11 +16,11 @@
 use Twig\Extension\CoreExtension;
 use Twig\Node\EmptyNode;
 use Twig\Node\Expression\AbstractExpression;
-use Twig\Node\Expression\ConditionalExpression;
 use Twig\Node\Expression\ConstantExpression;
 use Twig\Node\Expression\FilterExpression;
 use Twig\Node\Expression\GetAttrExpression;
 use Twig\Node\Expression\NameExpression;
+use Twig\Node\Expression\Ternary\ConditionalTernary;
 use Twig\Node\Expression\Test\DefinedTest;
 use Twig\Node\Node;
 use Twig\TwigFilter;
@@ -57,7 +57,7 @@ public function __construct(Node $node, TwigFilter|ConstantExpression $filter, N
             $test = new DefinedTest(clone $node, new TwigTest('defined'), new EmptyNode(), $node->getTemplateLine());
             $false = \count($arguments) ? $arguments->getNode('0') : new ConstantExpression('', $node->getTemplateLine());
 
-            $node = new ConditionalExpression($test, $default, $false, $node->getTemplateLine());
+            $node = new ConditionalTernary($test, $default, $false, $node->getTemplateLine());
         } else {
             $node = $default;
         }

diff --git a/src/Node/Expression/NullCoalesceExpression.php b/src/Node/Expression/NullCoalesceExpression.php
@@ -14,6 +14,7 @@
 use Twig\Compiler;
 use Twig\Node\EmptyNode;
 use Twig\Node\Expression\Binary\AndBinary;
+use Twig\Node\Expression\Binary\NullCoalesceBinary;
 use Twig\Node\Expression\Test\DefinedTest;
 use Twig\Node\Expression\Test\NullTest;
 use Twig\Node\Expression\Unary\NotUnary;
@@ -28,6 +29,8 @@ class NullCoalesceExpression extends ConditionalExpression
      */
     public function __construct(Node $left, Node $right, int $lineno)
     {
+        trigger_deprecation('twig/twig', '3.16', \sprintf('"%s" is deprecated; use "%s" instead.', __CLASS__, NullCoalesceBinary::class));
+
         if (!$left instanceof AbstractExpression) {
             trigger_deprecation('twig/twig', '3.15', 'Not passing a "%s" instance to the "left" argument of "%s" is deprecated ("%s" given).', AbstractExpression::class, static::class, get_class($left));
         }

diff --git a/src/Node/Expression/OperatorEscapeInterface.php b/src/Node/Expression/OperatorEscapeInterface.php
@@ -0,0 +1,25 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+ namespace Twig\Node\Expression;
+
+/**
+ * Interface implemented by n-ary operators for n > 1.
+ *
+ * @author Fabien Potencier <[email protected]>
+ */
+interface OperatorEscapeInterface
+{
+    /**
+     * @return string[]
+     */
+    public function getOperandNamesToEscape(): array;
+}
diff --git a/src/Node/Expression/Ternary/ConditionalTernary.php b/src/Node/Expression/Ternary/ConditionalTernary.php
@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * This file is part of Twig.
+ *
+ * (c) Fabien Potencier
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Twig\Node\Expression\Ternary;
+
+use Twig\Compiler;
+use Twig\Node\Expression\AbstractExpression;
+use Twig\Node\Expression\OperatorEscapeInterface;
+
+final class ConditionalTernary extends AbstractExpression implements OperatorEscapeInterface
+{
+    public function __construct(AbstractExpression $test, AbstractExpression $left, AbstractExpression $right, int $lineno)
+    {
+        parent::__construct(['test' => $test, 'left' => $left, 'right' => $right], [], $lineno);
+    }
+
+    public function compile(Compiler $compiler): void
+    {
+        $compiler
+            ->raw('((')
+            ->subcompile($this->getNode('test'))
+            ->raw(') ? (')
+            ->subcompile($this->getNode('left'))
+            ->raw(') : (')
+            ->subcompile($this->getNode('right'))
+            ->raw('))')
+        ;
+    }
+
+    public function getOperandNamesToEscape(): array
+    {
+        return ['left', 'right'];
+    }
+}