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.
diff --git a/benchmark/case/conditional.expect b/benchmark/case/conditional.expect
new file mode 100644
index 0000000..48e6f22
--- /dev/null
+++ 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
new file mode 100644
index 0000000..62ba360
--- /dev/null
+++ 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
new file mode 100644
index 0000000..33ea6bd
--- /dev/null
+++ 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
index 94983ea..43e4770 100644
--- a/lib/src/back_end/solver.dart
+++ b/lib/src/back_end/solver.dart
@@ -9,6 +9,13 @@
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 @@
// 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
index 8cd9d35..403b71e 100644
--- a/lib/src/front_end/ast_node_visitor.dart
+++ b/lib/src/front_end/ast_node_visitor.dart
@@ -324,21 +324,44 @@
// 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)];
- var thenPiece = pieces.build(() {
- pieces.token(node.question);
- pieces.space();
- pieces.visit(node.thenExpression, context: NodeContext.conditionalBranch);
- });
+ void addOperand(Token operator, Expression operand) {
+ operands.add(pieces.build(() {
+ pieces.token(operator);
+ pieces.space();
+ pieces.visit(operand, context: NodeContext.conditionalBranch);
+ }));
+ }
- var elsePiece = pieces.build(() {
- pieces.token(node.colon);
- pieces.space();
- pieces.visit(node.elseExpression, context: NodeContext.conditionalBranch);
- });
+ var conditional = node;
+ while (true) {
+ addOperand(conditional.question, conditional.thenExpression);
- var piece = InfixPiece(leadingComments, [condition, thenPiece, elsePiece]);
+ 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, 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
index b06ae45..991d2df 100644
--- a/test/tall/expression/condition.stmt
+++ b/test/tall/expression/condition.stmt
@@ -43,9 +43,22 @@
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);
\ No newline at end of file
+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;
\ No newline at end of file
diff --git a/test/tall/regression/0400/0407.unit b/test/tall/regression/0400/0407.unit
index fb18fa2..82a882f 100644
--- a/test/tall/regression/0400/0407.unit
+++ b/test/tall/regression/0400/0407.unit
@@ -32,11 +32,11 @@
_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 @@
_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);
}
\ No newline at end of file
diff --git a/test/tall/regression/0700/0713.stmt b/test/tall/regression/0700/0713.stmt
index e1fa594..2ad4f72 100644
--- a/test/tall/regression/0700/0713.stmt
+++ b/test/tall/regression/0700/0713.stmt
@@ -8,7 +8,7 @@
status == 'OK'
? 'notices'
: status == 'NO'
- ? 'warnings'
- : status == 'BAD'
- ? 'errors'
- : '';
\ No newline at end of file
+ ? 'warnings'
+ : status == 'BAD'
+ ? 'errors'
+ : '';
\ No newline at end of file
diff --git a/test/tall/regression/0700/0722.stmt b/test/tall/regression/0700/0722.stmt
index 2d12cc9..663ea2e 100644
--- a/test/tall/regression/0700/0722.stmt
+++ b/test/tall/regression/0700/0722.stmt
@@ -22,15 +22,12 @@
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),
+ ),
);
\ No newline at end of file
diff --git a/test/tall/regression/0900/0927.unit b/test/tall/regression/0900/0927.unit
index fa2bd64..0448d84 100644
--- a/test/tall/regression/0900/0927.unit
+++ b/test/tall/regression/0900/0927.unit
@@ -9,6 +9,6 @@
_currentSunAngleDeg < 0
? 1
: _currentSunAngleDeg < 10
- ? 2
- : 3;
+ ? 2
+ : 3;
}
\ No newline at end of file
