Flatten chained conditionals. (#1489)

When testing the new formatter on a large corpus, I noticed the solver would get stuck on large conditional chains because (unfortunately), we can't separately format the else clause of a split conditional expression. By merging long conditional chains into a single InfixPiece, we can separately format all but the very last dangling else clause. While I was at it, I also put a hard cap in the number of solutions the solver will try in case it still gets stuck. The old formatter has a similar limit. It's rare for real-world code to hit this limit in the new solver, but it's better than getting totally stuck when it happens.
dart-lang · May 16, 2024 · 6641bde · 6641bde
1 parent ce04d38
commit 6641bde
Show file tree

Hide file tree

Showing 10 changed files with 171 additions and 48 deletions.
diff --git a/benchmark/case/conditional.expect b/benchmark/case/conditional.expect
@@ -0,0 +1,29 @@
+void securityItem() {
+  return SelectableText(
+    itemSecurityScheme.securitySchemeType == SecuritySchemeType.QueryAPIKey
+        ? Constants.oneTwoThreeTxt
+        : itemSecurityScheme.securitySchemeType ==
+                SecuritySchemeType.HeaderAPIKey ||
+            itemSecurityScheme.securitySchemeType ==
+                SecuritySchemeType.CookieAPIKey
+        ? Constants.oneTwoThreeTxt
+        : itemSecurityScheme.securitySchemeType == SecuritySchemeType.BasicHTTP
+        ? Constants.demoUsernameTxt
+        : itemSecurityScheme.securitySchemeType == SecuritySchemeType.BearerHTTP
+        ? Constants.oneTwoThreeTxt
+        : itemSecurityScheme.securitySchemeType == SecuritySchemeType.DigestHTTP
+        ? Constants.digestDemoTxt
+        : itemSecurityScheme.securitySchemeType ==
+                SecuritySchemeType.OAuth2PasswordFlow ||
+            itemSecurityScheme.securitySchemeType ==
+                SecuritySchemeType.OAuth2ClientFlow
+        ? Constants.emptyTxt
+        : itemSecurityScheme.securitySchemeType ==
+            SecuritySchemeType.OAuth2ImplicitFlow
+        ? Constants.emptyTxt
+        : itemSecurityScheme.securitySchemeType ==
+            SecuritySchemeType.OAuth2CodeFlow
+        ? Constants.emptyTxt
+        : Constants.emptyTxt,
+  );
+}
diff --git a/benchmark/case/conditional.expect_short b/benchmark/case/conditional.expect_short
@@ -0,0 +1,32 @@
+void securityItem() {
+  return SelectableText(
+    itemSecurityScheme.securitySchemeType == SecuritySchemeType.QueryAPIKey
+        ? Constants.oneTwoThreeTxt
+        : itemSecurityScheme.securitySchemeType ==
+                    SecuritySchemeType.HeaderAPIKey ||
+                itemSecurityScheme.securitySchemeType ==
+                    SecuritySchemeType.CookieAPIKey
+            ? Constants.oneTwoThreeTxt
+            : itemSecurityScheme.securitySchemeType ==
+                    SecuritySchemeType.BasicHTTP
+                ? Constants.demoUsernameTxt
+                : itemSecurityScheme.securitySchemeType ==
+                        SecuritySchemeType.BearerHTTP
+                    ? Constants.oneTwoThreeTxt
+                    : itemSecurityScheme.securitySchemeType ==
+                            SecuritySchemeType.DigestHTTP
+                        ? Constants.digestDemoTxt
+                        : itemSecurityScheme.securitySchemeType ==
+                                    SecuritySchemeType.OAuth2PasswordFlow ||
+                                itemSecurityScheme.securitySchemeType ==
+                                    SecuritySchemeType.OAuth2ClientFlow
+                            ? Constants.emptyTxt
+                            : itemSecurityScheme.securitySchemeType ==
+                                    SecuritySchemeType.OAuth2ImplicitFlow
+                                ? Constants.emptyTxt
+                                : itemSecurityScheme.securitySchemeType ==
+                                        SecuritySchemeType.OAuth2CodeFlow
+                                    ? Constants.emptyTxt
+                                    : Constants.emptyTxt,
+  );
+}
diff --git a/benchmark/case/conditional.unit b/benchmark/case/conditional.unit
@@ -0,0 +1,23 @@
+void securityItem() {
+return SelectableText(
+  itemSecurityScheme.securitySchemeType == SecuritySchemeType.QueryAPIKey
+  ? Constants.oneTwoThreeTxt
+  : itemSecurityScheme.securitySchemeType == SecuritySchemeType.HeaderAPIKey ||
+    itemSecurityScheme.securitySchemeType == SecuritySchemeType.CookieAPIKey
+  ? Constants.oneTwoThreeTxt
+  : itemSecurityScheme.securitySchemeType == SecuritySchemeType.BasicHTTP
+  ? Constants.demoUsernameTxt
+  : itemSecurityScheme.securitySchemeType == SecuritySchemeType.BearerHTTP
+  ? Constants.oneTwoThreeTxt
+  : itemSecurityScheme.securitySchemeType == SecuritySchemeType.DigestHTTP
+  ? Constants.digestDemoTxt
+  : itemSecurityScheme.securitySchemeType == SecuritySchemeType.OAuth2PasswordFlow ||
+    itemSecurityScheme.securitySchemeType == SecuritySchemeType.OAuth2ClientFlow
+  ? Constants.emptyTxt
+  : itemSecurityScheme.securitySchemeType == SecuritySchemeType.OAuth2ImplicitFlow
+  ? Constants.emptyTxt
+  : itemSecurityScheme.securitySchemeType == SecuritySchemeType.OAuth2CodeFlow
+  ? Constants.emptyTxt
+  : Constants.emptyTxt,
+);
+}
diff --git a/lib/src/back_end/solver.dart b/lib/src/back_end/solver.dart
@@ -9,6 +9,13 @@ import '../profile.dart';
 import 'solution.dart';
 import 'solution_cache.dart';
 
+/// To ensure the solver doesn't go totally pathological on giant code, we cap
+/// it at a fixed number of attempts.
+///
+/// If the optimal solution isn't found after this many tries, it just uses the
+/// best it found so far.
+const _maxAttempts = 10000;
+
 /// Selects states for each piece in a tree of pieces to find the best set of
 /// line splits that minimizes overflow characters and line splitting costs.
 ///
@@ -85,18 +92,17 @@ class Solver {
     // The lowest cost solution found so far that does overflow.
     var best = solution;
 
-    var tries = 0;
+    var attempts = 0;
 
-    // TODO(perf): Consider bailing out after a certain maximum number of tries,
-    // so that it outputs suboptimal formatting instead of hanging entirely.
-    while (_queue.isNotEmpty) {
+    while (_queue.isNotEmpty && attempts < _maxAttempts) {
       Profile.begin('Solver dequeue');
       var solution = _queue.removeFirst();
       Profile.end('Solver dequeue');
 
+      attempts++;
+
       if (debug.traceSolver) {
-        tries++;
-        debug.log(debug.bold('Try #$tries $solution'));
+        debug.log(debug.bold('Try #$attempts $solution'));
         debug.log(solution.text);
         debug.log('');
       }

diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart
@@ -324,21 +324,44 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
     // conditional expression to split.
     var leadingComments = pieces.takeCommentsBefore(node.firstNonCommentToken);
 
-    var condition = nodePiece(node.condition);
+    // Flatten a series of else-if-like chained conditionals into a single long
+    // infix piece. This produces a flattened style like:
+    //
+    //     condition
+    //     ? thenBranch
+    //     : condition2
+    //     ? thenBranch2
+    //     : elseBranch;
+    //
+    // This (arguably) looks nicer. More importantly, it means that all but the
+    // last operand can be formatted separately, which is important to avoid
+    // pathological performance in the solved with long nested conditional
+    // chains.
+    var operands = [nodePiece(node.condition)];
+
+    void addOperand(Token operator, Expression operand) {
+      operands.add(pieces.build(() {
+        pieces.token(operator);
+        pieces.space();
+        pieces.visit(operand, context: NodeContext.conditionalBranch);
+      }));
+    }
 
-    var thenPiece = pieces.build(() {
-      pieces.token(node.question);
-      pieces.space();
-      pieces.visit(node.thenExpression, context: NodeContext.conditionalBranch);
-    });
+    var conditional = node;
+    while (true) {
+      addOperand(conditional.question, conditional.thenExpression);
 
-    var elsePiece = pieces.build(() {
-      pieces.token(node.colon);
-      pieces.space();
-      pieces.visit(node.elseExpression, context: NodeContext.conditionalBranch);
-    });
+      var elseBranch = conditional.elseExpression;
+      if (elseBranch is ConditionalExpression) {
+        addOperand(conditional.colon, elseBranch.condition);
+        conditional = elseBranch;
+      } else {
+        addOperand(conditional.colon, conditional.elseExpression);
+        break;
+      }
+    }
 
-    var piece = InfixPiece(leadingComments, [condition, thenPiece, elsePiece]);
+    var piece = InfixPiece(leadingComments, operands);
 
     // If conditional expressions are directly nested, force them all to split,
     // both parents and children.

diff --git a/test/tall/expression/condition.stmt b/test/tall/expression/condition.stmt
@@ -43,9 +43,22 @@ var kind =
     a
         ? b
         : c
-            ? d
-            : e;
+        ? d
+        : e;
 >>> Don't force split conditionals when indirectly nested.
 var kind = a ? b : (c ? d : e);
 <<<
-var kind = a ? b : (c ? d : e);
+var kind = a ? b : (c ? d : e);
+>>> Flatten a chain of else-if conditionals.
+var kind = c1 ? e1 : c2 ? e2 : c3 ? e3 : c4 ? e4 : e5;
+<<<
+var kind =
+    c1
+        ? e1
+        : c2
+        ? e2
+        : c3
+        ? e3
+        : c4
+        ? e4
+        : e5;
diff --git a/test/tall/regression/0400/0407.unit b/test/tall/regression/0400/0407.unit
@@ -32,11 +32,11 @@ receiver
             _total == 0
                 ? ""
                 : _chartType == "PieChart"
-                    ? _formatter.formatAsPercent(
-                      item.value / _total,
-                      fractionDigits: 1,
-                    )
-                    : _formatter.formatValue(item.value, item.valueType);
+                ? _formatter.formatAsPercent(
+                  item.value / _total,
+                  fractionDigits: 1,
+                )
+                : _formatter.formatValue(item.value, item.valueType);
     }
 >>> (indent 6)
 main() {
@@ -55,9 +55,9 @@ receiver
               _total == 0
                   ? ""
                   : _chartType == "PieChart"
-                      ? _formatter.formatAsPercent(
-                        item.value / _total,
-                        fractionDigits: 1,
-                      )
-                      : _formatter.formatValue(item.value, item.valueType);
+                  ? _formatter.formatAsPercent(
+                    item.value / _total,
+                    fractionDigits: 1,
+                  )
+                  : _formatter.formatValue(item.value, item.valueType);
       }
diff --git a/test/tall/regression/0700/0713.stmt b/test/tall/regression/0700/0713.stmt
@@ -8,7 +8,7 @@ String type =
     status == 'OK'
         ? 'notices'
         : status == 'NO'
-            ? 'warnings'
-            : status == 'BAD'
-                ? 'errors'
-                : '';
+        ? 'warnings'
+        : status == 'BAD'
+        ? 'errors'
+        : '';
diff --git a/test/tall/regression/0700/0722.stmt b/test/tall/regression/0700/0722.stmt
@@ -22,15 +22,12 @@ Widget(
       project.locked
           ? Icon(Icons.lock)
           : project.fav
-              ? Icon(Icons.star)
-              : project.taps == null
-                  ? Icon(Icons.notifications)
-                  : Text(
-                    suffixNumber(project.taps),
-                    textAlign: TextAlign.center,
-                    style: TextStyle(
-                      fontSize: 18.0,
-                      fontWeight: FontWeight.w600,
-                    ),
-                  ),
+          ? Icon(Icons.star)
+          : project.taps == null
+          ? Icon(Icons.notifications)
+          : Text(
+            suffixNumber(project.taps),
+            textAlign: TextAlign.center,
+            style: TextStyle(fontSize: 18.0, fontWeight: FontWeight.w600),
+          ),
 );
diff --git a/test/tall/regression/0900/0927.unit b/test/tall/regression/0900/0927.unit
@@ -9,6 +9,6 @@ class C {
       _currentSunAngleDeg < 0
           ? 1
           : _currentSunAngleDeg < 10
-              ? 2
-              : 3;
+          ? 2
+          : 3;
 }