Skip to content

Commit

Permalink
Always split switch expressions (#1537)
Browse files Browse the repository at this point in the history
Always split switch expressions.

I figured that since other comma-delimited expression forms don't split
if they don't have to, switch expressions should be the same. But I
went and checked on a huge corpus and every single place where a switch
expression wasn't split was clearly much worse than splitting it would
be.

Fix #1529.
  • Loading branch information
munificent authored Aug 19, 2024
1 parent 02d5bc2 commit 5749a94
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 11 deletions.
2 changes: 1 addition & 1 deletion lib/src/front_end/ast_node_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1718,7 +1718,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void> with PieceFactory {
}

list.rightBracket(node.rightBracket);
pieces.add(list.build());
pieces.add(list.build(forceSplit: node.cases.isNotEmpty));
}

@override
Expand Down
4 changes: 2 additions & 2 deletions lib/src/front_end/delimited_list_builder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class DelimitedListBuilder {

/// Creates the final [ListPiece] out of the added brackets, delimiters,
/// elements, and style.
Piece build() {
Piece build({bool forceSplit = false}) {
// To simplify the piece tree, if there are no elements, just return the
// brackets concatenated together. We don't have to worry about comments
// here since they would be in the [_elements] list if there were any.
Expand All @@ -66,7 +66,7 @@ class DelimitedListBuilder {

var piece =
ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, _style);
if (_mustSplit) piece.pin(State.split);
if (_mustSplit || forceSplit) piece.pin(State.split);
return piece;
}

Expand Down
11 changes: 5 additions & 6 deletions test/tall/expression/switch.stmt
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,20 @@
e = switch(y) {};
<<<
e = switch (y) {};
>>> All one line.
>>> Always split cases even if they would fit.
e = switch (c) { 0 => a, 1 => b };
<<<
e = switch (c) { 0 => a, 1 => b };
e = switch (c) {
0 => a,
1 => b,
};
>>> One case per line.
e = switch (c) { 0 => first, 1 => second };
<<<
e = switch (c) {
0 => first,
1 => second,
};
>>> Remove trailing comma if cases fit on one line.
e = switch (c) { 0 => a, 1 => b, };
<<<
e = switch (c) { 0 => a, 1 => b };
>>> Split some cases at "=>" but not all.
e = switch (c) {
first => a,
Expand Down
8 changes: 6 additions & 2 deletions test/tall/invocation/block_argument_multiple.stmt
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,12 @@ function(before, switch (n) {
function(switch (a) { 1 => 2 }, switch (b) { 1 => 2 });
<<<
function(
switch (a) { 1 => 2 },
switch (b) { 1 => 2 },
switch (a) {
1 => 2,
},
switch (b) {
1 => 2,
},
);
>>> Empty and non-empty switches.
function(switch (a) {}, switch (b) { 1 => 2 }, switch (c) {});
Expand Down
67 changes: 67 additions & 0 deletions test/tall/regression/1500/1529.stmt
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
>>> (indent 4)
return switch (that) { Family() => that, $FamilyOverride() => that.from };
<<<
return switch (that) {
Family() => that,
$FamilyOverride() => that.from,
};
>>> (indent 6)
x = switch (event) {
ProviderDisposeEvent() => switch (event
.debugOrigin) { ProviderDisposeEvent e => [e.refenaId], _ => null },
};
<<<
x = switch (event) {
ProviderDisposeEvent() => switch (event.debugOrigin) {
ProviderDisposeEvent e => [e.refenaId],
_ => null,
},
};
>>> (indent 6)
x = switch (event) {
RebuildEvent() => switch (event
.debugOrigin) { BaseReduxAction a => a.refenaId, _ => null },
ActionDispatchedEvent() => switch (event
.debugOriginRef) { BaseReduxAction a => a.refenaId, _ => null },
MessageEvent() => switch (event
.origin) { BaseReduxAction a => a.refenaId, _ => null },
};
<<<
x = switch (event) {
RebuildEvent() => switch (event.debugOrigin) {
BaseReduxAction a => a.refenaId,
_ => null,
},
ActionDispatchedEvent() => switch (event.debugOriginRef) {
BaseReduxAction a => a.refenaId,
_ => null,
},
MessageEvent() => switch (event.origin) {
BaseReduxAction a => a.refenaId,
_ => null,
},
};
>>> (indent 2)
T? valueOrNull() => switch (this) { Data(:var value) => value, _ => null };
<<<
T? valueOrNull() => switch (this) {
Data(:var value) => value,
_ => null,
};
>>> (indent 2)
String transcription(Locale intl) => switch (intl
.languageCode) { 'zh' => zhCN(this), 'be' => be(this), _ => basic(this) };
<<<
String transcription(Locale intl) => switch (intl.languageCode) {
'zh' => zhCN(this),
'be' => be(this),
_ => basic(this),
};
>>> (indent 4)
return switch (x) { <= 1 => x, >= 3 => x - 4, _ => 2 - x };
<<<
return switch (x) {
<= 1 => x,
>= 3 => x - 4,
_ => 2 - x,
};

0 comments on commit 5749a94

Please sign in to comment.