Fix failing less-2.5.3/css.less test
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	pmiazga
	Apr 11 2024, 3:15 PM

Description

The less-2.5.3/css.less fixture is failing. Most likely one of T352911 opr T352866 could introduce a regression. Please verify, fix the code and remove the

			'css' => false, // T352911 & T352866

from a list of known failing tests.

Failure:

... parse less.php/test/Fixtures/lessjs-2.5.3/less/css.less Fail
--- actual
+++ less.php/test/Fixtures/lessjs-2.5.3/css/css.css
@@ -64,7 +64,7 @@
   padding: 1px 0 2px 0;
   font: normal small / 20px 'Trebuchet MS', Verdana, sans-serif;
   font: 0/0 a;
-  border-radius: 0.5px;
+  border-radius: 5px / 10px;
 }

Acceptance criteria:

the CSS fixture is enabled
mixins-args override is removed (same issue)

Details

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	Copy strictMath flag in temporary env for mixin definitions	mediawiki/libs/less.php	master	+24 -274

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Open	None	T288498 Update less.php port to support Less.js 3.13 behaviours
Resolved	Hokwelum	T365481 Update less.php port to complete support Less.js 2.5.3 behaviours
Resolved	pmiazga	T362343 Fix failing less-2.5.3/css.less test

Event Timeline

pmiazga created this task.Apr 11 2024, 3:15 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 11 2024, 3:15 PM

Krinkle moved this task from Inbox, needs triage to Needs refinement on the MediaWiki-Platform-Team board.Apr 15 2024, 2:38 PM

pmiazga changed the task status from Open to In Progress.Apr 24 2024, 3:57 PM

pmiazga updated the task description. (Show Details)

pmiazga moved this task from Needs refinement to In progress on the MediaWiki-Platform-Team board.

pmiazga mentioned this in T352897: Fix ParseError in mixin-args.less test case.Apr 24 2024, 4:04 PM

pmiazga claimed this task.Apr 24 2024, 4:08 PM

As part of the previous PR, I had this incomplete investigation which I'll share:

$ cat input.less 
.border-radius(@r: 2px/5px) {
  border-radius: @r;
}
.slash-vs-math {
  .border-radius();
  .border-radius(5px/10px);
}

$ bin/lessc --strict-math=off input.less 
{"op":"/","isMathOn":true,"parensStack":0} 2px/5px
{"op":"/","isMathOn":true,"parensStack":0} 5px/10px
.slash-vs-math {
  border-radius: 0.4px;
  border-radius: 0.5px;
}

$ bin/lessc --strict-math=on input.less 
{"op":"/","isMathOn":true,"parensStack":0} 2px/5px
{"op":"/","isMathOn":false,"parensStack":0} 5px/10px
{"op":"/","isMathOn":false,"parensStack":0} 5px/10px
{"op":"/","isMathOn":false,"parensStack":0} 5px/10px
{"op":"/","isMathOn":true,"parensStack":0} 5px/10px
.slash-vs-math {
  border-radius: 0.4px;
  border-radius: 0.5px;
}

Debugging:

diff --git a/lib/Less/Tree/Expression.php b/lib/Less/Tree/Expression.php
index 277fcf1713..43a9a14721 100644
--- a/lib/Less/Tree/Expression.php
+++ b/lib/Less/Tree/Expression.php
@@ -11,6 +11,9 @@ class Less_Tree_Expression extends Less_Tree implements Less_Tree_HasValueProper
 	public function __construct( $value, $parens = null ) {
 		$this->value = $value;
 		$this->parens = $parens;
+		if ( $this->parens ) {
+			throw new Exception('peekaboo');
+		}
 	}
 
 	public function accept( $visitor ) {
diff --git a/lib/Less/Tree/Mixin/Call.php b/lib/Less/Tree/Mixin/Call.php
index 2c0d49f492..15eae41c9c 100644
--- a/lib/Less/Tree/Mixin/Call.php
+++ b/lib/Less/Tree/Mixin/Call.php
@@ -122,7 +122,7 @@ class Less_Tree_Mixin_Call extends Less_Tree {
 						}
 						$rules = array_merge( $rules, $mixin->evalCall( $env, $args, $this->important )->rules );
 					} catch ( Exception $e ) {
-						throw new Less_Exception_Compiler( $e->getMessage(), null, null, $this->currentFileInfo );
+						throw new Less_Exception_Compiler( $e->getMessage(), $e, null, $this->currentFileInfo );
 					}
 				}
 			}
diff --git a/lib/Less/Tree/Operation.php b/lib/Less/Tree/Operation.php
index d5df55a9fe..4bdbd0ae78 100644
--- a/lib/Less/Tree/Operation.php
+++ b/lib/Less/Tree/Operation.php
@@ -25,11 +25,18 @@ class Less_Tree_Operation extends Less_Tree {
 		$a = $this->operands[0]->compile( $env );
 		$b = $this->operands[1]->compile( $env );
 
+		print json_encode([
+			'op' => $this->op,
+			'isMathOn' => $env->isMathOn(),
+			'parensStack' => Less_Environment::$parensStack
+		], JSON_UNESCAPED_SLASHES) . ' ' . $this->toCSS() . "\n";
+
 		// 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 ( $env->isMathOn() ) {
+			throw new Exception('boo');
 
 			if ( $a instanceof Less_Tree_Dimension && $b instanceof Less_Tree_Color ) {
 				$a = $a->toColor();

The mathsOn check appears to temporarily turn itself on, by something that is not parensStack.

This broke relatively recently:

$ cat input.less 
.border-radius(@r: 2px/5px) {
  border-radius: @r;
}
.slash-vs-math {
  .border-radius();
  .border-radius(5px/10px);
}
less.php$ git co v3.0.0
…

$ bin/lessc --strict-math=on input.less 
.slash-vs-math {
  border-radius: 2px/5px;
  border-radius: 5px/10px;
}
less.php$ git co v4.0.0
…
$ bin/lessc --strict-math=on input.less 
.slash-vs-math {
  border-radius: 2px/5px;
  border-radius: 5px/10px;
}
$ git co v4.2.0
…
$ bin/lessc --strict-math=on input.less 
.slash-vs-math {
  border-radius: 2px/5px;
  border-radius: 5px/10px;
}
$ git co master
HEAD is now at 299a47fa99 Merge "Fix unexpected duplicating of uncalled mixin rules"
$ bin/lessc --strict-math=on input.less 
.slash-vs-math {
  border-radius: 0.4px;
  border-radius: 0.5px;
}

git-bisect should help in finding the exact commit that caused it. Based on prior experience with the ImportVisitor, my guess would be that one of the temporary Environment object clones is performed incorrectly, which would explain how the parser option can be lost for a short time and then magically restored later.

This issue was introduced in https://gerrit.wikimedia.org/r/c/mediawiki/libs/less.php/+/1005131.

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

[mediawiki/libs/less.php@master] Copy strictMath param when creating a temporary env

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

gerritbot added a project: Patch-For-Review.Apr 26 2024, 12:07 PM

Change #1024648 merged by jenkins-bot:

[mediawiki/libs/less.php@master] Copy strictMath flag in temporary env for mixin definitions

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

pmiazga closed this task as Resolved.Apr 29 2024, 2:15 PM

Maintenance_bot removed a project: Patch-For-Review.Apr 29 2024, 2:30 PM

Krinkle added a parent task: T365481: Update less.php port to complete support Less.js 2.5.3 behaviours.May 21 2024, 2:26 PM

Krinkle removed a parent task: T288498: Update less.php port to support Less.js 3.13 behaviours.