Merge pull request #1031 from dart-lang/837-block-comment-splits

Don't unnecessarily split argument lists with block comments.

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,9 +3770,18 @@ class SourceVisitor extends ThrowingAstVisitor {
         if (comment == token.precedingComments) linesBefore = 2;
       }
 
-      var sourceComment = SourceComment(text, linesBefore,
-          isLineComment: comment.type == TokenType.SINGLE_LINE_COMMENT,
-          flushLeft: flushLeft);
+      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
       // in the comment.

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';