After a bit of research and thinking about this. There are couple of possible ways of getting rid of the tech debt:

Solution 1 - Additional method in Less_Tree

We could provide a hasValue() method in Less_Tree that would return false by default, and then in classes that have $value property would return true. IMHO we should reject this idea as it would leak the implementation details that some nodes can have value. The Less_Tree shouldn't know how children implement the logic - this breaks the Open-Close principle from SOLID.

Solution 2 - Abstract class

Another approach would be to add an intermediary subclass that all classes with $value would extend. This class would be abstract, and only implement public $value. This class would become a parent for nodes that have value. The class itself would be very simple, having only one property. IMHO, this is not a proper way as we should promote composition over inheritance. In the long run, this might cause new technical debt - for example how to tackle this problem, if we want to type a node that has both value and name properties? Do we create another abstract class that handles name and extends the node that handles value? Additionally, it would decrease the readability as Nodes that extend the newly created intermediary subclass shouldn't declare the $value property (as the property should come from the abstract class. If we go this path, and then we decide to have a check that Node has both value and name for example, we would need to implement another class that would extend the newly created intermediary subclass and implement the handling for name.

Solution 3 - Interface and self-encapsulation

Instead, we can implement an interface and introduce self-encapsulation. We could add a new interface with the getValue() method and make all nodes with value implement the new interface. For self-encapsulation, all places that use $obj->value have to be refactored to $obj->getValue(); This way nodes can define their own way on how to handle values and everywhere instead of property_exist() we would do instanceof <INTEFACE>. Self-encapsulation would provide additional benefits of guarding the logic how to handle value, or even to make it read-only. Currently $value is defined as public, without a type, which can lead to unintentional overrides.

Solution 4 - Marker interface

The last proposed approach would be to define a Marker Interface - an interface without any methods ( we cannot define the property only as interfaces cannot have properties). All classes that have $value property would implement the newly created Marker interface and property_exists() would be replaced with instanceof <INTERFACE>. Without self-encapsulation, we will lose the way to control access to the $value property. In general empty interfaces are considered anti-pattern as those do not specify any type of contract/functionality. When empty interfaces are used in run-time to mark objects with metadata (which seems like our case) we could use those as Marker Interfaces, but in general those are seen as evil and might bring confusion in the future.

Also, a note - there is a workaround for declaring that interface has a property. We can add docblock with @property declaration, like this

/**
 * @property mixed $value
 */
interface Less_Tree_ValueNode
{
}

Some IDEs (PHPStorm for sure) will understand it and help with autocompletion.

I would suggest going with solution #3 - which seems the best for long-term support. @Krinkle what do you think?

@pmiazga Option 3 or Option 4 sound good to me. It is indeed unfortunate that PHP does not natively support properties to be part of an interface declaration. This is likely due to the legacy of classes supporting dynamic properties by default, and it did not make sense to make these part of the interface since any property can be read to yield null, and any property can be created. This is different as of PHP 8, so perhaps in the future this will be possible.

Note that it is intentional that most node properties are writable. They are classes solely used internally (not part of the stable Parser API that e.g. MediaWiki, Magento, or Matomo call into), and regularly read and written directly. We could add a bunch of getter and setter methods, but this would be of limited value. Once we required PHP 8, we could use native property type hints to enforce this beyond what we can already do with Phan. The methods would come with significant call overhead which is best to avoid in this hot code.

Looking at the code today, there is 1 match for git grep '>value =' lib/ today, in toCSS.php, which is for a different node, where we update Less_Tree_Rule->value. That one is separate from this task as that is specific to non-string values stored in Less_Tree_Rule which happens to have the same name and has its own instanceof check already.

If later upstream releases require compilation to write to "Value"-having nodes, we could later refactor to Option 4, no problem. Today, either way could work.

I would lean slightly toward Option 4 even today, because it means we can read the value in hot code without function call overhead. I'm fairly sure that we couldn't afford switching all properties to getter/setter methods. Doing it for just one might not affect latency, I don't recall whether these particular lines are considered "hot" or not. I suggest running the benchmark to see if there is measurable difference between the approaches. Using Option 4 means we avoid a long-term inconsistency where the compiler reads and writes all properties on Tree nodes directly, except value.

Regarding solution 4: Phan is also able to understand the @property annotation on an interface, so if we go with that one, we'd still get most of the benefits of encapsulation.

For example:

https://phan.github.io/demo/?code=<%3Fphp%0A%0A%2F**%0A+*+%40property+int+%24val%0A+*%2F%0Ainterface+HasProp+{%0A}%0A%0Aclass+A+{%0A}%0A%0Aclass+B+implements+HasProp+{%0A}%0A%0A%24a+%3D+new+A()%3B%0Aecho+%24a->val%3B%0A%24a->val+%3D+42%3B%0A%0A%24b+%3D+new+B()%3B%0Aecho+%24b->val%3B%0A%24b->val+%3D+42%3B%0A%0A%24c+%3D+rand()+%25+2+%3F+new+A()+%3A+new+B()%3B%0Aif+(+%24c+instanceof+B+)+{%0A++++echo+%24c->val%3B%0A}+else+{%0A++++echo+%24c->val%3B%0A}

We get a warning when accessing A::$val (line 16, 17, 27), but not when accessing B::$val (line 20, 21, 25).

I wouldn't worry about the function call overhead, as it's only a couple extra ops - and IMHO it's more a micro-optimization that we wouldn't be able to notice in the runtime. And I hope that in such cases compiler could optimize the code by just fetching the variable instead of doing a function call. Some languages already do that.

But I agree, it would leave the code in a bit weird state that some things are not only checked with property_exist() and others with instanceof but for some properties, we would reach directly and for others, we would use getters. I'll go with option #4 - the MarkerInterface -> as later - if we decide it's worth adding a getter - we can easily do that.

Thank you @Krinkle and @matmarex

For future reference:

Opcode:

with function call

line      #* E I O op                           fetch          ext  return  operands
-------------------------------------------------------------------------------------
   18     0  E >   NEW                                              $1      'Less_Tree_Value'
          1        SEND_VAL_EX                                              'test'
          2        DO_FCALL                                      0          
          3        ASSIGN                                                   !0, $1
   19     4        INIT_METHOD_CALL                                         !0, 'getValue'
          5        DO_FCALL                                      0  $4      
          6        ECHO                                                     $4
   20     7      > RETURN                                                   1

and getValue()

-------------------------------------------------------------------------------------
   13     0  E >   FETCH_OBJ_R                                      ~0      'value'
          1      > RETURN                                                   ~0
   14     2*     > RETURN                                                   null

Get variable directly

18     0  E >   NEW                                              $1      'Less_Tree_Value'
       1        SEND_VAL_EX                                              'test'
       2        DO_FCALL                                      0          
       3        ASSIGN                                                   !0, $1
19     4        FETCH_OBJ_R                                      ~4      !0, 'value'
       5        ECHO                                                     ~4
20     6      > RETURN                                                   1

Recently @daniel raised a very good point about the performance of each solution, and whether it is faster to do instanceof (as it may have to traverse the inheritance tree) instead of calling property_exists() or even maybe trying to switch to $node->value ?? checks. I developed a super simple benchmark to verify which option would be the best pick.

The benchmark is available as a paste https://phabricator.wikimedia.org/P50598 and I used docker to test it against different PHP versions. To check if you get the same results, this could be executed as
docker run --rm -it -v $(pwd):/opt php:{PHP_VER} php /opt/benchmark.php.

How I checked the performance:
For different PHP versions (7.4, 8.0, 8.1 and 8.2) I checked multiple options, and each option was checked 15 times I tried to do the same comparison 42000000 times and I executed this script multiple times to make sure processes on my host do not affect results. This is a synthetic test on my laptop, therefore please do not treat those results as rigorous checks, but more a insights on performance for different kinds of checks.

In short summary

• The fastest way is to access to $hasValue property that would return true|false based on class.
• the property_exists() check is the slowest, overall it takes 1.6s~1.8s to finish all 420M checks where other solutions take less than half of that time.
• The ?? operator takes a bit of time, and when used on a node that does not have property is slower than just doing isset().
• The isset() is pretty fast for checks when the property doesn't exist. ?? is faster when the property exists - which makes sense as it doesn't have to fetch the default value.
• interestingly, introducing a method $node->hasValue() that would return hardcoded value is slower than just using isset() or ??.
• the instanceof check is more than twice as fast as the property_exists(). Overall it's ~5% slower than property access.
• with every PHP version bump we get a small performance improvement.

+ docker run --rm -it -v projects/mediawiki/less.php:/opt php:7.4 php /opt/test/benchmark.php
Testing $hasValue property access                  [...............]  Results - avg: 0.8262735367s, min: 0.8224050999s, max: 0.8354020119s 
Testing ?? with not ValueNode                      [...............]  Results - avg: 1.0337988059s, min: 1.0304491520s, max: 1.0370311737s 
Testing ?? with ValueNode                          [...............]  Results - avg: 0.8433937709s, min: 0.8405070305s, max: 0.8472871780s 
Testing Call property_exists() with ValueNode      [...............]  Results - avg: 1.6410133521s, min: 1.6288690567s, max: 1.6625580788s 
Testing Call property_exists() with not ValueNode  [...............]  Results - avg: 1.8394373576s, min: 1.8281681538s, max: 1.8860251904s 
Testing Instanceof with node that implements       [...............]  Results - avg: 0.8384617964s, min: 0.8280420303s, max: 0.8523559570s 
Testing instanceof with node that do not implement [...............]  Results - avg: 0.8673528989s, min: 0.8621139526s, max: 0.8766109943s 
Testing isset with not ValueNode                   [...............]  Results - avg: 0.9297739665s, min: 0.9252538681s, max: 0.9331080914s 
Testing isset with ValueNode                       [...............]  Results - avg: 0.8936026255s, min: 0.8817169666s, max: 0.9203691483s 
Testing method hasValue that returns bool          [...............]  Results - avg: 1.1492320379s, min: 1.1400880814s, max: 1.1905899048s 
+ docker run --rm -it -v projects/mediawiki/less.php:/opt php:8.0 php /opt/test/benchmark.php
Testing $hasValue property access                  [...............]  Results - avg: 0.7391024590s, min: 0.7340049744s, max: 0.7443330288s 
Testing ?? with not ValueNode                      [...............]  Results - avg: 0.9655999025s, min: 0.9579279423s, max: 0.9788639545s 
Testing ?? with ValueNode                          [...............]  Results - avg: 0.8067283630s, min: 0.8031079769s, max: 0.8152201176s 
Testing Call property_exists() with ValueNode      [...............]  Results - avg: 1.6357766628s, min: 1.6252651215s, max: 1.6482799053s 
Testing Call property_exists() with not ValueNode  [...............]  Results - avg: 1.7813727220s, min: 1.7728450298s, max: 1.8037350178s 
Testing Instanceof with node that implements       [...............]  Results - avg: 0.7900250276s, min: 0.7796061039s, max: 0.8022580147s 
Testing instanceof with node that do not implement [...............]  Results - avg: 0.7702324231s, min: 0.7614760399s, max: 0.7812650204s 
Testing isset with not ValueNode                   [...............]  Results - avg: 0.8228953520s, min: 0.8205320835s, max: 0.8266360760s 
Testing isset with ValueNode                       [...............]  Results - avg: 0.8164281845s, min: 0.8133959770s, max: 0.8209888935s 
Testing method hasValue that returns bool          [...............]  Results - avg: 1.0259145260s, min: 1.0214040279s, max: 1.0317931175s 
+ docker run --rm -it -v projects/mediawiki/less.php:/opt php:8.1 php /opt/test/benchmark.php
Testing $hasValue property access                  [...............]  Results - avg: 0.7467519283s, min: 0.7413289547s, max: 0.7571599483s 
Testing ?? with not ValueNode                      [...............]  Results - avg: 0.9321365515s, min: 0.9267091751s, max: 0.9465749264s 
Testing ?? with ValueNode                          [...............]  Results - avg: 0.8159927209s, min: 0.8107259274s, max: 0.8238379955s 
Testing Call property_exists() with ValueNode      [...............]  Results - avg: 1.6219490846s, min: 1.6134350300s, max: 1.6497631073s 
Testing Call property_exists() with not ValueNode  [...............]  Results - avg: 1.7828493118s, min: 1.7609789371s, max: 1.8449490070s 
Testing Instanceof with node that implements       [...............]  Results - avg: 0.7921526909s, min: 0.7878818512s, max: 0.7984280586s 
Testing instanceof with node that do not implement [...............]  Results - avg: 0.7635336558s, min: 0.7597370148s, max: 0.7674109936s 
Testing isset with not ValueNode                   [...............]  Results - avg: 0.8368697643s, min: 0.8335850239s, max: 0.8406541348s 
Testing isset with ValueNode                       [...............]  Results - avg: 0.8325014909s, min: 0.8281950951s, max: 0.8390011787s 
Testing method hasValue that returns bool          [...............]  Results - avg: 1.0532999992s, min: 1.0497992039s, max: 1.0631871223s 
+ docker run --rm -it -v projects/mediawiki/less.php:/opt php:8.2 php /opt/test/benchmark.php
Testing $hasValue property access                  [...............]  Results - avg: 0.7541549365s, min: 0.7430310249s, max: 0.7608890533s 
Testing ?? with not ValueNode                      [...............]  Results - avg: 0.9169308027s, min: 0.9139399529s, max: 0.9210581779s 
Testing ?? with ValueNode                          [...............]  Results - avg: 0.8114304066s, min: 0.7966530323s, max: 0.8183579445s 
Testing Call property_exists() with ValueNode      [...............]  Results - avg: 1.6184610049s, min: 1.6023759842s, max: 1.6450829506s 
Testing Call property_exists() with not ValueNode  [...............]  Results - avg: 1.7615748088s, min: 1.7514770031s, max: 1.7708871365s 
Testing Instanceof with node that implements       [...............]  Results - avg: 0.7791677157s, min: 0.7758691311s, max: 0.7922229767s 
Testing instanceof with node that do not implement [...............]  Results - avg: 0.7616595109s, min: 0.7537279129s, max: 0.7739520073s 
Testing isset with not ValueNode                   [...............]  Results - avg: 0.8644951979s, min: 0.8590309620s, max: 0.8694081306s 
Testing isset with ValueNode                       [...............]  Results - avg: 0.8616048495s, min: 0.8549449444s, max: 0.8828289509s 
Testing method hasValue that returns bool          [...............]  Results - avg: 1.0643759410s, min: 1.0537049770s, max: 1.0779569149s

Change 950203 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] Start using interface instead of property_exists

https://gerrit.wikimedia.org/r/950203

Change 950203 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Start using interface instead of property_exists

https://gerrit.wikimedia.org/r/950203

Change 950724 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/libs/less.php@master] Less_Parser: Check strict types for Less_Tree_Url in parseEntitiesUrl()

https://gerrit.wikimedia.org/r/950724

Change 950724 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Less_Parser: Check strict types for Less_Tree_Url in parseEntitiesUrl()

https://gerrit.wikimedia.org/r/950724

@daniel wrote on change 950203:

@Krinkle wrote:

It uses !obj.value or obj.value == null, which is where JS yields undefined if the property doesn't exist and is then casted either to boolean false or (loosely) equated to null.

Sounds like we could just have a value property in the base class, and set it to null per default.

This felt hacky to me at first, based on the following assumptions:

• might weaken static analysis, which might lead to mistakes if you forget to check the type since the property now can be accessed without warning.
• implies that this property conceptually means the same thing in the subclasses that use it, in actually all the base class can document is @type mixed $value Might contain something used for something.

Thinking about it a bit more now, with the code on my side and trying this out for one or two sub classes, I think the opposite is true.

I think it would strengthen analysis because mixed (or if we write it out: null|bool|int|string|array|Less_Tree`) makes Phan confident that unless you've narrowed it down, it really could be anything.

Regarding the property not meaning the same thing, this is true. But we have two things on our side. Number 1, less.js does the same thing, which gives us a safety net and prevents unintended property confusion. Number 2, the same thing is true for the interface we just added. All it says is that the property can exist. It can hold totally different types for different purposes depending on the class that implements it. So while it is true that classes could have this property for different purposes, in practice, each of those would implement this shared interface in the way we wrote it now.

Another assumption we made is that we can't know the actual type of the node, that the code is too dynamic to "just" know which Less_Tree subclases an object can be and thus naturally we know that it has a certain value property and what type that can hold. I no longer think this assumption holds either. The previous maintainers of less.php definitely didn't do much to make it obvious what types can go where, but it's quite strict in practice, just not well documented.

I went through this excercise for https://gerrit.wikimedia.org/r/950724 and indeed the parser is very specific about what nodes can be related to what other nodes. This makes sense of course from a compiler point of view. While it is true that user input could be anything, this is caught while we parse the tree, and is always either interpreted as something valid from a small set of choices, or produces a syntax error.

A few examples that make me hopeful:

-class Less_Tree_Dimension extends Less_Tree implements Less_Tree_HasValueProperty {
+class Less_Tree_Dimension extends Less_Tree {

+   /** @var float */
    public $value;

…

-class Less_Tree_Expression extends Less_Tree implements Less_Tree_HasValueProperty {
+class Less_Tree_Expression extends Less_Tree {
-   /** @var array */
-   public $value = [];
+   /** @var Less_Tree[] */
+   public $value;

…

-class Less_Tree_Value extends Less_Tree implements Less_Tree_HasValueProperty {
+class Less_Tree_Value extends Less_Tree {
 
+   /** @var Less_Tree[] */
    public $value;

And for example, existing code:

			if ( $isCall ) {
				$arg = $this->parseDetachedRuleset() ?? $this->parseExpression();
				# $arg is Less_Tree_DetachedRuleset|Less_Tree_Expression|null
			}

			/* … */

			if ( $isCall ) {
				if ( is_array( $arg->value ) ) {
					# $arg can only be Less_Tree_Expression
					if ( count( $arg->value ) == 1 ) {
						$val = $arg->value[0];
					}

We also have:

$ git grep T327082 
lib/Less/Tree/Variable.php:         // TODO: Solve better (https://phabricator.wikimedia.org/T327082).
lib/Less/Visitor/processExtends.php:        // @phan-suppress-next-line PhanUndeclaredProperty https://phabricator.wikimedia.org/T327082
lib/Less/Visitor/processExtends.php:        // @phan-suppress-next-line PhanUndeclaredProperty https://phabricator.wikimedia.org/T327082

Someone submitted a patch to us at https://github.com/wikimedia/less.php/pull/101/files, indicating that our latest version still emits a warning under PHP 8.2 due to undefined property:

div[class^=nonquoted] {
	font-weight: normal;
}
.test {
	&:extend(div[class^=nonquoted]);
}

- // @phan-suppress-next-line PhanUndeclaredProperty https://phabricator.wikimedia.org/T327082
- $elementValue1 = ( $elementValue1->value->value ?: $elementValue1->value );
+ $elementValue1 = $elementValue1->value;
+ if ( $elementValue1 instanceof Less_Tree_Quoted ) {
+   $elementValue1 = $elementValue1->value;
+ }

That may be worth carrying over to Gerrit as its own patch or as part of a larger one where we remove use of conditional >value entirely (including the interface).

Change 967573 had a related patch set uploaded (by Krinkle; author: Gr8b):

[mediawiki/libs/less.php@master] Fix PHP warning when @extend path contains non-quoted attribute

https://gerrit.wikimedia.org/r/967573

Change 967573 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Fix PHP warning when @extend path contains non-quoted attribute

https://gerrit.wikimedia.org/r/967573

I analysed the possibility of dropping the new interface by using specific types instead. I found a couple of places we can easily fix. But I also found some methods that return type can be anything of Less_Tree, or one of multiple return types.

Selector

Inside Selector::CacheElements() we iterate over elements that can be on of 7 types: [ Less_Tree_Paren, Less_Tree_Dimension, Less_Tree_Attribute, Less_Tree_Selector, Less_Tree_Variable, Less_Keyword. Less_Quoted ] . Two of do not have value, therefore we could rewrite code

from:

if ( !( $v->value instanceof Less_Tree_HasValueProperty ) || !is_string( $v->value->value ) ) {

to

if ( ( $v->value instanceof Less_Tree_Selector || $v->value instanceof Less_Tree_Variable ) || !is_string( $v->value->value ) ) {

but I'm not sure if this is a valid assumption.

isem(), ispixel(), ispercentage(), isunit()

Those less functions take a string or Less_Tree - and here I'm not sure if less allows us to pass anything that would map to not a string nor Less_Tree_Keyword.

extract()

This less function supports works types too, Less_Tree_Color, Less_Tree_Dimension, Less_Tree_Expression, Less_Tree_Keyword, Less_Tree_Quoted, Less_Tree_Value and most of them provide $value. We would end up doing instanceof against multiple types. I could check against not Less_Tree_Color but I'm not sure if this is sufficient.

length()

similar to extract() can take almost any argument, we would need to check against multiple types again.

min() and max()

Similarly to previous two, those work on lists, therefore we would need to check against multiple types - unless we can exactly specify what could be the type of any element

Change 969371 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] phan: update Less_Tree_Variable::compile() return types

https://gerrit.wikimedia.org/r/969371

Change 969372 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] Better docblocks

https://gerrit.wikimedia.org/r/969372

Change 969378 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] risky: Expect Less_Tree_Expression instead of HasValueProperty interface

https://gerrit.wikimedia.org/r/969378

Change 969379 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] DNM: Testing possible cases where we can get rid of HasValueProperty

https://gerrit.wikimedia.org/r/969379

Change 969371 merged by jenkins-bot:

[mediawiki/libs/less.php@master] phan: update Less_Tree_Variable::compile() return types

https://gerrit.wikimedia.org/r/969371

Change 969372 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Less_Tree: Improve docblocks around $value property

https://gerrit.wikimedia.org/r/969372

Change 969379 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Functions,Selector: Type specific Less_Tree nodes instead of generic HasValueProperty

https://gerrit.wikimedia.org/r/969379

Change 969378 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Expect Less_Tree_Expression instead of HasValueProperty interface

https://gerrit.wikimedia.org/r/969378

In T327082#9287634, @pmiazga wrote:

[…]

length()

similar to extract() can take almost any argument, we would need to check against multiple types again.

It can syntactically accept any argument type and "supports" that without failure. But, I believe, it only does something with Less_Tree_Expression. For everything else, it returns a hardcoded 1.

	public function length( $values ) {
		$n = ( $values instanceof Less_Tree_HasValueProperty && is_array( $values->value ) ) ?
			count( $values->value ) : 1;
		return new Less_Tree_Dimension( $n );
	}

The use of the object is limited to calling count( $obj->value ) once. And only if is_array returned true.

Which Less_Tree types have an array $value? I suspect its only Less_Tree_Expression, like you found with svg-gradient() already.

min() and max()

Similarly to previous two, those work on lists, therefore we would need to check against multiple types - unless we can exactly specify what could be the type of any element

If I read these functions correctly, their Less_Tree_HasValueProperty check applies to the list itself, not to the elements.

This use case is indeed more complex, and supports all possible types (including those that don't have a value!), but that happens after the list is extracted already. Similar to the length() code above, it seems the value check only applies to reading an array. The only Less_Tree_HasValueProperty that can be a function agument, and holds an array, is Less_Tree_Expression I think?

for length we would have Less_Tree_Expression and Less_Tree_Value at least. Those are the ones that are passed via our testing suite. I applied the change, checked the logic and looks ok. I'll push the PR for it.

For the minmax case we don't have an extensive test suite - Our tests check mostly against Less_Tree_Dimension or Less_Tree_Color - nothing else.
I checked the code coverage and

				if ( $args[$i] instanceof Less_Tree_HasValueProperty && is_array( $args[$i]->value ) ) {
					$args[] = $args[$i]->value;
				}

has no coverage in tests, therefore I'm hesitant to fix it within this ticket. Therefore I suggest - that once the lenght case is solved -> let's close this ticket.
I'll submit a separate ticket for the minimax case (and cleanup) plus improve the test scenarios to make sure this path is properly tested.

@pmiazga Sounds good to me! You might also want to cross-reference with T349580: Add Less.js v2.5.3 spec to less.php test suite which will expand test coverage.

Change 975801 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/libs/less.php@master] lenght() function counts only Tree_Expression or Tree_Value

https://gerrit.wikimedia.org/r/975801

Change 975801 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Functions: Let length() function count only Expression or Value

https://gerrit.wikimedia.org/r/975801