Don't unnecessarily split argument lists with block comments.

A very subtle bug in the comment handling would cause a /* */ inline
comment to force the surrounding code to full split when not needed in
some cases.

This fixes that issue. It also slightly tweaks the formatting of code
containing inline block comments in some rare cases. In practice, the
only code I see affected by that is formatter tests. In the real world,
all affected code is related to the bug itself and is fixed by this
change.

Fix #837.

Author: munificent

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 2.0.2-dev
+
+* Don't unnecessarily split argument lists with `/* */` comments (#837).
+
 # 2.0.1
 
 * Support triple-shift `>>>` and `>>>=` operators (#992).

lib/src/chunk.dart

@@ -384,32 +384,43 @@ class Span extends FastHash {
   String toString() => '$id\$$cost';
 }
 
+enum CommentType {
+  /// A `///` or `/**` doc comment.
+  doc,
+
+  /// A non-doc line comment.
+  line,
+
+  /// A `/* ... */` comment that should be on its own line.
+  block,
+
+  /// A `/* ... */` comment that can share a line with other code.
+  inlineBlock,
+}
+
 /// A comment in the source, with a bit of information about the surrounding
 /// whitespace.
 class SourceComment extends Selection {
   /// The text of the comment, including `//`, `/*`, and `*/`.
   @override
   final String text;
 
+  /// What kind of comment this is.
+  final CommentType type;
+
   /// The number of newlines between the comment or token preceding this comment
   /// and the beginning of this one.
   ///
   /// Will be zero if the comment is a trailing one.
   int linesBefore;
 
-  /// Whether this comment is a line comment.
-  final bool isLineComment;
-
   /// Whether this comment starts at column one in the source.
   ///
   /// Comments that start at the start of the line will not be indented in the
   /// output. This way, commented out chunks of code do not get erroneously
   /// re-indented.
   final bool flushLeft;
 
-  /// Whether this comment is an inline block comment.
-  bool get isInline => linesBefore == 0 && !isLineComment;
-
-  SourceComment(this.text, this.linesBefore,
-      {required this.isLineComment, required this.flushLeft});
+  SourceComment(this.text, this.type, this.linesBefore,
+      {required this.flushLeft});
 }

lib/src/chunk_builder.dart

@@ -253,22 +253,19 @@ class ChunkBuilder {
     // Edge case: if the comments are completely inline (i.e. just a series of
     // block comments with no newlines before, after, or between them), then
     // they will eat any pending newlines. Make sure that doesn't happen by
-    // putting the pending whitespace before the first comment and moving them
-    // to their own line. Turns this:
+    // putting the pending whitespace before the first comment. Turns this:
     //
     //     library foo; /* a */ /* b */ import 'a.dart';
     //
     // into:
     //
     //     library foo;
     //
-    //     /* a */ /* b */
-    //     import 'a.dart';
+    //     /* a */ /* b */ import 'a.dart';
     if (linesBeforeToken == 0 &&
-        comments.every((comment) => comment.isInline) &&
-        _pendingWhitespace.minimumLines > 0) {
+        _pendingWhitespace.minimumLines > comments.first.linesBefore &&
+        comments.every((comment) => comment.type == CommentType.inlineBlock)) {
       comments.first.linesBefore = _pendingWhitespace.minimumLines;
-      linesBeforeToken = 1;
     }
 
     // Write each comment and the whitespace between them.
@@ -284,18 +281,19 @@ class ChunkBuilder {
       }
       _emitPendingWhitespace();
 
-      if (comment.linesBefore == 0) {
+      // See if the comment should follow text on the current line.
+      if (comment.linesBefore == 0 || comment.type == CommentType.inlineBlock) {
         // If we're sitting on a split, move the comment before it to adhere it
         // to the preceding text.
-        if (_shouldMoveCommentBeforeSplit(comment.text)) {
+        if (_shouldMoveCommentBeforeSplit(comment)) {
           _chunks.last.allowText();
         }
 
         // The comment follows other text, so we need to decide if it gets a
         // space before it or not.
         if (_needsSpaceBeforeComment(comment)) _writeText(' ');
       } else {
-        // The comment starts a line, so make sure it stays on its own line.
+        // The comment is not inline, so start a new line.
         _writeHardSplit(
             flushLeft: comment.flushLeft,
             isDouble: comment.linesBefore > 1,
@@ -743,17 +741,23 @@ class ChunkBuilder {
 
   /// Returns `true` if the last chunk is a split that should be moved after the
   /// comment that is about to be written.
-  bool _shouldMoveCommentBeforeSplit(String comment) {
+  bool _shouldMoveCommentBeforeSplit(SourceComment comment) {
     // Not if there is nothing before it.
     if (_chunks.isEmpty) return false;
 
+    // Don't move a comment to a preceding line.
+    if (comment.linesBefore != 0) return false;
+
     // Multi-line comments are always pushed to the next line.
-    if (comment.contains('\n')) return false;
+    if (comment.type == CommentType.doc) return false;
+    if (comment.type == CommentType.block) return false;
 
     var text = _chunks.last.text;
 
     // A block comment following a comma probably refers to the following item.
-    if (text.endsWith(',') && comment.startsWith('/*')) return false;
+    if (text.endsWith(',') && comment.type == CommentType.inlineBlock) {
+      return false;
+    }
 
     // If the text before the split is an open grouping character, it looks
     // better to keep it with the elements than with the bracket itself.
@@ -788,13 +792,11 @@ class ChunkBuilder {
     // Not at the start of a line.
     if (!_chunks.last.canAddText) return false;
 
-    var text = _chunks.last.text;
-    if (text.endsWith('\n')) return false;
-
     // Always put a space before line comments.
-    if (comment.isLineComment) return true;
+    if (comment.type == CommentType.line) return true;
 
     // Magic generic method comments like "Foo/*<T>*/" don't get spaces.
+    var text = _chunks.last.text;
     if (_isGenericMethodComment(comment) &&
         _trailingIdentifierChar.hasMatch(text)) {
       return false;

lib/src/source_visitor.dart

@@ -3770,8 +3770,17 @@ class SourceVisitor extends ThrowingAstVisitor {
         if (comment == token.precedingComments) linesBefore = 2;
       }
 
-      var sourceComment = SourceComment(text, linesBefore,
-          isLineComment: comment.type == TokenType.SINGLE_LINE_COMMENT,
+      var type = CommentType.block;
+      if (text.startsWith('///') && !text.startsWith('////') ||
+      text.startsWith('/**')) {
+        type = CommentType.doc;
+      } else if (comment.type == TokenType.SINGLE_LINE_COMMENT) {
+        type = CommentType.line;
+      } else if (commentLine == previousLine || commentLine == tokenLine) {
+        type = CommentType.inlineBlock;
+      }
+
+      var sourceComment = SourceComment(text, type, linesBefore,
           flushLeft: flushLeft);
 
       // If this comment contains either of the selection endpoints, mark them

pubspec.yaml

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

test/comments/classes.unit

@@ -33,9 +33,7 @@ class A {
 class A {
   /* comment */}
 <<<
-class A {
-  /* comment */
-}
+class A {/* comment */}
 >>> inline block comment
 class A {  /* comment */  }
 <<<

test/comments/extensions.unit

@@ -33,9 +33,7 @@ extension A on B {
 extension A on B {
   /* comment */}
 <<<
-extension A on B {
-  /* comment */
-}
+extension A on B {/* comment */}
 >>> inline block comment
 extension A on B {  /* comment */  }
 <<<

test/comments/functions.unit

@@ -143,4 +143,36 @@ function([
   parameter,
 ] /* c */) {
   ;
+}
+>>>
+function(
+    /* comment */ int a, int b, int c,
+    [direction]) {
+  ;
+}
+<<<
+function(
+    /* comment */ int a, int b, int c,
+    [direction]) {
+  ;
+}
+>>>
+function(
+    /* comment */ int a, int b, int c) {
+  ;
+}
+<<<
+function(
+    /* comment */ int a, int b, int c) {
+  ;
+}
+>>>
+function(
+    /* comment */ int a, int b, int c, int d) {
+  ;
+}
+<<<
+function(/* comment */ int a, int b,
+    int c, int d) {
+  ;
 }

test/comments/statements.stmt

@@ -54,4 +54,33 @@ while (true) {
 <<<
 while (true) {
   // comment
+}
+>>>
+main() {
+  /* comment */ statement;
+}
+<<<
+main() {
+  /* comment */ statement;
+}
+>>>
+main() {
+  code;
+
+  /* comment */ statement;
+}
+<<<
+main() {
+  code;
+
+  /* comment */ statement;
+}
+>>>
+main() {
+  while (b)
+  /*unreachable*/ {}
+}
+<<<
+main() {
+  while (b) /*unreachable*/ {}
 }

test/comments/top_level.unit

@@ -190,14 +190,12 @@ library a; /* comment */ import 'b.dart';
 <<<
 library a;
 
-/* comment */
-import 'b.dart';
+/* comment */ import 'b.dart';
 >>> inline block comment between directives
 import 'a.dart'; /* comment */ import 'b.dart';
 <<<
 import 'a.dart';
-/* comment */
-import 'b.dart';
+/* comment */ import 'b.dart';
 >>> block comment between directives
 import 'a.dart'; /* comment */
 import 'b.dart';