Skip to content

Commit da2d0fa

Browse files
authored
Smarter handling of "block" and "headline" formatting. (#1687)
* Start adding support for "shape" parent constraints. Currently, a parent can only indicate whether a child can contain newlines or not when determining whether a solution is invalid. This works fine for most pieces, but isn't sophisticated enough to allow fixing #1465 and #1466 where we want to say that the child can have newlines but only in certain styles. This is a step towards supporting those more sophisticated constraints. It doesn't change the underlying semantics at all, but it replaces the basic boolean newlines yes/no, with a Set<Shape>. * Make indentation semantic. Instead of pushing a raw integer for how much indentation a piece adds, use an enum with different values for each kind of indentation. Then, instead of a separate "canCollapse" parameter, have some logic that understands that certain pairs of semantic indentation can be merged. The hope is that this should then simplify some of the complex indentation handling in AssignPiece (and maybe elsewhere). There's no behavioral change or deep refactoring in this commit. It just replaces the int indentation and "canCollapse" parameter with an enum. Note that the old short style code also has a class named Indent which has static constants for various indentation levels. The tall code also used that same class. Now it uses its own Indent class which is an actual enum. That's why much of the change in this commit is just removing an import. The existing uses of Indent silently switch over to the new enum in code_writer.dart. * Use indentation merging to handle flattened indentation in assignments. It looks nice if we align binary operands when the first operand is on its own line, which is often the case after `=`, `:`, and `=>`: ```dart variable = operand + another; ``` This is also true for adjacent strings and conditional expressions. Previously, we had some somewhat hacky code that looked at the AST node surrounding an expression to determine if it should indent itself or not. This removes that in favor of using the new merging ability of Indent. We use a separate Indent type for assignment and infix operators and then merge those two together. This simplifies a bunch of things and lends itself to more uniform formatting since every AST node that lowers to a piece using Indent.infix will get this behavior. In fact, this commit highlighted that `is` and `as` expressions currently *aren't* formatted the same way as other binary operators, which I think was just an oversight. This commit still formats them differently, but I'll probably change that later. For now, it just leaves TODOs. This commit does change the formatter behavior very slightly, but only in obscure edge cases. * Add support for block shapes and use that for block formatting. This is (hopefully) the big commit. It's largely a refactoring but also has some style changes. Most of the style changes in places where the prior formatting was unintentional or a compromise to the old architecture. The changes in this commit are: - Add a new "block" shape. Pieces that split into delimited lists (collection literals, blocks, argument lists) have block shape. - Change AssignPiece, CasePiece, ChainPiece, and ForInPiece to use that for handling their block-formatted child pieces. Previous commits already let a piece constrain its children to a subset of the allowed shapes, and now we use that to let them say not just whether not a newline is allowed, but what resulting shape the child must have. - Add some machinery to CodeWriter to control the shape of the resulting piece when a newline is written. Coming up with a nice API for that is pretty hard, but I think what I have here works OK. There is also some machinery to determine how a child piece's shape affects the resulting shape of the parent. It isn't used heavily yet but will probably be used more in future changes to tweak the style for more complex nested expressions. This commit has two negative consequences: - Perf is regressed roughly 15% across the board. That's not great, but it's not catastrophic. To avoid a new pathological corner case, I added a hack where nested `=>` expressions (which occur when someone really wants to write Haskell in Dart) always force the outer ones to split. Without that, curried code gets catastrophically slow. I have some ideas on how to get some performance back but I'll save that for later. - Some "headline"-shaped output is no longer supported. It's always been a goal of the formatter to support output like: ```dart variable = target .method() .another(); ``` Prior to this commit, that *sort of* worked for most expressions, but really only incidentally and you could run into places where it should work but didn't. Moving to shape-based constraints makes it uniformly *not* work. In a future commit, I plan to add another "headline" shape that will get code like this working again, reliably. On the plus side, the behavior of the formatter is overall more consistent with this change. * Fix double indentation of const pattern. * Get "headline"-style formatting working. We now allow some non-block shaped pieces on the right side of an assignment without splitting if it those pieces are "headline"-shaped. That means they have a single first line that makes sense on the `=` line followed by other subordinate code. There are three kinds of expressions that support that: ```dart // Conditionals: variable = condition ? thenBranch : elseBranch; // Split method chains: variable = target .method() .another(); // (Also with unsplit properties): variable = target.some.props .method() .another(); // Multi-line strings: variable = ''' A very long string that is multiple lines.'''; ```
1 parent 71ac085 commit da2d0fa

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

88 files changed

+1331
-1172
lines changed

CHANGELOG.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,38 @@
22

33
* Format null-aware elements.
44

5+
* Allow the target or property chain part of a split method chain on the RHS of
6+
`=`, `:`, and `=>` (#1466).
7+
8+
```dart
9+
// Before:
10+
variable =
11+
target.property
12+
.method()
13+
.another();
14+
15+
// After:
16+
variable = target.property
17+
.method()
18+
.another();
19+
```
20+
21+
* Allow the condition part of a split conditional expression on the RHS of `=`,
22+
`:`, and `=>` (#1465).
23+
24+
```dart
25+
// Before:
26+
variable =
27+
condition
28+
? longThenBranch
29+
: longElseBranch;
30+
31+
// After:
32+
variable = condition
33+
? longThenBranch
34+
: longElseBranch;
35+
```
36+
537
* Don't indent conditional branches redundantly after `=`, `:`, and `=>`.
638

739
```dart

benchmark/case/interpolation.expect

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
main() {
2-
var a = '''
2+
var a =
3+
'''
34
{
45
selected:
56
hovered: $hoverColor, otherwise: ${colorScheme?.primary.withOpacity(0.04)},

benchmark/case/interpolation_1516.expect

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
var _ = '''
1+
var _ =
2+
'''
23
${x(1).x(1).xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx}
34
${x(1).x(1).xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx}
45
${x(1).x(1).xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx}

benchmark/case/large.expect

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -754,14 +754,13 @@ class Traverser {
754754
}
755755

756756
// Replace any overridden dependencies.
757-
deps =
758-
deps.map((dep) {
759-
var override = _solver._overrides[dep.name];
760-
if (override != null) return override;
757+
deps = deps.map((dep) {
758+
var override = _solver._overrides[dep.name];
759+
if (override != null) return override;
761760

762-
// Not overridden.
763-
return dep;
764-
}).toSet();
761+
// Not overridden.
762+
return dep;
763+
}).toSet();
765764

766765
// Make sure the package doesn't have any bad dependencies.
767766
for (var dep in deps) {
@@ -974,8 +973,7 @@ class Traverser {
974973
// Use the same source and description as the dependency override if one
975974
// exists. This is mainly used by the pkgbuild tests, which use dependency
976975
// overrides for all repo packages.
977-
var pubDep =
978-
override == null
976+
var pubDep = override == null
979977
? new PackageDep(depName, "hosted", constraint, depName)
980978
: override.withConstraint(constraint);
981979
return _registerDependency(

lib/src/ast_extensions.dart

Lines changed: 42 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,26 @@ extension AstIterableExtensions on Iterable<AstNode> {
162162
}
163163

164164
extension ExpressionExtensions on Expression {
165+
/// Whether this expression is a list, set, or map literal whose elements
166+
/// all have a homogeneous type.
167+
///
168+
/// In that case, the elements are relatively loosely related to each other.
169+
/// The collection has an unbounded number of them and the contents tend to
170+
/// change frequently.
171+
///
172+
/// This isn't true for record literal fields and function call arguments. In
173+
/// those cases, each argument position is meaningful and it's easiest to
174+
/// read them all together.
175+
///
176+
/// Thus it makes sense for the formatter to be looser about splitting list,
177+
/// map, and set literals, while trying to avoid splitting argument lists and
178+
/// records.
179+
bool get isHomogeneousCollectionBody =>
180+
this is ListLiteral || this is SetOrMapLiteral;
181+
182+
// TODO(rnystrom): We're moving towards using child piece shape to determine
183+
// how a parent is formatted and how it indents its children. Once all pieces
184+
// are moved over to that, delete this.
165185
/// Whether this expression is a non-empty delimited container for inner
166186
/// expressions that allows "block-like" formatting in some contexts. For
167187
/// example, in an assignment, a split in the assigned value is usually
@@ -187,6 +207,9 @@ extension ExpressionExtensions on Expression {
187207
/// splitting inside them, so are not considered block-like.
188208
bool get canBlockSplit => blockFormatType != BlockFormat.none;
189209

210+
// TODO(rnystrom): We're moving towards using child piece shape to determine
211+
// how a parent is formatted and how it indents its children. Once all pieces
212+
// are moved over to that, delete this.
190213
/// When this expression is in an argument list, what kind of block formatting
191214
/// category it belongs to.
192215
BlockFormat get blockFormatType {
@@ -375,28 +398,27 @@ extension AdjacentStringsExtensions on AdjacentStrings {
375398
/// Whether subsequent strings should be indented relative to the first
376399
/// string.
377400
///
378-
/// We generally want to indent adjacent strings because it can be confusing
379-
/// otherwise when they appear in a list of expressions, like:
401+
/// We generally prefer to align the strings because it makes them easier to
402+
/// read as a single paragraph of text (which they often are):
380403
///
381-
/// [
382-
/// "one",
383-
/// "two"
384-
/// "three",
385-
/// "four"
386-
/// ]
404+
/// function(
405+
/// 'This is a long string message '
406+
/// 'split across multiple lines.',
407+
/// )
387408
///
388-
/// Especially when these strings are longer, it can be hard to tell that
389-
/// "three" is a continuation of the previous element.
409+
/// But this is hard to read if there are other string arguments:
390410
///
391-
/// However, the indentation is distracting in places that don't suffer from
392-
/// this ambiguity:
411+
/// function(
412+
/// 'This is a long string message '
413+
/// 'split across multiple lines.',
414+
/// 'This is a separate argument.',
415+
/// )
393416
///
394-
/// var description =
395-
/// "A very long description..."
396-
/// "this extra indentation is unnecessary.");
417+
/// Here, unless you carefully notice the commas, it's hard to tell how many
418+
/// arguments there are.
397419
///
398-
/// To balance these, we omit the indentation when an adjacent string
399-
/// expression is in a context where it's unlikely to be confusing.
420+
/// To balance these, we omit the indentation in argument lists only if there
421+
/// are no other string arguments.
400422
bool get indentStrings {
401423
bool hasOtherStringArgument(List<Expression> arguments) => arguments.any(
402424
(argument) => argument != this && argument is StringLiteral,
@@ -411,27 +433,15 @@ extension AdjacentStringsExtensions on AdjacentStrings {
411433
if (message != null) message,
412434
]),
413435

414-
// Don't add extra indentation in a variable initializer or assignment:
415-
//
416-
// var variable =
417-
// "no extra"
418-
// "indent";
419-
VariableDeclaration() => false,
420-
AssignmentExpression(:var rightHandSide) when rightHandSide == this =>
421-
false,
422-
423-
// Don't indent when following `:`.
424-
MapLiteralEntry(:var value) when value == this => false,
425-
NamedExpression() => false,
426-
427-
// Don't indent when the body of a `=>` function.
428-
ExpressionFunctionBody() => false,
429436
_ => true,
430437
};
431438
}
432439
}
433440

434441
extension PatternExtensions on DartPattern {
442+
// TODO(rnystrom): We're moving towards using child piece shape to determine
443+
// how a parent is formatted and how it indents its children. Once all pieces
444+
// are moved over to that, delete this.
435445
/// Whether this expression is a non-empty delimited container for inner
436446
/// expressions that allows "block-like" formatting in some contexts.
437447
///

0 commit comments

Comments
 (0)