Format adjacent strings. (#1364)
Format adjacent strings.
Most of this was fairly straightforward, but the two tricky bits are:
### 1. Deciding whether or not to indent
There are some rules around whether subsequent strings in the adjacent
strings get indented or not. The answer is yes in some cases to avoid
confusion:
```
var list = function(
'string 1',
'adjacent'
'string 2',
'string 3',
];
```
But not in others since it looks nicer to line them up when possible:
```
var description =
'some text '
'more text';
```
### 2. Handling `test()` and `group()`
It's really important that test functions don't end up fully split
because doing so would lead to the inner function expression getting
indented +2:
```
test('this looks good', () {
body;
});
test(
'this looks bad',
() {
body;
},
);
```
Test descriptions often span multiple lines using adjacent strings:
```
test('this is a very long test description '
'spanning multiple lines, () {
body;
});
```
Normally, the newline inside the adjacent strings would cause the entire
argument list to split. The old style handles that (I think) by allowing
multiple block-formatted arguments and then treating both the adjacent
strings and the function expressions as block arguments.
The new style currently only allows a single block argument (because in
almost all of the Flutter code I found using block formatting, one
argument was sufficient). So I chose a more narrowly targeted rule here
where we allow adjacent strings to not prevent block formatting only if
the adjacent strings are the first argument and the block argument is a
function expression as the next argument.
I left a TODO to see if we want to iterate on that rule, but I think it
works pretty well.
### Other stuff
Unlike the old style, I chose to always split between adjacent strings.
The old style will preserve newlines there but if a user chooses to
deliberately put multiple adjacent strings on the same line and they
fit, it will honor it. That didn't seem useful to me, so now they just
always split. I don't think adjacent strings ever look good on the same
line.
I ended up moving the state to track which elements in a ListPiece out
of ListPiece and into the ListElements themselves. I think it's clearer
this way and will be easier to evolve if we end up supporting multiple
block formatted elements in a single list.
diff --git a/lib/src/ast_extensions.dart b/lib/src/ast_extensions.dart
index 92434e6..214338b 100644
--- a/lib/src/ast_extensions.dart
+++ b/lib/src/ast_extensions.dart
@@ -314,6 +314,63 @@
}
}
+extension AdjacentStringsExtensions on AdjacentStrings {
+ /// Whether subsequent strings should be indented relative to the first
+ /// string.
+ ///
+ /// We generally want to indent adjacent strings because it can be confusing
+ /// otherwise when they appear in a list of expressions, like:
+ ///
+ /// [
+ /// "one",
+ /// "two"
+ /// "three",
+ /// "four"
+ /// ]
+ ///
+ /// Especially when these strings are longer, it can be hard to tell that
+ /// "three" is a continuation of the previous element.
+ ///
+ /// However, the indentation is distracting in places that don't suffer from
+ /// this ambiguity:
+ ///
+ /// var description =
+ /// "A very long description..."
+ /// "this extra indentation is unnecessary.");
+ ///
+ /// To balance these, we omit the indentation when an adjacent string
+ /// expression is in a context where it's unlikely to be confusing.
+ bool get indentStrings {
+ bool hasOtherStringArgument(List<Expression> arguments) => arguments
+ .any((argument) => argument != this && argument is StringLiteral);
+
+ return switch (parent) {
+ ArgumentList(:var arguments) => hasOtherStringArgument(arguments),
+
+ // Treat asserts like argument lists.
+ Assertion(:var condition, :var message) =>
+ hasOtherStringArgument([condition, if (message != null) message]),
+
+ // Don't add extra indentation in a variable initializer or assignment:
+ //
+ // var variable =
+ // "no extra"
+ // "indent";
+ VariableDeclaration() => false,
+ AssignmentExpression(:var rightHandSide) when rightHandSide == this =>
+ false,
+
+ // Don't indent when following `:`.
+ MapLiteralEntry(:var value) when value == this => false,
+ NamedExpression() => false,
+
+ // Don't indent when the body of a `=>` function.
+ ExpressionFunctionBody() => false,
+ _ => true,
+ };
+ }
+}
+
extension PatternExtensions on DartPattern {
/// Whether this expression is a non-empty delimited container for inner
/// expressions that allows "block-like" formatting in some contexts.
diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart
index 00c863c..be076c3 100644
--- a/lib/src/front_end/ast_node_visitor.dart
+++ b/lib/src/front_end/ast_node_visitor.dart
@@ -10,6 +10,7 @@
import '../constants.dart';
import '../dart_formatter.dart';
import '../piece/adjacent.dart';
+import '../piece/adjacent_strings.dart';
import '../piece/assign.dart';
import '../piece/block.dart';
import '../piece/constructor.dart';
@@ -110,7 +111,8 @@
@override
Piece visitAdjacentStrings(AdjacentStrings node) {
- throw UnimplementedError();
+ return AdjacentStringsPiece(node.strings.map(nodePiece).toList(),
+ indent: node.indentStrings);
}
@override
diff --git a/lib/src/front_end/delimited_list_builder.dart b/lib/src/front_end/delimited_list_builder.dart
index 8612c96..429a958 100644
--- a/lib/src/front_end/delimited_list_builder.dart
+++ b/lib/src/front_end/delimited_list_builder.dart
@@ -52,11 +52,10 @@
/// Creates the final [ListPiece] out of the added brackets, delimiters,
/// elements, and style.
ListPiece build() {
- var blockElement = -1;
- if (_style.allowBlockElement) blockElement = _findBlockElement();
+ _setBlockElementFormatting();
- var piece = ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket,
- _style, blockElement);
+ var piece =
+ ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, _style);
if (_mustSplit) piece.pin(State.split);
return piece;
}
@@ -155,6 +154,9 @@
// See if it's an expression that supports block formatting.
var format = switch (element) {
+ AdjacentStrings(indentStrings: true) =>
+ BlockFormat.indentedAdjacentStrings,
+ AdjacentStrings() => BlockFormat.unindentedAdjacentStrings,
FunctionExpression() when element.canBlockSplit => BlockFormat.function,
Expression() when element.canBlockSplit => BlockFormat.block,
DartPattern() when element.canBlockSplit => BlockFormat.block,
@@ -388,12 +390,39 @@
);
}
- /// If [_blockCandidates] contains a single expression that can receive
- /// block formatting, then returns its index. Otherwise returns `-1`.
- int _findBlockElement() {
+ /// Looks at the [BlockFormat] types of all of the elements to determine if
+ /// one of them should be block formatted.
+ ///
+ /// Also, if an argument list has an adjacent strings expression followed by a
+ /// block formattable function expression, we allow the adjacent strings to
+ /// split without forcing the list to split so that it can continue to have
+ /// block formatting. This is pretty special-cased, but it makes calls to
+ /// `test()` and `group()` look better and those are so common that it's
+ /// worth massaging them some. It allows:
+ ///
+ /// test('some long description'
+ /// 'split across multiple lines', () {
+ /// expect(1, 1);
+ /// });
+ ///
+ /// Without this special rule, the newline in the adjacent strings would
+ /// prevent block formatting and lead to the entire test body to be indented:
+ ///
+ /// test(
+ /// 'some long description'
+ /// 'split across multiple lines',
+ /// () {
+ /// expect(1, 1);
+ /// },
+ /// );
+ ///
+ /// Stores the result of this calculation by setting flags on the
+ /// [ListElement]s.
+ void _setBlockElementFormatting() {
// TODO(tall): These heuristics will probably need some iteration.
var functions = <int>[];
var others = <int>[];
+ var adjacentStrings = <int>[];
for (var i = 0; i < _elements.length; i++) {
switch (_elements[i].blockFormat) {
@@ -401,19 +430,46 @@
functions.add(i);
case BlockFormat.block:
others.add(i);
+ case BlockFormat.indentedAdjacentStrings:
+ case BlockFormat.unindentedAdjacentStrings:
+ adjacentStrings.add(i);
case BlockFormat.none:
break; // Not a block element.
}
}
- // A function expression takes precedence over other block arguments.
- if (functions.length == 1) return functions.first;
+ switch ((functions, others, adjacentStrings)) {
+ // Only allow block formatting in an argument list containing adjacent
+ // strings when:
+ //
+ // 1. The block argument is a function expression.
+ // 2. It is the second argument, following an adjacent strings expression.
+ // 3. There are no other adjacent strings in the argument list.
+ //
+ // This matches the `test()` and `group()` and other similar APIs where
+ // you have a message string followed by a block-like function expression
+ // but little else.
+ // TODO(tall): We may want to iterate on these heuristics. For now,
+ // starting with something very narrowly targeted.
+ case ([1], _, [0]):
+ // The adjacent strings.
+ _elements[0].allowNewlines = true;
+ if (_elements[0].blockFormat == BlockFormat.unindentedAdjacentStrings) {
+ _elements[0].indentWhenBlockFormatted = true;
+ }
- // Otherwise, if there is single block argument, it can be block formatted.
- if (functions.isEmpty && others.length == 1) return others.first;
+ // The block-formattable function.
+ _elements[1].allowNewlines = true;
- // There are no block arguments, or it's ambiguous as to which one should
- // be it.
+ // A function expression takes precedence over other block arguments.
+ case ([var blockArgument], _, _):
+ // Otherwise, if there one block argument, it can be block formatted.
+ case ([], [var blockArgument], _):
+ _elements[blockArgument].allowNewlines = true;
+ }
+
+ // If we get here, there are no block arguments, or it's ambiguous as to
+ // which one should be it so none are.
// TODO(tall): The old formatter allows multiple block arguments, like:
//
// function(() {
@@ -426,6 +482,5 @@
// sometimes. We'll probably want to experiment to see if it's worth
// supporting multiple block arguments. If so, we should at least require
// them to be contiguous with no non-block arguments in the middle.
- return -1;
}
}
diff --git a/lib/src/piece/adjacent_strings.dart b/lib/src/piece/adjacent_strings.dart
new file mode 100644
index 0000000..0015c29
--- /dev/null
+++ b/lib/src/piece/adjacent_strings.dart
@@ -0,0 +1,35 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+import '../back_end/code_writer.dart';
+import '../constants.dart';
+import 'piece.dart';
+
+/// Piece for a series of adjacent strings, like:
+///
+/// var message =
+/// 'This is a long message '
+/// 'split into multiple strings';
+class AdjacentStringsPiece extends Piece {
+ final List<Piece> _strings;
+
+ /// Whether strings after the first should be indented.
+ final bool _indent;
+
+ AdjacentStringsPiece(this._strings, {bool indent = true}) : _indent = indent;
+
+ @override
+ void format(CodeWriter writer, State state) {
+ if (_indent) writer.setIndent(Indent.expression);
+
+ for (var i = 0; i < _strings.length; i++) {
+ if (i > 0) writer.newline();
+ writer.format(_strings[i]);
+ }
+ }
+
+ @override
+ void forEachChild(void Function(Piece piece) callback) {
+ _strings.forEach(callback);
+ }
+}
diff --git a/lib/src/piece/list.dart b/lib/src/piece/list.dart
index 10b1e52..c815cd4 100644
--- a/lib/src/piece/list.dart
+++ b/lib/src/piece/list.dart
@@ -55,12 +55,8 @@
/// The details of how this particular list should be formatted.
final ListStyle _style;
- /// If this list has an element that can receive block formatting, this is
- /// the elements's index. Otherwise `-1`.
- final int _blockElement;
-
ListPiece(this._before, this._elements, this._blanksAfter, this._after,
- this._style, this._blockElement);
+ this._style);
@override
List<State> get additionalStates => [if (_elements.isNotEmpty) State.split];
@@ -102,16 +98,27 @@
Commas.none => false,
};
- // Only allow newlines in the block element or in all elements if we're
- // fully split.
- writer.setAllowNewlines(i == _blockElement || state == State.split);
-
var element = _elements[i];
+
+ // Only some elements (usually a single block element) allow newlines
+ // when the list itself isn't split.
+ writer.setAllowNewlines(element.allowNewlines || state == State.split);
+
+ // If this element allows newlines when the list isn't split, add
+ // indentation if it requires it.
+ if (state == State.unsplit && element.indentWhenBlockFormatted) {
+ writer.setIndent(Indent.expression);
+ }
+
element.format(writer,
appendComma: appendComma,
// Only allow newlines in comments if we're fully split.
allowNewlinesInComments: state == State.split);
+ if (state == State.unsplit && element.indentWhenBlockFormatted) {
+ writer.setIndent(Indent.none);
+ }
+
// Write a space or newline between elements.
if (!isLast) {
writer.splitIf(state != State.unsplit,
@@ -172,6 +179,35 @@
/// What kind of block formatting can be applied to this element.
final BlockFormat blockFormat;
+ /// Whether newlines are allowed in this element when this list is unsplit.
+ ///
+ /// This is generally only true for a single "block" element, as in:
+ ///
+ /// function(argument, [
+ /// block,
+ /// element,
+ /// ], another);
+ bool allowNewlines = false;
+
+ /// Whether we should increase indentation when formatting this element when
+ /// the list isn't split.
+ ///
+ /// This only comes into play for unsplit lists and is only relevant when the
+ /// element contains newlines, which means that this is only ever useful when
+ /// [allowNewlines] is also true.
+ ///
+ /// This is used for adjacent strings expression at the beginning of an
+ /// argument list followed by a function expression, like in a `test()` call.
+ /// Since the adjacent strings may not require indentation when the list is
+ /// fully split, this ensures that they are indented properly when the list
+ /// isn't split. Avoids:
+ //
+ // test('long description'
+ // 'that should be indented', () {
+ // body;
+ // });
+ bool indentWhenBlockFormatted = false;
+
/// If this piece has an opening delimiter after the comma, this is its
/// lexeme, otherwise an empty string.
///
@@ -286,6 +322,16 @@
/// can be block formatted.
block,
+ /// The element is an adjacent strings expression that's in an list that
+ /// requires its subsequent lines to be indented (because there are other
+ /// string literal in the list).
+ indentedAdjacentStrings,
+
+ /// The element is an adjacent strings expression that's in an list that
+ /// doesn't require its subsequent lines to be indented (because there
+ /// are no other string literals in the list).
+ unindentedAdjacentStrings,
+
/// The element can't be block formatted.
none,
}
diff --git a/test/expression/string_adjacent.stmt b/test/expression/string_adjacent.stmt
new file mode 100644
index 0000000..01f3484
--- /dev/null
+++ b/test/expression/string_adjacent.stmt
@@ -0,0 +1,259 @@
+40 columns |
+>>> Always split even if there is no existing newline and they fit.
+var name = 'a' 'b''c';
+<<<
+var name =
+ 'a'
+ 'b'
+ 'c';
+>>> Split when they don't fit too, of course.
+var name = "the first very long string" "the second very longstring";
+<<<
+var name =
+ "the first very long string"
+ "the second very longstring";
+>>> All adjacent strings split or don't split together.
+var text = "first" "second" "third" "fourth" "fifth";
+<<<
+var text =
+ "first"
+ "second"
+ "third"
+ "fourth"
+ "fifth";
+>>> Don't preserve newlines between adjacent strings.
+var name = "the first string"
+"the second string"
+
+
+
+"the third string";
+<<<
+var name =
+ "the first string"
+ "the second string"
+ "the third string";
+>>> Adjacent strings inside interpolation.
+var x = '${ "a" "b" }';
+<<<
+var x =
+ '${"a"
+ "b"}';
+>>> Don't indent in argument list if other arguments are not strings.
+function(notString, "adjacent" "string");
+<<<
+function(
+ notString,
+ "adjacent"
+ "string",
+);
+>>> Do indent if another argument is a string.
+function("string",notString,"adjacent" "string");
+<<<
+function(
+ "string",
+ notString,
+ "adjacent"
+ "string",
+);
+>>> Do indent if another argument is a string interpolation.
+function("${str}${ing}",notString,"adjacent" "string");
+<<<
+function(
+ "${str}${ing}",
+ notString,
+ "adjacent"
+ "string",
+);
+>>> Do indent if another argument is an adjacent string.
+function("adjacent" "string",notString,"adjacent" "string");
+<<<
+function(
+ "adjacent"
+ "string",
+ notString,
+ "adjacent"
+ "string",
+);
+>>> Don't indent in assert if other arguments are not strings.
+assert(
+ condition,
+ "adjacent"
+ "string");
+<<<
+assert(
+ condition,
+ "adjacent"
+ "string",
+);
+>>> Do indent in assert if other argument is a string.
+assert("condition","adjacent" "string");
+<<<
+assert(
+ "condition",
+ "adjacent"
+ "string",
+);
+>>> Do indent adjacent strings in lists.
+var list = ["adjacent""string"];
+<<<
+var list = [
+ "adjacent"
+ "string",
+];
+>>> Do indent adjacent strings in lists.
+var list = [
+ "adjacent"
+ "string",
+ "another"
+ "adjacent"
+ "string"
+];
+<<<
+var list = [
+ "adjacent"
+ "string",
+ "another"
+ "adjacent"
+ "string",
+];
+>>> Do indent in map keys.
+var map = {"adjacent" "string": value};
+<<<
+var map = {
+ "adjacent"
+ "string":
+ value,
+};
+>>> Don't indent in map values.
+var map = {key: "adjacent" "string"};
+<<<
+var map = {
+ key:
+ "adjacent"
+ "string",
+};
+>>> Do indent in sets.
+var set = {"adjacent" "string"};
+<<<
+var set = {
+ "adjacent"
+ "string",
+};
+>>> Do indent positional record fields.
+var record = ("adjacent" "string",);
+<<<
+var record = (
+ "adjacent"
+ "string",
+);
+>>> Don't indent in named record fields.
+var record = (field: "adjacent" "string",);
+<<<
+var record = (
+ field:
+ "adjacent"
+ "string",
+);
+>>> Indent only positional fields in a mixed record.
+var record = ("adjacent" "string", field: "another" "one", "third" "field");
+<<<
+var record = (
+ "adjacent"
+ "string",
+ field:
+ "another"
+ "one",
+ "third"
+ "field",
+);
+>>> Don't indent in `=>` body.
+main() => "adjacent"
+"string"
+"another";
+<<<
+main() =>
+ "adjacent"
+ "string"
+ "another";
+>>> Don't indent in long `=>` body.
+main() => "very very very very long adjacent"
+"string"
+"another";
+<<<
+main() =>
+ "very very very very long adjacent"
+ "string"
+ "another";
+>>> Don't indent in `=>` function expression.
+function(
+(parameter) => "string" "adjacent",
+(parameter) => "long long long long string" "adjacent",
+another);
+<<<
+function(
+ (parameter) =>
+ "string"
+ "adjacent",
+ (parameter) =>
+ "long long long long string"
+ "adjacent",
+ another,
+);
+>>> Indent in then branch of `?:`.
+var string = condition ? "adjacent"
+"string" : "other";
+<<<
+var string =
+ condition
+ ? "adjacent"
+ "string"
+ : "other";
+>>> Indent in else branch of `?:`.
+var string = condition ? "other" : "adjacent"
+"string";
+<<<
+var string =
+ condition
+ ? "other"
+ : "adjacent"
+ "string";
+>>> Don't indent in initializer.
+var longVariableName = "very long adjacent"
+"string";
+<<<
+var longVariableName =
+ "very long adjacent"
+ "string";
+>>> Don't indent in assignment.
+long.receiver.expression = "very long adjacent"
+"string";
+<<<
+long.receiver.expression =
+ "very long adjacent"
+ "string";
+>>>
+function(variable = "very long adjacent"
+"string");
+<<<
+function(
+ variable =
+ "very long adjacent"
+ "string",
+);
+>>> Don't indent inside named arguments.
+function(named: "adjacent"
+"string",
+another: "adjacent"
+"string"
+"more");
+<<<
+function(
+ named:
+ "adjacent"
+ "string",
+ another:
+ "adjacent"
+ "string"
+ "more",
+);
\ No newline at end of file
diff --git a/test/expression/string_adjacent_comment.stmt b/test/expression/string_adjacent_comment.stmt
new file mode 100644
index 0000000..fd45d28
--- /dev/null
+++ b/test/expression/string_adjacent_comment.stmt
@@ -0,0 +1,60 @@
+40 columns |
+>>> Line comment before adjacent string.
+string = // comment
+'adjacent' 'string';
+<<<
+string = // comment
+ 'adjacent'
+ 'string';
+>>> Line comment inside adjacent string.
+string = 'adjacent' // comment
+'string';
+<<<
+string =
+ 'adjacent' // comment
+ 'string';
+>>>
+string = 'adjacent'
+// comment
+'string';
+<<<
+string =
+ 'adjacent'
+ // comment
+ 'string';
+>>> Line comment after adjacent string.
+### Looks weird but users don't put comment here.
+string = 'adjacent' 'string' // comment
+;
+<<<
+string =
+ 'adjacent'
+ 'string' // comment
+ ;
+>>> Inline block comment before adjacent string.
+string = /* comment */ 'adjacent' 'string';
+<<<
+string = /* comment */
+ 'adjacent'
+ 'string';
+>>> Inline block comment inside adjacent string.
+string = 'adjacent' /* comment */ 'string';
+<<<
+string =
+ 'adjacent' /* comment */
+ 'string';
+>>>
+string = 'adjacent'
+/* comment */
+'string';
+<<<
+string =
+ 'adjacent'
+ /* comment */
+ 'string';
+>>> Inline block comment after adjacent string.
+string = 'adjacent' 'string' /* comment */;
+<<<
+string =
+ 'adjacent'
+ 'string' /* comment */;
\ No newline at end of file
diff --git a/test/invocation/block_argument_multiple.stmt b/test/invocation/block_argument_multiple.stmt
index 643dac7..fd32a17 100644
--- a/test/invocation/block_argument_multiple.stmt
+++ b/test/invocation/block_argument_multiple.stmt
@@ -72,4 +72,71 @@
[element, element],
'''multiple
lines''',
+);
+>>> Adjacent strings preceding a function expression doesn't prevent block formatting.
+test('First adjacent string' 'second adjacent string'
+'third adjacent string', () async {
+ ;
+});
+<<<
+test('First adjacent string'
+ 'second adjacent string'
+ 'third adjacent string', () async {
+ ;
+});
+>>> Don't block format a function with a preceding adjacent string if it doesn't fit.
+test('First adjacent string' 'second long adjacent string', () async {
+ ;
+});
+<<<
+test(
+ 'First adjacent string'
+ 'second long adjacent string',
+ () async {
+ ;
+ },
+);
+>>> Don't block format adjacent strings preceding a non-function block argument.
+test('First adjacent string'
+ 'second adjacent string'
+ 'third adjacent string', [
+ element1,
+ element2,
+ element3,
+ element4,
+]);
+<<<
+test(
+ 'First adjacent string'
+ 'second adjacent string'
+ 'third adjacent string',
+ [
+ element1,
+ element2,
+ element3,
+ element4,
+ ],
+);
+>>> Other string arguments don't prevent block formatting.
+test('First string line 1' 'first string line 2', () {
+ ;
+}, 'Another simple string');
+<<<
+test('First string line 1'
+ 'first string line 2', () {
+ ;
+}, 'Another simple string');
+>>> Other adjacent string arguments prevent block formatting.
+test('First string line 1' 'first string line 2', () {
+ ;
+}, 'Another adjacent' 'string argument');
+<<<
+test(
+ 'First string line 1'
+ 'first string line 2',
+ () {
+ ;
+ },
+ 'Another adjacent'
+ 'string argument',
);
\ No newline at end of file
