From 1faf81cee46ad486028df236636671107deab815 Mon Sep 17 00:00:00 2001 From: Nicholas Behrens <43619548+nickbehrens@users.noreply.github.com> Date: Thu, 2 Jun 2022 18:46:12 -0700 Subject: [PATCH] Fix #417 preserve the location of trailing loud comments (#849) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Co-authored-by: Carlos Israel Ortiz García Co-Authored-By: Natalie Weizenbaum --- lib/src/util/span.dart | 9 ++ lib/src/visitor/serialize.dart | 120 ++++++++++++----- test/dart_api_test.dart | 2 +- test/output_test.dart | 237 +++++++++++++++++++++++++++++++++ 4 files changed, 330 insertions(+), 38 deletions(-) diff --git a/lib/src/util/span.dart b/lib/src/util/span.dart index 4c191a31..d54b4f42 100644 --- a/lib/src/util/span.dart +++ b/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]. diff --git a/lib/src/visitor/serialize.dart b/lib/src/visitor/serialize.dart index 1f1c0c9e..8e9a6917 100644 --- a/lib/src/visitor/serialize.dart +++ b/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 value) => _for(value, () => _buffer.write(value.value)); - /// Emits [children] in a block. - void _visitChildren(List 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; diff --git a/test/dart_api_test.dart b/test/dart_api_test.dart index db82dd7d..036a7820 100644 --- a/test/dart_api_test.dart +++ b/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(); diff --git a/test/output_test.dart b/test/output_test.dart index df06bd42..ff132d11 100644 --- a/test/output_test.dart +++ b/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 */""", + ); + }); + }); + }); }