Skip to content

Commit

Permalink
Fix ParseError for comments in more places and preserve them in output
Browse files Browse the repository at this point in the history 
This implements the commentStore feature in the Less_Parser.

To be able to parse and re-inject comments we need to store
them in the commentStore instead of just eating up. In cases
where we cannot render comments the commentStore array will
be cleaned up.

This causes situations when instead of a single argument we
might pass the Less_Tree_Exception. As per less.js we
introduce the functionCaller (for now just a method instead
of additional class) that will clean up Expresssions from
comments before calling a function.

This fixes the following crash, and more generally preserves comments
in various places where we ate them previously.

```
bin/lessc test/Fixtures/lessjs-3.13.1/less/_main/comments2.less

ParseError: unmatched `/*` in comments2.less on line 4, column 53
2| @-webkit-keyframes hover /* Safari and Chrome */{  }
3| .bg {
4|   background-image: linear-gradient(#333 /*{comment}*/, wikimedia#111);
```

Bug: T353132
Change-Id: I99c42a1969762869023e91deb72f31773b43a06e
  • Loading branch information
@polishdeveloper @Krinkle
polishdeveloper authored and Krinkle committed Apr 23, 2024
1 parent 8c8f446 commit 76b1210
Show file tree
Hide file tree
Showing 8 changed files with 161 additions and 170 deletions.
166 changes: 100 additions & 66 deletions lib/Less/Parser.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,12 @@ class Less_Parser {
private $furthest;
private $mb_internal_encoding = ''; // for remember exists value of mbstring.internal_encoding

private $autoCommentAbsorb = true;
/**
* @var array<array{index:int,text:string,isLineComment:bool}>
*/
private $commentStore = [];

/**
* @var Less_Environment
*/
Expand Down Expand Up @@ -659,7 +665,7 @@ private function GetRules( $file_path ) {
}
}
}

$this->skipWhitespace( 0 );
$rules = $this->parsePrimary();

if ( $this->pos < $this->input_len ) {
Expand Down Expand Up @@ -853,8 +859,51 @@ private function peekChar( $tok ) {
*/
private function skipWhitespace( $length ) {
$this->pos += $length;
// Optimization: Skip over irrelevant chars without slow loop
$this->pos += strspn( $this->input, "\n\r\t ", $this->pos );

// we do not increment the $this->pos as upstream, because $this->pos is updated with
// comment length or by amount of whitespaces strsspn finds. What upstream does is ignores
// whitespaces, which makes the for loop keep iterating.
// phpcs:ignore Generic.CodeAnalysis.ForLoopShouldBeWhileLoop.CanSimplify
for ( ; $this->pos < $this->input_len; ) {
$currentChar = $this->input[$this->pos];

if ( $this->autoCommentAbsorb && $currentChar === '/' ) {
$nextChar = $this->input[$this->pos + 1] ?? '';
if ( $nextChar === '/' ) {
$comment = [ 'index' => $this->pos, 'isLineComment' => true ];
$nextNewLine = strpos( $this->input, "\n", $this->pos + 2 );
if ( $nextNewLine === false ) {
$nextNewLine = $this->input_len ?? 0;
}
$this->pos = $nextNewLine;
$comment['text'] = substr( $this->input, $this->pos, $nextNewLine - $this->pos );
$this->commentStore[] = $comment;
continue;
} elseif ( $nextChar === '*' ) {
$nextStarSlash = strpos( $this->input, "*/", $this->pos + 2 );
if ( $nextStarSlash ) {
$comment = [
'text' => substr( $this->input, $this->pos, $nextStarSlash + 2 -
$this->pos ),
'isLineComment' => false,
'index' => $this->pos
];
$this->commentStore[] = $comment;
$this->pos += strlen( $comment['text'] );
continue;
}
}
break;
}

if ( $currentChar !== " " && $currentChar !== "\n"
&& $currentChar !== "\t" && $currentChar !== "\r" ) {
break;
}

// Optimization: Skip over irrelevant chars without slow loop
$this->pos += strspn( $this->input, "\n\r\t ", $this->pos );
}
}

/**
Expand Down Expand Up @@ -941,18 +990,30 @@ private function parsePrimary() {

while ( true ) {

while ( true ) {
$node = $this->parseComment();
if ( !$node ) {
break;
}
$root[] = $node;
}

// always process comments before deciding if finished
if ( $this->pos >= $this->input_len ) {
break;
}

if ( $this->peekChar( '}' ) ) {
break;
}

$node = $this->parseExtend( true );
if ( $node ) {
$root = array_merge( $root, $node );
continue;
}

$node = $this->parseComment()
?? $this->parseMixinDefinition()
$node = $this->parseMixinDefinition()
// Optimisation: NameValue is specific to less.php
?? $this->parseNameValue()
?? $this->parseRule()
Expand All @@ -967,52 +1028,28 @@ private function parsePrimary() {
break;
}

if ( $this->peekChar( '}' ) ) {
break;
}
}

return $root;
}

// We create a Comment node for CSS comments `/* */`,
// but keep the LeSS comments `//` silent, by just skipping
// over them.
/**
* comments are collected by the main parsing mechanism and then assigned to nodes
* where the current structure allows it
* @return Less_Tree_Comment|void
*/
private function parseComment() {
$char = $this->input[$this->pos] ?? null;
if ( $char !== '/' ) {
return;
}

$nextChar = $this->input[$this->pos + 1] ?? null;
if ( $nextChar === '/' ) {
$match = $this->matchReg( '/\\G\/\/.*/' );
return new Less_Tree_Comment( $match, true, $this->pos, $this->env->currentFileInfo );
}

// not the same as less.js to prevent fatal errors
// $this->matchReg( '/\\G\/\*(?:[^*]|\*+[^\/*])*\*+\/\n?/') ;
$comment = $this->matchReg( '/\\G\/\*(?s).*?\*+\/\n?/' );
$comment = array_shift( $this->commentStore );
if ( $comment ) {
return new Less_Tree_Comment( $comment, false, $this->pos, $this->env->currentFileInfo );
return new Less_Tree_Comment(
$comment['text'],
$comment['isLineComment'],
$comment['index'],
$this->env->currentFileInfo
);
}
}

private function parseComments() {
$comments = [];

while ( $this->pos < $this->input_len ) {
$comment = $this->parseComment();
if ( !$comment ) {
break;
}

$comments[] = $comment;
}

return $comments;
}

/**
* A string, which supports escaping " and '
*
Expand Down Expand Up @@ -1194,16 +1231,19 @@ private function parseEntitiesAssignment() {
//
private function parseEntitiesUrl() {
$char = $this->input[$this->pos] ?? null;

$this->autoCommentAbsorb = false;
// Optimisation: 'u' check is specific to less.php
if ( $char !== 'u' || !$this->matchReg( '/\\Gurl\(/' ) ) {
$this->autoCommentAbsorb = true;
return;
}

$value = $this->parseEntitiesQuoted() ?? $this->parseEntitiesVariable() ?? $this->matchReg( '/\\Gdata\:.*?[^\)]+/' ) ?? $this->matchReg( '/\\G(?:(?:\\\\[\(\)\'"])|[^\(\)\'"])+/' ) ?? null;
if ( !$value ) {
$value = '';
}

$this->autoCommentAbsorb = true;
$this->expectChar( ')' );

if ( $value instanceof Less_Tree_Quoted || $value instanceof Less_Tree_Variable ) {
Expand Down Expand Up @@ -1488,7 +1528,7 @@ private function parseMixinArgs( $isCall ) {
if ( $isCall ) {
$arg = $this->parseDetachedRuleset() ?? $this->parseExpression();
} else {
$this->parseComments();
$this->commentStore = [];
if ( $this->input[ $this->pos ] === '.' && $this->matchStr( '...' ) ) {
$returner['variadic'] = true;
if ( $this->matchChar( ";" ) && !$isSemiColonSeperated ) {
Expand Down Expand Up @@ -1651,7 +1691,7 @@ private function parseMixinDefinition() {
return;
}

$this->parseComments();
$this->commentStore = [];

if ( $this->matchStr( 'when' ) ) { // Guard
$cond = $this->expect( 'parseConditions', 'Expected conditions' );
Expand All @@ -1675,13 +1715,13 @@ private function parseMixinDefinition() {
// and can be found inside a rule's value.
//
private function parseEntity() {
return $this->parseEntitiesLiteral() ??
return $this->parseComment() ??
$this->parseEntitiesLiteral() ??
$this->parseEntitiesVariable() ??
$this->parseEntitiesUrl() ??
$this->parseEntitiesCall() ??
$this->parseEntitiesKeyword() ??
$this->parseEntitiesJavascript() ??
$this->parseComment();
$this->parseEntitiesJavascript();
}

//
Expand Down Expand Up @@ -1942,7 +1982,7 @@ private function parseRuleset() {
break;
}
$selectors[] = $s;
$this->parseComments();
$this->commentStore = [];

if ( $s->condition && count( $selectors ) > 1 ) {
$this->Error( 'Guards are only currently allowed on a single selector.' );
Expand All @@ -1954,7 +1994,7 @@ private function parseRuleset() {
if ( $s->condition ) {
$this->Error( 'Guards are only currently allowed on a single selector.' );
}
$this->parseComments();
$this->commentStore = [];
}

if ( $selectors ) {
Expand Down Expand Up @@ -1983,6 +2023,9 @@ private function parseNameValue() {
if ( $match ) {

if ( $match[4] == '}' ) {
// because we will parse all comments after closing }, we need to reset the store as
// we're going to reset the position to closing }
$this->commentStore = [];
$this->pos = $index + strlen( $match[0] ) - 1;
$match[2] = rtrim( $match[2] );
}
Expand Down Expand Up @@ -2019,7 +2062,7 @@ private function parseRule( $tryAnonymous = null ) {
if ( $isVariable ) {
$value = $this->parseDetachedRuleset();
}
$this->parseComments();
$this->commentStore = [];
if ( !$value ) {
// a name returned by this.ruleProperty() is always an array of the form:
// [string-1, ..., string-n, ""] or [string-1, ..., string-n, "+"]
Expand Down Expand Up @@ -2303,8 +2346,8 @@ private function parseDirective() {
$isRooted = false;
break;
}
// TODO: T353132 - differs from less.js - we don't have the ParserInput yet
$this->parseComments();

$this->commentStore = [];

if ( $hasIdentifier ) {
$value = $this->parseEntity();
Expand All @@ -2324,9 +2367,6 @@ private function parseDirective() {
}
}

// TODO: T353132 - differs from less.js - we don't have the ParserInput yet
$this->parseComments();

if ( $hasBlock ) {
$rules = $this->parseBlockRuleset();
}
Expand Down Expand Up @@ -2551,6 +2591,11 @@ private function parseExpression() {
$entities = [];

do {
$e = $this->parseComment();
if ( $e ) {
$entities[] = $e;
continue;
}
$e = $this->parseAddition() ?? $this->parseEntity();
if ( $e ) {
$entities[] = $e;
Expand Down Expand Up @@ -2607,8 +2652,6 @@ private function parseRuleProperty() {
// Consume!
// @phan-suppress-next-line PhanPluginEmptyStatementWhileLoop
while ( $this->rulePropertyMatch( '/\\G((?:[\w-]+)|(?:@\{[\w-]+\}))/', $index, $name ) );
// @phan-suppress-next-line PhanPluginEmptyStatementWhileLoop
while ( $this->rulePropertyCutOutBlockComments() );

if ( ( count( $name ) > 1 ) && $this->rulePropertyMatch( '/\\G((?:\+_|\+)?)\s*:/', $index, $name ) ) {
$this->forget();
Expand Down Expand Up @@ -2642,15 +2685,6 @@ private function rulePropertyMatch( $re, &$index, &$name ) {
}
}

private function rulePropertyCutOutBlockComments() {
// not the same as less.js to prevent fatal errors
// similar to parseComment()
// '/\\G\s*\/\*(?:[^*]|\*+[^\/*])*\*+\//'
$re = '/\\G\s*\/\*(?s).*?\*+\//';
$chunk = $this->matchReg( $re );
return $chunk != null;
}

public static function serializeVars( $vars ) {
$s = '';

Expand Down
34 changes: 22 additions & 12 deletions lib/Less/Tree/Call.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,27 @@ public function accept( $visitor ) {
$this->args = $visitor->visitArray( $this->args );
}

/**
* @see less-2.5.3.js#functionCaller.call
*/
private function functionCaller( $function, array $arguments ) {
// This code is terrible and should be replaced as per this issue...
// https://github.com/less/less.js/issues/2477
$filtered = [];
foreach ( $arguments as $argument ) {
if ( $argument instanceof Less_Tree_Comment ) {
continue;
}
$filtered[] = $argument;
}
foreach ( $filtered as $index => $argument ) {
if ( $argument instanceof Less_Tree_Expression ) {
$filtered[$index] = $argument->mapToFunctionCallArgument();
}
}
return $function( ...$filtered );
}

//
// When evaluating a function call,
// we either find the function in Less_Functions,
Expand All @@ -46,17 +67,6 @@ public function compile( $env ) {
$args[] = $a->compile( $env );
}

foreach ( $args as $key => $arg ) {
if ( $arg instanceof Less_Tree_Expression ) {
$arg->throwAwayComments();

if ( count( $arg->value ) === 1 ) {
$subNode = $arg->value[0];
array_splice( $args, $key, 1, [ $subNode ] );
}
}
}

$env->strictMath = $currentMathContext;

$nameLC = strtolower( $this->name );
Expand Down Expand Up @@ -92,7 +102,7 @@ public function compile( $env ) {
// If the function name isn't known to LESS, output it unchanged as CSS.
if ( $func ) {
try {
$result = $func( ...$args );
$result = $this->functionCaller( $func, $args );
} catch ( Exception $e ) {
// Preserve original trace, especially from custom functions.
// https://github.com/wikimedia/less.php/issues/38
Expand Down
Loading

0 comments on commit 76b1210

Please sign in to comment.