[CFE] Coverage merger adds braces on dangling if, if adding an ignore comment

Fixes #61171

Change-Id: Ie08492636fdf4f73d261c0b744e8b5978c0f6b40
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443980
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Jens Johansen <[email protected]>

Author: jensjoha

pkg/front_end/test/coverage_merger_tests/if_dangling_uncovered.dart.commented.expect

@@ -1,6 +1,7 @@
 void main() {
   print("hello");
-  if (1 + 1 == 3)
-    // Coverage-ignore(suite): Not run.
+  if (1 + 1 == 3) {
+    // Coverage-ignore-block(suite): Not run.
     print("no?!?");
+  }
 }

pkg/front_end/test/coverage_merger_tests/if_dangling_uncovered_2.dart

@@ -0,0 +1,16 @@
+void main() {
+  print("hello");
+  Forest forest = new Forest();
+  if (1 + 1 == 3) push(forest.createBlock(noLocation, noLocation, []));
+  push(forest.createBlock(42, 42, [42]));
+}
+
+const int noLocation = -1;
+
+void push(dynamic whatever) {}
+
+class Forest {
+  dynamic createBlock(int a, int b, List c) {
+    return "$a$b$c";
+  }
+}

pkg/front_end/test/coverage_merger_tests/if_dangling_uncovered_2.dart.commented.expect

@@ -0,0 +1,19 @@
+void main() {
+  print("hello");
+  Forest forest = new Forest();
+  if (1 + 1 == 3) {
+    // Coverage-ignore-block(suite): Not run.
+    push(forest.createBlock(noLocation, noLocation, []));
+  }
+  push(forest.createBlock(42, 42, [42]));
+}
+
+const int noLocation = -1;
+
+void push(dynamic whatever) {}
+
+class Forest {
+  dynamic createBlock(int a, int b, List c) {
+    return "$a$b$c";
+  }
+}

pkg/front_end/test/coverage_merger_tests/if_dangling_uncovered_2.dart.visualization.expect

@@ -0,0 +1,5 @@
+package:front_end/main.dart: 79% (3 misses)
+package:front_end/main.dart:4:
+In 'main':
+  if (1 + 1 == 3) push(forest.createBlock(noLocation, noLocation, []));
+                  ^^^^        ^^^^^^^^^^^                         ^

pkg/front_end/tool/coverage_merger.dart

@@ -362,6 +362,12 @@ CoverageInfo _process(
               commentOn.commentOnToken.lexeme == "?.") {
             // A comment on the actual call isn't pretty.
             // Allow the "bigger one".
+          } else if (prevAdded.beginToken.charOffset ==
+                  commentOn.beginToken.charOffset &&
+              prevAdded.endToken.charEnd == commentOn.endToken.charEnd &&
+              prevAdded.allowToWrapInBlock) {
+            // They have the same size and the previous one should be wrapped.
+            // Keep that one so we do wrap.
           } else if (prevAdded.canBeReplaced) {
             // Though if there aren't any possible extra coverage in the
             // previous block compared to this one, we do prefer the smaller
@@ -514,14 +520,23 @@ CoverageInfo _process(
 
       // The extra spaces at the end makes the formatter format better if for
       // instance there's comments after this.
+      bool addedStartBrace = false;
       if (entry.isBlock) {
         sb.write("$extra// Coverage-ignore-block(suite): Not run.\n  ");
+      } else if (entry.allowToWrapInBlock) {
+        sb.write("$extra {\n  // Coverage-ignore-block(suite): Not run.\n  ");
+        addedStartBrace = true;
       } else {
         sb.write("$extra// Coverage-ignore(suite): Not run.\n  ");
       }
       ignoredIntervalsBuilder?.addIntervalIncludingEnd(
           entry.beginToken.charOffset, entry.endToken.charEnd);
       from = token.charOffset;
+      if (addedStartBrace) {
+        addChunk(from, entry.endToken.charEnd);
+        from = entry.endToken.charEnd;
+        sb.write("\n  }");
+      }
     }
     addChunk(from, sourceText.length);
     f.writeAsStringSync(sb.toString());
@@ -888,10 +903,10 @@ class AstIndexerAndIgnoreCollector extends AstIndexer {
   /// there is possible coverage but no actual coverage).
   bool _checkCommentAndIgnoreCoverage(
       Token tokenWithPossibleComment, BeginAndEndTokenParserAstNode ignoreRange,
-      {required bool allowReplace}) {
+      {required bool allowReplace, bool allowToWrapInBlock = false}) {
     return _checkCommentAndIgnoreCoverageWithBeginAndEnd(
         tokenWithPossibleComment, ignoreRange.beginToken, ignoreRange.endToken,
-        allowReplace: allowReplace);
+        allowReplace: allowReplace, allowToWrapInBlock: allowToWrapInBlock);
   }
 
   /// Check if there is an ignore comment on [tokenWithPossibleComment] and
@@ -905,7 +920,8 @@ class AstIndexerAndIgnoreCollector extends AstIndexer {
       final Token endToken,
       {required bool allowReplace,
       bool isBlock = false,
-      bool allowOnBraceStart = false}) {
+      bool allowOnBraceStart = false,
+      bool allowToWrapInBlock = false}) {
     if (_hasIgnoreComment(tokenWithPossibleComment, isBlock: isBlock)) {
       ignoredStartEnd.addIntervalIncludingEnd(
           beginToken.charOffset, endToken.charEnd);
@@ -976,6 +992,7 @@ class AstIndexerAndIgnoreCollector extends AstIndexer {
         endToken: endToken,
         canBeReplaced: allowReplace,
         isBlock: isBlock,
+        allowToWrapInBlock: allowToWrapInBlock,
       ));
     }
 
@@ -1302,7 +1319,9 @@ class _AstIndexerAndIgnoreCollectorBody extends RecursiveParserAstVisitor {
   @override
   void visitThenStatementEnd(ThenStatementEnd node) {
     if (_collector._checkCommentAndIgnoreCoverage(node.beginToken, node,
-        allowReplace: true)) {
+        allowReplace: true,
+        allowToWrapInBlock:
+            !node.beginToken.isA(TokenType.OPEN_CURLY_BRACKET))) {
       return;
     }
     super.visitThenStatementEnd(node);
@@ -1556,13 +1575,15 @@ class _CommentOn implements Comparable<_CommentOn> {
   final Token endToken;
   final bool canBeReplaced;
   final bool isBlock;
+  final bool allowToWrapInBlock;
 
   _CommentOn({
     required this.commentOnToken,
     required this.beginToken,
     required this.endToken,
     required this.canBeReplaced,
     required this.isBlock,
+    required this.allowToWrapInBlock,
   });
 
   @override