Fix #417 preserve location of trailing loud comments

[nickbehrens]

Fix #417 by preserving the location of trailing loud comments.

Loud comment AST nodes now have the notion of whether they're "trailing". During parsing, we look for loud comments at the end of lines and if found, tag them as trailing. This information is then plumbed through and made available during serialization.

During serialization, where we would previously unconditionally insert newlines between nodes, we now check to see if the next node is a trailing loud comment. If so, we instead write out a space followed by the trailing loud comment.

Ran the following locally, all tests passing:
$ pub run grinder && dartanalyzer lib test && pub run test -x node

See sass/sass-spec#1485

[nex3]

Apologies for not realizing this in advance, but I think you can actually do this without touching the parser at all. A comment is trailing iff it starts on the same line as the previous (or parent) statement, right? So you can just compare their span.start.line values in the serializer to tell if the comment should be treated as "trailing".

[nickbehrens]

Apologies for not realizing this in advance, but I think you can actually do this without touching the parser at all. A comment is trailing iff it starts on the same line as the previous (or parent) statement, right? So you can just compare their span.start.line values in the serializer to tell if the comment should be treated as "trailing".

No worries. I've reverted the parser changes and instead am now using the span line numbers to look for trailing comments.

[nex3]

sass-spec found a crash with the following input:

@media screen and (orientation:landscape) {
  span {
    background: blue;
  }
  /* fudge */
  // @include foo;
  /* budge */
  div {
    color: red;
  }
}

@mixin testComments {
  /* crash */
  p {
    width: 100px;
  }
}

@media screen and (orientation:landscape) {
  @include testComments;
}

Unexpected exception:
RangeError: Invalid value: Not in range 0..70, inclusive: -44


dart:core                                                  _StringBase.lastIndexOf
package:sass/src/visitor/serialize.dart 1146:41            _SerializeVisitor._isTrailingComment
package:sass/src/visitor/serialize.dart 1094:11            _SerializeVisitor._visitChildren
package:sass/src/visitor/serialize.dart 236:5              _SerializeVisitor.visitCssMediaRule
package:sass/src/ast/css/modifiable/media_rule.dart 26:15  ModifiableCssMediaRule.accept
package:sass/src/visitor/serialize.dart 172:13             _SerializeVisitor.visitCssStylesheet
package:sass/src/ast/css/modifiable/stylesheet.dart 19:15  ModifiableCssStylesheet.accept
package:sass/src/visitor/serialize.dart 62:8               serialize
package:sass/src/compile.dart 142:25                       _compileStylesheet
package:sass/src/compile.dart 64:10                        compile
package:sass/src/executable/compile_stylesheet.dart 91:13  compileStylesheet
package:sass/src/executable.dart 65:15                     main

Can you investigate?

[nickbehrens]

I looked at the crash and reduced the repro to the following:

@mixin loudComment {
  /* ... */
}

selector {
  @include loudComment;
}

In short, @include breaks the assumption of the current _isTrailingComment logic that node.span.start.offset will be strictly after prevNode.span.start.offset.

I think we just need to check for searchFrom < 0 and if so, it's never a trailing comment. I'll commit that change (with a unit test) and see what happens with the spec tests.

[nickbehrens]

After the most recent SassSpec test run, there are 4 remaining issues. 2 of them I think are reasonable formatting changes (i.e. we could update SassSpec test cases to match):

1. SassSpec::Test#test__spec/libsass-closed-issues/issue_894

--- expected
+++ actual
-"a { /**/
-}
+"a { /**/ }

2. SassSpec::Test#test__spec/css/keyframes/bubble/empty

--- expected
+++ actual
-"@keyframes { /**/
-}
+"@keyframes { /**/ }

One of them involves a standalone loud comment following a block end (rbrace) that's now getting treated as a trailing comment. I can add a test case for this and investigate.

3. SassSpec::Test#test__spec/libsass-closed-issues/issue_1007

--- expected
+++ actual
 foo baz {
   /* before */
   margin: 0; /* after */
-}
-/* end */
+} /* end */

The last one I could use some help parsing/understanding:

4. SassSpec::Test#test__spec/directives/use/css/order/import_order/comments_and_imports

--- expected
+++ actual
 "/* upstream comment before import */
-@import \"upstream.css\"; /* midstream comment before use */
+@import \"upstream.css\";
+/* midstream comment before use */
 /* midstream comment before first import */
 @import \"midstream1.css\";
 /* midstream comment before second import */
-@import \"midstream2.css\"; /* input comment before use */
+@import \"midstream2.css\";
+/* input comment before use */
 /* input comment before import */
 @import \"input.css\";
 /* upstream comment after import */

e.g. the expected output seems to have @import \"upstream.css\"; /* midstream comment before use */ all on one line but I don't see that in the .hrx file (?)

[nex3]

The last one I could use some help parsing/understanding:

1. SassSpec::Test#test__spec/directives/use/css/order/import_order/comments_and_imports

--- expected
+++ actual
 "/* upstream comment before import */
-@import \"upstream.css\"; /* midstream comment before use */
+@import \"upstream.css\";
+/* midstream comment before use */
 /* midstream comment before first import */
 @import \"midstream1.css\";
 /* midstream comment before second import */
-@import \"midstream2.css\"; /* input comment before use */
+@import \"midstream2.css\";
+/* input comment before use */
 /* input comment before import */
 @import \"input.css\";
 /* upstream comment after import */

e.g. the expected output seems to have @import \"upstream.css\"; /* midstream comment before use */ all on one line but I don't see that in the .hrx file (?)

Don't forget that this PRs is running against the fix-417 branch in sass-spec, so you'll want to look at that branch's version of the HRX file.

[nex3]

This case raises a couple other possible edge cases:

• What if the loud comment comes after the style rule but still isn't contained within it, for example when the style rule is also in a mixin?
• What if the loud comment is in a totally different file from the mixin?

I'd probably solve these by writing a bool spanContainsLocation(SourceSpan span, SourceLocation location) helper and checking that in the outer conditional.

[Goodwine]

So taking back this conversation to what Nick spotted I saw the following problems:

1. The formatting issues spotted from before were solved. (e.g. @keyframes { /**/ }) 🆗

2. Unsure what the correct format is for loud comments after open and close braces, e. g.

   foo { /* open */
     margin: none;
   } /* close */

   currently this becomes

   foo { /* open */
     margin: none;
   } /* close */

   but there are very odd cases with nesting 😵‍💫

3. I'm confused why the loud comments from comments and imports should become trailing comments instead of staying where they are originally 🤔

[Goodwine]

one comment still WIP