From 07b4a1b93a5247e807ace44ff11b09985ad4700d Mon Sep 17 00:00:00 2001 From: Nick Behrens Date: Tue, 15 Oct 2019 20:44:25 -0700 Subject: [PATCH] Fix sass#417 preserve location of trailing loud comments. --- lib/src/ast/css/comment.dart | 4 ++ lib/src/ast/css/modifiable/comment.dart | 4 +- lib/src/ast/sass/statement/loud_comment.dart | 6 +- lib/src/parse/parser.dart | 10 +++- lib/src/parse/sass.dart | 3 +- lib/src/parse/scss.dart | 39 ++++++++++--- lib/src/visitor/async_evaluate.dart | 5 +- lib/src/visitor/clone_css.dart | 2 +- lib/src/visitor/evaluate.dart | 9 +-- lib/src/visitor/serialize.dart | 58 ++++++++++++++++---- test/output_test.dart | 30 ++++++++++ 11 files changed, 138 insertions(+), 32 deletions(-) diff --git a/lib/src/ast/css/comment.dart b/lib/src/ast/css/comment.dart index 4724a83b2..04c8b8ebd 100644 --- a/lib/src/ast/css/comment.dart +++ b/lib/src/ast/css/comment.dart @@ -16,5 +16,9 @@ abstract class CssComment extends CssNode { /// compressed mode. bool get isPreserved; + /// Whether this comment follows non-comment text on a line and should remain + /// attached to that non-comment text when being serialized. + bool get isTrailing; + T accept(CssVisitor visitor) => visitor.visitCssComment(this); } diff --git a/lib/src/ast/css/modifiable/comment.dart b/lib/src/ast/css/modifiable/comment.dart index 40e7838c7..a6b7749bd 100644 --- a/lib/src/ast/css/modifiable/comment.dart +++ b/lib/src/ast/css/modifiable/comment.dart @@ -13,10 +13,12 @@ import 'node.dart'; class ModifiableCssComment extends ModifiableCssNode implements CssComment { final String text; final FileSpan span; + final bool _isTrailing; bool get isPreserved => text.codeUnitAt(2) == $exclamation; + bool get isTrailing => _isTrailing; - ModifiableCssComment(this.text, this.span); + ModifiableCssComment(this.text, this.span, this._isTrailing); T accept(ModifiableCssVisitor visitor) => visitor.visitCssComment(this); } diff --git a/lib/src/ast/sass/statement/loud_comment.dart b/lib/src/ast/sass/statement/loud_comment.dart index 41e824645..d9443f1b3 100644 --- a/lib/src/ast/sass/statement/loud_comment.dart +++ b/lib/src/ast/sass/statement/loud_comment.dart @@ -15,7 +15,11 @@ class LoudComment implements Statement { FileSpan get span => text.span; - LoudComment(this.text); + /// Whether this comment follows non-comment text on a line and should remain + /// attached to that non-comment text when being serialized. + final bool isTrailing; + + LoudComment(this.text, this.isTrailing); T accept(StatementVisitor visitor) => visitor.visitLoudComment(this); diff --git a/lib/src/parse/parser.dart b/lib/src/parse/parser.dart index 8fa257b07..8c7494652 100644 --- a/lib/src/parse/parser.dart +++ b/lib/src/parse/parser.dart @@ -79,11 +79,17 @@ class Parser { } /// Consumes whitespace, but not comments. + /// + /// Returns whether any newlines were seen in the consumed whitespace. @protected - void whitespaceWithoutComments() { + bool whitespaceWithoutComments() { + bool sawAnyNewlines = false; while (!scanner.isDone && isWhitespace(scanner.peekChar())) { - scanner.readChar(); + if (isNewline(scanner.readChar())) { + sawAnyNewlines = true; + } } + return sawAnyNewlines; } /// Consumes spaces and tabs. diff --git a/lib/src/parse/sass.dart b/lib/src/parse/sass.dart index bd2b89f4d..0a4d93a90 100644 --- a/lib/src/parse/sass.dart +++ b/lib/src/parse/sass.dart @@ -306,7 +306,8 @@ class SassParser extends StylesheetParser { } if (!buffer.trailingString.trimRight().endsWith("*/")) buffer.write(" */"); - return LoudComment(buffer.interpolation(scanner.spanFrom(start))); + // The sass syntax does not support trailing loud comments ==> isTrailing = false. + return LoudComment(buffer.interpolation(scanner.spanFrom(start)), false); } void whitespace() { diff --git a/lib/src/parse/scss.dart b/lib/src/parse/scss.dart index aec633d57..8dd7e0320 100644 --- a/lib/src/parse/scss.dart +++ b/lib/src/parse/scss.dart @@ -61,8 +61,8 @@ class ScssParser extends StylesheetParser { List children(Statement child()) { scanner.expectChar($lbrace); - whitespaceWithoutComments(); var children = []; + _whitespaceAndOptionalTrailingLoudComment(children); while (true) { switch (scanner.peekChar()) { case $dollar: @@ -76,7 +76,7 @@ class ScssParser extends StylesheetParser { whitespaceWithoutComments(); break; case $asterisk: - children.add(_loudComment()); + children.add(_loudComment(/* isTrailing= */ false)); whitespaceWithoutComments(); break; default: @@ -87,7 +87,7 @@ class ScssParser extends StylesheetParser { case $semicolon: scanner.readChar(); - whitespaceWithoutComments(); + _whitespaceAndOptionalTrailingLoudComment(children); break; case $rbrace: @@ -103,7 +103,7 @@ class ScssParser extends StylesheetParser { List statements(Statement statement()) { var statements = []; - whitespaceWithoutComments(); + _whitespaceAndOptionalTrailingLoudComment(statements); while (!scanner.isDone) { switch (scanner.peekChar()) { case $dollar: @@ -117,7 +117,7 @@ class ScssParser extends StylesheetParser { whitespaceWithoutComments(); break; case $asterisk: - statements.add(_loudComment()); + statements.add(_loudComment(/* isTrailing= */ false)); whitespaceWithoutComments(); break; default: @@ -129,7 +129,7 @@ class ScssParser extends StylesheetParser { case $semicolon: scanner.readChar(); - whitespaceWithoutComments(); + _whitespaceAndOptionalTrailingLoudComment(statements); break; default: @@ -163,7 +163,7 @@ class ScssParser extends StylesheetParser { } /// Consumes a statement-level loud comment block. - LoudComment _loudComment() { + LoudComment _loudComment(bool isTrailing) { var start = scanner.state; scanner.expect("/*"); var buffer = InterpolationBuffer()..write("/*"); @@ -182,7 +182,8 @@ class ScssParser extends StylesheetParser { if (scanner.peekChar() != $slash) break; buffer.writeCharCode(scanner.readChar()); - return LoudComment(buffer.interpolation(scanner.spanFrom(start))); + return LoudComment( + buffer.interpolation(scanner.spanFrom(start)), isTrailing); case $cr: scanner.readChar(); @@ -200,4 +201,26 @@ class ScssParser extends StylesheetParser { } } } + + /// Consumes whitespace and optionally a trailing loud comment, adding the + /// trailing loud comment to [statements] if consumed. + /// + /// A trailing loud comment is only possible if the initially consumed + /// whitespace lacks any newlines. If that's the case and a loud comment + /// immediately follows, the loud comment will also be consumed. + void _whitespaceAndOptionalTrailingLoudComment(List statements) { + bool sawAnyNewlines = whitespaceWithoutComments(); + if (_peekForStartOfLoudComment()) { + bool isTrailing = !sawAnyNewlines; + statements.add(_loudComment(isTrailing)); + whitespaceWithoutComments(); + } + } + + /// Returns whether the scanner is pointing at the start of a loud comment. + bool _peekForStartOfLoudComment() { + return !scanner.isDone && + scanner.peekChar() == $slash && + scanner.peekChar(1) == $asterisk; + } } diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index 2f5cb2e31..edc2d02f1 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -1500,7 +1500,7 @@ class _EvaluateVisitor } _parent.addChild(ModifiableCssComment( - await _performInterpolation(node.text), node.span)); + await _performInterpolation(node.text), node.span, node.isTrailing)); return null; } @@ -2424,7 +2424,8 @@ class _EvaluateVisitor _endOfImports++; } - _parent.addChild(ModifiableCssComment(node.text, node.span)); + _parent + .addChild(ModifiableCssComment(node.text, node.span, node.isTrailing)); } Future visitCssDeclaration(CssDeclaration node) async { diff --git a/lib/src/visitor/clone_css.dart b/lib/src/visitor/clone_css.dart index c5d9f855f..f317859d4 100644 --- a/lib/src/visitor/clone_css.dart +++ b/lib/src/visitor/clone_css.dart @@ -37,7 +37,7 @@ class _CloneCssVisitor implements CssVisitor { } ModifiableCssComment visitCssComment(CssComment node) => - ModifiableCssComment(node.text, node.span); + ModifiableCssComment(node.text, node.span, node.isTrailing); ModifiableCssDeclaration visitCssDeclaration(CssDeclaration node) => ModifiableCssDeclaration(node.name, node.value, node.span, diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index 38031840d..3bd895346 100644 --- a/lib/src/visitor/evaluate.dart +++ b/lib/src/visitor/evaluate.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_evaluate.dart. // See tool/grind/synchronize.dart for details. // -// Checksum: f4f4c5d1cbc9894d14b6d8ce7c1a3c09146db9ba +// Checksum: 56067ad7a31ee7adf5bb4e9d484fe95f38d3762c // // ignore_for_file: unused_import @@ -1498,8 +1498,8 @@ class _EvaluateVisitor _endOfImports++; } - _parent.addChild( - ModifiableCssComment(_performInterpolation(node.text), node.span)); + _parent.addChild(ModifiableCssComment( + _performInterpolation(node.text), node.span, node.isTrailing)); return null; } @@ -2409,7 +2409,8 @@ class _EvaluateVisitor _endOfImports++; } - _parent.addChild(ModifiableCssComment(node.text, node.span)); + _parent + .addChild(ModifiableCssComment(node.text, node.span, node.isTrailing)); } void visitCssDeclaration(CssDeclaration node) { diff --git a/lib/src/visitor/serialize.dart b/lib/src/visitor/serialize.dart index 34ce654ae..8fee59c56 100644 --- a/lib/src/visitor/serialize.dart +++ b/lib/src/visitor/serialize.dart @@ -162,7 +162,12 @@ class _SerializeVisitor if (previous != null) { if (_requiresSemicolon(previous)) _buffer.writeCharCode($semicolon); - _writeLineFeed(); + if (_shouldWriteLineFeedBeforeChild(child)) { + _writeLineFeed(); + } else { + _writeOptionalSpace(); + } + if (previous.isGroupEnd) _writeLineFeed(); } previous = child; @@ -1085,23 +1090,41 @@ class _SerializeVisitor return; } - _writeLineFeed(); + bool indentNextChild; + if (_shouldWriteLineFeedBeforeChildren(children)) { + _writeLineFeed(); + indentNextChild = true; + } else { + _writeOptionalSpace(); + indentNextChild = false; + } + CssNode previous; - _indent(() { - for (var i = 0; i < children.length; i++) { - var child = children[i]; - if (_isInvisible(child)) continue; + for (var i = 0; i < children.length; i++) { + var child = children[i]; + if (_isInvisible(child)) continue; - if (previous != null) { - if (_requiresSemicolon(previous)) _buffer.writeCharCode($semicolon); + if (previous != null) { + if (_requiresSemicolon(previous)) _buffer.writeCharCode($semicolon); + if (_shouldWriteLineFeedBeforeChild(child)) { _writeLineFeed(); - if (previous.isGroupEnd) _writeLineFeed(); + indentNextChild = true; + } else { + _writeOptionalSpace(); + indentNextChild = false; } - previous = child; + if (previous.isGroupEnd) _writeLineFeed(); + } + previous = child; - child.accept(this); + if (indentNextChild) { + _indentation++; } - }); + child.accept(this); + if (indentNextChild) { + _indentation--; + } + } if (_requiresSemicolon(previous) && !_isCompressed) { _buffer.writeCharCode($semicolon); @@ -1115,6 +1138,17 @@ class _SerializeVisitor bool _requiresSemicolon(CssNode node) => node is CssParentNode ? node.isChildless : node is! CssComment; + /// Whether [child] should have a line feed written before the child is serialized. + bool _shouldWriteLineFeedBeforeChild(CssNode child) => + child is! CssComment || !(child as CssComment).isTrailing; + + /// Whether [children] should have a line feed written before the children are serialized. + bool _shouldWriteLineFeedBeforeChildren(List children) { + return children.isEmpty || + !(children.first is CssComment) || + !((children.first as CssComment).isTrailing); + } + /// Writes a line feed, unless this emitting compressed CSS. void _writeLineFeed() { if (!_isCompressed) _buffer.write(_lineFeed.text); diff --git a/test/output_test.dart b/test/output_test.dart index 582e59d6c..484d5cb4c 100644 --- a/test/output_test.dart +++ b/test/output_test.dart @@ -59,4 +59,34 @@ void main() { }); }); }); + + // Regression test for sass/dart-sass#417. + group("preserve trailing loud comments", () { + // No need for "in Sass" cases as it's not possible to have + // trailing loud comments in the Sass syntax. + group("in SCSS", () { + test("after open block", () { + expect(compileString(""" +selector { /* please don't move me */ + name: value; +}"""), equals(""" +selector { /* please don't move me */ + name: value; +}""")); + }); + test("after declaration", () { + expect(compileString(""" +selector { + name: value; /* please don't move me */ +}"""), equals(""" +selector { + name: value; /* please don't move me */ +}""")); + }); + test("after top-level statement", () { + expect(compileString("@rule; /* please don't move me */"), + equals("@rule; /* please don't move me */")); + }); + }); + }); }