Fix "Undefined property" warning when calc() is used with CSS var()

== Less_Tree_Operation ==

In Less_Tree_Operation::operate(), we call Dimension::operate() with
argument $b (named $other there), which Dimension expects to be a
number with a unit, i.e. something we can perform math on.

Change Operation::operate() to not call this if argument $b is
something else, which avoids the reported PHP warning of $other->unit
being undefined. Noting that the only class with a 'unit' property
is Less_Tree_Dimension.

As a side-effect, this immediately fixes another bug, namely that the
"var()" right-hand side of `calc(100% - var(foo))` was completely
ignored previously when it wasn't a number. For example, it can be
a CSS keyword or CSS function like var() that the browser handles
client-side. Noting that CSS also natively supports math. LESS should
only handle math when the input uses PHP variables or literals, and
thus effectively has a constant outcome.

Previously, Dimention::operate() would implicitly read $other->value
as NULL when $other value wasn't a numerical Dimension object, and
thus the math operation silently failed before this commit, e.g.:
> "100% - var(foo)"
> "100 - NULL" (as %)
> "100%"

With the warning avoided/fixed, we now correctly preserve the
equation in its entirely when $b is e.g. a var() call.

== Less_Tree_Call ==

The above change on its own is insufficient, as that elevates the
warning to a fatal error. The added test case exposes this,
where `calc(100% - var(foo) - var(bar))` is parsed as:

  Call (calc)
  \-- Expression
      \-- Operation (-)
          |-- Operation (-_
          |    |- Dimension (100%)
          |    |- Call (var foo)
          |-- Call (var bar)

The above change, makes the first Operation pair no longer compile
Dimension+Call down to a Dimension, instead preserving Operation as
literal CSS output. This means the second Operation will now see a
left-hand value of Operation, which doesn't have an "operate" method,
and thus correctly triggers the `throw` statement that I cleaned up,
because that's not something we can compute server-side.

Fix this by recognising `calc()` higher up as something that
server-side LESS math should not be performed on in the first place.

This effectively backports less/less.js#3162
(less/less.js@a48c24c4dd3c13e00a20ece803237)
for the similarly reported issue upstream at
less/less.js#974.

That bugfix was originally released with less.js 3.0.0, but the
part of that I'm backporting here doesn't change behaviour of any
of our less.js 2.5.3 compliance tests, thus safe to backport.

We already threw a fatal error when one of the arguments wasn't a
Dimension or Color (no other class has the 'operate' method), so this
only adds non-fatal outcomes to previously fatal outcomes.

Bug: T331688
Change-Id: Idd443a76372682de4edddeb64ab5a65b23bbd3ce

lib/Less/Environment.php

@@ -41,6 +41,8 @@ class Less_Environment {
 
 	public static $mixin_stack = 0;
 
+	public static $mathOn = true;
+
 	/**
 	 * @var array
 	 */
@@ -93,7 +95,14 @@ public function copyEvalEnv( $frames = [] ) {
 		return $new_env;
 	}
 
+	/**
+	 * @return bool
+	 * @see Eval.prototype.isMathOn in less.js 3.0.0 https://github.com/less/less.js/blob/v3.0.0/dist/less.js#L1007
+	 */
 	public static function isMathOn() {
+		if ( !self::$mathOn ) {
+			return false;
+		}
 		return !Less_Parser::$options['strictMath'] || self::$parensStack;
 	}
 

lib/Less/Tree/Call.php

@@ -1,19 +1,23 @@
 <?php
 /**
  * @private
+ * @see less.tree.Call in less.js 3.0.0 https://github.com/less/less.js/blob/v3.0.0/dist/less.js#L6336
  */
 class Less_Tree_Call extends Less_Tree {
 	public $value;
 
 	public $name;
 	public $args;
+	/** @var bool */
+	public $mathOn;
 	public $index;
 	public $currentFileInfo;
 	public $type = 'Call';
 
 	public function __construct( $name, $args, $index, $currentFileInfo = null ) {
 		$this->name = $name;
 		$this->args = $args;
+		$this->mathOn = ( $name !== 'calc' );
 		$this->index = $index;
 		$this->currentFileInfo = $currentFileInfo;
 	}
@@ -24,22 +28,27 @@ public function accept( $visitor ) {
 
 	//
 	// When evaluating a function call,
-	// we either find the function in `tree.functions` [1],
-	// in which case we call it, passing the  evaluated arguments,
-	// or we simply print it out as it appeared originally [2].
+	// we either find the function in Less_Functions,
+	// in which case we call it, passing the evaluated arguments,
+	// or we simply print it out as it literal CSS.
 	//
-	// The *functions.js* file contains the built-in functions.
-	//
-	// The reason why we evaluate the arguments, is in the case where
-	// we try to pass a variable to a function, like: `saturate(@color)`.
+	// The reason why we compile the arguments, is in the case one
+	// of them is a LESS variable that only PHP knows the value of,
+	// like: `saturate(@mycolor)`.
 	// The function should receive the value, not the variable.
 	//
 	public function compile( $env = null ) {
+		// Turn off math for calc(). https://phabricator.wikimedia.org/T331688
+		$currentMathContext = Less_Environment::$mathOn;
+		Less_Environment::$mathOn = $this->mathOn;
+
 		$args = [];
 		foreach ( $this->args as $a ) {
 			$args[] = $a->compile( $env );
 		}
 
+		Less_Environment::$mathOn = $currentMathContext;
+
 		$nameLC = strtolower( $this->name );
 		switch ( $nameLC ) {
 			case '%':

lib/Less/Tree/Color.php

@@ -98,6 +98,7 @@ public function toCSS( $doNotCompress = false ) {
 
 	/**
 	 * @param string $op
+	 * @param Less_Tree_Color $other
 	 */
 	public function operate( $op, $other ) {
 		$rgb = [];

lib/Less/Tree/Dimension.php

@@ -72,6 +72,7 @@ public function __toString() {
 
 	/**
 	 * @param string $op
+	 * @param Less_Tree_Dimension $other
 	 */
 	public function operate( $op, $other ) {
 		$value = Less_Functions::operate( $op, $this->value, $other->value );

lib/Less/Tree/Operation.php

@@ -26,21 +26,26 @@ public function compile( $env ) {
 		$a = $this->operands[0]->compile( $env );
 		$b = $this->operands[1]->compile( $env );
 
+		// Skip operation if argument was not compiled down to a non-operable value.
+		// For example, if one argument is a Less_Tree_Call like 'var(--foo)' then we
+		// preserve it as literal for native CSS.
+		// https://phabricator.wikimedia.org/T331688
 		if ( Less_Environment::isMathOn() ) {
 
 			if ( $a instanceof Less_Tree_Dimension && $b instanceof Less_Tree_Color ) {
 				$a = $a->toColor();
 
 			} elseif ( $b instanceof Less_Tree_Dimension && $a instanceof Less_Tree_Color ) {
 				$b = $b->toColor();
-
 			}
 
-			if ( !method_exists( $a, 'operate' ) ) {
+			if ( !( $a instanceof Less_Tree_Dimension || $a instanceof Less_Tree_Color ) ) {
 				throw new Less_Exception_Compiler( "Operation on an invalid type" );
 			}
 
-			return $a->operate( $this->op, $b );
+			if ( $b instanceof Less_Tree_Dimension || $b instanceof Less_Tree_Color ) {
+				return $a->operate( $this->op, $b );
+			}
 		}
 
 		return new Less_Tree_Operation( $this->op, [ $a, $b ], $this->isSpaced );

test/Fixtures/less.php/css/T331688-calc.css

@@ -0,0 +1,6 @@
+.highlight {
+  width: calc(100% - 2px);
+  height: calc(50% - 2*1px);
+  padding-left: 9px;
+  padding-right: calc(8px + 1px);
+}

test/Fixtures/less.php/css/T331688-op-var.css

@@ -0,0 +1,3 @@
+.highlight {
+  height: calc(100% - var(--margin-top) - var(--margin-bottom));
+}

test/Fixtures/less.php/less/T331688-calc.less

@@ -0,0 +1,10 @@
+@border-width-base: 1px;
+@padding-highlight: 8px;
+@half: 0.5;
+
+.highlight {
+	width: calc( 100% - 2px );
+	height: calc( percentage( @half ) - 2*@border-width-base );
+	padding-left: @padding-highlight + @border-width-base;
+	padding-right: calc( @padding-highlight + @border-width-base );
+}

test/Fixtures/less.php/less/T331688-op-var.less

@@ -0,0 +1,3 @@
+.highlight {
+    height: calc(100% - var(--margin-top) - var(--margin-bottom));
+}