Ensure that line comments always get newlines after them. (#1325)

Ensure that line comments always get newlines after them.

The formatter currently mostly relies on the surrounding piece's
setAllowedWhitespace() rules to make sure that a line comment forces
the surrounding piece to split in a way that ensures there is a newline
after the comment.

This works correctly in lots of places where line comments are actually
expected, but isn't reliable when a line comment occurs in a weird
location. When that happens, it's important that we still always put a
newline after the comment. Otherwise the code is meaningfully changed.

This does that. Whenever a TextPiece ends in a line comment, it will
know that it has a trailing newline. It then always writes that to the
CodeWriter. That leads to redundant newlines in cases where the
surrounding piece is already going to put a newline, so I also moved
some of the "pending" newline collapsing code from PieceWriter farther
down the pipeline to CodeWriter.

This doesn't change the behavior of any of the current tests. It will
fix many untested corners of the language. I'm working on some new tests
for comments in if statements that will be fixed by this change, but
I'm also changing the actual formatting of if statements, so I want to
do that in a separate change.

Author: munificent

lib/src/back_end/code_writer.dart

@@ -28,6 +28,19 @@ class CodeWriter {
   /// Buffer for the code being written.
   final StringBuffer _buffer = StringBuffer();
 
+  /// What whitespace should be written before the next non-whitespace text.
+  ///
+  /// When whitespace is written, instead of immediately writing it, we queue
+  /// it as pending. This ensures that we don't write trailing whitespace,
+  /// avoids writing spaces at the beginning of lines, and allows collapsing
+  /// multiple redundant newlines.
+  _Whitespace _pendingWhitespace = _Whitespace.none;
+
+  /// The number of spaces of indentation that should be begin the next line
+  /// when [_pendingWhitespace] is [_Whitespace.newline] or
+  /// [_Whitespace.blankLine].
+  int _pendingIndent = 0;
+
   /// The cost of the currently chosen line splits.
   int _cost = 0;
 
@@ -143,6 +156,7 @@ class CodeWriter {
     // TextPieces because that will preserve selection markers. Consider doing
     // something smarter for commas in lists and semicolons in for loops.
 
+    _flushWhitespace();
     _buffer.write(text);
     _column += text.length;
 
@@ -180,7 +194,10 @@ class CodeWriter {
 
   /// Writes a single space to the output.
   void space() {
-    write(' ');
+    // If a newline is already pending, then ignore the space.
+    if (_pendingWhitespace == _Whitespace.none) {
+      _pendingWhitespace = _Whitespace.space;
+    }
   }
 
   /// Inserts a line split in the output.
@@ -193,12 +210,15 @@ class CodeWriter {
     if (indent != null) setIndent(indent);
 
     handleNewline();
-    _finishLine();
-    _buffer.writeln();
-    if (blank) _buffer.writeln();
 
-    _column = _options.indent;
-    _buffer.write(' ' * _column);
+    // Collapse redundant newlines.
+    if (blank) {
+      _pendingWhitespace = _Whitespace.blankLine;
+    } else if (_pendingWhitespace != _Whitespace.blankLine) {
+      _pendingWhitespace = _Whitespace.newline;
+    }
+
+    _pendingIndent = _options.indent;
   }
 
   /// Sets whether newlines are allowed to occur from this point on for the
@@ -246,15 +266,46 @@ class CodeWriter {
   /// Sets [selectionStart] to be [start] code units into the output.
   void startSelection(int start) {
     assert(_selectionStart == null);
+
+    _flushWhitespace();
     _selectionStart = _buffer.length + start;
   }
 
   /// Sets [selectionEnd] to be [end] code units into the output.
   void endSelection(int end) {
     assert(_selectionEnd == null);
+
+    _flushWhitespace();
     _selectionEnd = _buffer.length + end;
   }
 
+  /// Write any pending whitespace.
+  ///
+  /// This is called before non-whitespace text is about to be written, or
+  /// before the selection is updated since the latter requires an accurate
+  /// count of the written text, including whitespace.
+  void _flushWhitespace() {
+    switch (_pendingWhitespace) {
+      case _Whitespace.none:
+        break; // Nothing to do.
+
+      case _Whitespace.newline:
+      case _Whitespace.blankLine:
+        _finishLine();
+        _buffer.writeln();
+        if (_pendingWhitespace == _Whitespace.blankLine) _buffer.writeln();
+
+        _column = _pendingIndent;
+        _buffer.write(' ' * _column);
+
+      case _Whitespace.space:
+        _buffer.write(' ');
+        _column++;
+    }
+
+    _pendingWhitespace = _Whitespace.none;
+  }
+
   void _finishLine() {
     // If the completed line is too long, track the overflow.
     if (_column >= _pageWidth) {
@@ -276,6 +327,21 @@ class CodeWriter {
   }
 }
 
+/// Different kinds of pending whitespace that have been requested.
+enum _Whitespace {
+  /// No pending whitespace.
+  none,
+
+  /// A single newline.
+  newline,
+
+  /// Two newlines.
+  blankLine,
+
+  /// A single space.
+  space
+}
+
 /// The mutable state local to a single piece being formatted.
 class _PieceOptions {
   /// The absolute number of spaces of leading indentation coming from

lib/src/front_end/comment_writer.dart

@@ -78,7 +78,6 @@ mixin CommentWriter {
 
       if (comments.isHanging(i)) {
         // Attach the comment to the previous token.
-        pieces.writeSpace();
         pieces.writeComment(comment, hanging: true);
       } else {
         pieces.writeNewline();
@@ -202,8 +201,7 @@ class SourceComment {
       {required this.flushLeft, required this.offset});
 
   /// Whether this comment contains a mandatory newline, either because it's a
-  /// comment that should be on its own line or a lexeme with a newline inside
-  /// it (i.e. multi-line block comment, multi-line string).
+  /// comment that should be on its own line or is a multi-line block comment.
   bool get containsNewline =>
       type != CommentType.inlineBlock || text.contains('\n');
 

lib/src/front_end/piece_writer.dart

@@ -124,10 +124,6 @@ class PieceWriter {
   /// Whether we should write a space before the next text that is written.
   bool _pendingSpace = false;
 
-  /// Whether we should write a newline in the current [TextPiece] before the
-  /// next text that is written.
-  bool _pendingNewline = false;
-
   /// Whether we should create a new [TextPiece] the next time text is written.
   bool _pendingSplit = false;
 
@@ -221,14 +217,14 @@ class PieceWriter {
 
   /// Writes a mandatory newline from a comment to the current [TextPiece].
   void writeNewline() {
-    _pendingNewline = true;
+    _currentText!.newline();
   }
 
   /// Write the contents of [comment] to the current innermost [TextPiece],
   /// handling any newlines that may appear in it.
   ///
   /// If [hanging] is `true`, then the comment is appended to the current line
-  /// even if call to [split()] has just happened. This is used for writing a
+  /// even if a call to [split()] has happened. This is used for writing a
   /// comment that should be on the end of a line.
   void writeComment(SourceComment comment, {bool hanging = false}) {
     _write(comment.text,
@@ -246,10 +242,9 @@ class PieceWriter {
     // current text.
     if (textPiece == null || _pendingSplit && !hanging) {
       textPiece = _currentText = TextPiece();
-    } else if (_pendingNewline) {
-      textPiece.newline();
-    } else if (_pendingSpace) {
-      textPiece.append(' ');
+    } else if (_pendingSpace || hanging) {
+      // Always write a space before hanging comments.
+      textPiece.appendSpace();
     }
 
     if (offset != null) {
@@ -267,7 +262,6 @@ class PieceWriter {
     textPiece.append(text, containsNewline: containsNewline);
 
     _pendingSpace = false;
-    _pendingNewline = false;
     if (!hanging) _pendingSplit = false;
   }
 

lib/src/piece/piece.dart

@@ -88,14 +88,19 @@ class TextPiece extends Piece {
   /// multiline strings, etc.
   bool _containsNewline = false;
 
+  /// Whether this piece should have a newline written at the end of it.
+  ///
+  /// This is true during piece construction while lines are still being
+  /// written. It may also be true once a piece is fully complete if it ends in
+  /// a line comment.
+  bool _trailingNewline = false;
+
   /// The offset from the beginning of [text] where the selection starts, or
   /// `null` if the selection does not start within this chunk.
-  int? get selectionStart => _selectionStart;
   int? _selectionStart;
 
   /// The offset from the beginning of [text] where the selection ends, or
   /// `null` if the selection does not start within this chunk.
-  int? get selectionEnd => _selectionEnd;
   int? _selectionEnd;
 
   /// Whether the last line of this piece's text ends with [text].
@@ -106,16 +111,25 @@ class TextPiece extends Piece {
   /// If [text] internally contains a newline, then [containsNewline] should
   /// be `true`.
   void append(String text, {bool containsNewline = false}) {
-    if (_lines.isEmpty) _lines.add('');
+    if (_lines.isEmpty || _trailingNewline) _lines.add('');
 
     // TODO(perf): Consider a faster way of accumulating text.
     _lines.last = _lines.last + text;
 
     if (containsNewline) _containsNewline = true;
+
+    _trailingNewline = false;
+  }
+
+  void appendSpace() {
+    // Don't write an unnecessary space at the beginning of a line.
+    if (_trailingNewline) return;
+
+    append(' ');
   }
 
   void newline() {
-    _lines.add('');
+    _trailingNewline = true;
   }
 
   @override
@@ -136,6 +150,8 @@ class TextPiece extends Piece {
       if (i > 0) writer.newline();
       writer.write(_lines[i]);
     }
+
+    if (_trailingNewline) writer.newline();
   }
 
   @override
@@ -149,7 +165,6 @@ class TextPiece extends Piece {
       start += line.length;
     }
 
-    // print('TextPiece start $start (absolute = $abs)');
     _selectionStart = start;
   }
 
@@ -161,7 +176,6 @@ class TextPiece extends Piece {
       end += line.length;
     }
 
-    // print('TextPiece end $end (absolute = $abs)');
     _selectionEnd = end;
   }