Ensure comment formatting is idempotent. (#1610)

* Ensure comment formatting is idempotent.

In some cases, a line comment can appear between two tokens that otherwise never split, like after "if (" and before the condition. That leads to some tricky edge case behavior. If you formatted:

```dart
if (
  // Comment
  condition) {
  ;
}
```

It would see the newline before the comment and decide the comment was a "leading comment" which means it gets attached to the condition expression. Then the formatter would output it like:

```dart
if (// Comment
  condition) {
  ;
}
```

That's because leading comments don't write a newline before themselves. Then if you format that again, there's no newline before the `//`, so now it's a hanging comment. Hanging comments get a space before them, so you get:

```dart
if ( // Comment
  condition) {
  ;
}
```

Really, leading comments (as the name implies) are intended to always begin a line. So this PR makes sure they do that.

While I was at it, I modified the test runner to run the formatter twice on *every* test to ensure that everything is idempotent. That doesn't *prove* that the formatter will always produce idempotent output, but it at least gives us pretty good test coverage that it *does* behave idempotent-ly.

In practice, most normal looking code would never hit this edge case. You have to put a comment in an unusual spot where a split doesn't occur.

This still feels like a fairly brittle part of the formatter to me. Comments appearing between tokens that never split otherwise is handled on a pretty ad hoc basis (which is why some of the tests in this PR have weird indentation). I'd like a cleaner more systematic solution, but I'm not sure what that would look like.

Fix #1606.

* Fix version number in CHANGELOG.

* Update lib/src/back_end/code_writer.dart

Co-authored-by: Nate Bosch <nbosch@google.com>

* Apply review feedback.

---------

Co-authored-by: Nate Bosch <nbosch@google.com>

CHANGELOG.md

@@ -1,3 +1,7 @@
+## 3.0.1-wip
+
+* Ensure comment formatting is idempotent (#1606).
+
 ## 3.0.0
 
 This is a large change. Under the hood, the formatter was almost completely

lib/src/back_end/code_writer.dart

@@ -372,12 +372,13 @@ final class CodeWriter {
     // If we found a problematic line, and there is are pieces on the line that
     // we can try to split, then remember them so that the solution will expand
     // them next.
-    if (!_foundExpandLine && (_column > _pageWidth || !_solution.isValid)) {
-      // We found a problematic line, so remember the pieces on it.
-      _foundExpandLine = true;
+    if (_foundExpandLine) return;
+    if (_currentLinePieces.isNotEmpty &&
+        (_column > _pageWidth || !_solution.isValid)) {
       _expandPieces.addAll(_currentLinePieces);
-    } else if (!_foundExpandLine) {
-      // This line was OK, so we don't need to expand the piece on it.
+      _foundExpandLine = true;
+    } else {
+      // This line was OK, so we don't need to expand the pieces on it.
       _currentLinePieces.clear();
     }
   }

lib/src/dart_formatter.dart

@@ -124,7 +124,7 @@ final class DartFormatter {
     if (!source.isCompilationUnit) {
       var prefix = 'void foo() { ';
       inputOffset = prefix.length;
-      text = '$prefix$text }';
+      text = '$prefix$text\n }';
       unitSourceCode = SourceCode(
         text,
         uri: source.uri,

lib/src/piece/leading_comment.dart

@@ -31,6 +31,13 @@ final class LeadingCommentPiece extends Piece {
 
   @override
   void format(CodeWriter writer, State state) {
+    // If a piece has a leading comment, that comment should not also be a
+    // hanging comment, so ensure it begins its own line. This is also important
+    // to ensure that formatting is idempotent: If we don't do this, a comment
+    // might be a leading comment in the input and then get output on the same
+    // line as some preceding code, which would lead it to be a hanging comment
+    // the next time the formatter runs.
+    writer.newline();
     for (var comment in _comments) {
       writer.format(comment);
     }

lib/src/testing/test_file.dart

@@ -137,9 +137,19 @@ final class TestFile {
       }
 
       var isCompilationUnit = file.path.endsWith('.unit');
+
+      // The output always has a trailing newline. When formatting a statement,
+      // the formatter (correctly) doesn't output trailing newlines when
+      // formatting a statement, so remove it from the expectation to match.
+      var outputText = outputBuffer.toString();
+      if (!isCompilationUnit) {
+        assert(outputText.endsWith('\n'));
+        outputText = outputText.substring(0, outputText.length - 1);
+      }
+
       var input = _extractSelection(_unescapeUnicode(inputBuffer.toString()),
           isCompilationUnit: isCompilationUnit);
-      var output = _extractSelection(_unescapeUnicode(outputBuffer.toString()),
+      var output = _extractSelection(_unescapeUnicode(outputText),
           isCompilationUnit: isCompilationUnit);
 
       tests.add(FormatTest(

pubspec.yaml

@@ -1,6 +1,6 @@
 name: dart_style
 # Note: See tool/grind.dart for how to bump the version.
-version: 3.0.0
+version: 3.0.1-wip
 description: >-
   Opinionated, automatic Dart source code formatter.
   Provides an API and a CLI tool.

test/tall/pattern/cast_comment.stmt

@@ -33,7 +33,8 @@ if (obj case
   constant as Type) {;}
 <<<
 if (obj
-    case // comment
+    case
+        // comment
         constant as Type) {
   ;
 }

test/tall/regression/1606/1606.unit

@@ -0,0 +1,37 @@
+>>>
+class AaaaAaaaAaaaAaaaaaaaAaaaaaaa {
+  @aaaaaaaa
+  AaaaaAaaaAaaaAaaaaaaa get aaaaAaaaAaaaaaaa => aaaaaaaaAaaaaaaaaAaaaaa
+      ? (AaaaaaaaaAaaAaaaaaaa()
+        // AAA/Aaaaa aaaaaaaaa aaaaaaaa aaaa AAAAAAAA aaaaaaaaa aaaaaa aaaaaaaa.
+        ..aaaaAaaaaaaaaAaaaaaAaagetaaa =
+            AaaaAaaaaaaaaAaaaaaAaagetaaa.aaaaAaaaaa(aaaaaaaa: [
+          AaaaaaaaAaaaaaa()..aaaaaaaa = AaaaaaaaAA_Aaaaaaaa.AAAAA,
+        ], aaaaaaAaaaaa: [
+          AaaaaaAaaaaaAaaaaaa()
+            ..aaaaaaAaaaaa = AaaaaaAaaaaa.AAAAAA_AAAAAAAAAA_AAAAAA,
+          AaaaaaAaaaaaAaaaaaa()..aaaaaaAaaaaa = AaaaaaAaaaaa.AAAAAA_AAAA_AAAAAAA
+        ]))
+      : AaaaAaaaaaaAaaaaaaAaaaaaaa();
+}
+<<<
+class AaaaAaaaAaaaAaaaaaaaAaaaaaaa {
+  @aaaaaaaa
+  AaaaaAaaaAaaaAaaaaaaa get aaaaAaaaAaaaaaaa =>
+      aaaaaaaaAaaaaaaaaAaaaaa
+          ? (AaaaaaaaaAaaAaaaaaaa()
+            // AAA/Aaaaa aaaaaaaaa aaaaaaaa aaaa AAAAAAAA aaaaaaaaa aaaaaa aaaaaaaa.
+            ..aaaaAaaaaaaaaAaaaaaAaagetaaa =
+                AaaaAaaaaaaaaAaaaaaAaagetaaa.aaaaAaaaaa(
+                  aaaaaaaa: [
+                    AaaaaaaaAaaaaaa()..aaaaaaaa = AaaaaaaaAA_Aaaaaaaa.AAAAA,
+                  ],
+                  aaaaaaAaaaaa: [
+                    AaaaaaAaaaaaAaaaaaa()
+                      ..aaaaaaAaaaaa = AaaaaaAaaaaa.AAAAAA_AAAAAAAAAA_AAAAAA,
+                    AaaaaaAaaaaaAaaaaaa()
+                      ..aaaaaaAaaaaa = AaaaaaAaaaaa.AAAAAA_AAAA_AAAAAAA,
+                  ],
+                ))
+          : AaaaAaaaaaaAaaaaaaAaaaaaaa();
+}

test/tall/statement/if_comment.stmt

@@ -91,4 +91,44 @@ if (true) {
   body;
 } else {
   other;
-} // comment
+} // comment
+>>> Hanging line comment before infix condition.
+if (// comment
+a && b) { body; }
+<<<
+### The indentation is odd here because it's an odd place for a comment.
+if ( // comment
+a && b) {
+  body;
+}
+>>> Non-hanging line comment before infix condition.
+if (
+  // comment
+a && b) { body; }
+<<<
+### The indentation is odd here because it's an odd place for a comment.
+if (
+// comment
+a && b) {
+  body;
+}
+>>> Hanging line comment before infix chain condition.
+if (// comment
+a && b && c) { body; }
+<<<
+### The indentation is odd here because it's an odd place for a comment.
+if ( // comment
+a && b && c) {
+  body;
+}
+>>> Non-hanging line comment before infix chain condition.
+if (
+// comment
+a && b && c) { body; }
+<<<
+### The indentation is odd here because it's an odd place for a comment.
+if (
+// comment
+a && b && c) {
+  body;
+}

test/tall/top_level/import_comment.unit

@@ -7,11 +7,13 @@ import 'foo.dart'
     hide
         First, //
         Second;
->>> Don't split `==` because of leading comment before left operand.
+>>> Don't split `==` because of comment before left operand.
 import 'uri.dart' if (
   // comment
 config == 'value') 'c';
 <<<
+### The indentation is odd here because it's an odd place for a comment.
 import 'uri.dart'
-    if (// comment
+    if (
+    // comment
     config == 'value') 'c';

test/utils.dart

@@ -134,34 +134,46 @@ void _testFile(TestFile testFile) {
             indent: formatTest.leadingIndent,
             experimentFlags: const ['macros']);
 
-        var actual = formatter.formatSource(formatTest.input);
-
-        // The test files always put a newline at the end of the expectation.
-        // Statements from the formatter (correctly) don't have that, so add
-        // one to line up with the expected result.
-        var actualText = actual.text;
-        if (!testFile.isCompilationUnit) actualText += '\n';
-
-        // Fail with an explicit message because it's easier to read than
-        // the matcher output.
-        if (actualText != formatTest.output.text) {
-          fail('Formatting did not match expectation. Expected:\n'
-              '${formatTest.output.text}\nActual:\n$actualText');
-        } else if (actual.selectionStart != formatTest.output.selectionStart ||
-            actual.selectionLength != formatTest.output.selectionLength) {
-          fail('Selection did not match expectation. Expected:\n'
-              '${formatTest.output.textWithSelectionMarkers}\n'
-              'Actual:\n${actual.textWithSelectionMarkers}');
-        }
-
-        expect(actual.selectionStart, equals(formatTest.output.selectionStart));
-        expect(
-            actual.selectionLength, equals(formatTest.output.selectionLength));
+        var actual = _validateFormat(
+            formatter,
+            formatTest.input,
+            formatTest.output,
+            'did not match expectation',
+            testFile.isCompilationUnit);
+
+        // Make sure that formatting is idempotent. Format the output and make
+        // sure we get the same result.
+        _validateFormat(formatter, actual, actual, 'was not idempotent',
+            testFile.isCompilationUnit);
       });
     }
   });
 }
 
+/// Run [formatter] on [input] and validate that the result matches [expected].
+///
+/// If not, fails with an error using [reason].
+///
+/// Returns the formatted output.
+SourceCode _validateFormat(DartFormatter formatter, SourceCode input,
+    SourceCode expected, String reason, bool isCompilationUnit) {
+  var actual = formatter.formatSource(input);
+
+  // Fail with an explicit message because it's easier to read than
+  // the matcher output.
+  if (actual.text != expected.text) {
+    fail('Formatting $reason. Expected:\n'
+        '${expected.text}\nActual:\n${actual.text}');
+  } else if (actual.selectionStart != expected.selectionStart ||
+      actual.selectionLength != expected.selectionLength) {
+    fail('Selection $reason. Expected:\n'
+        '${expected.textWithSelectionMarkers}\n'
+        'Actual:\n${actual.textWithSelectionMarkers}');
+  }
+
+  return actual;
+}
+
 /// Create a test `.dart_tool` directory with a package config for a package
 /// with [rootPackageName] and language version [major].[minor].
 ///