Fix #417 preserve the location of trailing loud comments (#849)

See sass/sass-spec#1485

- Update lib/src/visitor/serialize.dart to stop using old-style int-based for loop.
- Extend FileSpan with a .contains(targetSpan) method

Co-authored-by:  Nick Behrens <nbehrens@google.com>
Co-authored-by:  Carlos Israel Ortiz García <goodwine@google.com>
Co-Authored-By: Natalie Weizenbaum <nweiz@google.com>

lib/src/util/span.dart

@@ -77,6 +77,15 @@ extension SpanExtensions on FileSpan {
     _scanIdentifier(scanner);
     return subspan(scanner.position).trimLeft();
   }
+
+  /// Whether [this] FileSpan contains the [target] FileSpan.
+  ///
+  /// Validates the FileSpans to be in the same file and for the [target] to be
+  /// within [this] FileSpan inclusive range [start,end].
+  bool contains(FileSpan target) =>
+      file.url == target.file.url &&
+      start.offset <= target.start.offset &&
+      end.offset >= target.end.offset;
 }
 
 /// Consumes an identifier from [scanner].

lib/src/visitor/serialize.dart

@@ -21,6 +21,7 @@ import '../util/character.dart';
 import '../util/no_source_map_buffer.dart';
 import '../util/number.dart';
 import '../util/source_map_buffer.dart';
+import '../util/span.dart';
 import '../value.dart';
 import 'interface/css.dart';
 import 'interface/selector.dart';
@@ -153,14 +154,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 +211,7 @@ class _SerializeVisitor
 
     if (!node.isChildless) {
       _writeOptionalSpace();
-      _visitChildren(node.children);
+      _visitChildren(node);
     }
   }
 
@@ -226,7 +229,7 @@ class _SerializeVisitor
     });
 
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node);
   }
 
   void visitCssImport(CssImport node) {
@@ -273,7 +276,7 @@ class _SerializeVisitor
         () =>
             _writeBetween(node.selector.value, _commaSeparator, _buffer.write));
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node);
   }
 
   void _visitMediaQuery(CssMediaQuery query) {
@@ -298,7 +301,7 @@ class _SerializeVisitor
 
     _for(node.selector, () => node.selector.value.accept(this));
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node);
   }
 
   void visitCssSupportsRule(CssSupportsRule node) {
@@ -315,7 +318,7 @@ class _SerializeVisitor
     });
 
     _writeOptionalSpace();
-    _visitChildren(node.children);
+    _visitChildren(node);
   }
 
   void visitCssDeclaration(CssDeclaration node) {
@@ -1286,45 +1289,80 @@ class _SerializeVisitor
   void _write(CssValue<String> value) =>
       _for(value, () => _buffer.write(value.value));
 
-  /// Emits [children] in a block.
-  void _visitChildren(List<CssNode> children) {
+  /// Emits [parent.children] in a block.
+  void _visitChildren(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;
+    for (var child in parent.children) {
+      if (_isInvisible(child)) continue;
 
-        var previous = previous_; // dart-lang/sdk#45348
-        if (previous != null) {
-          if (_requiresSemicolon(previous)) _buffer.writeCharCode($semicolon);
-          _writeLineFeed();
-          if (previous.isGroupEnd) _writeLineFeed();
-        }
-        previous_ = child;
-
-        child.accept(this);
+      if (previous != null && _requiresSemicolon(previous)) {
+        _buffer.writeCharCode($semicolon);
       }
-    });
 
-    if (_requiresSemicolon(previous_!) && !_isCompressed) {
-      _buffer.writeCharCode($semicolon);
+      if (_isTrailingComment(child, previous ?? parent)) {
+        _writeOptionalSpace();
+        _withoutIndendation(() => child.accept(this));
+      } else {
+        _writeLineFeed();
+        _indent(() {
+          child.accept(this);
+        });
+      }
+
+      prePrevious = previous;
+      previous = child;
     }
-    _writeLineFeed();
-    _writeIndentation();
+
+    if (previous != null) {
+      if (_requiresSemicolon(previous) && !_isCompressed) {
+        _buffer.writeCharCode($semicolon);
+      }
+
+      if (prePrevious == null && _isTrailingComment(previous, parent)) {
+        _writeOptionalSpace();
+      } else {
+        _writeLineFeed();
+        _writeIndentation();
+      }
+    }
+
     _buffer.writeCharCode($rbrace);
   }
 
   /// Whether [node] requires a semicolon to be written after it.
-  bool _requiresSemicolon(CssNode? node) =>
+  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;
+
+    if (!previous.span.contains(node.span)) {
+      return node.span.start.line == previous.span.end.line;
+    }
+
+    // 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 previous.span.text as that might contain
+    // other left braces.
+    var searchFrom = node.span.start.offset - previous.span.start.offset - 1;
+    var endOffset = previous.span.text.lastIndexOf("{", searchFrom);
+    endOffset = math.max(0, endOffset);
+    var span = previous.span.file.span(
+        previous.span.start.offset, previous.span.start.offset + endOffset);
+    return node.span.start.line == span.end.line;
+  }
+
   /// Writes a line feed, unless this emitting compressed CSS.
   void _writeLineFeed() {
     if (!_isCompressed) _buffer.write(_lineFeed.text);
@@ -1373,6 +1411,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/dart_api_test.dart

@@ -304,7 +304,7 @@ a {
     expect(compileStringAsync("""
       @use 'sass:meta';
       @include meta.load-css("other.scss");
-    """, loadPaths: [d.sandbox]), completion(equals("/**/\n/**/")));
+    """, loadPaths: [d.sandbox]), completion(equals("/**/ /**/")));
 
     // Give the race condition time to appear.
     await pumpEventQueue();

test/output_test.dart

@@ -110,4 +110,241 @@ 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("double trailing empty block", () {
+      expect(compileString("""
+selector { /* please don't move me */ /* please don't move me */ }"""),
+          equals("""
+selector { /* please don't move me */ /* please don't move me */
+}"""));
+    });
+
+    test("double trailing style rule", () {
+      expect(compileString("""
+selector {
+  margin: 1px; /* please don't move me */ /* please don't move me */
+}"""), equals("""
+selector {
+  margin: 1px; /* please don't move me */ /* 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;"""));
+    });
+
+    group("loud comments in mixin", () {
+      test("empty with spacing", () {
+        expect(compileString("""
+@mixin loudComment {
+  /* ... */
+}
+
+selector {
+  @include loudComment;
+}"""), """
+selector {
+  /* ... */
+}""");
+      });
+
+      test("empty no spacing", () {
+        expect(compileString("""
+@mixin loudComment{/* ... */}
+selector {@include loudComment;}"""), """
+selector {
+  /* ... */
+}""");
+      });
+
+      test("with style rule", () {
+        expect(compileString("""
+@mixin loudComment {
+  margin: 1px; /* mixin */
+} /* mixin-out */
+
+selector {
+  @include loudComment; /* selector */
+}"""), """
+/* mixin-out */
+selector {
+  margin: 1px; /* mixin */
+  /* selector */
+}""");
+      });
+    });
+
+    group("loud comments in nested blocks", () {
+      test("with styles", () {
+        expect(
+          compileString("""
+foo { /* foo */
+  padding: 1px; /* foo padding */
+  bar { /* bar */
+    padding: 2px; /* bar padding */
+    baz { /* baz */
+      padding: 3px; /* baz padding */
+      margin: 3px; /* baz margin */
+    } /* baz end */
+    biz { /* biz */
+      padding: 3px; /* biz padding */
+      margin: 3px; /* biz margin */
+    } /* biz end */
+    margin: 2px; /* bar margin */
+  } /* bar end */
+  margin: 1px; /* foo margin */
+} /* foo end */
+"""),
+          """
+foo { /* foo */
+  padding: 1px; /* foo padding */
+  /* bar end */
+  margin: 1px; /* foo margin */
+}
+foo bar { /* bar */
+  padding: 2px; /* bar padding */
+  /* baz end */
+  /* biz end */
+  margin: 2px; /* bar margin */
+}
+foo bar baz { /* baz */
+  padding: 3px; /* baz padding */
+  margin: 3px; /* baz margin */
+}
+foo bar biz { /* biz */
+  padding: 3px; /* biz padding */
+  margin: 3px; /* biz margin */
+}
+
+/* foo end */""",
+        );
+      });
+
+      test("empty", () {
+        expect(
+          compileString("""
+foo { /* foo */
+  bar { /* bar */
+    baz { /* baz */
+    } /* baz end */
+    biz { /* biz */
+    } /* biz end */
+  } /* bar end */
+} /* foo end */
+"""),
+          """
+foo { /* foo */
+  /* bar end */
+}
+foo bar { /* bar */
+  /* baz end */
+  /* biz end */
+}
+foo bar baz { /* baz */ }
+foo bar biz { /* biz */ }
+
+/* foo end */""",
+        );
+      });
+    });
+  });
 }