Fix #417 preserve location of trailing loud comments.

parent 1a5102b
author Nick Behrens <nbehrens@google.com> 1571200514 -0700
committer Carlos Israel Ortiz García <goodwine@google.com> 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 <nweiz@google.com>

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 d937f87.

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 <nweiz@google.com>

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

@@ -156,14 +156,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;
 
@@ -213,7 +215,7 @@ class _SerializeVisitor
 
     if (!node.isChildless) {
       _writeOptionalSpace();
-      _visitChildren(node.children);
+      _visitChildren(node.children, node);
     }
   }
 
@@ -231,7 +233,7 @@ class _SerializeVisitor
     });
 
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node.children, node);
   }
 
   void visitCssImport(CssImport node) {
@@ -282,7 +284,7 @@ class _SerializeVisitor
         () =>
             _writeBetween(node.selector.value, _commaSeparator, _buffer.write));
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node.children, node);
   }
 
   void _visitMediaQuery(CssMediaQuery query) {
@@ -307,7 +309,7 @@ class _SerializeVisitor
 
     _for(node.selector, () => node.selector.value.accept(this));
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node.children, node);
   }
 
   void visitCssSupportsRule(CssSupportsRule node) {
@@ -324,7 +326,7 @@ class _SerializeVisitor
     });
 
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node.children, node);
   }
 
   void visitCssDeclaration(CssDeclaration node) {
@@ -1078,43 +1080,87 @@ class _SerializeVisitor
       _for(value, () => _buffer.write(value.value));
 
   /// Emits [children] in a block.
-  void _visitChildren(List<CssNode> children) {
+  void _visitChildren(List<CssNode> 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);
+      }
 
-        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);
+        });
+      }
+
+      prePrevious = previous;
+      previous = child;
+    }
 
-        child.accept(this);
+    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);
   }
 
   /// Whether [node] requires a semicolon to be written after it.
   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);
@@ -1163,6 +1209,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;

test/output_test.dart

@@ -59,4 +59,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}");
+    });
+  });
 }