Revamp conditional expression indentation. (#1626)

Revamp conditional expression indentation.

This PR makes two changes to how conditional expressions are indented:

**Don't indent subsequent conditional branches if an assignment context.**

The formatter has some special handling for infix operators inside assignment-like contexts (after `=`, `:`, or `=>`) to avoid otherwise redundant-seeming indentation. Normally, you'd get:

```dart
variable =
    operand +
        another;
```

The +4 before `operand +` is from the assignment indenting the RHS. And then the +4 before `another;` is that plus the +4 indentation from the binary operator. It looks better if the operands line up, so when a binary operator is in an assignment context, we don't indent it. That gives:

```dart
variable =
    operand +
    another;
```

I think that looks nice, and users have asked for it. For no good reason, when writing the new formatter we didn't do that for conditional expressions. You get:

```dart
variable =
    condition
        ? thenBranch
        : elseBranch;
```

This PR applies the same rule to conditionals that we do for binary operators, giving:

```dart
variable =
    condition
    ? thenBranch
    : elseBranch;
```

**Indent then and else branches past their operator.**

If there's a split *inside* a conditional operator's operand, we have to decide how to indent it. Currently, there is no indentation at all. That looks OK if the internal split is from something like a binary operator:

```dart
condition
    ? longOperand +
        anotherLongOperand
    : thirdOperand +
        anotherLongOperand;
```

But it looks less good if the split is something like a function call:

```dart
condition
    ? function(
      argument,
      another,
    )
    : function(
      argument,
      another,
    );
```

It seems weird that the `)` are at the same indentation as the conditional operators themselves.

This PR fixes #1534 and indents the then and else branches (but not the condition) two spaces past the operands. It does this for all operand subexpressions, giving:

```dart
condition
    ? longOperand +
          anotherLongOperand
    : function(
        argument,
        another,
      );
```

**Together:**

If you put these changes together, then in an assignment context, the `?` and `:` move fours spaces left and their subexpressions move two spaces left:

```dart
// Before:
SomeWidget(
  child:
      condition
          ? AnotherWidget(
            argument,
            argument,
          )
          : ThirdWidget(
            argument,
            argument,
          )
);

// After:
SomeWidget(
  child:
      condition
      ? AnotherWidget(
          argument,
          argument,
        )
      : ThirdWidget(
          argument,
          argument,
        )
);
```

The overall effect is to move conditionals to the left. That in turn means some code that would wrap no longer has to. This ends up being pretty valuable because conditional expressions are very common in Flutter widget code since we don't allow `if` inside argument lists.

I've tested this on a big corpus of pub packages and every diff I saw looked better to my eyes.

CHANGELOG.md

@@ -1,8 +1,48 @@
 ## 3.0.2-wip
 
-* Add tests for digit separators.
+* Don't indent conditional branches redundantly after `=`, `:`, and `=>`.
+
+  ```dart
+  // Before:
+  function(
+    argument:
+        condition
+            ? thenBranch
+            : elseBranch,
+  )
+
+  // After:
+  function(
+    argument:
+        condition
+        ? thenBranch
+        : elseBranch,
+  )
+  ```
+
+* Indent conditional branches past the operators (#1534).
+
+  ```dart
+  // Before:
+  condition
+      ? thenBranch +
+          anotherOperand
+      : elseBranch(
+        argument,
+      );
+
+  // After:
+  condition
+      ? thenBranch +
+            anotherOperand
+      : elseBranch(
+          argument,
+        );
+  ```
+
 * Don't add a trailing comma in lists that don't allow it, even when there is
   a trailing comment (#1639).
+* Add tests for digit separators.
 
 ## 3.0.1
 

benchmark/case/conditional.expect

@@ -3,9 +3,9 @@ void securityItem() {
     itemSecurityScheme.securitySchemeType == SecuritySchemeType.QueryAPIKey
         ? Constants.oneTwoThreeTxt
         : itemSecurityScheme.securitySchemeType ==
-                SecuritySchemeType.HeaderAPIKey ||
-            itemSecurityScheme.securitySchemeType ==
-                SecuritySchemeType.CookieAPIKey
+                  SecuritySchemeType.HeaderAPIKey ||
+              itemSecurityScheme.securitySchemeType ==
+                  SecuritySchemeType.CookieAPIKey
         ? Constants.oneTwoThreeTxt
         : itemSecurityScheme.securitySchemeType == SecuritySchemeType.BasicHTTP
         ? Constants.demoUsernameTxt
@@ -14,15 +14,15 @@ void securityItem() {
         : itemSecurityScheme.securitySchemeType == SecuritySchemeType.DigestHTTP
         ? Constants.digestDemoTxt
         : itemSecurityScheme.securitySchemeType ==
-                SecuritySchemeType.OAuth2PasswordFlow ||
-            itemSecurityScheme.securitySchemeType ==
-                SecuritySchemeType.OAuth2ClientFlow
+                  SecuritySchemeType.OAuth2PasswordFlow ||
+              itemSecurityScheme.securitySchemeType ==
+                  SecuritySchemeType.OAuth2ClientFlow
         ? Constants.emptyTxt
         : itemSecurityScheme.securitySchemeType ==
-            SecuritySchemeType.OAuth2ImplicitFlow
+              SecuritySchemeType.OAuth2ImplicitFlow
         ? Constants.emptyTxt
         : itemSecurityScheme.securitySchemeType ==
-            SecuritySchemeType.OAuth2CodeFlow
+              SecuritySchemeType.OAuth2CodeFlow
         ? Constants.emptyTxt
         : Constants.emptyTxt,
   );

benchmark/case/large.expect

@@ -976,8 +976,8 @@ class Traverser {
         // overrides for all repo packages.
         var pubDep =
             override == null
-                ? new PackageDep(depName, "hosted", constraint, depName)
-                : override.withConstraint(constraint);
+            ? new PackageDep(depName, "hosted", constraint, depName)
+            : override.withConstraint(constraint);
         return _registerDependency(
           new Dependency("pub itself", Version.none, pubDep),
         );

lib/src/back_end/code_writer.dart

@@ -98,15 +98,24 @@ final class CodeWriter {
   final Set<Piece> _currentLinePieces = {};
 
   /// [leadingIndent] is the number of spaces of leading indentation at the
-  /// beginning of each line independent of indentation created by pieces being
+  /// beginning of the first line and [subsequentIndent] is the indentation of
+  /// each line after that, independent of indentation created by pieces being
   /// written.
-  CodeWriter(this._pageWidth, int leadingIndent, this._cache, this._solution)
+  CodeWriter(this._pageWidth, int leadingIndent, int subsequentIndent,
+      this._cache, this._solution)
       : _code = GroupCode(leadingIndent) {
     _indentStack.add(_Indent(leadingIndent, 0));
 
     // Track the leading indent before the first line.
     _pendingIndent = leadingIndent;
     _column = _pendingIndent;
+
+    // If there is additional indentation on subsequent lines, then push that
+    // onto the stack. When the first newline is written, [_pendingIndent] will
+    // pick this up and use it for subsequent lines.
+    if (subsequentIndent > leadingIndent) {
+      _indentStack.add(_Indent(subsequentIndent, 0));
+    }
   }
 
   /// Returns the final formatted code and the next pieces that can be expanded
@@ -249,8 +258,10 @@ final class CodeWriter {
   /// Format [piece] using a separate [Solver] and merge the result into this
   /// writer's [_solution].
   void _formatSeparate(Piece piece) {
-    var solution = _cache.find(
-        _pageWidth, piece, _pendingIndent, _solution.pieceStateIfBound(piece));
+    var solution = _cache.find(piece, _solution.pieceStateIfBound(piece),
+        pageWidth: _pageWidth,
+        indent: _pendingIndent,
+        subsequentIndent: _indentStack.last.indent);
 
     _pendingIndent = 0;
     _flushWhitespace();

lib/src/back_end/solution.dart

@@ -114,15 +114,17 @@ final class Solution implements Comparable<Solution> {
   /// implicitly means they have state [State.unsplit] unless they're pinned to
   /// another state).
   factory Solution(SolutionCache cache, Piece root,
-      {required int pageWidth, required int leadingIndent, State? rootState}) {
-    var solution =
-        Solution._(cache, root, pageWidth, leadingIndent, 0, {}, {}, rootState);
-    solution._format(cache, root, pageWidth, leadingIndent);
+      {required int pageWidth,
+      required int leadingIndent,
+      required int subsequentIndent,
+      State? rootState}) {
+    var solution = Solution._(cache, root, 0, {}, {}, rootState);
+    solution._format(cache, root, pageWidth, leadingIndent, subsequentIndent);
     return solution;
   }
 
-  Solution._(SolutionCache cache, Piece root, int pageWidth, int leadingIndent,
-      this._cost, this._pieceStates, this._allowedStates,
+  Solution._(SolutionCache cache, Piece root, this._cost, this._pieceStates,
+      this._allowedStates,
       [State? rootState]) {
     Profile.count('create Solution');
 
@@ -196,7 +198,9 @@ final class Solution implements Comparable<Solution> {
   /// If there is no potential piece to expand, or all attempts to expand it
   /// fail, returns an empty list.
   List<Solution> expand(SolutionCache cache, Piece root,
-      {required int pageWidth, required int leadingIndent}) {
+      {required int pageWidth,
+      required int leadingIndent,
+      required int subsequentIndent}) {
     // If there is no piece that we can expand on this solution, it's a dead
     // end (or a winner).
     if (_expandPieces.isEmpty) return const [];
@@ -210,8 +214,8 @@ final class Solution implements Comparable<Solution> {
       var expandPiece = _expandPieces[i];
       for (var state
           in _allowedStates[expandPiece] ?? expandPiece.additionalStates) {
-        var expanded = Solution._(cache, root, pageWidth, leadingIndent, _cost,
-            {..._pieceStates}, {..._allowedStates});
+        var expanded = Solution._(
+            cache, root, _cost, {..._pieceStates}, {..._allowedStates});
 
         // Bind all preceding expand pieces to their unsplit state. Their
         // other states have already been expanded by earlier iterations of
@@ -233,7 +237,8 @@ final class Solution implements Comparable<Solution> {
 
         // Discard the solution if we hit a constraint violation.
         if (!expanded._isDeadEnd) {
-          expanded._format(cache, root, pageWidth, leadingIndent);
+          expanded._format(
+              cache, root, pageWidth, leadingIndent, subsequentIndent);
 
           // TODO(rnystrom): These come mostly (entirely?) from hard newlines
           // in sequences, comments, and multiline strings. It should be
@@ -290,9 +295,10 @@ final class Solution implements Comparable<Solution> {
 
   /// Run a [CodeWriter] on this solution to produce the final formatted output
   /// and calculate the overflow and expand pieces.
-  void _format(
-      SolutionCache cache, Piece root, int pageWidth, int leadingIndent) {
-    var writer = CodeWriter(pageWidth, leadingIndent, cache, this);
+  void _format(SolutionCache cache, Piece root, int pageWidth,
+      int leadingIndent, int subsequentIndent) {
+    var writer =
+        CodeWriter(pageWidth, leadingIndent, subsequentIndent, cache, this);
     writer.format(root);
 
     var (code, expandPieces) = writer.finish();

lib/src/back_end/solution_cache.dart

@@ -26,18 +26,25 @@ final class SolutionCache {
   final _cache = <_Key, Solution>{};
 
   /// Returns a previously cached solution for formatting [root] with leading
-  /// [indent] or produces a new solution, caches it, and returns it.
+  /// [indent] (and [subsequentIndent] for lines after the first) or produces a
+  /// new solution, caches it, and returns it.
   ///
   /// If [root] is already bound to a state in the surrounding piece tree's
   /// [Solution], then [stateIfBound] is that state. Otherwise, it is treated
   /// as unbound and the cache will find a state for [root] as well as its
   /// children.
-  Solution find(int pageWidth, Piece root, int indent, State? stateIfBound) {
+  Solution find(Piece root, State? stateIfBound,
+      {required int pageWidth,
+      required int indent,
+      required int subsequentIndent}) {
     // See if we've already formatted this piece at this indentation. If not,
     // format it and store the result.
     return _cache.putIfAbsent(
-        (root, indent: indent),
-        () => Solver(this, pageWidth: pageWidth, leadingIndent: indent)
+        (root, indent: indent, subsequentIndent: subsequentIndent),
+        () => Solver(this,
+                pageWidth: pageWidth,
+                leadingIndent: indent,
+                subsequentIndent: subsequentIndent)
             .format(root, stateIfBound));
   }
 }
@@ -51,4 +58,4 @@ final class SolutionCache {
 /// In particular, note that if surrounding pieces split in *different* ways
 /// that still end up producing the same overall leading indentation, we are
 /// able to reuse a previously cached Solution for some Piece.
-typedef _Key = (Piece, {int indent});
+typedef _Key = (Piece, {int indent, int subsequentIndent});

lib/src/back_end/solver.dart

@@ -46,13 +46,25 @@ final class Solver {
   final SolutionCache _cache;
 
   final int _pageWidth;
+
+  /// The number of spaces of indentation on the first line.
   final int _leadingIndent;
 
+  /// The number of spaces of indentation on all lines after the first.
+  final int _subsequentIndent;
+
   final PriorityQueue<Solution> _queue = PriorityQueue();
 
-  Solver(this._cache, {required int pageWidth, int leadingIndent = 0})
+  /// Creates a solver that fits code into the given [pageWidth].
+  ///
+  /// The first line is indented by [leadingIndent] spaces and all lines after
+  /// that are indented by [subsequentIndent]. If [subsequentIndent] is omitted,
+  /// defaults to [leadingIndent].
+  Solver(this._cache,
+      {required int pageWidth, int leadingIndent = 0, int? subsequentIndent})
       : _pageWidth = pageWidth,
-        _leadingIndent = leadingIndent;
+        _leadingIndent = leadingIndent,
+        _subsequentIndent = subsequentIndent ?? leadingIndent;
 
   /// Finds the best set of line splits for [root] piece and returns the
   /// resulting formatted code.
@@ -83,6 +95,7 @@ final class Solver {
     var solution = Solution(_cache, root,
         pageWidth: _pageWidth,
         leadingIndent: _leadingIndent,
+        subsequentIndent: _subsequentIndent,
         rootState: rootState);
 
     Profile.begin('Solver enqueue');
@@ -134,7 +147,9 @@ final class Solver {
       // Otherwise, try to expand the solution to explore different splitting
       // options.
       for (var expanded in solution.expand(_cache, root,
-          pageWidth: _pageWidth, leadingIndent: _leadingIndent)) {
+          pageWidth: _pageWidth,
+          leadingIndent: _leadingIndent,
+          subsequentIndent: _subsequentIndent)) {
         Profile.count('Solver enqueue');
         _queue.add(expanded);
 

lib/src/front_end/ast_node_visitor.dart

@@ -345,6 +345,10 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
     var operands = [nodePiece(node.condition)];
 
     void addOperand(Token operator, Expression operand) {
+      // If there are comments before a branch, then hoist them so they aren't
+      // indented with the branch body.
+      operands.addAll(pieces.takeCommentsBefore(operator));
+
       operands.add(pieces.build(() {
         pieces.token(operator);
         pieces.space();
@@ -366,7 +370,9 @@ final class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
       }
     }
 
-    var piece = InfixPiece(operands);
+    // Don't redundantly indent a split conditional in an assignment context.
+    var piece = InfixPiece(operands,
+        indent: _parentContext != NodeContext.assignment, conditional: true);
 
     // If conditional expressions are directly nested, force them all to split,
     // both parents and children.

lib/src/piece/infix.dart

@@ -20,7 +20,12 @@ final class InfixPiece extends Piece {
   /// Whether operands after the first should be indented if split.
   final bool _indent;
 
-  InfixPiece(this._operands, {bool indent = true}) : _indent = indent;
+  /// Whether this piece is for a conditional expression.
+  final bool _isConditional;
+
+  InfixPiece(this._operands, {bool indent = true, bool conditional = false})
+      : _indent = indent,
+        _isConditional = conditional;
 
   @override
   List<State> get additionalStates => const [State.split];
@@ -32,14 +37,21 @@ final class InfixPiece extends Piece {
   void format(CodeWriter writer, State state) {
     if (_indent) writer.pushIndent(Indent.expression);
 
-    for (var i = 0; i < _operands.length; i++) {
+    writer.format(_operands[0]);
+
+    for (var i = 1; i < _operands.length; i++) {
+      writer.splitIf(state == State.split);
+
+      // If this is a branch of a conditional expression, then indent the
+      // branch's contents past the `?` or `:`.
+      if (_isConditional) writer.pushIndent(Indent.block);
+
       // We can format each operand separately if the operand is on its own
       // line. This happens when the operator is split and we aren't the first
       // or last operand.
-      var separate = state == State.split && i > 0 && i < _operands.length - 1;
-
+      var separate = state == State.split && i < _operands.length - 1;
       writer.format(_operands[i], separate: separate);
-      if (i < _operands.length - 1) writer.splitIf(state == State.split);
+      if (_isConditional) writer.popIndent();
     }
 
     if (_indent) writer.popIndent();