From 76822b18f99b4fcdec309cb0db79637b396c9c4b Mon Sep 17 00:00:00 2001 From: Nick Behrens Date: Tue, 15 Oct 2019 21:35:14 -0700 Subject: [PATCH] Fix sass#417 preserve location of trailing loud comments. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parent 1a5102bedb2512e2bf71feb7d7cb93bea7019e8d author Nick Behrens 1571200514 -0700 committer Carlos Israel Ortiz García 1654024100 -0700 Delete now unused _indent helper method in serialize.dart. Update lib/src/visitor/serialize.dart to stop using old-style int-based for loop. Co-Authored-By: Natalie Weizenbaum Address PR feedback. Revert parser changes. Instead look at node span line numbers to determine whether a loud comment is trailing. Add some more test cases. Remove TODO in _isTrailingComment helper and add a better comment in there. Revert one unintentional chunk of edits to lib/src/parse/sass.dart from commit d937f878dabf46abe490c60b3fd4e3671a6ce299. Address some review feedback Update lib/src/visitor/serialize.dart to use var instead of CssNode as for loop variable. Co-Authored-By: Natalie Weizenbaum Addressing review feedback Remove some now irrelevant comments after last commit. Add some comments on some code I'd like some feedback on in the PR. Fix typo in recently added comment. Remove dupe impl of beforeChildren from CssStylesheet and some other minor cleanup. Rewrite _visitChildren to be simpler. Remove duplicate math import. Restore for loop in visitChildren to how it used to be. Comment cleanup. Don't add a trailing newline after an only-child trailing comment Minor style tweaks Short-circuit _isTrailingComment in compressed mode Update isTrailingComment to handle case where parent node contains left braces that don't open a child block. Address PR feedback, fixing indexing issue in isTrailingComment. Fix isTrailingComment to handle mixins that include loud comments. --- lib/src/visitor/serialize.dart | 121 ++++++++++++++++++++++++--------- test/output_test.dart | 115 +++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+), 34 deletions(-) diff --git a/lib/src/visitor/serialize.dart b/lib/src/visitor/serialize.dart index 1f1c0c9ec..4cdf844fb 100644 --- a/lib/src/visitor/serialize.dart +++ b/lib/src/visitor/serialize.dart @@ -153,14 +153,16 @@ class _SerializeVisitor void visitCssStylesheet(CssStylesheet node) { CssNode? previous; - for (var i = 0; i < node.children.length; i++) { - var child = node.children[i]; + for (var child in node.children) { if (_isInvisible(child)) continue; - if (previous != null) { if (_requiresSemicolon(previous)) _buffer.writeCharCode($semicolon); - _writeLineFeed(); - if (previous.isGroupEnd) _writeLineFeed(); + if (_isTrailingComment(child, previous)) { + _writeOptionalSpace(); + } else { + _writeLineFeed(); + if (previous.isGroupEnd) _writeLineFeed(); + } } previous = child; @@ -208,7 +210,7 @@ class _SerializeVisitor if (!node.isChildless) { _writeOptionalSpace(); - _visitChildren(node.children); + _visitChildren(node.children, node); } } @@ -226,7 +228,7 @@ class _SerializeVisitor }); _writeOptionalSpace(); - _visitChildren(node.children); + _visitChildren(node.children, node); } void visitCssImport(CssImport node) { @@ -273,7 +275,7 @@ class _SerializeVisitor () => _writeBetween(node.selector.value, _commaSeparator, _buffer.write)); _writeOptionalSpace(); - _visitChildren(node.children); + _visitChildren(node.children, node); } void _visitMediaQuery(CssMediaQuery query) { @@ -298,7 +300,7 @@ class _SerializeVisitor _for(node.selector, () => node.selector.value.accept(this)); _writeOptionalSpace(); - _visitChildren(node.children); + _visitChildren(node.children, node); } void visitCssSupportsRule(CssSupportsRule node) { @@ -315,7 +317,7 @@ class _SerializeVisitor }); _writeOptionalSpace(); - _visitChildren(node.children); + _visitChildren(node.children, node); } void visitCssDeclaration(CssDeclaration node) { @@ -1287,37 +1289,44 @@ class _SerializeVisitor _for(value, () => _buffer.write(value.value)); /// Emits [children] in a block. - void _visitChildren(List children) { + void _visitChildren(List children, CssParentNode parent) { _buffer.writeCharCode($lbrace); - if (children.every(_isInvisible)) { - _buffer.writeCharCode($rbrace); - return; - } - _writeLineFeed(); - CssNode? previous_; - _indent(() { - for (var i = 0; i < children.length; i++) { - var child = children[i]; - if (_isInvisible(child)) continue; + CssNode? prePrevious_; + CssNode previous = parent; + for (var child in children) { + if (_isInvisible(child)) continue; + if (previous != parent && _requiresSemicolon(previous)) { + _buffer.writeCharCode($semicolon); + } - var previous = previous_; // dart-lang/sdk#45348 - if (previous != null) { - if (_requiresSemicolon(previous)) _buffer.writeCharCode($semicolon); - _writeLineFeed(); - if (previous.isGroupEnd) _writeLineFeed(); - } - previous_ = child; + if (_isTrailingComment(child, previous)) { + _writeOptionalSpace(); + _withoutIndendation(() => child.accept(this)); + } else { + _writeLineFeed(); + _indent(() { + child.accept(this); + }); + } - child.accept(this); + prePrevious_ = previous; + previous = child; + } + + if (previous != parent) { + if (_requiresSemicolon(previous) && !_isCompressed) { + _buffer.writeCharCode($semicolon); } - }); - if (_requiresSemicolon(previous_!) && !_isCompressed) { - _buffer.writeCharCode($semicolon); + if (prePrevious_ == parent && _isTrailingComment(previous, prePrevious_)) { + _writeOptionalSpace(); + } else { + _writeLineFeed(); + _writeIndentation(); + } } - _writeLineFeed(); - _writeIndentation(); + _buffer.writeCharCode($rbrace); } @@ -1325,6 +1334,42 @@ class _SerializeVisitor bool _requiresSemicolon(CssNode? node) => node is CssParentNode ? node.isChildless : node is! CssComment; + /// Whether [node] represents a trailing comment when it appears after + /// [previous] in a sequence of nodes being serialized. + /// + /// Note [previous] could be either a sibling of [node] or the parent of + /// [node], with [node] being the first visible child. + bool _isTrailingComment(CssNode node, CssNode previous) { + // Short-circuit in compressed mode to avoid expensive span shenanigans + // (shespanigans?), since we're compressing all whitespace anyway. + if (_isCompressed) return false; + if (node is! CssComment) return false; + + var previousSpan = previous.span; + if (previous is CssParentNode && previous.children.contains(node)) { + // Walk back from just before the current node starts looking for the + // parent's left brace (to open the child block). This is safer than + // a simple forward search of the previousSpan.text as that might contain + // other left braces. + var searchFrom = node.span.start.offset - previousSpan.start.offset - 1; + if (searchFrom < 0) { + // This can happen when the loud comment is the first statement in a + // mixin that's later included. In that case, node.span.start.offset + // (representing the loud comment in the mixin) will be less than + // previousSpan.start.offset (representing the statement where the + // mixin is later included) ==> the loud comment cannot possibly be + // trailing. + return false; + } + + var endOffset = previousSpan.text.lastIndexOf("{", searchFrom); + endOffset = math.max(0, endOffset); + previousSpan = previousSpan.file.span( + previousSpan.start.offset, previousSpan.start.offset + endOffset); + } + return node.span.start.line == previousSpan.end.line; + } + /// Writes a line feed, unless this emitting compressed CSS. void _writeLineFeed() { if (!_isCompressed) _buffer.write(_lineFeed.text); @@ -1373,6 +1418,14 @@ class _SerializeVisitor _indentation--; } + /// Runs [callback] without any indentation. + void _withoutIndendation(void callback()) { + var savedIndentation = _indentation; + _indentation = 0; + callback(); + _indentation = savedIndentation; + } + /// Returns whether [node] is considered invisible. bool _isInvisible(CssNode node) { if (_inspect) return false; diff --git a/test/output_test.dart b/test/output_test.dart index df06bd42b..b6718d957 100644 --- a/test/output_test.dart +++ b/test/output_test.dart @@ -110,4 +110,119 @@ void main() { }); }); }); + + // Tests for sass/dart-sass#417. + // + // Note there's no need for "in Sass" cases as it's not possible to have + // trailing loud comments in the Sass syntax. + group("preserve trailing loud comments 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 open block (multi-line selector)", () { + expect(compileString(""" +selector1, +selector2 { /* please don't move me */ + name: value; +}"""), equals(""" +selector1, +selector2 { /* please don't move me */ + name: value; +}""")); + }); + + test("after close block", () { + expect(compileString(""" +selector { + name: value; +} /* please don't move me */"""), equals(""" +selector { + name: value; +} /* please don't move me */""")); + }); + + test("only content in block", () { + expect(compileString(""" +selector { + /* please don't move me */ +}"""), equals(""" +selector { + /* please don't move me */ +}""")); + }); + + test("only content in block (no newlines)", () { + expect(compileString(""" +selector { /* please don't move me */ }"""), equals(""" +selector { /* please don't move me */ }""")); + }); + + test("after property in block", () { + expect(compileString(""" +selector { + name1: value1; /* please don't move me 1 */ + name2: value2; /* please don't move me 2 */ + name3: value3; /* please don't move me 3 */ +}"""), equals(""" +selector { + name1: value1; /* please don't move me 1 */ + name2: value2; /* please don't move me 2 */ + name3: value3; /* please don't move me 3 */ +}""")); + }); + + test("after rule in block", () { + expect(compileString(""" +selector { + @rule1; /* please don't move me 1 */ + @rule2; /* please don't move me 2 */ + @rule3; /* please don't move me 3 */ +}"""), equals(""" +selector { + @rule1; /* please don't move me 1 */ + @rule2; /* please don't move me 2 */ + @rule3; /* please don't move me 3 */ +}""")); + }); + + test("after top-level statement", () { + expect(compileString("@rule; /* please don't move me */"), + equals("@rule; /* please don't move me */")); + }); + + // The trailing comment detection logic looks for left braces to detect + // whether a comment is on the same line as its parent node. This test + // checks to make sure it isn't confused by syntax that uses braces for + // things other than starting child blocks. + test("selector contains left brace", () { + expect(compileString("""@rule1; +@rule2; +selector[href*=\"{\"] +{ /* please don't move me */ } + +@rule3;"""), equals("""@rule1; +@rule2; +selector[href*=\"{\"] { /* please don't move me */ } + +@rule3;""")); + }); + + test("loud comments in mixin", () { + expect(compileString(""" +@mixin loudComment { + /* ... */ +} + +selector { + @include loudComment; +}"""), "selector {\n /* ... */\n}"); + }); + }); }