Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse silent comments in _interpolatedDeclarationValue() #2266

Merged
merged 1 commit into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
## 1.77.7

* **Potentially breaking bug fix:** `//` in certain places such as unknown
at-rule values was being preserved in the CSS output, leading to potentially
invalid CSS. It's now properly parsed as a silent comment and omitted from the
CSS output.

## 1.77.6

* Fix a few cases where comments and occasionally even whitespace wasn't allowed
Expand Down
3 changes: 2 additions & 1 deletion lib/src/parse/parser.dart
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,8 @@ class Parser {
whitespace();
}

/// Consumes and ignores a silent (Sass-style) comment.
/// Consumes and ignores a single silent (Sass-style) comment, not including
/// the trailing newline.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"not including the trailing newline" makes me wonder about what happens when there are consecutive lines with silent comments, but otherwise LGTM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will only consume one of them, but whitespace() will consume them all and the whitespace.

///
/// Returns whether the comment was consumed.
@protected
Expand Down
86 changes: 66 additions & 20 deletions lib/src/parse/stylesheet.dart
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,8 @@ abstract class StylesheetParser extends Parser {
// Parse custom properties as declarations no matter what.
var name = nameBuffer.interpolation(scanner.spanFrom(start, beforeColon));
if (name.initialPlain.startsWith('--')) {
var value = StringExpression(_interpolatedDeclarationValue());
var value = StringExpression(
_interpolatedDeclarationValue(silentComments: false));
expectStatementSeparator("custom property");
return Declaration(name, value, scanner.spanFrom(start));
}
Expand Down Expand Up @@ -532,7 +533,8 @@ abstract class StylesheetParser extends Parser {
scanner.expectChar($colon);

if (parseCustomProperties && name.initialPlain.startsWith('--')) {
var value = StringExpression(_interpolatedDeclarationValue());
var value = StringExpression(
_interpolatedDeclarationValue(silentComments: false));
expectStatementSeparator("custom property");
return Declaration(name, value, scanner.spanFrom(start));
}
Expand Down Expand Up @@ -1550,7 +1552,7 @@ abstract class StylesheetParser extends Parser {

Interpolation? value;
if (scanner.peekChar() != $exclamation && !atEndOfStatement()) {
value = almostAnyValue();
value = _interpolatedDeclarationValue(allowOpenBrace: false);
}

AtRule rule;
Expand All @@ -1575,7 +1577,7 @@ abstract class StylesheetParser extends Parser {
/// This declares a return type of [Statement] so that it can be returned
/// within case statements.
Statement _disallowedAtRule(LineScannerState start) {
almostAnyValue();
_interpolatedDeclarationValue(allowEmpty: true, allowOpenBrace: false);
error("This at-rule is not allowed here.", scanner.spanFrom(start));
}

Expand Down Expand Up @@ -2748,13 +2750,11 @@ abstract class StylesheetParser extends Parser {
///
/// Differences from [_interpolatedDeclarationValue] include:
///
/// * This does not balance brackets.
/// * This always stops at curly braces.
///
/// * This does not interpret backslashes, since the text is expected to be
/// re-parsed.
///
/// * This supports Sass-style single-line comments.
///
/// * This does not compress adjacent whitespace characters.
@protected
Interpolation almostAnyValue({bool omitComments = false}) {
Expand All @@ -2773,11 +2773,21 @@ abstract class StylesheetParser extends Parser {
buffer.addInterpolation(interpolatedString().asInterpolation());

case $slash:
var commentStart = scanner.position;
if (scanComment()) {
if (!omitComments) buffer.write(scanner.substring(commentStart));
} else {
buffer.writeCharCode(scanner.readChar());
switch (scanner.peekChar(1)) {
case $asterisk when !omitComments:
buffer.write(rawText(loudComment));

case $asterisk:
loudComment();

case $slash when !omitComments:
buffer.write(rawText(silentComment));

case $slash:
silentComment();

case _:
buffer.writeCharCode(scanner.readChar());
}

case $hash when scanner.peekChar(1) == $lbrace:
Expand All @@ -2794,12 +2804,17 @@ abstract class StylesheetParser extends Parser {

case $u || $U:
var beforeUrl = scanner.state;
if (!scanIdentifier("url")) {
buffer.writeCharCode(scanner.readChar());
var identifier = this.identifier();
if (identifier != "url" &&
// This isn't actually a standard CSS feature, but it was
// supported by the old `@document` rule so we continue to support
// it for backwards-compatibility.
identifier != "url-prefix") {
buffer.write(identifier);
continue loop;
}

if (_tryUrlContents(beforeUrl) case var contents?) {
if (_tryUrlContents(beforeUrl, name: identifier) case var contents?) {
buffer.addInterpolation(contents);
} else {
scanner.state = beforeUrl;
Expand Down Expand Up @@ -2830,11 +2845,19 @@ abstract class StylesheetParser extends Parser {
///
/// If [allowColon] is `false`, this stops at top-level colons.
///
/// If [allowOpenBrace] is `false`, this stops at top-level colons.
///
/// If [silentComments] is `true`, this will parse silent comments as
/// comments. Otherwise, it will preserve two adjacent slashes and emit them
/// to CSS.
///
/// Unlike [declarationValue], this allows interpolation.
Interpolation _interpolatedDeclarationValue(
{bool allowEmpty = false,
bool allowSemicolon = false,
bool allowColon = true}) {
bool allowColon = true,
bool allowOpenBrace = true,
bool silentComments = true}) {
// NOTE: this logic is largely duplicated in Parser.declarationValue. Most
// changes here should be mirrored there.

Expand All @@ -2854,7 +2877,22 @@ abstract class StylesheetParser extends Parser {
buffer.addInterpolation(interpolatedString().asInterpolation());
wroteNewline = false;

case $slash when scanner.peekChar(1) == $asterisk:
case $slash:
switch (scanner.peekChar(1)) {
case $asterisk:
buffer.write(rawText(loudComment));
wroteNewline = false;

case $slash when silentComments:
silentComment();
wroteNewline = false;

case _:
buffer.writeCharCode(scanner.readChar());
wroteNewline = false;
}

case $slash when silentComments && scanner.peekChar(1) == $slash:
buffer.write(rawText(loudComment));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this really parse a loud comment when finding // ? Or is this dead code because of going through the previous case all the time ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that it's incorrect and also that it's dead code. I'll send out a PR to remove it.

wroteNewline = false;

Expand Down Expand Up @@ -2882,6 +2920,9 @@ abstract class StylesheetParser extends Parser {
scanner.readChar();
wroteNewline = true;

case $lbrace when !allowOpenBrace:
break loop;

case $lparen || $lbrace || $lbracket:
var bracket = scanner.readChar();
buffer.writeCharCode(bracket);
Expand All @@ -2907,13 +2948,18 @@ abstract class StylesheetParser extends Parser {

case $u || $U:
var beforeUrl = scanner.state;
if (!scanIdentifier("url")) {
buffer.writeCharCode(scanner.readChar());
var identifier = this.identifier();
if (identifier != "url" &&
// This isn't actually a standard CSS feature, but it was
// supported by the old `@document` rule so we continue to support
// it for backwards-compatibility.
identifier != "url-prefix") {
buffer.write(identifier);
wroteNewline = false;
continue loop;
}

if (_tryUrlContents(beforeUrl) case var contents?) {
if (_tryUrlContents(beforeUrl, name: identifier) case var contents?) {
buffer.addInterpolation(contents);
} else {
scanner.state = beforeUrl;
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: sass
version: 1.77.6
version: 1.77.7-dev
description: A Sass implementation in Dart.
homepage: https://github.com/sass/dart-sass

Expand Down
Loading