Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #417 preserve location of trailing loud comments #849

Merged
merged 6 commits into from
Jun 3, 2022

Conversation

nickbehrens
Copy link
Contributor

@nickbehrens nickbehrens commented Oct 16, 2019

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

@googlebot

This comment has been minimized.

@googlebot

This comment has been minimized.

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
test/output_test.dart Outdated Show resolved Hide resolved
test/output_test.dart Outdated Show resolved Hide resolved
test/output_test.dart Outdated Show resolved Hide resolved
test/output_test.dart Outdated Show resolved Hide resolved
test/output_test.dart Outdated Show resolved Hide resolved
@nickbehrens
Copy link
Contributor Author

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.

@nickbehrens nickbehrens requested a review from nex3 October 18, 2019 07:42
lib/src/parse/parser.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
test/output_test.dart Show resolved Hide resolved
@nickbehrens nickbehrens requested a review from nex3 October 26, 2019 07:02
test/output_test.dart Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
@nickbehrens nickbehrens requested a review from nex3 November 1, 2019 23:18
test/output_test.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
@nickbehrens nickbehrens requested a review from nex3 November 2, 2019 03:24
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 { /**/ }
  1. 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.

  1. 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:

  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 (?)

Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -162,5 +162,16 @@ selector[href*=\"{\"] { /* please don't move me */ }

@rule3;"""));
});

test("loud comments in mixin", () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nickbehrens and others added 2 commits May 31, 2022 16:04
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.
@Goodwine
Copy link
Member

Goodwine commented Jun 1, 2022

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 🤔

lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
test/output_test.dart Show resolved Hide resolved
test/output_test.dart Outdated Show resolved Hide resolved
Copy link
Member

@Goodwine Goodwine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment still WIP

lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
lib/src/visitor/serialize.dart Outdated Show resolved Hide resolved
… it to validate whether a comment is trailing
@Goodwine Goodwine requested a review from nex3 June 2, 2022 20:17
lib/src/util/span.dart Outdated Show resolved Hide resolved
@Goodwine Goodwine merged commit 1faf81c into sass:main Jun 3, 2022
Goodwine added a commit to sass/sass-spec that referenced this pull request Jun 3, 2022
See Issue sass/dart-sass#417 
See PR sass/dart-sass#849

Co-authored-by: Carlos Israel Ortiz García <goodwine@google.com>
Co-Authored-By: Natalie Weizenbaum <nweiz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inline comments should be kept inline
4 participants