Skip to content

Commit

Permalink
Format || patterns like fallthrough cases in switch expressions. (#…
Browse files Browse the repository at this point in the history
…1620)

Switch statements allow multiple cases to share a body like:

```dart
switch (obj) {
  case pattern1:
  case pattern2:
    body;
}
```

Switch expressions don't support that, but `||` patterns are the idiomatic way to accomplish the same thing. Because of that, the formatter has some special formatting when the outermost pattern in a switch expression case is `||`:

```dart
x = switch (obj) {
  pattern1 ||
  pattern2 =>
    body,
};
```

Note how the `pattern2` operand isn't indented.

This PR extends that special handling to allow the `=>` on the same line as the `=>` even if the pattern is a split `||` pattern, like:

```dart
x = switch (obj) {
  pattern1 ||
  pattern2 => body,
};
```

And it prefers to split the `||` over the body when the body is block formatted:

```dart
// Prefer:
x = switch (obj) {
  pattern1 ||
  pattern2 => function(argument),
};

// Over:
x = switch (obj) {
  pattern1 || pattern2 => function(
    argument,
  ),
};
```

This is one of those rules that's mostly a matter of taste, but I ran this on a large corpus and most of the diffs look better to me. Here are a few examples:

```dart
// Before:
      typeName = switch (targetType) {
        DriftSqlType.int || DriftSqlType.bigInt || DriftSqlType.bool =>
          'INTEGER',
        DriftSqlType.string => 'CHAR',
        DriftSqlType.double => 'DOUBLE',
        DriftSqlType.blob => 'BINARY',
        DriftSqlType.dateTime => 'DATETIME',
        DriftSqlType.any => '',
        CustomSqlType() || DialectAwareSqlType() => targetType.sqlTypeName(
          context,
        ),
      };

// After:
      typeName = switch (targetType) {
        DriftSqlType.int ||
        DriftSqlType.bigInt ||
        DriftSqlType.bool => 'INTEGER',
        DriftSqlType.string => 'CHAR',
        DriftSqlType.double => 'DOUBLE',
        DriftSqlType.blob => 'BINARY',
        DriftSqlType.dateTime => 'DATETIME',
        DriftSqlType.any => '',
        CustomSqlType() ||
        DialectAwareSqlType() => targetType.sqlTypeName(context),
      };

// Before:
    return switch (side) {
      AxisSide.right || AxisSide.left =>
        titlesPadding.vertical + borderPadding.vertical,
      AxisSide.top || AxisSide.bottom =>
        titlesPadding.horizontal + borderPadding.horizontal,
    };

// After:
    return switch (side) {
      AxisSide.right ||
      AxisSide.left => titlesPadding.vertical + borderPadding.vertical,
      AxisSide.top ||
      AxisSide.bottom => titlesPadding.horizontal + borderPadding.horizontal,
    };

// Before:
    final defaultConstraints = switch (side) {
      ShadSheetSide.top || ShadSheetSide.bottom => BoxConstraints(
        minWidth: mSize.width,
      ),
      ShadSheetSide.left || ShadSheetSide.right => BoxConstraints(
        minHeight: mSize.height,
      ),
    };

    final defaultConstraints = switch (side) {
      ShadSheetSide.top ||
      ShadSheetSide.bottom => BoxConstraints(minWidth: mSize.width),
      ShadSheetSide.left ||
      ShadSheetSide.right => BoxConstraints(minHeight: mSize.height),
    };
```

Fix #1602.
  • Loading branch information
munificent authored Dec 10, 2024
1 parent 47bcc07 commit 7c4a960
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 29 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## 3.0.1-wip

* Handle trailing commas in for-loop updaters (#1354).
* Format `||` patterns like fallthrough cases in switch expressions (#1602).
* Handle comments and metadata before variables more gracefully (#1604).
* Ensure comment formatting is idempotent (#1606).
* Better indentation of leading comments on property accesses in binary operator
Expand Down
30 changes: 22 additions & 8 deletions lib/src/piece/case.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,20 @@ import 'piece.dart';

/// Piece for a case pattern, guard, and body in a switch expression.
final class CaseExpressionPiece extends Piece {
/// Split inside the body, which must be block formattable, like:
///
/// pattern => function(
/// argument,
/// ),
static const State _blockSplitBody = State(1, cost: 0);

/// Split after the `=>` before the body.
static const State _beforeBody = State(1);
static const State _beforeBody = State(2);

/// Split before the `when` guard clause and after the `=>`.
static const State _beforeWhenAndBody = State(2);
static const State _beforeWhenAndBody = State(3);

/// The pattern the value is matched against along with the leading `case`.
/// The pattern the value is matched against.
final Piece _pattern;

/// If there is a `when` clause, that clause.
Expand Down Expand Up @@ -54,17 +61,23 @@ final class CaseExpressionPiece extends Piece {

@override
List<State> get additionalStates => [
if (_canBlockSplitBody) _blockSplitBody,
_beforeBody,
if (_guard != null) ...[_beforeWhenAndBody],
];

@override
bool allowNewlineInChild(State state, Piece child) {
return switch (state) {
// If the outermost pattern is `||`, then always let it split even while
// allowing the body on the same line as `=>`.
_ when child == _pattern && _patternIsLogicalOr => true,

// There are almost never splits in the arrow piece. It requires a comment
// in a funny location, but if it happens, allow it.
_ when child == _arrow => true,
State.unsplit when child == _body && _canBlockSplitBody => true,
_beforeBody when child == _pattern =>
_guard == null || _patternIsLogicalOr,
_blockSplitBody when child == _body => true,
_beforeBody when child == _pattern => _guard == null,
_beforeBody when child == _body => true,
_beforeWhenAndBody => true,
_ => false,
Expand Down Expand Up @@ -94,12 +107,13 @@ final class CaseExpressionPiece extends Piece {
writer.space();
writer.format(_arrow);

if (state != State.unsplit) writer.pushIndent(Indent.block);
var indentBody = state != State.unsplit && state != _blockSplitBody;
if (indentBody) writer.pushIndent(Indent.block);

writer.splitIf(state == _beforeBody || state == _beforeWhenAndBody);
writer.format(_body);

if (state != State.unsplit) writer.popIndent();
if (indentBody) writer.popIndent();
}

@override
Expand Down
16 changes: 13 additions & 3 deletions test/tall/expression/switch.stmt
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,7 @@ e = switch (obj) {
e = switch (obj) {
oneConstant ||
twoConstant ||
threeConstant =>
body,
threeConstant => body,
};
>>> Nested logic-or operands are indented.
e = switch (obj) {
Expand Down Expand Up @@ -175,4 +174,15 @@ e = switch (obj) {
veryLongElement,
veryLongElement,
),
};
};
>>> Prefer to split `||` pattern instead of case body.
e = switch (obj) {
pattern || another => function(
argument,
),
};
<<<
e = switch (obj) {
pattern ||
another => function(argument),
};
3 changes: 1 addition & 2 deletions test/tall/expression/switch_guard.stmt
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,7 @@ e = switch (obj) {
<<<
e = switch (obj) {
veryVeryLongPattern ||
reallyMustSplitHere when true =>
body,
reallyMustSplitHere when true => body,
};
>>> Outermost logic-or split in pattern, expression split in guard.
e = switch (obj) {
Expand Down
36 changes: 20 additions & 16 deletions test/tall/regression/1100/1197.unit
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ main() {
}
}
<<<
### TODO(1466): Ideally, the first case would also split at the `||` instead of
### of before `.`, but the formatter can't distinguish that case without fixing
### #1466.
main() {
{
return TextFieldTapRegion(
Expand All @@ -38,14 +41,13 @@ main() {
TargetPlatform.android ||
TargetPlatform.fuchsia ||
TargetPlatform.linux ||
TargetPlatform.windows =>
_renderEditable.selectWordsInRange(
from:
longPressMoveUpdateDetails.globalPosition -
longPressMoveUpdateDetails.offsetFromOrigin,
to: longPressMoveUpdateDetails.globalPosition,
cause: SelectionChangedCause.longPress,
),
TargetPlatform.windows => _renderEditable.selectWordsInRange(
from:
longPressMoveUpdateDetails.globalPosition -
longPressMoveUpdateDetails.offsetFromOrigin,
to: longPressMoveUpdateDetails.globalPosition,
cause: SelectionChangedCause.longPress,
),
});
},
);
Expand Down Expand Up @@ -77,6 +79,9 @@ main() {
}
}
<<<
### TODO(1466): Ideally, the first case would also split at the `||` instead of
### of before `.`, but the formatter can't distinguish that case without fixing
### #1466.
main() {
{
return TextFieldTapRegion(
Expand All @@ -92,14 +97,13 @@ main() {
TargetPlatform.android ||
TargetPlatform.fuchsia ||
TargetPlatform.linux ||
TargetPlatform.windows =>
_renderEditable.selectWordsInRange(
from:
longPressMoveUpdateDetails.globalPosition -
longPressMoveUpdateDetails.offsetFromOrigin,
to: longPressMoveUpdateDetails.globalPosition,
cause: SelectionChangedCause.longPress,
),
TargetPlatform.windows => _renderEditable.selectWordsInRange(
from:
longPressMoveUpdateDetails.globalPosition -
longPressMoveUpdateDetails.offsetFromOrigin,
to: longPressMoveUpdateDetails.globalPosition,
cause: SelectionChangedCause.longPress,
),
},
);
}
Expand Down
12 changes: 12 additions & 0 deletions test/tall/regression/1600/1602.stmt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
>>> (indent 4)
return switch (_advance().type) {
TokenType.float || TokenType.int || TokenType.string => LiteralExpression(
_previous.value!,
),
};
<<<
return switch (_advance().type) {
TokenType.float ||
TokenType.int ||
TokenType.string => LiteralExpression(_previous.value!),
};

0 comments on commit 7c4a960

Please sign in to comment.